487550 - "Assertion failure: cx->bailExit" with {__proto__: window}

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Jesse Ruderman]

Attachment: testcase (crashes Firefox when loaded)

Assertion failure: cx->bailExit, at /Users/jruderman/central/js/src/jstracer.cpp:4940

Tested on mozilla-central

Comment 1

[Jason Orendorff [:jorendorff]]

Taking.  It sounds like bug 468782 and bug 475761.

Comment 2

[Jason Orendorff [:jorendorff]]

Yep, there's a very clear gap in the checks we perform in jsbuiltin.cpp:HasProperty.  We check that the object we're looking at has no custom resolve hook, but we don't check its prototype.

Easy to loop over the prototype chain and check.  (The problem with shape-guarding here is that the target object is likely to be a plain old object that the program is using as a map.)

Comment 3

[Jason Orendorff [:jorendorff]]

Attachment: v1

Probable fix - will test in a fresh browser build tomorrow morning.

Comment 4

[Jason Orendorff [:jorendorff]]

Correction, tested it in the browser just now and it doesn't crash.

Comment 5

[Brendan Eich [:brendan]]

Comment on attachment 372510 [details] [diff] [review]
v1

Traces are triggered within requests and do not yield, suspend, or end their requests. Or so I claim, with nervous optimism.

>+    for (JSObject* pobj = obj; pobj; pobj = OBJ_GET_PROTO(cx, pobj)) {
>+        if (pobj->map->ops->lookupProperty != js_LookupProperty)
>+            return JSVAL_TO_PSEUDO_BOOLEAN(JSVAL_VOID);

All these dependent loads are safe because of requests, btw (see bug 408416 comment 13).

>+#ifdef JS_THREADSAFE
>+        // Do not race to finish the lookup before another thread modifies the
>+        // prototype chain.
>+        if (OBJ_SCOPE(pobj)->title.ownercx != cx)
>+            return JSVAL_TO_PSEUDO_BOOLEAN(JSVAL_VOID);
>+#endif

But this "can't [shouldn't] happen" -- if we are in a request and we do not use JS_LOCK_OBJ or one of its underpinnings, or (equivalently) js_{Get,Set}SlotThreadSafe, then all is lost.

It's true we do not bail off trace on an object whose title isn't claimed exclusively by the thread that is running the trace. We are shared-nothing in hg.mozilla.org code, AFAIK. We could assert, but we can't take the runtime overhead of a bail-check, and we shouldn't have to.

The __proto__ mutator runs as a special kind of GC, so no requests can run while it is running. Requests therefore see a consistent prototype chain.

All this to say I don't think this JS_THREADSAFE code is enough where it's truly needed, but it is not needed to handle __proto__ assignment. r=me without it, and if we don't have a bug about asserting ST-ownership of all titles on trace, we should.

If you want to patch HasProperty to assert its ST claims, that would be a good start, although it looks like many other places would need to assert too. Compiling extra LIR to assert before every optimized array element or shape-guarded object property access will slow down DEBUG builds, and probably give rise to debug vs. release differences that hide or create bugs. Not sure now is the time.

We should really assert shared-nothing at a higher (lower, not sure -- more central, anyway) layer.

/be

Comment 6

[Jason Orendorff [:jorendorff]]

(In reply to comment #5)
> (From update of attachment 372510 [details] [diff] [review])
> Traces are triggered within requests and do not yield, suspend, or end their
> requests. Or so I claim, with nervous optimism.

Well, bug 480301.  But yeah.

> The __proto__ mutator runs as a special kind of GC, so no requests can run
> while it is running. Requests therefore see a consistent prototype chain.

Ah, I didn't know this rule (failed to infer the second sentence from the first).  I agree that makes this safe without the check, unless the embedding shares objects *and* uses JS_SetPrototype.  I take it we don't care about that case?

> r=me without
> it, and if we don't have a bug about asserting ST-ownership of all titles on
> trace, we should.

I think it would be awfully easy to make those assertions fail, in an embedding that shares objects. We could instead detect MT titles at run time and bail out (bug 485988).

Comment 7

[Brendan Eich [:brendan]]

(In reply to comment #6)
> (In reply to comment #5)
> > (From update of attachment 372510 [details] [diff] [review] [details])
> > Traces are triggered within requests and do not yield, suspend, or end their
> > requests. Or so I claim, with nervous optimism.
> 
> Well, bug 480301.  But yeah.

That's the bug -- thanks.

> > The __proto__ mutator runs as a special kind of GC, so no requests can run
> > while it is running. Requests therefore see a consistent prototype chain.
> 
> Ah, I didn't know this rule (failed to infer the second sentence from the
> first).  I agree that makes this safe without the check, unless the embedding
> shares objects *and* uses JS_SetPrototype.  I take it we don't care about that
> case?

Less and less every minute. I left the cycle checking (which requires the stop the world GC mode) in for DEBUG builds at first, but it really kills perf, and makes for a difference w.r.t. release builds that bit us in another way (whose details elude me... and hg annotate isn't helping mrbkap and me find the rev that removed the lines, since the removal did not alter any remaining lines).

> > r=me without
> > it, and if we don't have a bug about asserting ST-ownership of all titles on
> > trace, we should.
> 
> I think it would be awfully easy to make those assertions fail, in an embedding
> that shares objects. We could instead detect MT titles at run time and bail out
> (bug 485988).

Yes, but after b4 :-/. Embedders who share objects across threads (which is a hard case, it should and will hurt) get what they deserve in the interim. Indeed we've left them hurting for arrays, iterators, and E4X for a while now.

/be

Comment 8

[Brendan Eich [:brendan]]

(In reply to comment #7)
> I left the cycle checking (which requires the stop
> the world GC mode) in for DEBUG builds at first, but it really kills perf, and
> makes for a difference w.r.t. release builds that bit us in another way (whose
> details elude me... and hg annotate isn't helping mrbkap and me find the rev
> that removed the lines, since the removal did not alter any remaining lines).

Bug 437325 (found by mrbkap with lucky typo-guessing, then he used hg bisect to show luddite-me the better way [unless you have his luck and converge faster than log2(n)!]).

/be

Comment 9

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/3209beb7c790

Comment 10

[Brendan Eich [:brendan]]

Comment on attachment 372510 [details] [diff] [review]
v1

This landed in tm without a bug comment.

/be

Comment 11

[Robert Sayre]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/3f730f8ba027

Comment 12

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Verified fixed on trunk and 1.9.1 with:

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090421 Minefield/3.6a1pre ID:20090421032809

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b4pre) Gecko/20090421 Shiretoko/3.5b4pre ID:20090421030848