916845 - Assertion failure: hasSourceData(), at js/src/jsscript.h:350

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: Crash log

Debugger.Source doesn't seem to play all that well with chrome debugging. Just opening the Browser Debugger leads to the assertion:

Assertion failure: hasSourceData(), at js/src/jsscript.h:350

Stack trace:

0   XUL DebuggerSource_getText(JSContext*, unsigned int, JS::Value*) + 454 (RootingAPI.h:661)
1   XUL js::jit::DoCallNativeGetter(JSContext*, JS::Handle<JSFunction*>, JS::Handle<JSObject*>, JS::MutableHandle<JS::Value>) + 132 (BaselineIC.cpp:1584)
2   ??? 0 + 4303276235
3   ??? 0 + 5598336360
4   ??? 0 + 4303239647
5   XUL EnterBaseline(JSContext*, js::jit::EnterJitData&) + 547 (BaselineJIT.cpp:123)
6   XUL js::jit::EnterBaselineMethod(JSContext*, js::RunState&) + 221 (BaselineJIT.cpp:152)
7   XUL js::RunScript(JSContext*, js::RunState&) + 209 (Interpreter.cpp:414)
8   XUL js::Invoke(JSContext*, JS::CallArgs, js::MaybeConstruct) + 599 (Interpreter.cpp:497)
9   XUL js::Invoke(JSContext*, JS::Value const&, JS::Value const&, unsigned int, JS::Value*, JS::MutableHandle<JS::Value>) + 451 (Interpreter.cpp:528)
10  XUL js::jit::DoCallFallback(JSContext*, js::jit::BaselineFrame*, js::jit::ICCall_Fallback*, unsigned int, JS::Value*, JS::MutableHandle<JS::Value>) + 5391 (BaselineIC.cpp:7565)

Comment 1

[Panos Astithas (he/him) [:past] (please ni?)]

This is actually in SpiderMonkey code, albeit Debugger-related.

Comment 2

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: Crash log w/o optimizations

Here is a stack trace from a non-optimized debug build:

0   XUL DebuggerSource_getText(JSContext*, unsigned int, JS::Value*) + 454 (RootingAPI.h:661)
1   XUL js::CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) + 407 (jscntxtinlines.h:218)
2   XUL js::Invoke(JSContext*, JS::CallArgs, js::MaybeConstruct) + 580 (Interpreter.cpp:471)
3   XUL js::Invoke(JSContext*, JS::Value const&, JS::Value const&, unsigned int, JS::Value*, JS::MutableHandle<JS::Value>) + 451 (Interpreter.cpp:528)
4   XUL js::InvokeGetterOrSetter(JSContext*, JSObject*, JS::Value, unsigned int, JS::Value*, JS::MutableHandle<JS::Value>) + 111 (Interpreter.cpp:599)
5   XUL js::Shape::get(JSContext*, JS::Handle<JSObject*>, JSObject*, JSObject*, JS::MutableHandle<JS::Value>) + 81 (Shape-inl.h:69)
6   XUL bool NativeGetInline<(js::AllowGC)1>(JSContext*, js::MaybeRooted<JSObject*, (js::AllowGC)1>::HandleType, js::MaybeRooted<JSObject*, (js::AllowGC)1>::HandleType, js::MaybeRooted<JSObject*, (js::AllowGC)1>::HandleType, js::MaybeRooted<js::Shape*, (js::AllowGC)1>::HandleType, unsigned int, js::MaybeRooted<JS::Value, (js::AllowGC)1>::MutableHandleType) + 400 (jsobj.cpp:4071)
7   XUL bool GetPropertyHelperInline<(js::AllowGC)1>(JSContext*, js::MaybeRooted<JSObject*, (js::AllowGC)1>::HandleType, js::MaybeRooted<JSObject*, (js::AllowGC)1>::HandleType, js::MaybeRooted<jsid, (js::AllowGC)1>::HandleType, unsigned int, js::MaybeRooted<JS::Value, (js::AllowGC)1>::MutableHandleType) + 270 (jsobj.cpp:4242)
8   XUL js::GetPropertyHelper(JSContext*, JS::Handle<JSObject*>, JS::Handle<jsid>, unsigned int, JS::MutableHandle<JS::Value>) + 18 (jsobj.cpp:4251)
9   XUL GetPropertyOperation(JSContext*, js::StackFrame*, JS::Handle<JSScript*>, unsigned char*, JS::MutableHandle<JS::Value>, JS::MutableHandle<JS::Value>) + 744 (Interpreter.cpp:281)
10  XUL Interpret(JSContext*, js::RunState&) + 12578 (Interpreter.cpp:2293)

Comment 4

[Eddy Bruel [:ejpbruel]]

The root problem is that DebuggerSource_getText ends up calling ScriptSource::length(), which asserts whether we actually *have* any source data.

Apparently it's possible for scripts to not have any source data, but this source data can optionally be obtained via cx->runtime()->sourceHook (provided sourceRetrievable is true).

Jim, can you explain the intended semantics for cx->runtime()->sourceHook and sourceRetrievable in the context of script sources? It seems to me that we should either guard in DebuggerSource_getText that hasSourceData is true, or lazily call JSScript::loadSource.

Comment 5

[Panos Astithas (he/him) [:past] (please ni?)]

Some more debugging info:

(gdb) bt 2
#0  js::ScriptSource::length (this=0x124f3e480) at jsscript.h:350
#1  0x0000000105904f6a in DebuggerSource_getText (cx=0x100676540, argc=0, vp=0x7fff5fbe59b0) at /Users/past/src/fx-team/js/src/vm/Debugger.cpp:3718
(More stack frames follow...)
(gdb) frame 0
#0  js::ScriptSource::length (this=0x124f3e480) at jsscript.h:350
350	        JS_ASSERT(hasSourceData());
(gdb) print hasSourceData()
$2 = false
(gdb) print sourceRetrievable_
$3 = true
(gdb) frame 1
#1  0x0000000105904f6a in DebuggerSource_getText (cx=0x100676540, argc=0, vp=0x7fff5fbe59b0) at /Users/past/src/fx-team/js/src/vm/Debugger.cpp:3718
3718	    JSString *str = ss->substring(cx, 0, ss->length());
(gdb) print cx->runtime()->sourceHook
$4 = (JS_SourceHook) 0x10324abe0 <SourceHook(JSContext*, JS::Handle<JSScript*>, unsigned short**, unsigned int*)>

Comment 6

[Jim Blandy :jimb]

After discussing things with Eddy on IRC, it seems like this is what's going on:

Firefox uses CompileOptions::setSourcePolicy to tell SpiderMonkey not to retain the source code for some JavaScript; lazy compilation isn't helpful in these cases, and we can easily retrieve the source code if it's ever needed (it's in a zip file), so keeping it around would just be a waste of memory.

Debugger.Source isn't set up to deal with these cases, although it could perfectly well be; it should imitate JS_DecompileScript, and call JSScript::loadSource when needed.

However, we also need some testing support for this. The JS shell should have a built-in function that registers a sourceHook with the JSRuntime (JS_SetSourceHook); the API might be cheezy, since the source hook only gets a pointer to the JSScript; but really almost anything will do, for tests. And then the 'evaluate' function needs an option to make it possible to create LAZY_SOURCE scripts.

Comment 7

[Eddy Bruel [:ejpbruel]]

Attachment: Proposed fix (still needs tests)

This still needs proper tests, but to prevent past and fitzgen from having to back out, we should probably land this asap. Let's just keep the bug open and I'll make sure to write the tests as soon as I can.

Jimb, provided this gets an r+ from you, can you land this for me tonight?  Because we don't have proper tests yet I've started a try run for this patch, just in case:

https://tbpl.mozilla.org/?tree=Try&rev=d592c8ebadfa

Comment 8

[Jim Blandy :jimb]

Comment on attachment 806300 [details] [diff] [review]
Proposed fix (still needs tests)

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

Nice and simple.

Comment 9

[Panos Astithas (he/him) [:past] (please ni?)]

Thanks Eddy! I landed the fix and marked the bug [leave open] for the tests:

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

Comment 10

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

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

Comment 11

[Jim Blandy :jimb]

Okay, here come some work-in-progress patches for testing:

Comment 12

[Jim Blandy :jimb]

Attachment: Use size_t to describe length of source code in SpiderMonkey SourceHook lazy source hook.

Comment 13

[Jim Blandy :jimb]

Attachment: Change sourceHook to a nice C++ object with a destructor.

Comment 14

[Jim Blandy :jimb]

Attachment: Add functions for testing lazily-retrieved sources.

Comment 15

[Jim Blandy :jimb]

One problem is that it's possible for a JS source hook to return bogus code that makes SpiderMonkey assert. If we can't fix this, then we'll need to hide the testing function from the fuzzers, which would be too bad.

js> setSourceHook((url) => { print(url); return "hard day's night"; })
js> var g = evaluate('(function () { return "snoo"; })', { sourcePolicy: "LAZY_SOURCE" })
js> g
@evaluate
Assertion failure: src->length() > 0 && chars[0] == '(', at /home/jimb/moz/dbg/js/src/jsfun.cpp:680
Segmentation fault (core dumped)

Comment 16

[Terrence Cole [:terrence]]

(In reply to Jim Blandy :jimb from comment #15)
> One problem is that it's possible for a JS source hook to return bogus code
> that makes SpiderMonkey assert. If we can't fix this, then we'll need to
> hide the testing function from the fuzzers, which would be too bad.

The shell has a --fuzzing-safe option, which only defines the functions in fuzzing_safe_functions if it is not set.

Comment 17

[Jim Blandy :jimb]

(In reply to Terrence Cole [:terrence] from comment #16)
> (In reply to Jim Blandy :jimb from comment #15)
> > One problem is that it's possible for a JS source hook to return bogus code
> > that makes SpiderMonkey assert. If we can't fix this, then we'll need to
> > hide the testing function from the fuzzers, which would be too bad.
> 
> The shell has a --fuzzing-safe option, which only defines the functions in
> fuzzing_safe_functions if it is not set.

You mean fuzzing_unsafe_functions? Thanks!

Comment 18

[Jim Blandy :jimb]

Here's a try push for those three patches:
https://tbpl.mozilla.org/?tree=Try&rev=5b9fb727b38d

Comment 19

[Jim Blandy :jimb]

Attachment: Add functions for testing lazily-retrieved sources.

In this revision, instead of a 'setSourceHook' primitive, we have a 'withSourceHook' primitive that saves and restores the runtime's original hook. This should make the primitive more suitable for use within the browser: running tests that use it won't wipe out the browser's source hook permanently, only temporarily supplant it.

Comment 20

[Jim Blandy :jimb]

Here's a new try push:
https://tbpl.mozilla.org/?tree=Try&rev=ff77c483b775

Comment 21

[Jason Orendorff [:jorendorff]]

Comment on attachment 807452 [details] [diff] [review]
Use size_t to describe length of source code in SpiderMonkey SourceHook lazy source hook.

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

::: js/xpconnect/src/XPCJSRuntime.cpp
@@ +2773,5 @@
>      return mozilla::dom::IsDOMObject(obj) && mozilla::dom::TryPreserveWrapper(obj);
>  }
>  
>  static nsresult
> +ReadSourceFromFilename(JSContext *cx, const char *filename, jschar **src, size_t *len)

Note that this function still contains the lines:

>  if (rawLen > UINT32_MAX)
>    return NS_ERROR_FILE_TOO_BIG;

which I think is still fair, given the circumstances...

Comment 22

[Jason Orendorff [:jorendorff]]

Comment on attachment 807453 [details] [diff] [review]
Change sourceHook to a nice C++ object with a destructor.

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

::: js/src/jsfriendapi.h
@@ +217,5 @@
> + * and hope that it will be able to find the source.
> + */
> +class SourceHook {
> +  public:
> +    virtual ~SourceHook() { };

no semicolon needed here

::: js/src/vm/Runtime.h
@@ +1274,5 @@
>      bool hasContexts() const {
>          return !contextList.isEmpty();
>      }
>  
> +    js::SourceHook      *sourceHook;

You could use a mozilla::ScopedDeletePtr<js::SourceHook>
(defined in "mozilla/Scoped.h") instead.

::: js/xpconnect/src/XPCJSRuntime.cpp
@@ +2851,5 @@
> +    bool load(JSContext *cx, const char *filename, jschar **src, size_t *length) {
> +        *src = NULL;
> +        *length = 0;
> +
> +        if (!nsContentUtils::IsCallerChrome())

I'm pretty sure the prevailing style in this file is two-space indents, more's the pity.

Comment 23

[Jim Blandy :jimb]

(In reply to Jason Orendorff [:jorendorff] from comment #21)
> Note that this function still contains the lines:
> 
> >  if (rawLen > UINT32_MAX)
> >    return NS_ERROR_FILE_TOO_BIG;
> 
> which I think is still fair, given the circumstances...

I agree, but the difference is still worth a note; I added a comment.

Comment 24

[Jim Blandy :jimb]

(In reply to Jason Orendorff [:jorendorff] from comment #22)
> no semicolon needed here

D'oh; fixed.

> You could use a mozilla::ScopedDeletePtr<js::SourceHook>
> (defined in "mozilla/Scoped.h") instead.

Ah! Changed to use that. Unfortunately, I still need to take explicit steps to clear it early in ~JSRuntime, as SourceHook subclass destructors should be able to do things like delete roots --- which must be done before ~JSRuntime's body completes.

> I'm pretty sure the prevailing style in this file is two-space indents,
> more's the pity.

Actually, prevailing style in that file is four-space indents, but the immediately preceding ReadSourceFromFilename function uses the wrong indentation.

Comment 25

[Jim Blandy :jimb]

Attachment: Reindent ReadSourceFromFilename to match the surrounding style.

Comment 26

[Jim Blandy :jimb]

Comment on attachment 808038 [details] [diff] [review]
Reindent ReadSourceFromFilename to match the surrounding style.

I guess Luke isn't someone who reviews this file. Sorry about that.

Comment 27

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 808038 [details] [diff] [review]
Reindent ReadSourceFromFilename to match the surrounding style.

r=me

Comment 28

[Jim Blandy :jimb]

https://hg.mozilla.org/integration/mozilla-inbound/rev/81b30f8c376e
https://hg.mozilla.org/integration/mozilla-inbound/rev/989cc9b220c2

Comment 29

[Jim Blandy :jimb]

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

Comment 30

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/81b30f8c376e
https://hg.mozilla.org/mozilla-central/rev/989cc9b220c2
https://hg.mozilla.org/mozilla-central/rev/a656666e63b6

Comment 31

[Eddy Bruel [:ejpbruel]]

Comment on attachment 807899 [details] [diff] [review]
Add functions for testing lazily-retrieved sources.

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

Meticulous as always. r+ with comments addressed.

::: js/src/jit-test/tests/basic/withSourceHook.js
@@ +3,5 @@
> +
> +load(libdir + 'asserts.js');
> +
> +if (typeof withSourceHook != 'function')
> +  quit(0);

Why isn't this an assertion?

@@ +15,5 @@
> +  assertThrowsValue(function () {
> +    // Establish a source hook that throws.
> +    withSourceHook(function (url) {
> +      assertEq(url, 'middle');
> +      throw 'borborygmus'; // middle

Had to look this word up. Then I lol'd :D

::: js/src/shell/js.cpp
@@ +3711,5 @@
>      RootedPropertyName srcName(cx, srcAtom->asPropertyName());
>      return cx->runtime()->cloneSelfHostedValue(cx, srcName, args.rval());
>  }
>  
> +class ShellSourceHook: public SourceHook {

It looks like the main purpose of this class is to wrap a JS function in a native function, and automatically root the former. Since JS functions can already wrap native functions, wouldn't it be nicer if the JSRuntime just stored the source hook as a JS function?

I don't want to r- just because of this, but I feel pretty strongly that this would be the better approach.

@@ +3768,5 @@
> +    }
> +};
> +
> +static bool
> +WithSourceHook(JSContext *cx, unsigned argc, jsval *vp)

Do we really need all this logic here to save the old source hook, temporarily set up a new one, and then restore the old one? We're only using this in the shell, and the shell doesn't set up a source hook by default, so I feel it would be better to move this logic over to the tests (the best C++ code is no C++ code).

I'm not going to fight you over that though. Your current solution is clean, so if it works, its fine. I just think its overkill :-)

@@ +3784,5 @@
> +        ReportUsageError(cx, callee, "First and second arguments must be functions.");
> +        return false;
> +    }
> +
> +    ShellSourceHook *hook = new ShellSourceHook;

I didn't even remember you could call a constructor without an argument list in C++, and as far as I know, this is not a very common pattern in SpiderMonkey either. Perhaps its better to follow the principle of least surprise, and use new ShellSourceHook() here instead?

@@ +3837,5 @@
> +"         property is omitted or |v| is null, don't attribute the source to\n"
> +"         any DOM element.\n"
> +"      sourceMapURL: if present with value |v|, convert |v| to a string, and\n"
> +"         provide that as the code's source map URL. If omitted, attach no\n"
> +"         source map URL to the code (although the code may provide one itself,\n"

Thanks for adding those!

Comment 32

[Jim Blandy :jimb]

(In reply to Eddy Bruel [:ejpbruel] from comment #31)
> ::: js/src/jit-test/tests/basic/withSourceHook.js
> @@ +3,5 @@
> > +
> > +load(libdir + 'asserts.js');
> > +
> > +if (typeof withSourceHook != 'function')
> > +  quit(0);
> 
> Why isn't this an assertion?

Since it's possible to crash SpiderMonkey by returning bogus source (as noted in the 'help' test for 'WithSourceHook'), we can't expose it to fuzzers, as fuzzers will find any function present anywhere and try to call it. So, withSourceHook is registered in the 'fuzzing_unsafe_functions' list, and the 'if' in the test makes the shell skip it when run with the --fuzzing-safe option.

> ::: js/src/shell/js.cpp
> @@ +3711,5 @@
> >      RootedPropertyName srcName(cx, srcAtom->asPropertyName());
> >      return cx->runtime()->cloneSelfHostedValue(cx, srcName, args.rval());
> >  }
> >  
> > +class ShellSourceHook: public SourceHook {
> 
> It looks like the main purpose of this class is to wrap a JS function in a
> native function, and automatically root the former. Since JS functions can
> already wrap native functions, wouldn't it be nicer if the JSRuntime just
> stored the source hook as a JS function?
> 
> I don't want to r- just because of this, but I feel pretty strongly that
> this would be the better approach.

That's a good point. But the only non-test use of JS_SetSourceHook is in XPCJSRuntime.cpp:
http://dxr.mozilla.org/mozilla-central/source/js/xpconnect/src/XPCJSRuntime.cpp#l3018

and that just wants to provide a C++ function. Do we want to impose JS function call overhead on that? The other patches in this bug replaced the original raw function pointer with a class (SourceHook) that has a virtual destructor; that seemed like the simplest way to support both use cases: those that need to own something, and those that don't. I felt like the test functions should bear the heavier burden, and we should encumber the real application as little as possible.

> @@ +3768,5 @@
> > +    }
> > +};
> > +
> > +static bool
> > +WithSourceHook(JSContext *cx, unsigned argc, jsval *vp)
> 
> Do we really need all this logic here to save the old source hook,
> temporarily set up a new one, and then restore the old one? We're only using
> this in the shell, and the shell doesn't set up a source hook by default, so
> I feel it would be better to move this logic over to the tests (the best C++
> code is no C++ code).
> 
> I'm not going to fight you over that though. Your current solution is clean,
> so if it works, its fine. I just think its overkill :-)

It may be overkill, but here's what I was thinking:

My first attempt just had a 'setSourceHook' shell function, and was a bit simpler. But there's talk of getting the jit-test suite to run under the browser, so I was thinking about how to make these tests usable under the browser. If the browser's own source hook doesn't get restored after a test exits (perhaps the test exits messily), then you can get weird crashes later on if the wrong source hook gets asked for sources it doesn't know about.

The way 'withSourceHook' is designed, you can be confident that the browser's original source hook always gets restored when the test is done, even if it exits messily. With the 'setSourceHook' primitive, the tests would have to be carefully written to reliably restore the original source hook. I thought it was better to provide a primitive that was difficult to use incorrectly.

Besides, the difference between 'with' and 'set' is only this:

--- a/js/src/shell/js.cpp
+++ b/js/src/shell/js.cpp
@@ -3904,13 +3904,11 @@ WithSourceHook(JSContext *cx, unsigned a
         delete hook;
         return false;
     }
 
-    SourceHook *savedHook = js::ForgetSourceHook(cx->runtime());
     js::SetSourceHook(cx->runtime(), hook);
-    bool result = Call(cx, UndefinedValue(), &args[1].toObject(), 0, NULL, args.rval());
-    js::SetSourceHook(cx->runtime(), savedHook);
-    return result;
+    args.rval().setUndefined();
+    return true;
 }
 
 static const JSFunctionSpecWithHelp shell_functions[] = {
     JS_FN_HELP("version", Version, 0, 0,

There's really not that much complexity that goes away. Even if we made the source hook into a JS function, all the junk in ShellSourceHook::load is necessary if a JS function is involved anywhere, so it would just move to JSScript::loadSource. And remember that we'll need a 'get' function, too. So 'with' actually gets us browser-usable safety for a rather low price.

> I didn't even remember you could call a constructor without an argument list
> in C++, and as far as I know, this is not a very common pattern in
> SpiderMonkey either. Perhaps its better to follow the principle of least
> surprise, and use new ShellSourceHook() here instead?

Done.

You can do the no-parens version in JS, too!

Comment 33

[Jim Blandy :jimb]

(In reply to Jim Blandy :jimb from comment #32)
> noted in the 'help' test for 'WithSourceHook'), we can't expose it to

"'help' text", I meant...

Comment 34

[Jim Blandy :jimb]

(In reply to Jim Blandy :jimb from comment #32)
> Since it's possible to crash SpiderMonkey by returning bogus source (as
> noted in the 'help' test for 'WithSourceHook'), we can't expose it to
> fuzzers, as fuzzers will find any function present anywhere and try to call
> it. So, withSourceHook is registered in the 'fuzzing_unsafe_functions' list,
> and the 'if' in the test makes the shell skip it when run with the
> --fuzzing-safe option.

I'll add comments to the test to this effect.

Comment 35

[Jim Blandy :jimb]

(In reply to Jim Blandy :jimb from comment #34)
> I'll add comments to the test to this effect.

... and add the check to the other test...

Comment 36

[Jim Blandy :jimb]

https://hg.mozilla.org/integration/mozilla-inbound/rev/41ad19b85797

Comment 37

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/41ad19b85797

Comment 38

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 806300 [details] [diff] [review]
Proposed fix (still needs tests)

[Approval Request Comment]
Bug caused by (feature/regressing bug #): caused by bug 914930 using a new and untested API
User impact if declined: the Browser Debugger is broken in Aurora, making debugging add-ons impossible
Testing completed (on m-c, etc.): m-c and Aurora (locally)
Risk to taking this patch (and alternatives if risky): low risk, since this is code exercised by developer tools AFAIK. Alternatively we could perhaps backout bug 914930, but I haven't tested that
String or IDL/UUID changes made by this patch: none

Comment 39

[Panos Astithas (he/him) [:past] (please ni?)]

Jim or Eddy, do you think that we need to uplift the other patches as well, or is the main one enough for Aurora?

Comment 41

[Jeff Walden [:Waldo]]

Note that |new A| and |new A();| don't necessarily have the same semantics in C++.  Look at the spec and the initialization bits if you want details.  (Something about zero-initialized versus default-initialized, but I don't remember details.)  I think () is what you generally want because it'll always construct, whereas no-parens doesn't, but I might be mistaken.

Comment 42

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 806300 [details] [diff] [review]
Proposed fix (still needs tests)

Don't need to track this since it's in a new and unused API and not a regression, go ahead with uplifting this to 26 so we can get testing there before ship - and on Beta in a couple of weeks.

Comment 43

[Victor Porof [:vporof][:vp]]

Looks like it might be happening on Beta too? 
https://twitter.com/Foxtrick/status/388792370458030080
https://twitter.com/Foxtrick/status/388795796747202560

Comment 44

[Victor Porof [:vporof][:vp]]

False alarm, that was Aurora.

Comment 45

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/e391dad00380

You'll need to request approval for the other patches if you decide that you want them uplifted since they affect more than test-only code.