This bug is about adding an API to the Debugger to expose an implementation of Code Coverage.

The current patches are prototypes of what a Debugger code coverage API might be.

Jordan and Mike, you might want to have a look at this.

WIP devtools patch using the new debugger interface. No UI yet, but hitcounts are dumped as logs.

(Or maybe not. I'll try finishing up my patch to add a code heatmap feature to the debugger panel.)

Useless comment but I just want to tell you guys I'm SUPER excited about this. We would like to use the logs dumped by this for Gaia tests! :D I'm adding my meta bug for the tooling for JSMarionette, Taskcluster and Treeherder as a dependent of this work. :)

James, I'll be gone for a month, do you mind taking this WIP patch over? It just misses UI integration (hitcounts for each line are being dumped to the console), and many people are excited about this.

(see previous comment) Feel free to steal my patch. Simply apply the 4 other patches from Nicolas, and my patch should give you hitcounts. I was planning to use these hitcounts to set different background colors in CodeMirror's lines (e.g. 0 hitcounts is transparent, a few hitcounts is light green, and up all the way to red for the hottest lines).

(In reply to Jan Keromnes [:janx] (back on 10 August) from comment #11) > (see previous comment) > > Feel free to steal my patch. Simply apply the 4 other patches from Nicolas, > and my patch should give you hitcounts. > > I was planning to use these hitcounts to set different background colors in > CodeMirror's lines (e.g. 0 hitcounts is transparent, a few hitcounts is > light green, and up all the way to red for the hottest lines). Oh, didn't realize you were going to be gone for a bit. Eddy is also gone and I'm already taking over his service worker patches, which a lot of people are also excited about (I would expect more people), so I'm not sure I'll be able to take this on. I will look at this code and review it, not sure I'll have time to push it through though. We'll see.

Nicolas, are you planning on getting your patches reviewed by somebody?

The current set of patches was just a quick proof of concept of a Debugger API. We planned on implementing Code Coverage "the proper way" in Q3/Q4, at least for RelMngt, so I will ask shu for feedback on the current Debugger API.

Comment on attachment 8625481 [details] [diff] [review] Add Debugger.Script.getOffsetsCoverage. r= This patch add a Code Coverage API to the Debugger. Do you think this is the right way to expose code coverage?

Comment on attachment 8625482 [details] [diff] [review] Add a flag on the Debugger & Compartment to record code-coverage information. r= Review of attachment 8625482 [details] [diff] [review]: ----------------------------------------------------------------- Same thing, for a potential flag which might have to be turned ON on the Debugger, if the code coverage overhead is important.

(In reply to Nicolas B. Pierron [:nbp] from comment #14) > The current set of patches was just a quick proof of concept of a Debugger > API. > > We planned on implementing Code Coverage "the proper way" in Q3/Q4, at least > for RelMngt, so I will ask shu for feedback on the current Debugger API. Ah cool, that makes more sense. I can probably finish up Jan's patch then, even if it's not ready for produciton.

Comment on attachment 8625481 [details] [diff] [review] Add Debugger.Script.getOffsetsCoverage. r= Review of attachment 8625481 [details] [diff] [review]: ----------------------------------------------------------------- If I'm reading right, the format of the array returned is an array of arrays of coverage counts. I suppose you intended this array-of-arrays to be correlated with the array-of-offsets from getLineOffsets. Those are entrypoints only, and as noted below, I think coverage data should have every pc. Should the array returned be array of arrays of (line, coverage) pairs? ::: js/src/vm/Debugger.cpp @@ +5370,5 @@ > + size_t lineno = r.frontLineNumber(); > + > + // Make a note, if the current instruction is an entry point for the > + // current line. > + if (!flowData[offset].hasNoEdges() && flowData[offset].lineno() != lineno) { For PCCounts, we want counts for every offset on a line, not just the entry points. IIRC, "entry point" is defined as any pc that's reachable from another pc on a different line.

Comment on attachment 8625482 [details] [diff] [review] Add a flag on the Debugger & Compartment to record code-coverage information. r= Review of attachment 8625482 [details] [diff] [review]: ----------------------------------------------------------------- Two main things: 1. You should update the observesCoverage on the debuggee compartments when toggling enabled. (Yes, .enabled is a horrible API, but until we remove it...) 2. You should ensure on-stack interpreter frames have interrupts toggled when turning on coverage. See InterpreterActivation::enableInterruptsIfRunning. ::: js/src/jscompartment.cpp @@ +846,5 @@ > for (Debugger * const* p = v->begin(); p != v->end(); p++) { > Debugger* dbg = *p; > if (flag == DebuggerObservesAllExecution > ? dbg->observesAllExecution() > : dbg->observesAsmJS()) You should be checking dbg->observesCoverage() here. ::: js/src/jscompartment.h @@ +361,5 @@ > enum { > IsDebuggee = 1 << 0, > DebuggerObservesAllExecution = 1 << 1, > DebuggerObservesAsmJS = 1 << 2, > + DebuggerCollectCoverageInfo = 1 << 3, Maybe DebuggerObservesCoverage for consistency?

(In reply to Nicolas B. Pierron [:nbp] from comment #15) > Comment on attachment 8625481 [details] [diff] [review] > Add Debugger.Script.getOffsetsCoverage. r= > > This patch add a Code Coverage API to the Debugger. > Do you think this is the right way to expose code coverage? I think exposing a Debugger.Script.prototype.getOffsetsCoverage is fine. I gave the particular format feedback in the comment above. What's the plan for pc counts for baseline? You think the debug mode OSR can handle recompiling for pc counts?

(In reply to Shu-yu Guo [:shu] from comment #20) > What's the plan for pc counts for baseline? You think the debug mode OSR can > handle recompiling for pc counts? Currently attachment 8625483 [details] [diff] [review] updates PC counts in baseline. Do we already invalidate Baseline frames? If so then I do not see much of an issue. Ultimately, the code coverage should have a minimal/no overhead, and we could have it enabled, just to remove dead blocks in Ion. (In reply to Shu-yu Guo [:shu] from comment #18) > Review of attachment 8625481 [details] [diff] [review]: > ----------------------------------------------------------------- > > If I'm reading right, the format of the array returned is an array of arrays > of coverage counts. > > I suppose you intended this array-of-arrays to be correlated with the > array-of-offsets from getLineOffsets. Those are entrypoints only, and as > noted below, I think coverage data should have every pc. > > Should the array returned be array of arrays of (line, coverage) pairs? Oh, right! I got confused by getAllOffsets example which only show entry points, and no blocks with multiples lines.

(In reply to Nicolas B. Pierron [:nbp] from comment #21) > (In reply to Shu-yu Guo [:shu] from comment #20) > > What's the plan for pc counts for baseline? You think the debug mode OSR can > > handle recompiling for pc counts? > > Currently attachment 8625483 [details] [diff] [review] updates PC counts in > baseline. Do we already invalidate Baseline frames? If so then I do not > see much of an issue. We do on-stack recompilation of baseline frames explicitly in the debugger, when setting a hook that requires the script to be observed. See calls to |ensureExecutionObservabilityOfScript|. You'd need to call that. Currently, it caches whether a JSScript has already been made observable via JSScript::isDebuggee. You would need to change that predicate to also consider pc counts as putting it in debug mode or something.

This part add a flag on the debugger and a function on the compartment to check if any profiling is enabled at the moment. This also use the update method to interrupt all interpreter activations, and reclaim memory. This patch still lacks the suggestion made in comment 22, but this is enough to start experimenting on newly created compartment.

This part add the Debugger.Script.getOffsetsCoverage function, which returns an array of { offset, lineNumber, columnNumber, count } objects. This array should be easier to process in the Debugger, than handling arrays of arrays. This patch lacks documentation, but the information collected by this patch is should be correct, as examplified in the LCov test case re-converted into a Debugger test case.

I'm not working on this currently, Nicolas want to take over?

(In reply to James Long (:jlongster) from comment #26) > I'm not working on this currently, Nicolas want to take over? I am working on the JS engine part, I have little to no idea how to integrate it in the Debugger except form what Jan made already. Also, if you don't want some poor user-facing design choices, it is wise to not let me touch to the interface ;)

Can someone clarify for me why this bug would have any UI? It's for an API, right? So either we add code coverage to our devtools or third party code coverage tools can be attached to the browser. Right now some of the third party tools that do thios add code tracking instrumentation into the code. If we do add code coverage, there is also the performance consideration, isn't it going to slow things down? If someone can clarify, that would be great. Thanks.

Axel, the performance impact of using :nbp's interface should be negligible (since it's just a callback incrementing a counter somewhere). In addition to :nbp's script API to request the detailed code coverage of a JS file, we'd also like to add a visual indicator in the DevTools' Debugger panel (e.g. painting the lines that are called the most often in red, or something like that). I've attached an unfinished patch aiming to do just that ("Line-based code heatmap in the debugger tool"), but we'll have to wait for :nbp's patches to land first. Also, we might open a separate bug for the devtools change, because it has nothing to do in Core > JavaScript Engine.

(In reply to Axel Kratel from comment #28) > Can someone clarify for me why this bug would have any UI? It's for an API, > right? This bug was open right before Whistler, to first implement a prototype for the devtools during whistler. > So either we add code coverage to our devtools or third party code > coverage tools can be attached to the browser. Right now some of the third > party tools that do thios add code tracking instrumentation into the code. We had a discussion about this topic at Whistler[1]. Devtools people were invited, but apparently the distance between the conference center and the Hilton convinced most of them to not come to this meeting. To summarize, the JS engine implements features which are not supported by external libraries, and we cannot use these for .jsm files. We already have some instrumentation, and we are more likely to have less overhead and more precision than any library. > If we do add code coverage, there is also the performance consideration, > isn't it going to slow things down? Maybe, that's my goal to minimize that, and hopefully make it transparent enough. [1] https://etherpad.mozilla.org/JS-Code-Coverage-meeting-notes

(In reply to Nicolas B. Pierron [:nbp] from comment #30) > > If we do add code coverage, there is also the performance consideration, > > isn't it going to slow things down? > > Maybe, that's my goal to minimize that, and hopefully make it transparent > enough. > The code coverage is not running all the time when the debugger is open, right? It has to be enabled, and only then will you see a perf hit. It's *super* important that this isn't running all the time, because we are about to initialize the debugger whenever any tool is open. Right now we only initialize it when the debugger tab is clicked. Also, fwiw, it would be good to file a new bug just for the UI.

(In reply to James Long (:jlongster) from comment #31) > (In reply to Nicolas B. Pierron [:nbp] from comment #30) > > > If we do add code coverage, there is also the performance consideration, > > > isn't it going to slow things down? > > > > Maybe, that's my goal to minimize that, and hopefully make it transparent > > enough. > > > > The code coverage is not running all the time when the debugger is open, > right? It has to be enabled, and only then will you see a perf hit. So far, this is correct. If the performance hit is neglectable, I will see if we can enable it by default, in order to use it for instrumenting the Jit. > It's *super* important that this isn't running all the time, because we are > about to initialize the debugger whenever any tool is open. Right now we > only initialize it when the debugger tab is clicked. I think both use cases are desirable, and If the coverage is enabled all the time, then this is just a matter of removing the initial results of the code coverage, right? > Also, fwiw, it would be good to file a new bug just for the UI. Jan opened Bug 1200154 for that.

(In reply to Nicolas B. Pierron [:nbp] from comment #32) > > > It's *super* important that this isn't running all the time, because we are > > about to initialize the debugger whenever any tool is open. Right now we > > only initialize it when the debugger tab is clicked. > > I think both use cases are desirable, and If the coverage is enabled all the > time, then this is just a matter of removing the initial results of the code > coverage, right? The majority of the use cases for code coverage are fine with just enabling it, like "recording" in the perf tools. I don't what you mean by "removing the initial results". If it's enabled, then it incurs a perf hit no matter what. We have optimized the Debugger instance so we can unconditionally create it whenever the whole toolbox is open, even if the debugger tab isn't clicked, and even high-perf games won't see a perf hit. We want to do as little as possible when the Debugger is connected.

This patch should now recompile baseline scripts when the flag is toggled.

Comment on attachment 8656530 [details] [diff] [review] part 1 - Add a flag on the Debugger & Compartment to record code-coverage information. Review of attachment 8656530 [details] [diff] [review]: ----------------------------------------------------------------- Nice job moving code coverage to be a per-compartment concept. I have one concern, I see in Baseline, if the JSScript hasScriptCounts(), then we compile in code coverage stuff. In the case that we are enabling code coverage via the Debugger with live Ion and Baseline scripts on the stack that do not yet have a scriptCountsMap entry, when will they get it? They won't be resuming into the Interpreter. This situation seems analogous to me as setting the isDebuggee flag on frames. In your case, we need to set it on the scripts of the on-stack JIT frames. If this isn't a real issue, please explain and re-request r? ::: js/src/jscompartment.cpp @@ +995,5 @@ > + ScriptCounts* value = &r.front().value(); > + r.front().key()->takeOverScriptCountsMapEntry(value); > + } > + > + js_delete(scriptCountsMap); Side request: could scriptCountsMap be made a UniquePtr while you're here? ::: js/src/jscompartment.h @@ +665,5 @@ > + // The code coverage can be enabled either for each compartment, with the > + // Debugger API, or for the entire runtime. > + bool collectCoverage() const { > + return debuggerObservesCoverage() > + || runtimeFromAnyThread()->profilingScripts; nit: line up || with 'debuggerObservesCoverage' ::: js/src/jsscript.cpp @@ +1450,5 @@ > return getScriptCounts().ionCounts_; > } > > void > +JSScript::takeOverScriptCountsMapEntry(ScriptCounts* entryValue) I don't understand the name of this method. What's being taken over? Maybe |releaseScriptCountsMapEntry|?

(In reply to Shu-yu Guo [:shu] from comment #35) > Comment on attachment 8656530 [details] [diff] [review] > part 1 - Add a flag on the Debugger & Compartment to record code-coverage > information. > > Review of attachment 8656530 [details] [diff] [review]: > ----------------------------------------------------------------- > > I have one concern, I see in Baseline, if the JSScript hasScriptCounts(), > then we compile in code coverage stuff. In the case that we are enabling > code coverage via the Debugger with live Ion and Baseline scripts on the > stack that do not yet have a scriptCountsMap entry, when will they get it? > They won't be resuming into the Interpreter. This situation seems analogous > to me as setting the isDebuggee flag on frames. In your case, we need to set > it on the scripts of the on-stack JIT frames. > > If this isn't a real issue, please explain and re-request r? I assumed that the current code was invalidating baseline compilation in order to recompile it, and thus hit the Interpreter interrupt case, which is registering a ScriptCounts for the current running script. I will add a test to double check this hypothesis, If I am wrong I will add a check when we compile Baseline scripts to register the ScriptCounts on the compartment. > ::: js/src/jscompartment.cpp > @@ +995,5 @@ > > + ScriptCounts* value = &r.front().value(); > > + r.front().key()->takeOverScriptCountsMapEntry(value); > > + } > > + > > + js_delete(scriptCountsMap); > > Side request: could scriptCountsMap be made a UniquePtr while you're here? I will do that in a follow-up patch. > ::: js/src/jsscript.cpp > @@ +1450,5 @@ > > return getScriptCounts().ionCounts_; > > } > > > > void > > +JSScript::takeOverScriptCountsMapEntry(ScriptCounts* entryValue) > > I don't understand the name of this method. What's being taken over? Maybe > |releaseScriptCountsMapEntry|? I wanted to name this function to mention that we take the ownership of the ScriptCount entry of the current script. The current methods only gives us the ability to take the ownership of one ScriptCount at a time, which involves moving vectors and removing the current entry from the HashMap. This methods name is used to highlight that the ownership of the entry belongs to the caller, and no longer to the JSScript. Thus we can just unset the hasScriptCounts_ flag and delete the entire HashMap, without removing each entry from the HashMap. I wanted to avoid naming it |resetHasScriptCounts| as this name will lack the notion of ownership of the ScriptCounts entry, and thus implies leaking the entry without a known owner. Which is also the reason of the entryValue argument which is only use in Debug mode.

Comment on attachment 8654150 [details] [diff] [review] part 2 - Add Debugger.Script.getOffsetsCoverage. Review of attachment 8654150 [details] [diff] [review]: ----------------------------------------------------------------- I will make a part 3 to add the documentation.

Delta: - Prevent switching the collectCoverageInfo flag if there is any frame on the stack. - Instrument baseline compilation if we compile with Baseline without checking in interpreter. - Change Debugger::updateObservesCoverageOnDebuggees to only toggle each compartment once all scripts are scheduled for a recompilation. Otherwise this might have introduced some use-after-free with the pointers baked in baseline compilations. - Add test case to check that we can enable/disable the code coverage. (rely on part 2) - Add test cases to check that we do raise an exception as expected when we switch the flag with frames on the stack.

Comment on attachment 8657280 [details] [diff] [review] part 1 - Add a flag on the Debugger & Compartment to record code-coverage information. Review of attachment 8657280 [details] [diff] [review]: ----------------------------------------------------------------- Looks fine to me. Please double check that we don't unnecessarily throw DEBUG_NOT_IDLE when toggling Debugger.enabled. ::: js/src/jit/BaselineCompiler.cpp @@ +91,5 @@ > if (!script->ensureHasTypes(cx) || !script->ensureHasAnalyzedArgsUsage(cx)) > return Method_Error; > > + // When a Debugger set the collectCoverageInfo flag, we recompile baseline > + // scripts without entring the interpreter again. We have to create the typo: entring @@ +92,5 @@ > return Method_Error; > > + // When a Debugger set the collectCoverageInfo flag, we recompile baseline > + // scripts without entring the interpreter again. We have to create the > + // ScriptCounts if they do not exists. grammar: if they do not exist @@ +94,5 @@ > + // When a Debugger set the collectCoverageInfo flag, we recompile baseline > + // scripts without entring the interpreter again. We have to create the > + // ScriptCounts if they do not exists. > + if (!script->hasScriptCounts() && cx->compartment()->collectCoverage()) > + script->initScriptCounts(cx); Hm, I wonder if this is all that was needed to have the toggling work even with live frames. Try it out, if not, then keep the restriction. If so, then great, you can remove the restriction. ::: js/src/jscompartment.h @@ +665,5 @@ > + // The code coverage can be enabled either for each compartment, with the > + // Debugger API, or for the entire runtime. > + bool collectCoverage() const { > + return debuggerObservesCoverage() > + || runtimeFromAnyThread()->profilingScripts; nit: || on first line ::: js/src/vm/Debugger.cpp @@ +2262,5 @@ > + > + if (!updateExecutionObservability(cx, obs, observing)) > + return false; > + > + // all compartments can safely be toggled, and all script will be nit: capitalize All nit: all scripts @@ +2755,5 @@ > > + // Ensure that any mode of execution is instrumented to increment the > + // coverage counters. > + if (!dbg->updateObservesCoverageOnDebuggees(cx, dbg->observesCoverage())) > + return false; Would calling this not incorrectly report DEBUG_NOT_IDLE if there are frames on the stack needing to be marked as isDebuggee, even observesCoverage() is false?

Comment on attachment 8654150 [details] [diff] [review] part 2 - Add Debugger.Script.getOffsetsCoverage. Review of attachment 8654150 [details] [diff] [review]: ----------------------------------------------------------------- r=me with explanation of the test comment syntax, preferably as a block comment in the test itself or something. ::: js/src/jit-test/tests/debug/Script-getOffsetsCoverage-01.js @@ +9,5 @@ > +if (getJitCompilerOptions()["baseline.warmup.trigger"] != 10) > + setJitCompilerOption("baseline.warmup.trigger", 10); > + > +function checkGetOffsetsCoverage(fun) { > + var keys = [ "TN", "SF", "FN", "FNDA", "FNF", "FNH", "BRDA", "BRF", "BRH", "DA", "LF", "LH" ]; What on earth do these keys mean? Could you document the syntax you use in the comments? I'm not really making sense of them. @@ +23,5 @@ > + var source = fun.toSource(); > + source = source.slice(source.indexOf('{') + 1, source.lastIndexOf('}')); > + > + // Extract comment starting with the previous keys, as a reference of the > + // output expected from getLcovInfo. Outdated reference to getLcovInfo ::: js/src/vm/Debugger.cpp @@ +5494,5 @@ > + } > + > + ScriptCounts* sc = &script->getScriptCounts(); > + > + // If the main ever got visited, then assume that anycode before main got nit: any code @@ +5521,5 @@ > + RootedValue lineNumberValue(cx); > + RootedValue columnNumberValue(cx); > + RootedValue countValue(cx); > + > + // Iterate lineraly over the bytecode. typo: linearly

Comment on attachment 8658169 [details] [diff] [review] part 3 - Debugger.Script.getOffsetsCoverage: Add documentation. Review of attachment 8658169 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/doc/Debugger/Debugger.md @@ +38,5 @@ > step debugging a target JavaScript program. > > +`collectCoverageInfo` > +: A boolean value indicating whether code coverage should be enabled inside > + each debuggee of this `Debugger` instance. Changing this flag value, will nit: no need for comma @@ +39,5 @@ > > +`collectCoverageInfo` > +: A boolean value indicating whether code coverage should be enabled inside > + each debuggee of this `Debugger` instance. Changing this flag value, will > + recompile all jitted scripts to add or remove code coverage nit: jitted scripts -> JIT code @@ +41,5 @@ > +: A boolean value indicating whether code coverage should be enabled inside > + each debuggee of this `Debugger` instance. Changing this flag value, will > + recompile all jitted scripts to add or remove code coverage > + instrumentation. Changing this flag when any frame of the debuggee is > + currently active on the stack, will produce an exception. nit: no need for comma @@ +50,5 @@ > + pre-date the modification of this flag. Code coverage reports are monotone, > + thus one can take a snapshot when the Debugger is enabled, and output the > + difference. > + > + Setting this to `false` prevents this `Debugger` instance to require any grammar: prevents this `Debugger` instance from requiring any @@ +51,5 @@ > + thus one can take a snapshot when the Debugger is enabled, and output the > + difference. > + > + Setting this to `false` prevents this `Debugger` instance to require any > + code coverage instrumentation, but it does not garantee that the typo: guarantee

(In reply to Shu-yu Guo [:shu] from comment #40) > Comment on attachment 8657280 [details] [diff] [review] > part 1 - Add a flag on the Debugger & Compartment to record code-coverage > information. > > > Looks fine to me. Please double check that we don't unnecessarily throw > DEBUG_NOT_IDLE when toggling Debugger.enabled. > > @@ +2755,5 @@ > > > > + // Ensure that any mode of execution is instrumented to increment the > > + // coverage counters. > > + if (!dbg->updateObservesCoverageOnDebuggees(cx, dbg->observesCoverage())) > > + return false; > > Would calling this not incorrectly report DEBUG_NOT_IDLE if there are frames > on the stack needing to be marked as isDebuggee, even observesCoverage() is > false? Indeed, I made a test case that I will push with this patch, which toggle the enabled flag under the interruption callback. To work-around this issue, I removed this line, and removed the isDebuggee flag from JSCompartment::debuggerObservesCoverage mask, such that code coverage is enabled as soon as any Debugger turns it on, even if the Debugger is not it-self enabled. I guess this is fine, as the default value of collectCoverageInfo flag is "false" by default.

(In reply to Shu-yu Guo [:shu] from comment #41) > Comment on attachment 8654150 [details] [diff] [review] > part 2 - Add Debugger.Script.getOffsetsCoverage. > > Review of attachment 8654150 [details] [diff] [review]: > ----------------------------------------------------------------- > > r=me with explanation of the test comment syntax, preferably as a block > comment in the test itself or something. > > ::: js/src/jit-test/tests/debug/Script-getOffsetsCoverage-01.js > @@ +9,5 @@ > > +if (getJitCompilerOptions()["baseline.warmup.trigger"] != 10) > > + setJitCompilerOption("baseline.warmup.trigger", 10); > > + > > +function checkGetOffsetsCoverage(fun) { > > + var keys = [ "TN", "SF", "FN", "FNDA", "FNF", "FNH", "BRDA", "BRF", "BRH", "DA", "LF", "LH" ]; > > What on earth do these keys mean? Could you document the syntax you use in > the comments? I'm not really making sense of them. I will replicate the following comment in both test cases: These test cases are annotated with the output produced by LCOV [1]. Comment starting with //<key> without any spaces are used as a reference for the code coverage output. Any "$" in these line comments are replaced by the current line number, and any "%" are replaced with the current function name (defined by the FN key). [1] http://ltp.sourceforge.net/coverage/lcov/geninfo.1.php

I will land these patches tomorrow, for some reasons I got surprised by unexpected orange on JetPack tests, which seems to be coming from mozilla-central after all.

https://hg.mozilla.org/integration/mozilla-inbound/rev/1ac20ebb3bd7 https://hg.mozilla.org/integration/mozilla-inbound/rev/b617f0a6a77c https://hg.mozilla.org/integration/mozilla-inbound/rev/26ca3396791f

https://hg.mozilla.org/mozilla-central/rev/1ac20ebb3bd7 https://hg.mozilla.org/mozilla-central/rev/b617f0a6a77c https://hg.mozilla.org/mozilla-central/rev/26ca3396791f