867749 - Intl: Assertion failure: false, at vm/SelfHosting.cpp:134

Status: RESOLVED FIXED

(Core :: JavaScript: Internationalization API, defect)

Description

[Christian Holler (:decoder)]

The following testcase asserts on mozilla-central revision 02aa81c59df6 (no options required):


Object.defineProperty(Object.prototype, 1, {
    get: function(print,test, expr) { return 23 }
});
var locales = ["tlh", "en"];
new Intl.Collator(locales);

Comment 1

[Christian Holler (:decoder)]

This doesn't need a bisect, it's probably related to the recent Intl landing :) Ccing Norbert.

Comment 2

[Christian Holler (:decoder)]

Waldo just told me that he'll be working on Intl bugs, so... Ccing Waldo \o/

Comment 3

[Jeff Walden [:Waldo]]

Turns out, in CanonicalizeLocaleList (?), the object for a |new List()| has this prototype chain:

  seen -> ListProto with join/push/etc. methods -> Object.prototype (!) -> null

This...kind of breaks stuff!  Figuring out now what's at fault for this...

Comment 4

[Jeff Walden [:Waldo]]

Attachment: Clone plain-old objects with null [[Prototype]] for self-hosting, semi-speculative patch

Good times.  Utilities.js has this:

function List() {}
{
  let ListProto = std_Object_create(null);
  ListProto.indexOf = std_Array_indexOf;
  ListProto.join = std_Array_join;
  ListProto.push = std_Array_push;
  ListProto.slice = std_Array_slice;
  ListProto.sort = std_Array_sort;
  List.prototype = ListProto;
}
MakeConstructible(List);

When List gets cloned into each new global, the List function clones.  Then the properties of List are cloned.  List.prototype, as defined above, gets cloned by the last else in the CloneObject mega-condition:

        JS_ASSERT(srcObj->isNative());
        clone = NewObjectWithClassProto(cx, srcObj->getClass(), NULL, cx->global(),
                                        srcObj->tenuredGetAllocKind());

That creates an object *with class prototype*, which gets us an object with ListProto's properties but not the correctly corresponding prototype chain.

This is a speculative fix.  It eliminates the assertion in the initial test here, and it passes most/all of the test402 shell tests.  I don't know if it passes jstests or jit-tests as a whole.  Putting it out there for comment for now, at least -- obviously this prototype-nulling could break other stuff, but I'm guessing most such bustage would actually be a *good* thing!

Might also be worth looking into whether we can clone all the other objecty things with null prototype, to prevent similar issues there from accidentally using user-mutable standard prototypes.  But let's wait til this is actually deemed good before doing that.  :-)

This sort of issue is why cloning always felt hackish as a trick to solving all this, if I haven't said it before.

Comment 5

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

jsfunfuzz found a testcase that doesn't seem to involve Intl at first glance:

try {
    h2 = {}
    o1 = h2.__proto__
} catch (e) {} /**/
try {
    a0 = wrap(o1)
    Array.prototype.unshift.call(a0, 2)
} catch (e) {}
try {
    print(uneval(this))
} catch (e) {}

but asserts similarly. autoBisect though, points to Intl, so this may be related.

Comment 6

[Till Schneidereit [:till]]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #4)
> That creates an object *with class prototype*, which gets us an object with
> ListProto's properties but not the correctly corresponding prototype chain.

That is ... not ideal. Maybe we should add special handling for prototype-cloning or at least detect the null-prototype situation? Shouldn't be too hard to do, I'd guess.

> Might also be worth looking into whether we can clone all the other objecty
> things with null prototype, to prevent similar issues there from
> accidentally using user-mutable standard prototypes.  But let's wait til
> this is actually deemed good before doing that.  :-)

I'm not sure I understand what "the other objecty things" are.

> 
> This sort of issue is why cloning always felt hackish as a trick to solving
> all this, if I haven't said it before.

Vaguely rings a bell. I still tend to agree, but don't have any great alternative ideas.

Comment 7

[Jeff Walden [:Waldo]]

(In reply to Till Schneidereit [:till] from comment #6)
> That is ... not ideal. Maybe we should add special handling for
> prototype-cloning or at least detect the null-prototype situation? Shouldn't
> be too hard to do, I'd guess.

The patch passes try:

https://tbpl.mozilla.org/?tree=Try&rev=ebdf72533c1e

> I'm not sure I understand what "the other objecty things" are.

The worry would be that some objects would *expect* to be cloned and have a prototype chain that goes to Object.prototype or similar.  And somehow they'd rely on it.  And they'd rely on it without Object.prototype poisoning affecting them.  Given the try-result, it looks like happily nobody does this.  So it's not perfectly semantics-preserving, but I think killing off the prototype chain entirely when self-hosted-cloning is a good thing to do, so long as we're on the clone-all-the-things track.

Comment 8

[Norbert Lindenberg]

For the internationalization API I think self-hosted global variables come in two flavors: Those where I took steps to prevent having a prototype, and those where I convinced myself that the prototype is harmless. I don't think there's any case where I actually wanted a prototype.

It seems this change follows the pattern we have for functions already, to create objects in a form that's not fully ECMAScript conformant but convenient for self-hosting. Self-hosted functions are born without prototype property and [[Construct]] internal property, and where they're needed, they have to be added separately (e.g., MakeConstructible).

The try run, BTW, didn't test with the internationalization API - you need the patch from bug 853301 for that.

Comment 9

[Christian Holler (:decoder)]

Attachment: [crash-signature] Machine-readable crash signature

Comment 10

[Till Schneidereit [:till]]

Comment on attachment 744355 [details] [diff] [review]
Clone plain-old objects with null [[Prototype]] for self-hosting, semi-speculative patch

Ok, makes sense to me, now. r=me with a green try run that includes the Intl support.

Comment 11

[Jeff Walden [:Waldo]]

Hmm, yes, enabling Intl in desktop builds.  :-\  I should be able to get things arranged to land that patch on Monday, I believe.  In the meantime, yeah, I need to keep remembering that.

https://tbpl.mozilla.org/?tree=Try&rev=71996c706a98

Comment 12

[Jeff Walden [:Waldo]]

That looks green, and I opened up a J build to be doubly sure Intl tests got run, so I'll push this the next time I have stuff to push.

Comment 13

[Jeff Walden [:Waldo]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2b64fef138e2

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2b64fef138e2

Comment 15

[Gordon P. Hemsley [:GPHemsley]]

Mass-moving existing Intl-related bugs to the new Core :: JavaScript: Internationalization API component.

If you think this bug has been moved in error, feel free to move it back to Core :: JavaScript Engine.

[Mass change filter: core-js-intl-api-move]