Closed Bug 916845 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla27
Tracking Status
firefox24 --- unaffected
firefox25 --- unaffected
firefox26 --- fixed
firefox27 --- fixed

People

(Reporter: past, Assigned: ejpbruel)

References

Details

(Whiteboard: [qa-])

Attachments

(6 files, 2 obsolete files)

Attached file Crash log (obsolete) —
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)
This is actually in SpiderMonkey code, albeit Debugger-related.
Assignee: nobody → general
Blocks: 914930
Component: Developer Tools: Debugger → JavaScript Engine
Product: Firefox → Core
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)
Attachment #805352 - Attachment is obsolete: true
Duplicate of this bug: 916453
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.
Flags: needinfo?(jimb)
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*)>
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.
Flags: needinfo?(jimb)
Assignee: general → ejpbruel
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
Attachment #806300 - Flags: review?(jimb)
Comment on attachment 806300 [details] [diff] [review]
Proposed fix (still needs tests)

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

Nice and simple.
Attachment #806300 - Flags: review?(jimb) → review+
Thanks Eddy! I landed the fix and marked the bug [leave open] for the tests:

https://hg.mozilla.org/integration/mozilla-inbound/rev/baba94f254a0
Whiteboard: [leave open]
Okay, here come some work-in-progress patches for testing:
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)
(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.
(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!
Here's a try push for those three patches:
https://tbpl.mozilla.org/?tree=Try&rev=5b9fb727b38d
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.
Attachment #807899 - Flags: review?(ejpbruel)
Attachment #807454 - Attachment is obsolete: true
Attachment #807453 - Flags: review?(ejpbruel)
Attachment #807453 - Flags: review?(ejpbruel) → review?(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...
Attachment #807452 - Flags: review?(jorendorff) → review+
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.
Attachment #807453 - Flags: review?(jorendorff) → review+
(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.
(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 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.
Attachment #808038 - Flags: review?(luke) → review?(bzbarsky)
Comment on attachment 808038 [details] [diff] [review]
Reindent ReadSourceFromFilename to match the surrounding style.

r=me
Attachment #808038 - Flags: review?(bzbarsky) → review+
Attachment #807452 - Flags: checkin+
Attachment #807453 - Flags: checkin+
Attachment #808038 - Flags: checkin+
Attachment #806300 - Flags: checkin+
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!
Attachment #807899 - Flags: review?(ejpbruel) → review+
(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!
(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...
(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.
(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...
https://hg.mozilla.org/integration/mozilla-inbound/rev/41ad19b85797
Flags: in-testsuite+
OS: Mac OS X → All
Hardware: x86 → All
Whiteboard: [leave open]
Target Milestone: --- → mozilla27
https://hg.mozilla.org/mozilla-central/rev/41ad19b85797
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
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
Attachment #806300 - Flags: approval-mozilla-aurora?
Jim or Eddy, do you think that we need to uplift the other patches as well, or is the main one enough for Aurora?
Duplicate of this bug: 923287
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 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.
Attachment #806300 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
False alarm, that was Aurora.
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.
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.