Closed Bug 1522339 Opened 9 months ago Closed 9 months ago

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

Categories

(Core :: JavaScript Engine, defect, critical)

x86_64
Linux
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla67
Tracking Status
firefox-esr60 --- unaffected
firefox65 --- unaffected
firefox66 --- fixed
firefox67 --- fixed

People

(Reporter: gkw, Assigned: arai)

References

(Blocks 2 open bugs)

Details

(4 keywords, Whiteboard: [jsbugmon:update])

Attachments

(3 files)

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.

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 .

Flags: needinfo?(arai.unmht)

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

Assignee: nobody → arai.unmht
Status: NEW → ASSIGNED

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.

Flags: needinfo?(arai.unmht)
Attachment #9039435 - Attachment description: Bug 1522339 - Part 1: Do not assert the result of the delete operation when tracking unhandled promise rejections. r?jorendorff → Bug 1522339 - Part 1: Do not assert the result of the delete operation when tracking unhandled promise rejections. r=jorendorff
Attachment #9039436 - Attachment description: Bug 1522339 - Part 2: Throw error when oomTest is used with promises while unhandled rejection tracking is enabled. r?jorendorff → Bug 1522339 - Part 2: Throw error when oomTest is used with promises while unhandled rejection tracking is enabled. r=jorendorff
Pushed by arai_a@mac.com:
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
Status: ASSIGNED → RESOLVED
Closed: 9 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla67

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

Flags: needinfo?(arai.unmht)

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

Flags: needinfo?(arai.unmht) → needinfo?(nth10sd)

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

Flags: needinfo?(nth10sd) → needinfo?(lhenry)

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

Attachment #9039435 - Flags: approval-mozilla-beta?
Attachment #9039436 - Flags: approval-mozilla-beta?

Good to know, thanks arai and gary!

Flags: needinfo?(lhenry)

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.

Attachment #9039435 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #9039436 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.