Problem with Object.defineProperties on Android browsers

[garryyao]

The native implementation of Object.defineProperties on Android version >= 2.3.6 && <= 4.0.3 seems to be buggy, where the following test case fails as below, can we check if we need to shim up a bit on those devices?

==========Code==========

function fn() {};

try {
    Object.defineProperties(fn, {
        prototype: {
            value: {
                test: true
            }
        }
    });
}
catch (ex) {
    alert(ex);
}

alert(fn.prototype.test);

==========Code==========

==========Result=========
Platform APILevel Result
1.5 3 TypeError: Result of expression 'Object.defineProperties' [undefined] is not a function
1.6 4 TypeError: Result of expression 'Object.defineProperties' [undefined] is not a function
2.1 7 TypeError: Result of expression 'Object.defineProperties' [undefined] is not a function
2.2 8 true
2.3.3 10 true
2.3.6 x undefined
4.0 14 undefined
4.0.3 15 undefined
4.1.2 16 true
4.2.2 17 true
4.3 18 true
4.4.2 19 true
==========Result=========

[unscriptable]

Hello @garryyao,

It seems we should run a quick test to ensure that Object.defineProperties performs its basic function correctly. 👍

Should be an easy addition. Thanks for posting this issue.

Have you found any other inconsistencies or problems on Android?

-- John

[unscriptable]

Hm. I just checked our source code and we already test the basic functionality of the native features.
Here's our test:

try {
    // test it
    Object.defineProperties(object, { 'sentinel2': { value: 1 } });
    return 'sentinel2' in object;
}
catch (ex) { /* squelch */ }

For some reason, your tests are failing, though. Do you have any idea what the substantial difference is? I wonder if it is the fact that you are declaring a function's prototype.

[unscriptable]

Nm. I see the problem. We should change the code to test that the value is the same, not just that it exists:

try {
    // test it
    Object.defineProperties(object, { 'sentinel2': { value: true } });
    return 'sentinel2' in object && object.sentinel2 === true;
}
catch (ex) { /* squelch */ }

[unscriptable]

Hey @garryyao, I just pushed a quick fix to the dev branch. I don't have access to all those Android versions, atm. Do you think you could try it? Thanks! -- J

[KimiZH]

@unscriptable, sorry for late reply.
I am original writer of these test codes. Garry help me to post the issue here.
I think you misunderstood about these codes. The point is that 'prototype' can not be defined as a reserved words on some Android version. Maybe, first step, we should create a new test to check whether hosts support 'prototype' in 'Object.defineProperties'.

[garryyao]

Yes, defining the prototype property is the key for reproducing this issue, which is quite special a property on function.
@KimiZH Can you come with a poly TC that fails you on on those browsers?

[unscriptable]

Hey guys,

I'm trying to understand what to do next. Should we change the tests so that they try to define a property named "prototype" instead of "sentinel1" or "sentinel2"? Is that enough to detect the problem? I'm not convinced that that is the right solution.

For old Android, it seems like we should allow Object.defineProperty to work as it normally does on all property names except "prototype". Therefore, we need an additional test just for detecting failures when defining "prototype" on functions. Does this sound right?

@jaredcacurak: is this something you might have some time to work on?

-- John

[jaredcacurak]

I'd be glad to look into it @unscriptable.

[unscriptable]

Cool @jaredcacurak! Let's push something to a branch and see if @garryyao or @KimiZH can verify it works.

[jaredcacurak]

@garryyao @KimiZH Does the test, added in the commit above, recreate the issue?

[KimiZH]

I think we can replace the native function if 'prototype' property can not be defined.
KimiZH@bc8ef6d

[KimiZH]

Object.defineProperty has the same issue.
KimiZH@c083663

[jaredcacurak]

This issue looks to be the culprit.

[KimiZH]

It looks good. I think this would resolve.

[garryyao]

The changes are good, while we might want to check the speciality of descriptor of prototype property as well, illustrated by the following tests:

// tc.1 - Object.getOwnPropertyDescriptor
function Foo() {}
var desc = Object.getOwnPropertyDescriptor(Foo, 'prototype');
console.assert(desc.configurable === false, "function's prototype property should NOT  be configurable.");
console.assert(desc.enumerable === false, "function's prototype property should NOT be enumerable.");
console.assert(desc.writable === true, "function's prototype property should be writable.");


// tc.2 - Object.defineProperty
function Foo() {}
Object.defineProperty(Foo, 'prototype', {value: 'foo'});
console.assert(new Foo().__proto__ === Object.prototype, "function's prototype property should fallback to Object.prototype when it encounters invalid values, e.g. of non-object type.");

// tc.3 - Object.freeze
var foo = {};
function Bar() {}
Object.defineProperty(Bar, 'prototype', {value: foo});
Object.defineProperty(foo, 'name', {value: 'bar', writable: false});
var bar = new Bar();
bar.name = 'foo';
console.assert(bar.name === 'bar', 'object property should NOT writable specified by the prototype');

[jaredcacurak]

@garryyao can you elaborate on your previous comment?

[unscriptable]

We have to be careful not to test for every bug in every version of every browser. If these bugs re causing problems in your applications, @garryyao, then perhaps we should open another issue? Otherwise, I think we should just let old Android bugs die out with old Android phones. :)

[unscriptable]

We have to be careful not to test for every bug in every version of every browser.

So poly doesn't end up being 100KB. :)

[KimiZH]

@jaredcacurak
I suggest using native method if property is not 'prototype'. Beacuse other properties (configurable, enumerable, writable) should work.
KimiZH@51c2cc7

[unscriptable]

Hey @jaredcacurak, I think @KimiZH's idea is probably best. Let client code use the native function in the cases where it works correctly.

However, I think I'd code it differently. We should probably try to use the existing polyfill as much as possible. (If we add more assertions and such to the original polyfill, they'd also apply to this new polyfill.) I'm thinking something like this:

    function definePropertyFunctionPrototype (fn, name, descriptor) {
        if (name === 'prototype' && typeof fn === 'function') {
            // apply shim
            return defineProperty(fn, name, descriptor);
        }
        else {
            // use native
            return Object.defineProperty(fn, name, descriptor); 
        }
    }

See his commit link in his previous comments for the other half of the change.

Thoughts?

[garryyao]

Make sense.