1585068 - Assertion failure: !mCx->mPendingUnhandledRejections.Lookup(promiseID), at xpcom/base/CycleCollectedJSContext.cpp:751

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect, P2)

Description

[Bob Clary [:bc] (inactive)]

1. https://www.t-online.de/unterhaltung/tv/id_86518080/-bares-fuer-rares-wie-haendler-lucki-77-ueber-den-tv-ruhestand-denkt.html and 8 other urls in the last week.

2. Assertion failure: !mCx->mPendingUnhandledRejections.Lookup(promiseID), at /builds/worker/workspace/build/src/xpcom/base/CycleCollectedJSContext.cpp:751

Appears to assert on load. Bughunter reproduces on Linux and Windows on Nightly. I also reproduced on Beta so this is not an extremely recent regression if it is one at all.

S-S since CycleCollectedJSContext scares me.

Assertion failure: !mCx->mPendingUnhandledRejections.Lookup(promiseID), at /builds/worker/workspace/build/src/xpcom/base/CycleCollectedJSContext.cpp:751
#01: nsThread::ProcessNextEvent(bool, bool*) (/mozilla/builds/nightly/mozilla/firefox-debug/dist/bin/libxul.so)
#02: NS_ProcessNextEvent(nsIThread*, bool) (/mozilla/builds/nightly/mozilla/firefox-debug/dist/bin/libxul.so)
#03: mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) (/mozilla/builds/nightly/mozilla/firefox-debug/dist/bin/libxul.so)
#04: MessageLoop::RunInternal() (/mozilla/builds/nightly/mozilla/firefox-debug/dist/bin/libxul.so)
#05: MessageLoop::Run() (/mozilla/builds/nightly/mozilla/firefox-debug/dist/bin/libxul.so)
#06: nsBaseAppShell::Run() (/mozilla/builds/nightly/mozilla/firefox-debug/dist/bin/libxul.so)
#07: XRE_RunAppShell() (/mozilla/builds/nightly/mozilla/firefox-debug/dist/bin/libxul.so)
#08: mozilla::ipc::MessagePumpForChildProcess::Run(base::MessagePump::Delegate*) (/mozilla/builds/nightly/mozilla/firefox-debug/dist/bin/libxul.so)
#09: MessageLoop::RunInternal() (/mozilla/builds/nightly/mozilla/firefox-debug/dist/bin/libxul.so)

Comment 1

[Andrew McCreight [:mccr8]]

Edgar, how bad is this assertion? Thanks.

Comment 2

[Edgar Chen [:edgar]]

Hitting this assertion means the promise doesn't be removed from hashtable expectedly at a certain point and a leak could happen given that we won't have a chance to remove it again later. I tried to reproduce this with the URL provided in the above comment, but no luck.

Comment 3

[Bob Clary [:bc] (inactive)]

I've reproduced with 2019-10-03 and with today's nightly on Fedora 30 with and without pop up blocking enabled. It asserts on load after a 10 seconds or so for me.

Comment 4

[Emilio Cobos Álvarez (:emilio)]

Edgar, I can repro this consistently, see bug 1585226 for STR. That requires a chromium tree around but I can provide you a zipfile with the relevant files otherwise.

FWIW I poked a bit at it and the "unhandled" flag was removed shortly after calling the tracker callback in:

(rr) p DumpJSStack()
0 promise_test/tests.promise_tests</<(resolve = [function], [function]) ["file:///home/emilio/src/chromium/src/third_party/blink/web_tests/resources/testharness.js":612:21]
    this = [object Window]
1 promise_test/tests.promise_tests<(undefined) ["file:///home/emilio/src/chromium/src/third_party/blink/web_tests/resources/testharness.js":590:19]
    this = [object Window]

$5 = void

Comment 5

[Emilio Cobos Álvarez (:emilio)]

Attachment: Stack for the rejection tracker call

Unfortunately gdb was choking when callink DumpJSStack() at this point so I don't have the JS stack.

Comment 6

[Emilio Cobos Álvarez (:emilio)]

Attachment: Stack for the promise flags changing

This corresponds to the js stack pasted in my previous comment. In any case, it seems like it should be safe to just remove it from the hash table unconditionally? Though I don't know this code enough to be sure.

Comment 7

[Emilio Cobos Álvarez (:emilio)]

Oh, managed to get the js stack from comment 5, which is the same from comment 4.

Comment 8

[Emilio Cobos Álvarez (:emilio)]

Ah, the call in comment 5 is with State::Handled, but there is a previous call with State::Unhandled which adds it to the hash map for the same promise.

So probably the right thing to do is removing it from the map when we get the second Handled call, or something like that.

JS stack for the initial unhandled thing:

0 anonymous([object Object]) ["file:///home/emilio/src/chromium/src/third_party/blink/web_tests/gamepad/gamepad-polling-access.html":153:4]
1 Test.prototype.step(func = [function], this_obj = [object Object], [object Object]) ["file:///home/emilio/src/chromium/src/third_party/blink/web_tests/resources/testharness.js":1587:24]
this = [object Object]
2 promise_test/tests.promise_tests</<(resolve = [function], [function]) ["file:///home/emilio/src/chromium/src/third_party/blink/web_tests/resources/testharness.js":591:35]
this = [object Window]
3 promise_test/tests.promise_tests<(undefined) ["file:///home/emilio/src/chromium/src/third_party/blink/web_tests/resources/testharness.js":590:19]
this = [object Window]

Comment 9

[Emilio Cobos Álvarez (:emilio)]

This fixes it for me:

diff --git a/xpcom/base/CycleCollectedJSContext.cpp b/xpcom/base/CycleCollectedJSContext.cpp
index e63a286681be..9a8e6d33cd18 100644
--- a/xpcom/base/CycleCollectedJSContext.cpp
+++ b/xpcom/base/CycleCollectedJSContext.cpp
@@ -367,8 +367,7 @@ void CycleCollectedJSContext::PromiseRejectionTrackerCallback(
     }
   } else {
     PromiseDebugging::AddConsumedRejection(aPromise);
-    if (mozilla::StaticPrefs::dom_promise_rejection_events_enabled() &&
-        !aMutedErrors) {
+    if (mozilla::StaticPrefs::dom_promise_rejection_events_enabled()) {
       for (size_t i = 0; i < aboutToBeNotified.Length(); i++) {
         if (aboutToBeNotified[i] &&
             aboutToBeNotified[i]->PromiseObj() == aPromise) {
@@ -383,7 +382,7 @@ void CycleCollectedJSContext::PromiseRejectionTrackerCallback(
       }
       RefPtr<Promise> promise;
       unhandled.Remove(promiseID, getter_AddRefs(promise));
-      if (!promise) {
+      if (!promise && !aMutedErrors) {
         nsIGlobalObject* global = xpc::NativeGlobal(aPromise);
         if (nsCOMPtr<EventTarget> owner = do_QueryInterface(global)) {
           RootedDictionary<PromiseRejectionEventInit> init(aCx);

Not sure if it's right though.

Comment 10

[Emilio Cobos Álvarez (:emilio)]

Here's a trivial test-case that asserts:

<!doctype html>
<script src="testing/web-platform/tests/resources/testharness.js"></script>
<script>
promise_test(async () => {
  somethingThatDoesNotExist()
}, "Bar");
</script>

Comment 12

[Emilio Cobos Álvarez (:emilio)]

For some reason just inlining the testharness.js thing in a script doesn't crash, weird.

Comment 13

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1585068 - Cleanup rejections even if errors are muted.

For some reason the crashtest works if ran locally like:

./mach run dom/base/crashtests/1585068.html

But not from the crashtest suite. I assume the "muted errors" path is only for
remote (not same-origin) scripts? (and that's why it didn't trigger in regular
wpt?).

Any good way to test this in that case?

Comment 14

[Edgar Chen [:edgar]]

Okay, so the promise was rejected in the current script and then being handled in a cross-origin script., I missed this possibility while I tried to write some tests to reproduce it. Thank you, Emilio!!!

Comment 15

[Edgar Chen [:edgar]]

Attachment: Bug 1585068 - Add WPT for promise rejections event for cross-origin script;

Comment 16

[BugBot [:suhaib / :marco/ :calixte]]

Bugbug thinks this bug is a regression, but please revert this change in case of error.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/cd16bcbc7025
Cleanup rejections even if errors are muted. r=edgar,smaug
https://hg.mozilla.org/integration/autoland/rev/303f2802f3e8
Add WPT for promise rejections event for cross-origin script; r=smaug

Comment 18

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/19700 for changes under testing/web-platform/tests

Comment 19

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.

Comment 20

[Oana Pop-Rus]

https://hg.mozilla.org/mozilla-central/rev/cd16bcbc7025
https://hg.mozilla.org/mozilla-central/rev/303f2802f3e8

Comment 21

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged by moz-wptsync-bot