679939 - remove COMPILE_N_GO

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Luke Wagner [:luke]]

With compartment-per-global, a jit-compiled JSScript is only ever run with a single global so jits have all they need optimize global access without any unnecessary guarding.  Not only would this remove complexity (and interpreter opcodes) but this change is also necessary to be able to share the bytecode of a script between compartments (see dependent bug).

Comment 1

[Brian Hackett [Laid off!]]

Yay!  I think inference information would want to live in the same place as jitcode, which would also remove a lot of complexity and help precision a lot.

Comment 2

[Nicolas B. Pierron [:nbp]]

As of Bug 883154 comment 3, I'll take this bug to improve the transition time within Gaia application as they are likely to switch to iframes as way have a coarse grain memory management of panels [1].

As they want to prevent cross-compartment references, to avoid easy mistakes and simplify reviews, they need the platform to reload scripts in a fast way, and ideally without using more memory.  Removing COMPILE_N_GO should enable Bug 679940 on all scripts.

[1] https://groups.google.com/forum/?fromgroups=#!searchin/mozilla.dev.gaia/Proposition$20of$20an$20architectural$20change$20for$20Gaia$20apps|sort:relevance/mozilla.dev.gaia/Ls9U-CScxWA/1bcCvlG41U0J

Comment 3

[Luke Wagner [:luke]]

The original biggest dependence on compileAndGo I know of was JSOP_(GET|SET)GLOBAL opcodes which baked in, at bytecode-compile-time, the slot in the global object to load from.  These ops have been removed and replaced by the JSOP_(GET|SET)GNAME opcodes which are basically name lookups where we assume the lookup happens on the global object (and not any object before it on the scope chain) which we optimize using the global's TI to do efficient loads/stores.  So half of this bug's goals are already done.

Removing compileAndGo would break the GNAME assumption, though, if a JSScript was ever run again with a scope != script->global().  Event handlers are classically an example of this, but bz has plans to for how we can avoid doing this.  With that fixed, I wonder: do we have any other cases where we need to run a JSScript with anything other than script->global (for global code) or fun->environment (for function code)?  Said differently, could we remove the 'obj' parameter of JS_(Evaluate|Execute|Compile)Script (or repurpose it to mean only 'thisObj') as well as the 'parent' parameter to JS_CloneFunctionObj?  (I'm guessing 'no, XBL!'.  Can we handle XBL with a similar 'with' trick as event handlers?)  If we could have this, then, effectively, *every* script would be compileAndGo and fully optimized (except for, ya know, the ones with all the 'with' ;).  What a world it could be.

Comment 4

[Boris Zbarsky [:bzbarsky]]

So the main things that are run across globals are XBL and XUL prototype stuff, right?

The idea there is to have a compiled version, even saved/loaded with XDR, that can then be run across various globals.  The goal being to compile it only once.

Given bug 679940 is fixed, does that mean we can XDR only the shareable bits and then (cheaply) recreate the global-specific part as needed?

Comment 5

[Luke Wagner [:luke]]

(In reply to Boris Zbarsky [:bz] from comment #4)
> So the main things that are run across globals are XBL and XUL prototype
> stuff, right?

For the XUL prototype cache, we are actually (as a hack in JS_ExecuteScript) cloning the JSScript if obj->compartment != script->compartment().  Conceivably, we could remove this hack and do explicit cloning in the caller which would I think uphold the everything-is-compileAndGo property.  I have no idea what XBL is doing.

> Given bug 679940 is fixed, does that mean we can XDR only the shareable bits
> and then (cheaply) recreate the global-specific part as needed?

Yeah, that makes the abovementioned CloneScript cheaper.

Comment 6

[Boris Zbarsky [:bzbarsky]]

What XBL is doing is XDR-ing individual method/getter/setter functions.

Comment 7

[Luke Wagner [:luke]]

(In reply to Boris Zbarsky [:bz] from comment #6)
> What XBL is doing is XDR-ing individual method/getter/setter functions.

Are those method/getter/setter functions literally functions (produced by JS_DecodeInterpretedFunction)?  If so, that's great; that means their scope will be cx->global() and so we have no problem from XBL.

Overall, then, it is sounding to me like self-hosting event handler scoping using 'with' and tweaking how CloneScript happens for the XUL proto cache should allow us to assume compileAndGo everywhere.  Do you see any other problems?

Comment 8

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky [:bz] from comment #6)
> What XBL is doing is XDR-ing individual method/getter/setter functions.

For serialization, yes. But it also clones them from the compilation scope into the target scope. Or is that what you're referring to? I'm not sure if JS_CloneFunction* uses XDR under the hood.

Comment 9

[Luke Wagner [:luke]]

(In reply to Bobby Holley (:bholley) from comment #8)
Is scope passed to the JS_CloneFunction* call a global object or something else?  If it is something else, could we apply the same "random objects on the scope chain using 'with'" self-hosting strategy as event handlers?

Comment 10

[Bobby Holley (:bholley)]

(In reply to Luke Wagner [:luke] from comment #9)
> (In reply to Bobby Holley (:bholley) from comment #8)
> Is scope passed to the JS_CloneFunction* call a global object or something
> else?  If it is something else, could we apply the same "random objects on
> the scope chain using 'with'" self-hosting strategy as event handlers?

We definitely want to be cloning the functions in the XBL case. It's important that they run with the principals of the associated XBL scope.

Comment 11

[Luke Wagner [:luke]]

(In reply to Bobby Holley (:bholley) from comment #10)
Of course cloning, but I was asking about whether the scope of the cloned function object was cx->global and, if it wasn't, whether we could do the 'with' thing.

Comment 12

[Bobby Holley (:bholley)]

(In reply to Luke Wagner [:luke] from comment #11)
> (In reply to Bobby Holley (:bholley) from comment #10)
> Of course cloning, but I was asking about whether the scope of the cloned
> function object was cx->global and, if it wasn't, whether we could do the
> 'with' thing.

Oh. The scope is generally the bound node, which we depend on in various cases. I don't see any reason why the |with| trick wouldn't work.

Comment 13

[Boris Zbarsky [:bzbarsky]]

> Are those method/getter/setter functions literally functions (produced by
> JS_DecodeInterpretedFunction)? 

For the XDR situation, yes.

For the scoping situation XBL has several different kinds of function.

1)  Methods.  These are installed on the XBL proto, and their scope is whatever
    xpc::GetXBLScope() is returning.  For chrome, I believe this is the global; for
    content xbl we can hopefully use the with trick?
2)  Property getters/setters: same as methods.
3)  ctors/dtors: scope is the bound node.  With trick should work.
4)  event handlers: scope is the event target.  With trick should work.

Comment 14

[Nicolas B. Pierron [:nbp]]

(In reply to Boris Zbarsky [:bz] from comment #6)
> What XBL is doing is XDR-ing individual method/getter/setter functions.

FYI, anything which is not using compileAndGo at the moment is always executed by the interpreter and never Jitted.
And anything which is using XDR at the moment implies that compileAndGo is not set.

So, XBL's JavaScript is always running in the interpreter.

(In reply to Bobby Holley (:bholley) from comment #8)
> For serialization, yes. But it also clones them from the compilation scope
> into the target scope. Or is that what you're referring to? I'm not sure if
> JS_CloneFunction* uses XDR under the hood.

No, it does not.  And the way XDR is made, it is used for doing either encoding or decoding but not both at the same time.  On the other hand, we have nice comments to keep the Clone functions in-sync with XDR one and the opposite.

Comment 15

[Luke Wagner [:luke]]

(In reply to Boris Zbarsky [:bz] from comment #13)
In all these different scoping situation, is the originally-compiled JSFunction being JS_CloneFunctionObject'd once into each scope?

(In reply to Nicolas B. Pierron [:nbp] from comment #14)
> So, XBL's JavaScript is always running in the interpreter.

The baseline jit doesn't require compileAndGo.  See for yourself with evaluate("while(true) {}", {compileAndGo:true}).

Comment 16

[Luke Wagner [:luke]]

... of course I mean {compileAndGo:false} :)

Comment 17

[Boris Zbarsky [:bzbarsky]]

> being JS_CloneFunctionObject'd once into each scope?

No.  Methods and property getters/setters are cloned once per scope (since they live on the proto).  ctors/dtors/eventhandlers are cloned once per instance object.

Comment 18

[Luke Wagner [:luke]]

(In reply to Boris Zbarsky [:bz] from comment #17)
I didn't mean 'scope' as in XPC's scope as in global, I meant 'scope' as in your use of 'scope' in comment 13.  With that meaning of 'scope', it sounds like your answer then is 'yes'?

Comment 19

[Boris Zbarsky [:bzbarsky]]

Ah, yes.  In that case the answer is yes for ctors and dtors but not for event handlers.  Those are cloned every time the event fires.  We could try to fix that...

Comment 20

[Luke Wagner [:luke]]

(In reply to Boris Zbarsky [:bz] from comment #19)
Wow, that's a lot of cloning!  Cloning isn't particularly fast, it might be a good speedup to minimize cloning.

Random question: will DOM mutations change the contents of the scope chain of a single event handler over time?  (I don't mean change the variables visible, of course that will change, I mean which and how many DOM objects are on the scope chain.)  That could be a problem for 'with'.

Comment 21

[Boris Zbarsky [:bzbarsky]]

> will DOM mutations change the contents of the scope chain of a single event handler over 
> time?

Per spec they're not supposed to.  Currently they sometimes do (for the non-XBL case), but in those cases I believe we recompile the handlers anyway.

Comment 22

[Luke Wagner [:luke]]

(In reply to Boris Zbarsky [:bz] from comment #21)
> Per spec they're not supposed to.

Interesting.  So does that mean the spec is saying something like "take a snapshot of the DOM node parent chain at the time the event handler function is created and always use that"?

Comment 23

[Boris Zbarsky [:bzbarsky]]

Yes.  Specifically, what it says is this (at http://www.whatwg.org/specs/web-apps/current-work/multipage/webappapis.html#event-handler-content-attributes ):

  When an event handler content attribute is set, if the element is owned by a Document
  that is in a browsing context, and scripting is enabled for that browsing context, the
  user agent must run the following steps to create a script after setting the content
  attribute to its new value:
...
  Lexical Environment Scope

    1. Let Scope be the result of NewObjectEnvironment(the element's Document, the global
       environment).
    2. If the element has a form owner, let Scope be the result of
       NewObjectEnvironment(the element's form owner, Scope).
    3. Let Scope be the result of NewObjectEnvironment(the element's object, Scope).

then you create a new Function, per the rules of "ECMAScript edition 5 section 13.2 Creating Function Objects".

Comment 24

[Luke Wagner [:luke]]

(In reply to Boris Zbarsky [:bz] from comment #23)
Cool, thanks for explaining!  I suspect this exact definition was motivated by someone who already had done the self-hosting with with :)  Since we use the live parent field of the document, I assume we break this now if you manipulate the DOM tree in a way that changes parent links?

Comment 25

[Boris Zbarsky [:bzbarsky]]

We don't change parent links on JS objects once they're created.  So no, we don't break this now, unless we give the node a new JSObject altogether.  Which in this case only happens on adoption into a different document, and recompiles all the handlers anyway.  (All this modulo the fact that we compile handlers lazily, which technically the spec does not allow you to do.)

Comment 26

[Luke Wagner [:luke]]

Ah, thanks for explaining.

Overall, then, it sounds like the general strategy of "clone the JSScript when you would have otherwise run the same JSScript with a different scope chain" could work to remove all compileAndGo.

Nicolas was right to file bug 920322 as blocking this, though, since there are still some constructs in compileAndGo JSScripts that XDR can't serialize.

Comment 27

[Boris Zbarsky [:bzbarsky]]

So I'm trying to pick this up again (see bug 1095015 with some semi-related things).

I'd like to understand the proposal in comment 26, though.  How does that allow us to avoid the compileAndGo flag?   Say we have a script that we compile; it ends up using JSOP_GETGNAME.  Then we want to run against a different scope, clone it... but that won't replace the bytecode, right?  So we still have a JSOP_GETGNAME in the bytecode.  But the scope chain might now have something with that name on it...

Comment 28

[Luke Wagner [:luke]]

You're right; the invariants implied by GNAME would have to weaken to say "this is an unbound name" instead of "this is a name that resolves to the global object, if at all".  At script-clone time, you know the new scope chain and whether it is script->global() or something else, so it seems like we could just set a bit on JSScripts saying "has non-global scope" that effectively turns GNAME ops into NAME ops.  There is some recent discussion on this starting in bug 1045529 comment 11.  What I don't know (and am hopeful about) is how much simpler the situation is with all the JSAPI changes you've made and are planning.

Comment 29

[Boris Zbarsky [:bzbarsky]]

OK.  So here's our plan from discussion with Luke just now:

1) Make bug 1095015 comment 0 item 2 happen.

2) Change JSAPI entry points to release-assert the following invariants that I'm pretty
   sure hold in Gecko:
  *) Toplevel scripts are only compiled against a global.
  *) Toplevel scripts are either executed against a global or an object that's not
     same-compartment with their compilation global.
  *) Functions are only cloned into a non-global scope cross-compartment.

3) Add a new flag called hasDynamicScopeChain to scripts.  Set it in the API entrypoints that provide a dynamic scope chain, both compilation (for functions) and cloning.  Since per item 2 we guarantee that we clone whenever we might be picking up an interesting scope chain, this is good enough.

4) Change the places where we special-case the GNAME ops to only do that if !hasDynamicScopeChain.

5) Stop using the compileAndGo flag for deciding whether to emit GNAME ops.  Just emit them, and rely on the hasDynamicScopeChain checks doing the right thing.

6) Eliminate any remaining uses of compileAndGo that are left at that point.

I'll start filing some bugs.

Comment 31

[Luke Wagner [:luke]]

The only tweak I'd make is s/hasDynamiScopeChain/hasDynamicGlobalScope/ (or some other name to imply that the weirdness happens between the outermost lexical scope and the global object).

Comment 32

[Boris Zbarsky [:bzbarsky]]

> 6) Eliminate any remaining uses of compileAndGo that are left at that point.

These seem to be:

1)  CanLazilyParse.  This might be fixed once we do the above, since it basically depends on the GNAME behavior produced by CompileLazyFunction, which currently assumes compileAndGo == true.  We may want to store hasDynamicGlobalScope on LazyScript.

2)  Singleton function/context stuff in the bytecode emitter.  Not sure where to go with these.

3)  BytecodeEmitter::needsImplicitThis.  This might be solved implicitly by making sure we only allow With scopes when we have a non-global on the scope chain.

Comment 33

[Boris Zbarsky [:bzbarsky]]

OK, we're going to need to change the invariants from comment 29 step 2, like so:

  *) Toplevel scripts are either executed against a global or get cloned before execution.

See bug 1143793 comment 7 and bug 1143793 comment 9.

We also need to add handling of JSOP_IMPLICITTHIS similar to the GNAME bits described in comment 29.

Comment 34

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Extend the hasRunOnce/treatAsRunOnce setup to global and eval scripts

Comment 35

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Disallow execution of global/eval scripts that are flagged runOnce and have already run

Comment 36

[Boris Zbarsky [:bzbarsky]]

Attachment: part 3. Add a CompileOptions flag for indicating that the script should be compiled runOnce

Comment 37

[Boris Zbarsky [:bzbarsky]]

Attachment: part 4. Set the isRunOnce compile flag as needed

Comment 38

[Boris Zbarsky [:bzbarsky]]

Attachment: part 5. Stop using the compileAndGo script flag in the bytecode emitter

Comment 39

[Boris Zbarsky [:bzbarsky]]

Attachment: part 6. Drop function-cloning uses of compileAndGo, since it no longer affects the bytecode

Comment 40

[Boris Zbarsky [:bzbarsky]]

Attachment: part 7. Drop the now-unused JSScript::compileAndGo

Comment 41

[Boris Zbarsky [:bzbarsky]]

Attachment: part 8. Drop the now-unused compileAndGo from CompileOptions

Comment 42

[Luke Wagner [:luke]]

Comment on attachment 8583153 [details] [diff] [review]
part 3.  Add a CompileOptions flag for indicating that the script should be compiled runOnce

Review of attachment 8583153 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/frontend/BytecodeCompiler.cpp
@@ +291,5 @@
>      if (!script)
>          return nullptr;
>  
> +    if (options.isRunOnce)
> +        script->setTreatAsRunOnce();

Since options are passed into JSScript::Create, can you initialize this field inside Create like the other options?

Comment 43

[Luke Wagner [:luke]]

Comment on attachment 8583154 [details] [diff] [review]
part 4.  Set the isRunOnce compile flag as needed

Review of attachment 8583154 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/nsScriptLoader.cpp
@@ +1073,5 @@
>    aOptions->setIntroductionType("scriptElement");
>    aOptions->setFileAndLine(aRequest->mURL.get(), aRequest->mLineNo);
>    aOptions->setVersion(JSVersion(aRequest->mJSVersion));
>    aOptions->setCompileAndGo(JS_IsGlobalObject(aScopeChain));
> +  aOptions->setIsRunOnce(true);

I wonder if perhaps we should change the default to 'true' since it's almost always what you want.  Errors should be easily caught and fixed.

Comment 44

[Luke Wagner [:luke]]

Comment on attachment 8583155 [details] [diff] [review]
part 5.  Stop using the compileAndGo script flag in the bytecode emitter

Review of attachment 8583155 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/frontend/BytecodeEmitter.h
@@ +246,5 @@
>  
>      bool isInLoop();
>      bool checkSingletonContext();
> +    // Check whether our function is in a run-once context (a toplevel
> +    // run-one script or a run-once lambda).

Can you put a \n before comment?

Comment 45

[Luke Wagner [:luke]]

Comment on attachment 8583157 [details] [diff] [review]
part 7.  Drop the now-unused JSScript::compileAndGo

Review of attachment 8583157 [details] [diff] [review]:
-----------------------------------------------------------------

\o/

Comment 46

[Boris Zbarsky [:bzbarsky]]

> can you initialize this field inside Create like the other options?

OK, done.

> I wonder if perhaps we should change the default to 'true'

Hmm.  I suppose we could, but maybe in a followup?  I worry about various edge cases popping up...

> Can you put a \n before comment?

Done.

Comment 47

[Luke Wagner [:luke]]

Comment on attachment 8583158 [details] [diff] [review]
part 8.  Drop the now-unused compileAndGo from CompileOptions

Review of attachment 8583158 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/frontend/BytecodeCompiler.cpp
@@ +149,3 @@
>          !options.hasPollutedGlobalScope &&
>          !cx->compartment()->options().discardSource() &&
>          !options.sourceIsLazy;

Since you're touching, can you align the '!' with the 'o' in 'options' on the first line?

Comment 48

[Luke Wagner [:luke]]

(In reply to Not doing reviews right now from comment #46)
> > I wonder if perhaps we should change the default to 'true'
> 
> Hmm.  I suppose we could, but maybe in a followup?  I worry about various
> edge cases popping up...

A follow-up would be fine.  I was thinking that the situation with setIsRunOnce is similar to setHasPollutedScope where we default to the option that has better perf by limiting available JSAPI operations (which have safe runtime checks).

Comment 49

[Boris Zbarsky [:bzbarsky]]

> Since you're touching, can you align the '!' with the 'o' in 'options' on the first line?

Done.  Also applied most of our discussion on IRC comments.

Still waiting on the decision on what we want to do with XDR saving of a runOnce script.

Comment 50

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 2 updated to discussion with nbp with the XDR changes

Comment 51

[Luke Wagner [:luke]]

Comment on attachment 8583270 [details] [diff] [review]
Part 2 updated to discussion with nbp with the XDR changes

Review of attachment 8583270 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jsscript.cpp
@@ +607,5 @@
>  
> +        if (!fun && script->treatAsRunOnce()) {
> +            // This is a toplevel or eval script that's runOnce.  We want to
> +            // make sure that we're not XDR-saving an object we emitted for
> +            // JSOP_OBJECT that then gor modified.  So throw if we're not

s/gor/got/

Comment 52

[Boris Zbarsky [:bzbarsky]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/872edd1aedf7
https://hg.mozilla.org/integration/mozilla-inbound/rev/d3793e00b41f
https://hg.mozilla.org/integration/mozilla-inbound/rev/4425a55312c6
https://hg.mozilla.org/integration/mozilla-inbound/rev/ec2d863a3df9
https://hg.mozilla.org/integration/mozilla-inbound/rev/3bc68cf4610b
https://hg.mozilla.org/integration/mozilla-inbound/rev/4cbcb40e0ef4
https://hg.mozilla.org/integration/mozilla-inbound/rev/5336ae0fe919
https://hg.mozilla.org/integration/mozilla-inbound/rev/4a1a45bc093a

Comment 53

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

All backed out for various failures: https://hg.mozilla.org/integration/mozilla-inbound/rev/8c068f0ce341

Comment 54

[Boris Zbarsky [:bzbarsky]]

So this sucks.

These patches are consistently failing an assert in the cgc test run:

Assertion failure: hasScript(), at /builds/slave/try_l64-d_sm-compacting-000000/src/js/src/jsfun.h:449
TEST-UNEXPECTED-FAIL | js/src/jit-test/tests/basic/testLet.js | Assertion failure: hasScript(), at /builds/slave/try_l64-d_sm-compacting-000000/src/js/src/jsfun.h:449 (code -11, args "--ion-eager --ion-offthread-compile=off")
INFO stderr 2> Assertion failure: hasScript(), at /builds/slave/try_l64-d_sm-compacting-000000/src/js

There's no stack, unfortunately, like usual for the JS tests.  I can't reproduce locally.  I also can't reproduce running on a borrowed slave (!).  sfink can't reproduce on his slave.  I tried adding some printfs to at least see which of the mutableScript() callers is responsible, but then the cgc tests pass.

So I really have no idea where to go from here.  Certainly given the amount of time I actually have to spend on this....  :(

Comment 55

[Jan de Mooij [:jandem]]

(In reply to Not doing reviews right now from comment #54)
> I can't
> reproduce locally.  I also can't reproduce running on a borrowed slave (!). 
> sfink can't reproduce on his slave.

I've had similar issues with the SM(r) builds, they can be very fragile. Sometimes the problem is "gone" after rebasing patches on a newer inbound revision; maybe that's worth a try.

If it's possibly related to your patches, you could also request fuzzing or bisect on Try...

Comment 56

[Jan de Mooij [:jandem]]

Attachment: Stack

So this reproduces for me on Linux (I ran testLet.js with jit_test.py --tbpl and it fails with --baseline-eager and --baseline-eager --no-fpu, so different flags than the failures you got).

Anyway here's the stack trace, let me know if you need more and I'd be happy to do some remote debugging on IRC.

Comment 57

[Boris Zbarsky [:bzbarsky]]

Jan, thank you!  That's perfect.

Comment 58

[Boris Zbarsky [:bzbarsky]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fe5301bef40a
https://hg.mozilla.org/integration/mozilla-inbound/rev/6cf1d6c78f97
https://hg.mozilla.org/integration/mozilla-inbound/rev/4bde79bfc439
https://hg.mozilla.org/integration/mozilla-inbound/rev/c538f12b63bd
https://hg.mozilla.org/integration/mozilla-inbound/rev/796283553115
https://hg.mozilla.org/integration/mozilla-inbound/rev/4c7948f507a9
https://hg.mozilla.org/integration/mozilla-inbound/rev/fafbabc6147b
https://hg.mozilla.org/integration/mozilla-inbound/rev/f25e7176b9d6

and now it looks like it has stuck.

Comment 59

[Ryan VanderMeulen [:RyanVM][PTO June 24-28]]

https://hg.mozilla.org/mozilla-central/rev/fe5301bef40a
https://hg.mozilla.org/mozilla-central/rev/6cf1d6c78f97
https://hg.mozilla.org/mozilla-central/rev/4bde79bfc439
https://hg.mozilla.org/mozilla-central/rev/c538f12b63bd
https://hg.mozilla.org/mozilla-central/rev/796283553115
https://hg.mozilla.org/mozilla-central/rev/4c7948f507a9
https://hg.mozilla.org/mozilla-central/rev/fafbabc6147b
https://hg.mozilla.org/mozilla-central/rev/f25e7176b9d6