1522339 - Assertion failure: deleted, at js/src/shell/js.cpp:1098

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

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

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

// jsfunfuzz-generated
gczeal(14);
oomTest(function() {
    // Adapted from randomly chosen test: js/src/tests/test262/language/expressions/class/dstr-async-gen-meth-static-obj-ptrn-id-init-fn-name-cover.js
    eval(`
        var C = class {
            static async *method({
                x = function() {}
            }) {
                y;
            }
        };
        C.method({}).next().then();
	`);
});

Backtrace:

#0 TrackUnhandledRejections (cx=<optimized out>, state=JS::PromiseRejectionHandlingState::Handled, promise=...) at js/src/shell/js.cpp:1098
#1 ForwardingPromiseRejectionTrackerCallback (cx=0x7f0e5a019000, promise=..., state=JS::PromiseRejectionHandlingState::Handled, data=<optimized out>) at js/src/shell/js.cpp:1110
#2 0x0000555b732cefb9 in PerformPromiseThenWithReaction (cx=0x7f0e5a019000, promise=..., reaction=...) at js/src/builtin/Promise.cpp:4275
#3 0x0000555b732b4583 in PerformPromiseThen (cx=0x7f0e5a019000, promise=..., onFulfilled_=..., onRejected_=..., resultCapability=...) at js/src/builtin/Promise.cpp:4213
#4 0x0000555b732d232d in OriginalPromiseThenBuiltin (cx=0x7f0e5a019000, promiseVal=..., onFulfilled=..., onRejected=..., rval=..., rvalUsed=true) at js/src/builtin/Promise.cpp:3454
#5 0x0000555b7321a6e0 in CallJSNative (cx=0x7f0e5a019000, native=0x555b732bcfe0 <Promise_then(JSContext*, unsigned int, JS::Value*)>, args=...) at js/src/vm/Interpreter.cpp:441
/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)]

Regression window:

https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=f611bc50d11c&tochange=39d0c50a2209

Due to compile failures fixed by bug 1521284, the regression range is above. I'll keep trying to get a smaller range, but setting needinfo? from Arai-san as a start since previous similar-looking bugs were also fixed by :arai .

Comment 3

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

(I'm quite sure I'm unable to get a smaller range)

Comment 4

[Tooru Fujisawa [:arai]]

So.. if OOM happens, we cannot reliably track the set of unhandled rejections,
given it may fail to add/remove the promise to the unhandledRejectedPromises set.

And OOM can happen even before calling ForwardingPromiseRejectionTrackerCallback.

This means the unhandled rejection tracking itself is not something reliable against OOM (this is also true for Firefox),
and it can detect "false" unhandled rejection especially when we use oomTest :P

unfortunately we should disable the assertion and disable the tracking when we use oomTest with promises.

Comment 5

[Tooru Fujisawa [:arai]]

Attachment: Bug 1522339 - Part 1: Do not assert the result of the delete operation when tracking unhandled promise rejections. r=jorendorff

Comment 6

[Tooru Fujisawa [:arai]]

Attachment: Bug 1522339 - Part 2: Throw error when oomTest is used with promises while unhandled rejection tracking is enabled. r=jorendorff

Depends on D17781

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4545916fd898
Part 1: Do not assert the result of the delete operation when tracking unhandled promise rejections. r=jorendorff
https://hg.mozilla.org/integration/autoland/rev/8ea7e6bb5669
Part 2: Throw error when oomTest is used with promises while unhandled rejection tracking is enabled. r=jorendorff

Comment 8

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/4545916fd898
https://hg.mozilla.org/mozilla-central/rev/8ea7e6bb5669

Comment 9

[Liz Henry (:lizzard) (relman/hg->git project)]

Might this be good for beta uplift? Or, do you think it is better to let it ride with 67?

Comment 10

[Tooru Fujisawa [:arai]]

This is JS shell-only issue.
so it depends on whether we do fuzzing on beta/release.
forwarding ni? to :gkw.

Comment 11

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

I do test on mozilla-beta, so that would be preferred. Thanks!

Comment 12

[Tooru Fujisawa [:arai]]

Comment on attachment 9039435 [details]
Bug 1522339 - Part 1: Do not assert the result of the delete operation when tracking unhandled promise rejections. r=jorendorff

Beta/Release Uplift Approval Request

Feature/Bug causing the regression

Bug 1517868

User impact if declined

This affects only JS shell, that fuzzing team is using.
This is wrong assertion, and it may hide another critical bug.

Is this code covered by automated tests?

No

Has the fix been verified in Nightly?

Yes

Needs manual test from QE?

No

If yes, steps to reproduce

List of other uplifts needed

None

Risk to taking this patch

Low

Why is the change risky/not risky? (and alternatives if risky)

The code is not used by Firefox.

String changes made/needed

None

Comment 13

[Liz Henry (:lizzard) (relman/hg->git project)]

Good to know, thanks arai and gary!

Comment 14

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 9039435 [details]
Bug 1522339 - Part 1: Do not assert the result of the delete operation when tracking unhandled promise rejections. r=jorendorff

Fix an assertion on beta for fuzzing team.
OK for uplift for next Monday's beta 5 build.

Comment 15

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/releases/mozilla-beta/rev/b62d05922103
https://hg.mozilla.org/releases/mozilla-beta/rev/e808d1df24a8