1307633 - Crash [@ bool js::XDRScript<(js::XDRMode)1>]

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

The following testcase crashes on mozilla-central revision 42c95d88aaaa (build with --enable-debug --enable-more-deterministic, run with --fuzzing-safe --no-threads --no-baseline --no-ion):

// Adapted from randomly chosen test: js/src/jit-test/tests/xdr/function-flags.js
oomTest(function() {
    x = {};
    y = cacheEntry("function f() {};f();");
    x.global = newGlobal({
        cloneSingletons: true
    });
    evaluate(y, Object.create(x, {
        saveBytecode: {
            value: true
        }
    }));
    evaluate(y, Object.create(x, {
        loadBytecode: {
            value: true
        }
    }));
})


Backtrace:

0   js-dbg-64-dm-clang-darwin-42c95d88aaaa	0x0000000105dcb978 bool js::XDRScript<(js::XDRMode)1>(js::XDRState<(js::XDRMode)1>*, JS::Handle<js::Scope*>, JS::Handle<JSScript*>, JS::Handle<JSFunction*>, JS::MutableHandle<JSScript*>) + 6200 (atomic:848)
1   js-dbg-64-dm-clang-darwin-42c95d88aaaa	0x0000000106094015 js::XDRState<(js::XDRMode)1>::codeScript(JS::MutableHandle<JSScript*>) + 53 (Xdr.cpp:172)
2   js-dbg-64-dm-clang-darwin-42c95d88aaaa	0x0000000105d037a0 JS_DecodeScript(JSContext*, void const*, unsigned int) + 112 (RootingAPI.h:789)
3   js-dbg-64-dm-clang-darwin-42c95d88aaaa	0x0000000105796226 Evaluate(JSContext*, unsigned int, JS::Value*) + 3542 (RootingAPI.h:800)
4   js-dbg-64-dm-clang-darwin-42c95d88aaaa	0x0000000105f2b8cd js::CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) + 93 (jscntxtinlines.h:240)
/snip

For detailed crash information, see attachment.

Comment 1

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: Detailed Crash Information

Comment 2

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: OOM_VERBOSE=1 stack from m-c rev 42c95d88aaaa

This seems to go back to the addition of oomTest, so the OOM_VERBOSE stack is probably more useful.

Setting needinfo? from Jon as a start.

Comment 3

[Nicolas B. Pierron [:nbp]]

I will take over this issue, as we have to fix all of these before we enable Bug 900784.

Comment 4

[Julien Cristau [:jcristau]]

Too late for firefox 52, mass-wontfix.

Comment 5

[Nicolas B. Pierron [:nbp]]

Attachment: XDRScript: Do not attempt to free 0-initialized scriptData_ on OOM.

Comment 7

[Shu-yu Guo [:shu]]

Comment on attachment 8877205 [details] [diff] [review]
XDRScript: Do not attempt to free 0-initialized scriptData_ on OOM.

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

r=me with suggested change.

::: js/src/jsscript.cpp
@@ +670,5 @@
>      });
>  
>      if (mode == XDR_DECODE) {
> +        if (!script->createScriptData(cx, length, nsrcnotes, natoms)) {
> +            scriptDataGuard.release();

Seems clearer to simply move the ScopeExit to be below the XDR_DECODE if body.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/726d125c339b
XDRScript: Do not attempt to free 0-initialized scriptData_ on OOM. r=shu

Comment 9

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/726d125c339b

Comment 10

[Ryan VanderMeulen [:RyanVM]]

Should we consider this for backport to 55 or can it ride the 55 train?

Comment 11

[Nicolas B. Pierron [:nbp]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #10)
> Should we consider this for backport to 55 or can it ride the 55 train?

This is an OOM while decoding bytecode, we can backport it, but I consider this unlikely considering that the only active code able to run this is used during the start-up of firefox processes.

Comment 12

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8877205 [details] [diff] [review]
XDRScript: Do not attempt to free 0-initialized scriptData_ on OOM.

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1288715
[User impact if declined]: Extremely unlikely nullptr crash.
[Is this code covered by automated tests?]: No, the test takes too much time to integrate in the test suite.

[Has the fix been verified in Nightly?]: Yes.
[Needs manual test from QE? If yes, steps to reproduce]: No.
[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: No.
[Why is the change risky/not risky?]: It moves code to free a pointer after it has been allocated.

[String changes made/needed]: N/A

Comment 13

[Julien Cristau [:jcristau]]

Comment on attachment 8877205 [details] [diff] [review]
XDRScript: Do not attempt to free 0-initialized scriptData_ on OOM.

based on comment 11 it sounds like we can safely let this ride the trains to 56.