920322 - Add support for compileAndGo optimizations to XDRScript.

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Nicolas B. Pierron [:nbp]]

From what I understand, compileAndGo optimizations were made to improve the run-time of JavaScript code by baking in constants such as complex literals as objects instead of having the code for all generic cases.

This bug aims at adding support for compileAndGo optimizations into XDRScript such as we can remove the assertion on !compileAndGo.

Note that this bug is one step forward to remove compileAndGo flags (Bug 679939), as it will make XDRScript independent of this flag.

Comment 1

[Luke Wagner [:luke]]

With JSOP_(GET|SET)GLOBAL gone, the main problems I know of are:
 - asm.js-validated functions currently don't support XDR (bug 900669 could be used to embed a serialized asm.js module in an XDR stream)
 - ISTR some issue with self-hosting... maybe that was just that self-hosted code couldn't be serialized, which is orthogonal to compileAndGo

Are there any other issues you are aware of?

Comment 2

[Till Schneidereit [:till]]

(In reply to Luke Wagner [:luke] from comment #1)
>  - ISTR some issue with self-hosting... maybe that was just that self-hosted
> code couldn't be serialized, which is orthogonal to compileAndGo

It was the other way around, actually: we did something with self-hosted functions that would normally have forced them to not be compileAndGo. We made sure that for the specific case of self-hosting, this wasn't necessary, so we added an exception.

Now if I only remembered what that was, exactly. I do know that it's something that doesn't keep us from effectively making all functions compileAndGo and removing the flag.

Comment 3

[Nicolas B. Pierron [:nbp]]

Attachment: Remove compileAndGo assertions.

Comment 4

[Nicolas B. Pierron [:nbp]]

Attachment: Remove hasSingleton assertion.

Comment 5

[Nicolas B. Pierron [:nbp]]

Attachment: Use MutableHandleValue to code constants.

Comment 6

[Nicolas B. Pierron [:nbp]]

Attachment: Support object literals.

Comment 7

[Nicolas B. Pierron [:nbp]]

Attachment: Support array literals.

Comment 8

[Nicolas B. Pierron [:nbp]]

Attachment: Clone singletons & handle recursive objects serialization.

- This patch deep-clone the singletons before they are used in the interpreter.
- This patch also add deep cloning to XDR constants, such as XDR can code objects which have other objects inside them.

Comment 9

[Brian Hackett [Laid off!]]

Comment on attachment 812684 [details] [diff] [review]
Clone singletons & handle recursive objects serialization.

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

::: js/src/jsobj.cpp
@@ +1843,5 @@
> +
> +    RootedObject clone(cx);
> +    if (obj->getClass() ==  &ArrayObject::class_) {
> +        clone = NewDenseAllocatedArray(cx, obj->as<ArrayObject>().length(), NULL, newKind);
> +        if (!CopyArrayElements(cx, obj, &clone))

Object literals can have dense elements too, and this method will not propagate them to the cloned object.  I think that CopyArrayElements should be CopyDenseElements instead, always be called, and copy over all elements in the source object up to its elements' initialized length.

::: js/src/vm/Interpreter.cpp
@@ +2621,5 @@
>  BEGIN_CASE(JSOP_OBJECT)
> +{
> +    RootedObject &ref = rootObject0;
> +    ref = script->getObject(regs.pc);
> +    JSObject *obj = js::DeepCloneObjectLiteral(cx, ref, js::MaybeSingletonObject);

Per our IRC discussion, I think it's an inappropriate deoptimization (both time and memory) to always do this cloning for JSOP_OBJECT.  The only time this deep cloning is needed is if the script might be XDR'ed later, which is an obscure use case that might hold for certain classes of b2g scripts but will e.g. never hold for web content, whether this is b2g or not.  I think you should add a bit to CompileOptions indicating the script might be XDR'ed later and either (a) bans JSOP_OBJECT in the script, or (b) is propagated to the script and checked by the interpreter to see if it should deep clone.

Also, baseline and ion both handle JSOP_OBJECT and any changes to JSOP_OBJECT semantics need to be reflected there.

Comment 10

[Nicolas B. Pierron [:nbp]]

(In reply to Brian Hackett (:bhackett) from comment #9)
> ::: js/src/vm/Interpreter.cpp
> @@ +2621,5 @@
> >  BEGIN_CASE(JSOP_OBJECT)
> > +{
> > +    RootedObject &ref = rootObject0;
> > +    ref = script->getObject(regs.pc);
> > +    JSObject *obj = js::DeepCloneObjectLiteral(cx, ref, js::MaybeSingletonObject);
> 
> Per our IRC discussion, I think it's an inappropriate deoptimization (both
> time and memory) to always do this cloning for JSOP_OBJECT.  The only time
> this deep cloning is needed is if the script might be XDR'ed later, which is
> an obscure use case that might hold for certain classes of b2g scripts but
> will e.g. never hold for web content, whether this is b2g or not. 

As far as I know, this will also hold for the web based on the data that Riadh reported on Bug 813324.
Which from what I understand mean that we want to do this cloning on any scripts that we are running for the first time.

> I think
> you should add a bit to CompileOptions indicating the script might be XDR'ed
> later and either (a) bans JSOP_OBJECT in the script, or (b) is propagated to
> the script and checked by the interpreter to see if it should deep clone.

The problem is that any bytecode modification would be reflected to in the cached bytecode, which means that any deoptimization such as (a) would affect all pages

Using the CompileOptions means that we are either in a state in which we expect saving the bytecode or not.  To me, it sounds like we should always expect to be saving the bytecode as we might want to update/extend the cache entry.

> Also, baseline and ion both handle JSOP_OBJECT and any changes to
> JSOP_OBJECT semantics need to be reflected there.

Thanks, I will.

Comment 11

[Nicolas B. Pierron [:nbp]]

Attachment: XDR/Clone singletons.

Ok, this patch is working on the test suite after saving the bytecode and loading it.

I followed your suggestion about handling elements as part of all objects instead of arrays only, and so I merged the copy of array literals with it.

copyDenseElements is not enough since we also want to keep the inner-objects unchanged.  This was failing on one test case where one property was deleted.

I also add supports of ELEMENTS_HOLE to XDRScriptConst, as this needed to pass the test suite, and as this function is re-used to encode elements of objects.

Comment 12

[Nicolas B. Pierron [:nbp]]

Attachment: Add treatAsRunOnce to XDRScript.

Comment 13

[Nicolas B. Pierron [:nbp]]

Attachment: Save the displayAtom instead of only saving the qualified name.

ATM, we only saved the atom of named functions, but when we had guessed function names, we never stored the corresponding atom, but we were still restoring the guessed atom flags.

This patch stores the atom if there is one.

Without this patch the test suite fails because we expect to guess correctly the name of a function.

Comment 14

[Brian Hackett [Laid off!]]

Comment on attachment 817573 [details] [diff] [review]
XDR/Clone singletons.

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

::: js/src/jsobj.cpp
@@ +1839,5 @@
>  
>  JSObject *
> +js::DeepCloneObjectLiteral(JSContext *cx, HandleObject obj, NewObjectKind newKind)
> +{
> +    /* NB: Keep this in sync with XDRObjectLiteral. */

This should assert that obj is either an array or a plain object.

@@ +1854,5 @@
> +    } else {
> +        AllocKind kind = GetBackgroundAllocKind(GuessObjectGCKind(obj->numFixedSlots()));
> +        Rooted<TypeObject*> typeObj(cx, cx->getNewType(&JSObject::class_, cx->global()->getOrCreateObjectPrototype(cx)));
> +        RootedObject parent(cx, obj->getParent());
> +        clone = NewObjectWithType(cx, typeObj, parent, kind);

The objects created here and above should have the same type as the original object that was passed in.  Otherwise there will be no type information available about the objects and accesses on them will be slower.

@@ +1858,5 @@
> +        clone = NewObjectWithType(cx, typeObj, parent, kind);
> +    }
> +
> +    // Allocate the same number of slots.
> +    clone->ensureElements(cx, obj->getDenseCapacity());

This needs an rval check.

@@ +1869,5 @@
> +    for (uint32_t i = 0; i < initialized; ++i) {
> +        v = obj->getDenseElement(i);
> +        if (v.isObject()) {
> +            deepObj = &v.toObject();
> +            deepObj = js::DeepCloneObjectLiteral(cx, deepObj, newKind);

This needs a null check.

@@ +1880,5 @@
> +    if (clone->is<JSFunction>()) {
> +        JS_ReportErrorNumber(cx, js_GetErrorMessage, NULL,
> +                             JSMSG_CANT_CLONE_OBJECT);
> +        return NULL;
> +    }

This block of code is pointless, clone was created above and is either an array or an object.

@@ +1890,5 @@
> +    for (size_t i = 0; i < span; i++) {
> +        v = obj->getSlot(i);
> +        if (v.isObject()) {
> +            deepObj = &v.toObject();
> +            deepObj = js::DeepCloneObjectLiteral(cx, deepObj, newKind);

This needs a null check.

@@ +1915,5 @@
> +            isArray = obj->getClass() == &ArrayObject::class_ ? 1 : 0;
> +
> +        if (!xdr->codeUint32(&isArray))
> +            return false;
> +    }

This should assert that obj is either an array or a plain object.

@@ +1936,5 @@
> +        {
> +            if (mode == XDR_ENCODE) {
> +                JS_ASSERT(obj->getClass() == &JSObject::class_);
> +                kind = GetBackgroundAllocKind(GuessObjectGCKind(obj->numFixedSlots()));
> +                JS_ASSERT_IF(obj->isTenured(), kind == obj->tenuredGetAllocKind());

The objects embedded in scripts should always be tenured --- IonBuilder will use them, and IonBuilder can't have access to nursery pointers.  You should be able to use tenuredGetAllocKind() directly here.

@@ +1946,5 @@
> +            if (mode == XDR_DECODE) {
> +                RootedObject parent(cx, cx->global());
> +                Rooted<TypeObject*> typeObj(cx);
> +                typeObj = cx->getNewType(&JSObject::class_, cx->global()->getOrCreateObjectPrototype(cx));
> +                obj.set(NewObjectWithType(cx, typeObj, parent, kind));

This can use NewBuiltinClassInstance instead.

@@ +2080,5 @@
> +
> +        JS_ASSERT_IF(mode == XDR_DECODE, !obj->inDictionaryMode());
> +    }
> +
> +    return true;

To ensure that objects decoded this way have good type information, you'll need to call fixArrayType / fixObjectType before returning.

::: js/src/vm/Interpreter.cpp
@@ +2659,5 @@
>  BEGIN_CASE(JSOP_OBJECT)
> +{
> +    RootedObject &ref = rootObject0;
> +    ref = script->getObject(regs.pc);
> +    JSObject *obj = js::DeepCloneObjectLiteral(cx, ref, js::MaybeSingletonObject);

As with the earlier patch, this needs to be gated on some pref which indicates whether the script might be xdr'ed later.  I don't think bug 813324 is relevant to anything here --- it hasn't landed and hasn't even seen activity in nearly a year.

Comment 15

[Nicolas B. Pierron [:nbp]]

Attachment: XDR/Clone singletons.

Delta:
 - Apply nits.
 - Rebase: remove last argument of DefineNativeProperty, and accessors.
 - Replace NewObjectWithType (as it does not seems to work after a rebase) by NewObjectWithGivenProto, and ensure that the Fix-ed type is identical.

Comment 16

[Nicolas B. Pierron [:nbp]]

Try: https://tbpl.mozilla.org/?tree=Try&showall=1&rev=880434d6f5d6

https://hg.mozilla.org/integration/mozilla-inbound/rev/7294b1ba5056
https://hg.mozilla.org/integration/mozilla-inbound/rev/bfa229337eb2
https://hg.mozilla.org/integration/mozilla-inbound/rev/90f44c93d249
https://hg.mozilla.org/integration/mozilla-inbound/rev/231c4f316322
https://hg.mozilla.org/integration/mozilla-inbound/rev/78dbc3afea91

Add [leave-open] for the remaining patch.

Comment 17

[Brian Hackett [Laid off!]]

Comment on attachment 8350131 [details] [diff] [review]
XDR/Clone singletons.

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

::: js/src/vm/Interpreter.cpp
@@ +2767,5 @@
>  CASE(JSOP_OBJECT)
> +{
> +    RootedObject &ref = rootObject0;
> +    ref = script->getObject(REGS.pc);
> +    JSObject *obj = js::DeepCloneObjectLiteral(cx, ref, js::MaybeSingletonObject);

This still needs to be gated on some pref which indicates whether the script might be xdr'ed later, or you need to do some measurements of runtime and memory usage to convince me this unnecessary cloning doesn't matter much for scripts with large literals.

Comment 18

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

https://hg.mozilla.org/mozilla-central/rev/7294b1ba5056
https://hg.mozilla.org/mozilla-central/rev/bfa229337eb2
https://hg.mozilla.org/mozilla-central/rev/90f44c93d249
https://hg.mozilla.org/mozilla-central/rev/231c4f316322
https://hg.mozilla.org/mozilla-central/rev/78dbc3afea91

Comment 19

[Nicolas B. Pierron [:nbp]]

Attachment: XDR/Clone singletons.

Delta:
 - Add opt-in flag on the ContextOptions to enable saving/cloning object literals.

Comment 20

[Nicolas B. Pierron [:nbp]]

Attachment: XDR/Clone singletons.

Delta:
 - Move from ContextOptions to CompartmentOptions, at the end we want to do these optimizations as a per-compartment basis and not a per-runtime basis.
 - Use CompartmentOptionsRef instead of cx->….options().

Comment 21

[Nicolas B. Pierron [:nbp]]

Attachment: XDR/Clone singletons.

(This time with the updated patch.)

Comment 22

[Brian Hackett [Laid off!]]

Comment on attachment 8350647 [details] [diff] [review]
XDR/Clone singletons.

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

It still looks like baseline and ion are out of sync with the interpreter's behavior on JSOP_OBJECT.

::: js/src/jsapi.h
@@ +2635,5 @@
> +#ifdef DEBUG
> +        cloneSingletonsLocked_ = true;
> +#endif
> +        return cloneSingletons();
> +    }

This lockedCloneSingletons stuff doesn't belong in the API, there isn't any API client who would reasonably want to call this method.  Instead you can add a bit to JSCompartment like |allSingletonsCloned| or something.  That bit starts out true, is asserted during XDR and is set to false if the VM pushes an object directly in JSOP_OBJECT.

@@ +2641,5 @@
> +        JS_ASSERT(!cloneSingletonsLocked_);
> +        cloneSingletons_ = flag;
> +        return *this;
> +    }
> +    CompartmentOptions &toggleCloneSingletons() {

This method should be removed, the other bits in CompartmentOptions don't have toggle* methods and I don't think there is any way to use this method that isn't unnecessarily obfuscatory.

@@ +2666,5 @@
> +    // assertion will be raised.  This is an opt-in feature.
> +#ifdef DEBUG
> +    bool cloneSingletonsLocked_ : 1;
> +#endif
> +    bool cloneSingletons_ : 1;

Can you use Override for cloneSingletons_, like the other fields in CompartmentOptions?

Comment 23

[Nicolas B. Pierron [:nbp]]

Attachment: 0003-Bug-900789-XDR-Clone-singletons.-r.patch

Delta:
 - Modify flag, such as it is on the JSContext with its default value and overwritten by the compartment basis.  I check the compartment as the cache would be based on the origin, and not on the JSContext*.

 - Add baseline & ion handling of the flag, as well as a call to DeepCloneObjectLiteral.

Note that even if IonBuilder use setSingletonsAsValues, which reset the flag to false, this is still thread safe as the modification of this flag can only be toggled to one state.  Which means that even if there is a write race, they would race for the same value.

Comment 24

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8356106 [details] [diff] [review]
0003-Bug-900789-XDR-Clone-singletons.-r.patch

Changing reviewers from bhackett to jandem, due to vacation period, and that the remaining bits are related to the JITs.

Comment 25

[Jan de Mooij [:jandem]]

Comment on attachment 8356106 [details] [diff] [review]
0003-Bug-900789-XDR-Clone-singletons.-r.patch

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

Looks good but a few things I want to understand better before I r+.

::: js/src/jit/CompileWrappers.cpp
@@ +232,5 @@
>  }
>  
> +void
> +CompileCompartment::setSingletonsAsValues()
> +{

Nit: add a comment here explaining why this is ok.

@@ +257,5 @@
> +    cloneSingletons_ = options.cloneSingletons(cx);
> +}
> +
> +bool
> +JitCompileOptions::areOptionsUnchanged(JSContext *cx) const

Should we call this method somewhere?

::: js/src/jit/MIR.cpp
@@ +539,5 @@
>      return true;
>  }
>  
> +MCloneLiteral *
> +MCloneLiteral::New(TempAllocator &alloc, MDefinition *obj, types::CompilerConstraintList *constraints)

Nit: remove the |constraints| argument, it's always nullptr IIUC.

::: js/src/jit/MIRGenerator.h
@@ +165,5 @@
>      AsmJSPerfSpewer &perfSpewer() { return asmJSPerfSpewer_; }
>  #endif
> +
> +  public:
> +    const JitCompileOptions options;

const JitCompileOptions &options, to avoid a copy if this struct gets bigger.

::: js/src/jsapi.h
@@ +2644,5 @@
>      bool asmJS(JSContext *cx) const;
>      Override &asmJSOverride() { return asmJSOverride_; }
>  
> +    bool cloneSingletons(JSContext *cx) const;
> +    Override &cloneSingletons() { return cloneSingletonsOverride_; }

Nit: for consistency with the other overrides, this should be named cloneSingletonsOverride()

@@ +2656,5 @@
>      CompartmentOptions &setSameZoneAs(JSObject *obj);
>  
> +    void setSingletonsAsValues() {
> +#ifdef DEBUG
> +        singletonsAsTemplates_ = false;

Why is this #ifdef DEBUG? If it's only used in debug builds, I think it's clearer to define singletonsAsTemplates_ only in debug builds.

::: js/src/jsobj.cpp
@@ +1852,5 @@
> +    }
> +
> +    // Allocate the same number of slots.
> +    if (!clone || !clone->ensureElements(cx, obj->getDenseCapacity())) {
> +        JS_ReportOutOfMemory(cx);

Remove this call; the callees are responsible for calling it.

@@ +1869,5 @@
> +            deepObj = &v.toObject();
> +            deepObj = js::DeepCloneObjectLiteral(cx, deepObj, newKind);
> +            if (!deepObj) {
> +                JS_ReportOutOfMemory(cx);
> +                return nullptr;

Same here.

@@ +1887,5 @@
> +        if (v.isObject()) {
> +            deepObj = &v.toObject();
> +            deepObj = js::DeepCloneObjectLiteral(cx, deepObj, newKind);
> +            if (!deepObj) {
> +                JS_ReportOutOfMemory(cx);

And here.

@@ +1902,5 @@
> +        FixObjectType(cx, clone);
> +
> +#ifdef DEBUG
> +    Rooted<TypeObject*> typeObj(cx, obj->getType(cx));
> +    JS_ASSERT(typeObj == clone->getType(cx));

Nit: check getType() rval.

::: js/src/jsscript.cpp
@@ +843,4 @@
>                  return false;
>              *objp = tmp;
> +            break;
> +          }

Nit: add a "default: MOZ_ASSUME_UNREACHABLE(..)" to this switch. If we decode some invalid/corrupt value we should fail in debug builds.

Comment 26

[Nicolas B. Pierron [:nbp]]

(In reply to Jan de Mooij [:jandem] from comment #25)
> Comment on attachment 8356106 [details] [diff] [review]
> 0003-Bug-900789-XDR-Clone-singletons.-r.patch
> 
> Review of attachment 8356106 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good but a few things I want to understand better before I r+.
> 
> ::: js/src/jit/CompileWrappers.cpp
> @@ +232,5 @@
> >  }
> >  
> > +void
> > +CompileCompartment::setSingletonsAsValues()
> > +{
> 
> Nit: add a comment here explaining why this is ok.

This is ok, because even if we mutate the value of the compartment, this is a boolean which can only be set to false, and even if there is a race, they will always set the same value.  So the write race is not important, as both will write the same thing.

> @@ +257,5 @@
> > +    cloneSingletons_ = options.cloneSingletons(cx);
> > +}
> > +
> > +bool
> > +JitCompileOptions::areOptionsUnchanged(JSContext *cx) const
> 
> Should we call this method somewhere?

Ideally we should invalidate all code which depend on changing options.  In practice for JSOP_OBJECT, we would invalidate all the code to prevent cloning an object once, which is not ideal, and the JSOP_OBJECT is on a path which has not yet been executed (if it is going to be executed), and thus likely to bailout.

> ::: js/src/jit/MIR.cpp
> @@ +539,5 @@
> >      return true;
> >  }
> >  
> > +MCloneLiteral *
> > +MCloneLiteral::New(TempAllocator &alloc, MDefinition *obj, types::CompilerConstraintList *constraints)
> 
> Nit: remove the |constraints| argument, it's always nullptr IIUC.

Ok, I thought this was the new style.

> ::: js/src/jit/MIRGenerator.h
> @@ +165,5 @@
> >      AsmJSPerfSpewer &perfSpewer() { return asmJSPerfSpewer_; }
> >  #endif
> > +
> > +  public:
> > +    const JitCompileOptions options;
> 
> const JitCompileOptions &options, to avoid a copy if this struct gets bigger.

Sadly no, because the MIRGenerator is used by 2 threads and the JitCompileOptions refers to a class initialize on the stack of the thread which spawned the compilation, so we want to make a copy here.

> @@ +2656,5 @@
> >      CompartmentOptions &setSameZoneAs(JSObject *obj);
> >  
> > +    void setSingletonsAsValues() {
> > +#ifdef DEBUG
> > +        singletonsAsTemplates_ = false;
> 
> Why is this #ifdef DEBUG? If it's only used in debug builds, I think it's
> clearer to define singletonsAsTemplates_ only in debug builds.

This should not be debug-only, good catch.

> ::: js/src/jsscript.cpp
> @@ +843,4 @@
> >                  return false;
> >              *objp = tmp;
> > +            break;
> > +          }
> 
> Nit: add a "default: MOZ_ASSUME_UNREACHABLE(..)" to this switch. If we
> decode some invalid/corrupt value we should fail in debug builds.

Good catch, currently I do not plan to handle any corrupted input, but as we are going to add more XDR usage, I am planning to add “return abort("message")” on all XDR functions instead of the return false and ASSUME_UNREACHABLE.

Comment 27

[Nicolas B. Pierron [:nbp]]

Attachment: XDR/Clone singletons.

Delta:
 - Apply nits
 - Remove "ifdef DEBUG" around the template flag, as we need the flag to report error messages while testing in the JS shell.

Comment 28

[Jan de Mooij [:jandem]]

Comment on attachment 8359274 [details] [diff] [review]
XDR/Clone singletons.

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

Sorry for the delay.

::: js/src/jit/CompileWrappers.cpp
@@ +230,5 @@
>  {
>      return compartment()->hasObjectMetadataCallback();
>  }
>  
> +// Note: This function is thread-safe because setSingletonAsValue set a boolean

Nit: s/set/sets

@@ +236,5 @@
> +// true. So even if there is a concurrent write, this concurrent write will
> +// always have the same value.  If there is a concurrent read, then we will
> +// clone a singleton instead of using the value which is bake in the JSScript,
> +// and this would be an unfortunate allocation, but this will not change the
> +// semantic of the JavaScript code which is executed.

Nit: s/bake/baked, s/semantic/semantics

::: js/src/jsobj.cpp
@@ +1844,5 @@
> +    } else {
> +        // Object literals are tenured by default as holded by the JSScript.
> +        JS_ASSERT(obj->isTenured());
> +        AllocKind kind = obj->tenuredGetAllocKind();
> +        Rooted<TypeObject*> typeObj(cx, obj->getType(cx));

if (!typeObj) return nullptr; Or else decoder's OOM fuzzer will come after you.

Comment 29

[Nicolas B. Pierron [:nbp]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d6ff1c03c8e2

Comment 30

[Nicolas B. Pierron [:nbp]]

(In reply to Nicolas B. Pierron [:nbp] from comment #29)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/d6ff1c03c8e2

This was the corresponding try push: https://tbpl.mozilla.org/?tree=Try&rev=934155a15566

Comment 31

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d6ff1c03c8e2