Closed Bug 1307633 Opened 3 years ago Closed 2 years ago

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

Categories

(Core :: JavaScript Engine, defect, critical)

x86_64
macOS
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox52 --- wontfix
firefox-esr52 --- wontfix
firefox54 --- wontfix
firefox55 --- wontfix
firefox56 --- fixed

People

(Reporter: gkw, Assigned: nbp)

References

(Blocks 2 open bugs)

Details

(Keywords: crash, jsbugmon, testcase, Whiteboard: [jsbugmon:update])

Crash Data

Attachments

(3 files)

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.
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.
Flags: needinfo?(jcoppeard)
I will take over this issue, as we have to fix all of these before we enable Bug 900784.
Flags: needinfo?(jcoppeard) → needinfo?(nicolas.b.pierron)
Too late for firefox 52, mass-wontfix.
Duplicate of this bug: 1299115
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.
Attachment #8877205 - Flags: review?(shu) → review+
Pushed by npierron@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/726d125c339b
XDRScript: Do not attempt to free 0-initialized scriptData_ on OOM. r=shu
https://hg.mozilla.org/mozilla-central/rev/726d125c339b
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Should we consider this for backport to 55 or can it ride the 55 train?
Assignee: nobody → nicolas.b.pierron
(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.
Flags: needinfo?(nicolas.b.pierron)
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
Attachment #8877205 - Flags: approval-mozilla-beta?
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.
Attachment #8877205 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
You need to log in before you can comment on or make changes to this bug.