1612928 - Assertion failure: !mSyncLoopTarget, at /builds/worker/workspace/build/src/dom/xhr/XMLHttpRequestWorker.cpp:839

Status: RESOLVED FIXED

(Core :: DOM: Networking, defect, P2)

Description

[Jason Kratzer [:jkratzer]]

Attachment: testcase.html

Testcase found while fuzzing mozilla-central rev f4e3917a0fa1.

Assertion failure: !mSyncLoopTarget, at /builds/worker/workspace/build/src/dom/xhr/XMLHttpRequestWorker.cpp:839

rax = 0x000056118938a340   rdx = 0x0000000000000000
rcx = 0x00007ffbc707ddca   rbx = 0x00007ffba3004c90
rsi = 0x00007ffbd2b358b0   rdi = 0x00007ffbd2b34680
rbp = 0x00007ffce720a7d0   rsp = 0x00007ffce720a780
r8 = 0x00007ffbd2b358b0    r9 = 0x00007ffbd3c9c780
r10 = 0x0000000000000000   r11 = 0x0000000000000000
r12 = 0x0000000000000000   r13 = 0x00007ffce720a788
r14 = 0x00007ffce720a808   r15 = 0x00007ffba2e25e80
rip = 0x00007ffbc31c821c
OS|Linux|0.0.0 Linux 5.3.0-28-generic #30~18.04.1-Ubuntu SMP Fri Jan 17 06:14:09 UTC 2020 x86_64
CPU|amd64|family 6 model 94 stepping 3|8
GPU|||
Crash|SIGSEGV|0x0|0
0|0|libxul.so|mozilla::dom::Proxy::Teardown(bool)|hg:hg.mozilla.org/mozilla-central:dom/xhr/XMLHttpRequestWorker.cpp:f4e3917a0fa15fb009e0ec4403ea7d066ee0e3c0|816|0x0
0|1|libxul.so|AsyncTeardownRunnable::Run|hg:hg.mozilla.org/mozilla-central:dom/xhr/XMLHttpRequestWorker.cpp:f4e3917a0fa15fb009e0ec4403ea7d066ee0e3c0|350|0x13
0|2|libxul.so|mozilla::ThrottledEventQueue::Inner::ExecuteRunnable()|hg:hg.mozilla.org/mozilla-central:xpcom/threads/ThrottledEventQueue.cpp:f4e3917a0fa15fb009e0ec4403ea7d066ee0e3c0|252|0x12
0|3|libxul.so|mozilla::ThrottledEventQueue::Inner::Executor::Run()|hg:hg.mozilla.org/mozilla-central:xpcom/threads/ThrottledEventQueue.cpp:f4e3917a0fa15fb009e0ec4403ea7d066ee0e3c0|80|0xd
0|4|libxul.so|nsThread::ProcessNextEvent(bool, bool*)|hg:hg.mozilla.org/mozilla-central:xpcom/threads/nsThread.cpp:f4e3917a0fa15fb009e0ec4403ea7d066ee0e3c0|1220|0xe
0|5|libxul.so|NS_ProcessNextEvent(nsIThread*, bool)|hg:hg.mozilla.org/mozilla-central:xpcom/threads/nsThreadUtils.cpp:f4e3917a0fa15fb009e0ec4403ea7d066ee0e3c0|486|0x11
0|6|libxul.so|mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*)|hg:hg.mozilla.org/mozilla-central:ipc/glue/MessagePump.cpp:f4e3917a0fa15fb009e0ec4403ea7d066ee0e3c0|87|0xa
0|7|libxul.so|MessageLoop::RunInternal()|hg:hg.mozilla.org/mozilla-central:ipc/chromium/src/base/message_loop.cc:f4e3917a0fa15fb009e0ec4403ea7d066ee0e3c0|315|0x19
0|8|libxul.so|MessageLoop::Run()|hg:hg.mozilla.org/mozilla-central:ipc/chromium/src/base/message_loop.cc:f4e3917a0fa15fb009e0ec4403ea7d066ee0e3c0|290|0x8
0|9|libxul.so|nsBaseAppShell::Run()|hg:hg.mozilla.org/mozilla-central:widget/nsBaseAppShell.cpp:f4e3917a0fa15fb009e0ec4403ea7d066ee0e3c0|137|0xd
0|10|libxul.so|nsAppStartup::Run()|hg:hg.mozilla.org/mozilla-central:toolkit/components/startup/nsAppStartup.cpp:f4e3917a0fa15fb009e0ec4403ea7d066ee0e3c0|272|0x10
0|11|libxul.so|XREMain::XRE_mainRun()|hg:hg.mozilla.org/mozilla-central:toolkit/xre/nsAppRunner.cpp:f4e3917a0fa15fb009e0ec4403ea7d066ee0e3c0|4560|0x16
0|12|libxul.so|XREMain::XRE_main(int, char**, mozilla::BootstrapConfig const&)|hg:hg.mozilla.org/mozilla-central:toolkit/xre/nsAppRunner.cpp:f4e3917a0fa15fb009e0ec4403ea7d066ee0e3c0|4695|0x8
0|13|libxul.so|XRE_main(int, char**, mozilla::BootstrapConfig const&)|hg:hg.mozilla.org/mozilla-central:toolkit/xre/nsAppRunner.cpp:f4e3917a0fa15fb009e0ec4403ea7d066ee0e3c0|4746|0x5
0|14|firefox-bin|do_main|hg:hg.mozilla.org/mozilla-central:browser/app/nsBrowserApp.cpp:f4e3917a0fa15fb009e0ec4403ea7d066ee0e3c0|217|0x26
0|15|firefox-bin|main|hg:hg.mozilla.org/mozilla-central:browser/app/nsBrowserApp.cpp:f4e3917a0fa15fb009e0ec4403ea7d066ee0e3c0|339|0xf
0|16|libc-2.27.so||||0x21b97
0|17|firefox-bin|__cxa_throw_bad_array_new_length|hg:hg.mozilla.org/mozilla-central:build/unix/stdc++compat/stdc++compat.cpp:f4e3917a0fa15fb009e0ec4403ea7d066ee0e3c0|82|0x12
0|18|firefox-bin||||0x10e30
0|19|ld-2.27.so||||0x10733
0|20|libdl-2.27.so||||0x202d80
0|21|libpthread-2.27.so||||0x219bb0
0|22|firefox-bin||||0x10e30
0|23|firefox-bin|_start|||0x29

Comment 1

[Jason Kratzer [:jkratzer]]

Attachment: worker.js

Comment 2

[Tyson Smith [:tsmith]]

A Pernosco session is available here: https://pernos.co/debug/cmjv-vpfxcTHvUOLX2c3VQ/index.html

Comment 3

[Jason Kratzer [:jkratzer]]

Bugmon Analysis:
Verified bug as reproducible on mozilla-central 20200629154604-31fb4a2a6912.
Failed to bisect testcase (Start build crashes!):
> Start: 501f50ccafdfd30a6bc8d85f2be8704ca335d65f (20190701095159)
> End: 6d46db840d6e71ffa2c416ae018e27e921eac6e0 (20200203040425)
> BuildFlags: BuildFlags(asan=False, tsan=False, debug=True, fuzzing=False, coverage=False, valgrind=False)

Comment 4

[Tyson Smith [:tsmith]]

Jens, the fuzzers have been tripping over this fuzzblocker for a while now. Is anyone available to have a look?

Comment 5

[Jens Stutte [:jstutte]]

Hmm, the crash points to a MOZ_CRASH("We're going to hang at shutdown anyways.");, which looks questionable to me.

Actually I'd like to see us hanging with clear STR.

Comment 6

[Jens Stutte [:jstutte]]

Attachment: Bug 1612928: Ensure mSyncLoopTarget is null at the end of Teardown to avoid assertion and change some MOZ_CRASH to more graceful behavior. r=#dom-workers-and-storage-reviewers

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ad36a79133a3
Ensure mSyncLoopTarget is null at the end of Teardown to avoid assertion and change some MOZ_CRASH to more graceful behavior. r=dom-workers-and-storage-reviewers,asuth

Comment 8

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/ad36a79133a3

Comment 9

[Andrew Sutherland [:asuth] (he/him)]

:jens, I think you actually fixed the underlying problem here by my analysis in https://phabricator.services.mozilla.com/D97010#3247133 so I think this can probably be marked fixed (in Firefox 86 per the commit)? That is, I don't think we're actually expecting any new crash signatures from the patch as it landed. (In theory the MOZ_ALWAYS_TRUE(runnable->Dispatch()) could crash via MOZ_DIAGNOSTIC_ASSERT, but I'm not seeing any crashes in XMLHttpRequestWorker against version >=86 and I'm likewise not seeing any "moz crash reason" with Dispatch in it involving XHR.)

Comment 10

[Jens Stutte [:jstutte]]

Jason, can you confirm this through the test-case ?
Andrew, should we create a regression test from that test case then?

Comment 11

[Tyson Smith [:tsmith]]

This issue was last reported by fuzzers running m-c 20201217-2ab4142f19bc. Prior to that it was hit frequently. I don't see any obvious variations.

Comment 12

[Andrew Sutherland [:asuth] (he/him)]

This particular scenario was deterministic and so a deterministic test can be landed. That said, there's the meta question of whether it makes sense to land specific test cases that the fuzzer already reliably generates coverage for (hence this having been a fuzzblocker.)

The linearized deterministic worker logic then becomes:

self.onmessage = function (e) {
  const xhr = new XMLHttpRequest()
  xhr.open('G', '', false)
  xhr.send(undefined)
  // TODO: wrap this in a catch as it will throw.
  xhr.send(undefined)
  // TODO: postMessage back to the parent to indicate the test is complete.
}

and the worker creation from the document no longer needs to attempt to reload the worker, and would become:

        const worker = new Worker('worker.js')
        worker.postMessage([], [])
// TODO: Await a postMessage from the worker following the 2nd call to send before declaring the test over.

Comment 13

[Jason Kratzer [:jkratzer]]

Bugmon Analysis:
The bug appears to have been fixed in the following build range:

Start: 28b7a2b995c32e55107c8b41722396bbbe219565 (20201217150734)
End: ad36a79133a39117d6982cbd2e948b55a9ae0675 (20201217150757)
Pushlog: https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=28b7a2b995c32e55107c8b41722396bbbe219565&tochange=ad36a79133a39117d6982cbd2e948b55a9ae0675
Removing bugmon keyword as no further action possible.
Please review the bug and re-add the keyword for further analysis.

Comment 14

[Jens Stutte [:jstutte]]

Assuming then from comment 11, comment 12 and comment 13 that we have confidence that the issue has been fixed and that fuzzers will rediscover it in case of regressions (as Andrew already stated in the testing exception comment in the patch).