336054 - "Assertion failure: !OBJ_GET_PROTO(cx, ctor)" in JS_InitClass, involving Greasemonkey

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jesse Ruderman]

Steps to reproduce (I think):
1. Install Greasemonkey
2. Install http://www.squarefree.com/userscripts/bug-attachment-source.user.js
3. Load any bug (e.g. bug 12345) in a debug build.

Result: 

Assertion failure: !OBJ_GET_PROTO(cx, ctor), at /Users/admin/trunk/mozilla/js/src/jsapi.c:2177

Program received signal SIGABRT, Aborted.

Stack to follow.  Shaver and mrbkap are already on it (discussed in #content just now).

Comment 1

[Jesse Ruderman]

Attachment: stack trace (mac ppc debug, gdb)

Comment 2

[Mike Shaver (:shaver emeritus)]

The bug is that we're setting sandcx's global object to be one that has already had standard classes initialized against it, but sandcx->classObjects is of course still all NULLs: we initalized the classes against this global using another context!

I think this pattern needs to be supported, since we advocate context pooling, but I'm not sure the best way to get sandcx->classObjects refilled appropriately.  Filling from the global object in the "obvious" way could result in us treating a script-set "Function" (f.e.) as the canonical/original Function constructor, which we take pains to avoid for ECMA compliance and performance.

Brendan?

Comment 3

[Mike Shaver (:shaver emeritus)]

This is a regression from 304376.

Comment 4

[Brendan Eich [:brendan]]

Attachment: proposed fix

Comment 5

[Brendan Eich [:brendan]]

Attachment: js shell patch to help test

Comment 6

[Brendan Eich [:brendan]]

Attachment: corrected (!lazy && !JS_InitStandardClasses(...)) -- was (lazy && ...)

Comment 7

[Mike Shaver (:shaver emeritus)]

Comment on attachment 220449 [details] [diff] [review]
proposed fix

As Jesse found the hard way, this doesn't really handle the setting of global objects to NULL very well. :) sr-=shaver, likely a trivial fix but I haven't really thought through the implications of just NULL-checking obj.

Comment 8

[Brendan Eich [:brendan]]

Attachment: combined js library and shell patch

There's nothing profound about a null-test.  It's simply necessary when clearing the global object, which is done via JS_SetGlobalObject(cx, NULL).  This patch will clear cx->classObjects in that case.

/be

Comment 9

[Mike Shaver (:shaver emeritus)]

Comment on attachment 220479 [details] [diff] [review]
combined js library and shell patch

sr=shaver

Comment 10

[Brendan Eich [:brendan]]

Attachment: patch I'm checking in

Picked nit in js.c: set ok to JS API return value where possible.

/be

Comment 11

[Brendan Eich [:brendan]]

Fixed, mrbkap feel free to review -- I'll pick any nits tomorrow.

/be

Comment 12

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 220483 [details] [diff] [review]
patch I'm checking in

Cool.