Last Comment Bug 680456 - Don't run compileAndGo scripts on globals with a cleared scope
: Don't run compileAndGo scripts on globals with a cleared scope
[sg:critical][qa-] wanted-standalone-js
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: x86 Mac OS X
-- normal (vote)
: mozilla13
Assigned To: Brian Hackett (:bhackett)
: Jason Orendorff [:jorendorff]
Depends on: 637099 723910
Blocks: 679247 679599 683756 711695 715149 730703
  Show dependency treegraph
Reported: 2011-08-19 09:06 PDT by Brian Hackett (:bhackett)
Modified: 2015-10-16 11:50 PDT (History)
14 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

partial fix (2.31 KB, patch)
2012-01-27 07:14 PST, Brian Hackett (:bhackett)
dmandelin: review+
asa: approval‑mozilla‑aurora+
asa: approval‑mozilla‑beta-
lukasblakk+bugs: approval‑mozilla‑esr10+
Details | Diff | Splinter Review

Description User image Brian Hackett (:bhackett) 2011-08-19 09:06:59 PDT
Clearing a global's scope wrecks a whole host of assumptions which the JITs and TI make about scripts compiled against that global.  There are some guards against this when executing scripts, but these are not enough, and compileAndGo scripts can (and do) still run against the global after clearing it.

This is causing bug 679247 (intermittent TI tinderbox failure); I added some instrumentation, and found that a particular script is getting analyzed by inference, has its global cleared, and later on starts/resumes execution in the interpreter.  When the script executes a JSOP_LAMBDA it pushes a function with a different (reconstructed) Function.prototype, which TI assertions catch.

From cursory examination of the code, it looks like we only check for a cleared global in RunScript, and not when making inline calls from the interpreter.  It seems like interpreter inline calls could cross from an uncleared global to a cleared global, with no isCleared check occurring.

I don't know if fixing this would be enough, however.  Under what constraints are scopes cleared?  Can scripts associated with the global have frames on the stack?  Can such scripts still have mjit code?  If the latter holds, this would be a problem for the full call stubs which the mjit generates, which allow mjit frames to call arbitrary scripts which have jitcode, without a cleared global check.

The real fix here is to remove scope clearing entirely (bug 637099).
Comment 1 User image Luke Wagner [:luke] 2011-08-19 09:22:15 PDT
Heh, so it seems the answer to bug 630072 comment 113 is 'yes'.  I wonder if this is causing other random crashes.

peterv: I heard the major ClearScope call is going away soon, do you have a time estimate?  jst indicated this call site was the major offender for scripts-run-on-cleared-globals; do you know if this is the *sole* source of clear-scoped globals which have script run on them?
Comment 2 User image Brian Hackett (:bhackett) 2011-08-19 14:38:25 PDT
JS_ClearScope is also causing the other intermittent TI tinderbox orange, bug 679599.  This case is somewhat different: JS_ClearScope is called on Array.prototype (I have no idea why), and when code later runs in the interpreter it can't find Array.prototype.push.  I'm not sure why this works on m-c, but one possibility is that the script runs in jitcode on m-c, and some assumption the jitcode makes is being subtly broken.
Comment 3 User image Boris Zbarsky [:bz] (still a bit busy) 2011-08-23 19:41:59 PDT
> do you know if this is the *sole* source of clear-scoped globals 

Gecko currently makes ClearScope calls in 3 places:

1) Outer windows are cleared when a new inner is being set up.  Now that outer
   windows are proxies that presumably have no slots, this may not be needed,
   but the comments claim this was at once point needed for correctness.
2) When nulling out the docshell of an outer window.  This is presumably just
3) When calling nsGlobalWindow::FreeInnerObjects.  I assume this is also meant
   as leak-prevention.

> JS_ClearScope is called on Array.prototype 

That's pretty odd.  It should, as things stand, be called up the proto chain of the window, but not on Object.prototype....  and Array.prototype should not be there, unless someome manually munged the proto chain.
Comment 4 User image Blake Kaplan (:mrbkap) 2011-08-23 20:05:44 PDT
There's also 4) When unloading a component. Also for leak prevention.
Comment 5 User image Daniel Veditz [:dveditz] 2011-08-24 17:03:49 PDT
Is this only on the TI branch or is any shipping version of Firefox affected?
Comment 6 User image Johnny Stenback (:jst, 2011-08-24 17:06:00 PDT
The JS_ClearScope() removal bug is really close to done (right, peterv?), and that removes the clearing of the global window objects completely. But there's some cases left, like storage objects and what bz and mrbkap pointed out that's still left, but they're all non-global objects AFAIK.
Comment 7 User image David Mandelin [:dmandelin] 2011-10-25 11:52:05 PDT
Do we still want to track this for 9? AFAICT, the plan is to remote ClearScope, that will obviate this bug and another one.
Comment 8 User image Brian Hackett (:bhackett) 2011-10-25 14:43:29 PDT
(In reply to David Mandelin from comment #7)
> Do we still want to track this for 9? AFAICT, the plan is to remote
> ClearScope, that will obviate this bug and another one.

No, I think we can wait for ClearScope to be removed entirely.
Comment 9 User image Daniel Veditz [:dveditz] 2011-12-22 13:12:46 PST
Seems unlikely we'd remove ClearScope during beta, should we not expect this in Fx10?
Is Firefox 11 realistic?
Comment 10 User image Brian Hackett (:bhackett) 2012-01-27 07:14:16 PST
Created attachment 592130 [details] [diff] [review]
partial fix

This fixes the cases where the global is cleared when scripts have compiled code against it, and where the interpreter tries to push an inline frame for a script with a cleared global.  It is still possible to run code against the global in the interpreter (no security problem there), but only for frames which were on the stack when the global was cleared.  It's not clear what to do should the latter case come up, as JS_ClearScope is infallible and I don't want to change its callers.
Comment 11 User image Andreas Gal :gal 2012-01-27 08:02:08 PST
JS_ClearScope should simply be eliminated. The browser is pretty much ready to drop it I think. peterv probably knows the exact status.
Comment 12 User image Brian Hackett (:bhackett) 2012-01-27 08:06:51 PST
(In reply to Andreas Gal :gal from comment #11)
> JS_ClearScope should simply be eliminated. The browser is pretty much ready
> to drop it I think. peterv probably knows the exact status.

Yeah, that was my opinion in comment 8.  That was 3 months ago though, I don't think there has been progress on bug 637099 since then, and this is a small fix which removes an existing security risk and cuts off this steady trickle of issues like bug 715149.
Comment 13 User image David Mandelin [:dmandelin] 2012-01-27 11:33:59 PST
Comment on attachment 592130 [details] [diff] [review]
partial fix

Review of attachment 592130 [details] [diff] [review]:

::: js/src/vm/GlobalObject.cpp
@@ +365,5 @@
>       */
>      cx->compartment->newObjectCache.reset();
> +
> +#ifdef JS_METHODJIT
> +    /* Destroy compiled code for any scripts parented to this global. */

r+ with comment revised to something like "If the global was cleared, for safety ..."
Comment 14 User image Brian Hackett (:bhackett) 2012-01-31 16:38:30 PST

There was a test (js/xpconnect/tests/mochitest/test_bug605167.html) which consistently tried to run a script against a cleared global, and which was failing because of this change --- a different exception was being thrown than the one it was testing for (it expected a permission-denied exception, and got the compile-and-go one instead).  I just relaxed the test itself.
Comment 15 User image Ed Morley [:emorley] 2012-02-01 11:03:05 PST
Comment 16 User image Mark Banner (:standard8) 2012-02-03 06:36:24 PST
This broke Thunderbird's account provisioner - see bug 723910.
Comment 17 User image Luke Wagner [:luke] 2012-02-03 08:52:13 PST
Unless there is a bug in the patch, that would mean thunderbird was running script on a cleared global, which is unsafe and can cause crashes.
Comment 18 User image Bob Clary [:bc:] 2012-02-26 14:49:39 PST
See bug 730703. Alice's fix range includes this bug.
Comment 19 User image Brian Hackett (:bhackett) 2012-02-26 19:39:37 PST
Comment on attachment 592130 [details] [diff] [review]
partial fix

From the error in bug 730703 comment 2 it seems likely that crash was fixed by this bug.  I'm guessing it's too late for beta but asking anyways.

[Approval Request Comment]
User impact if declined: Potential repeatable crashes, e.g. bug 730703.
Testing completed (on m-c, etc.): Has been on m-c for ~1 month
Risk to taking this patch (and alternatives if risky): Some risk for changes in website behavior, can cause websites to behave differently if they run scripts on globals which have been cleared.
Comment 20 User image Luke Wagner [:luke] 2012-02-27 12:54:14 PST
I have yet another case (a commercial 3d model viewer) that is fixed by this patch.  I think this patch could really help stability.
Comment 21 User image Asa Dotzler [:asa] 2012-02-28 14:55:29 PST
Comment on attachment 592130 [details] [diff] [review]
partial fix

Please land the patch on Aurora. We can jump one train here for a crash fix and a security hole, but it's just too late to try to get something into Beta which might break web sites.
Comment 22 User image Brian Hackett (:bhackett) 2012-02-28 15:17:09 PST
Comment 23 User image Lukas Blakk [:lsblakk] use ?needinfo 2012-02-29 15:22:00 PST
[Triage Comment]
Don't land this until Firefox 12 is on mozilla-beta please.
Comment 24 User image Lukas Blakk [:lsblakk] use ?needinfo 2012-03-20 10:09:41 PDT
Comment on attachment 592130 [details] [diff] [review]
partial fix

[Triage Comment]
Please go ahead and land this on ESR branch (see
Comment 25 User image Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-03-29 12:53:01 PDT
qa- until testcase-wanted is satisfied
Comment 26 User image David Mandelin [:dmandelin] 2012-04-04 18:33:29 PDT
Brian, could you rebase this for ESR10? Most of the bustage is trivial, but the patch landed to aurora uses Recompiler::clearStackReferences, which ESR10 doesn't have (and it seems to lack some of the machinery used by that function too).
Comment 27 User image Al Billings [:abillings] 2012-04-10 15:35:59 PDT
Is there any direct way to verify this fix?
Comment 28 User image David Mandelin [:dmandelin] 2012-04-10 15:38:34 PDT
(In reply to Al Billings [:abillings] from comment #27)
> Is there any direct way to verify this fix?

Don't worry about it--this bug is a partial fix. The real fix is bug 637099.
Comment 29 User image Al Billings [:abillings] 2012-04-10 16:43:15 PDT
Heh. That bug isn't frightening at all. :-)
Comment 30 User image Brian Hackett (:bhackett) 2012-04-14 06:42:55 PDT
Comment 31 User image Brian Hackett (:bhackett) 2012-04-14 08:23:59 PDT
Backed out for orange.
Comment 32 User image Brian Hackett (:bhackett) 2012-04-15 18:14:13 PDT

Note You need to log in before you can comment on or make changes to this bug.