1315592 - Crash in JSScript::module

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Nicholas Nethercote [inactive]]

This bug was filed from the Socorro interface and is 
report bp-58124a69-d297-4eb1-96b1-6ddd42161104.
=============================================================

New crash, first showed up in Nightly 20161104030212, and has occurred 25 times across 6 installations. It's the #13 topcrash in Nightly 20161104030212.

The crash address is always 0x4 (32-bit) or 0x8 (64-bit). Looks like we call script->module() with a null |this|, and we call bodyScope() which ends up in scopes() which accesses |data| which is one word into the JSScript.

Shu, any ideas about the cause of this one?

Comment 1

[Shu-yu Guo [:shu]]

(In reply to Nicholas Nethercote [:njn] from comment #0)
> This bug was filed from the Socorro interface and is 
> report bp-58124a69-d297-4eb1-96b1-6ddd42161104.
> =============================================================
> 
> New crash, first showed up in Nightly 20161104030212, and has occurred 25
> times across 6 installations. It's the #13 topcrash in Nightly
> 20161104030212.
> 
> The crash address is always 0x4 (32-bit) or 0x8 (64-bit). Looks like we call
> script->module() with a null |this|, and we call bodyScope() which ends up
> in scopes() which accesses |data| which is one word into the JSScript.
> 
> Shu, any ideas about the cause of this one?

Not really. I agree with your diagnosis that it looks like a nullptr script. Looking at the stack in that crash report though, it seems like it's getting a nullptr script when trying to execute some global script. That script was supposed to be compiled by PrepareScript [1]. But that function looks very suspect, since it doesn't even check if JS::Compile fails or not, as those XXXbz comments point out. I don't understand how this can work right now at all -- if the script isn't successfully compiled, how can we just go ahead and try to execute it?

Who owns the subscript loader?

[1] http://searchfox.org/mozilla-central/source/js/xpconnect/loader/mozJSSubScriptLoader.cpp#170

Comment 2

[Shu-yu Guo [:shu]]

Okay, I mean... I have tested with an xpcshell test that if you try to loadSubScriptWithOptions(pathToFile, { async: true }), and the file at the pathToFile has an early error, the process just crashes, because what do you know, we don't check the return value of JS::Compile.

Comment 3

[Shu-yu Guo [:shu]]

Attachment: Handle JS compilation errors in the async subscript loader.

Comment 4

[Bobby Holley (:bholley)]

Comment on attachment 8808856 [details] [diff] [review]
Handle JS compilation errors in the async subscript loader.

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

::: js/xpconnect/loader/mozJSSubScriptLoader.cpp
@@ +374,5 @@
>  
>      rv = PrepareScript(uri, cx, target_obj, spec.get(), mCharset,
>                         reinterpret_cast<const char*>(aBuf), aLength,
>                         mReuseGlobal, &script, &function);
> +    if (NS_FAILED(rv) || (!script && !function)) {

I think it would be better to change the return value of PrepareScript to be a bool (a la standard JSAPI), propagated false in the normal way, and check that here.

::: js/xpconnect/tests/unit/test_asyncLoadSubScriptError.js
@@ +12,5 @@
> +  do_test_finished();
> +}
> +
> +function error() {
> +  ok(true, "loading script with early error asynchronously resulted in error.");

Can you pass the error and regexp it to make sure it's the right one?

Comment 5

[Shu-yu Guo [:shu]]

Comment on attachment 8808856 [details] [diff] [review]
Handle JS compilation errors in the async subscript loader.

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

::: js/xpconnect/loader/mozJSSubScriptLoader.cpp
@@ +374,5 @@
>  
>      rv = PrepareScript(uri, cx, target_obj, spec.get(), mCharset,
>                         reinterpret_cast<const char*>(aBuf), aLength,
>                         mReuseGlobal, &script, &function);
> +    if (NS_FAILED(rv) || (!script && !function)) {

This kind of error checking is in line with what's currently done for the non-async subscript loader [1]. Would you prefer I change both sites?

[1] http://searchfox.org/mozilla-central/source/js/xpconnect/loader/mozJSSubScriptLoader.cpp#681

Comment 6

[Bobby Holley (:bholley)]

Comment on attachment 8808856 [details] [diff] [review]
Handle JS compilation errors in the async subscript loader.

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

(In reply to Shu-yu Guo [:shu] from comment #5)
> This kind of error checking is in line with what's currently done for the
> non-async subscript loader [1]. Would you prefer I change both sites?

Ideally yes. My general view is that anything that takes a cx (and therefore has an AutoJSAPI or AutoEntryScript upstack) should use the JSAPI calling convention and report errors on the cx. But that's a fair amount of scope creep, so if you don't want to do it I'm ok with the original patch modulo the test change.

Comment 7

[Shu-yu Guo [:shu]]

Attachment: Handle JS compilation errors in the async subscript loader.

Also:
 - Convert error handling of JSContext-taking functions to signal error by bool.
 - Pass the pending exception, if any, to the rejection handler of the async
   subscript loader promise.

Comment 8

[Shu-yu Guo [:shu]]

try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0cee5d8c345e62dda2f72d66f14b86d6cf48993e

Comment 9

[Shu-yu Guo [:shu]]

new try run, old one had compilation error: https://treeherder.mozilla.org/#/jobs?repo=try&revision=96608bbbe6f6b488abe383a5aecb4a584db18da5

Comment 10

[Bobby Holley (:bholley)]

Comment on attachment 8811093 [details] [diff] [review]
Handle JS compilation errors in the async subscript loader.

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

::: js/xpconnect/loader/mozJSSubScriptLoader.cpp
@@ +180,3 @@
>          }
>      }
> +    return true;

Looks like all the control paths return a value - do we need this?

@@ +198,4 @@
>      if (function) {
> +        if (!JS_CallFunction(cx, target_obj, function, JS::HandleValueArray::empty(),
> +                             retval))
> +        {

- I think the brace should go on the previous line given the "visual separation" heuristic?

@@ +320,5 @@
> +        if (mPromise) {
> +            JSContext* cx = mAutoEntryScript.cx();
> +            RootedValue rejectionValue(cx, JS::UndefinedValue());
> +            if (mAutoEntryScript.HasException()) {
> +                (void) mAutoEntryScript.PeekException(&rejectionValue);

nit: should probably use mozilla::unused here.

@@ +698,5 @@
>      } else {
>          cache = nullptr;
>      }
>  
> +    (void) EvalScript(cx, targetObj, retval, uri, !!cache, script, function);

mozilla::unused here as well.

Also, can we MOZ_ASSERT script || function now, given that we don't appear to check that case anymore?

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d2c5b6581d6a
Handle JS compilation errors in the async subscript loader. (r=bholley)

Comment 12

[Phil Ringnalda (:philor)]

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

Comment 13

[Andrew McCreight [:mccr8]]

According to comment 0, this first showed up on 52. Shu, could you put this up for Aurora approval? I do see some crashes with this signature there. Thanks.

Comment 14

[Shu-yu Guo [:shu]]

Attachment: Uplift patch

Comment 15

[Shu-yu Guo [:shu]]

Comment on attachment 8818983 [details] [diff] [review]
Uplift patch

Approval Request Comment
[Feature/Bug causing the regression]: Dunno, the async loader has had this bug since it first landed. Unsure why crashes started recently.
[User impact if declined]: Crashes
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: From what?
[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: No, bug fix only.
[Why is the change risky/not risky?]: Bug fix only.
[String changes made/needed]: None.

Comment 16

[Julien Cristau [:jcristau]]

Comment on attachment 8818983 [details] [diff] [review]
Uplift patch

crash fix for aurora52

Comment 17

[Carsten Book [:Tomcat]]

the uplift patch needs rebasing

Comment 18

[Ryan VanderMeulen [:RyanVM]]

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

Comment 19

[Shu-yu Guo [:shu]]

Wait, did someone else rebase it?

Comment 20

[Ryan VanderMeulen [:RyanVM]]

It was an xpcshell.ini conflict, so yes, I did.

Comment 21

[Shu-yu Guo [:shu]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #20)
> It was an xpcshell.ini conflict, so yes, I did.

Thank you!