1588248 - Assertion failure: !mProxy->mSyncLoopTarget, at src/dom/xhr/XMLHttpRequestWorker.cpp:1416

Status: RESOLVED FIXED

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

Description

[Tyson Smith [:tsmith]]

Attachment: testcase.html

Reduced with m-c (build with --enable-debug):
BuildID=20191010092637
SourceStamp=f20fa8068ec25d685914a5945f2c54e36c89bc65

This issue is frequently hit by fuzzers.

The test case must be served via a web server to trigger the issue.

Assertion failure: !mProxy->mSyncLoopTarget, at src/dom/xhr/XMLHttpRequestWorker.cpp:1416

#0 mozilla::dom::SendRunnable::RunOnMainThread(mozilla::ErrorResult&) src/dom/xhr/XMLHttpRequestWorker.cpp:1416:3
#1 mozilla::dom::WorkerThreadProxySyncRunnable::MainThreadRun() src/dom/xhr/XMLHttpRequestWorker.cpp:1259:3
#2 mozilla::dom::WorkerMainThreadRunnable::Run() src/dom/workers/WorkerRunnable.cpp:575:20
#3 mozilla::ThrottledEventQueue::Inner::ExecuteRunnable() src/xpcom/threads/ThrottledEventQueue.cpp:252:22
#4 mozilla::ThrottledEventQueue::Inner::Executor::Run() src/xpcom/threads/ThrottledEventQueue.cpp:80:15
#5 nsThread::ProcessNextEvent(bool, bool*) src/xpcom/threads/nsThread.cpp:1225:14
#6 NS_ProcessNextEvent(nsIThread*, bool) src/xpcom/threads/nsThreadUtils.cpp:486:10
#7 mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) src/ipc/glue/MessagePump.cpp:88:21
#8 MessageLoop::RunInternal() src/ipc/chromium/src/base/message_loop.cc:315:10
#9 MessageLoop::Run() src/ipc/chromium/src/base/message_loop.cc:290:3
#10 nsBaseAppShell::Run() src/widget/nsBaseAppShell.cpp:137:27
#11 XRE_RunAppShell() src/toolkit/xre/nsEmbedFunctions.cpp:934:20
#12 mozilla::ipc::MessagePumpForChildProcess::Run(base::MessagePump::Delegate*) src/ipc/glue/MessagePump.cpp:238:9
#13 MessageLoop::RunInternal() src/ipc/chromium/src/base/message_loop.cc:315:10
#14 MessageLoop::Run() src/ipc/chromium/src/base/message_loop.cc:290:3
#15 XRE_InitChildProcess(int, char**, XREChildData const*) src/toolkit/xre/nsEmbedFunctions.cpp:769:34
#16 content_process_main(mozilla::Bootstrap*, int, char**) src/browser/app/../../ipc/contentproc/plugin-container.cpp:56:28
#17 main src/browser/app/nsBrowserApp.cpp:272:18

Comment 1

[Tyson Smith [:tsmith]]

Attachment: worker.js

Comment 2

[Tyson Smith [:tsmith]]

If a Pernosco session would be helpful please let me know.

Comment 3

[Michal Novotny [:michal]]

mProxy->mSyncLoopTarget is set in SendRunnable::RunOnMainThread() which performs XMLHttpRequest::send() at worker.js:5 and isn't cleared, so when SendRunnable is created for XMLHttpRequest::send() at worker.js:6 the aProxy->mSyncLoopTarget is non-null. I'm not sure how this is supposed to work, Proxy::mSyncLoopTarget is nulled out at https://searchfox.org/mozilla-central/rev/17756e2a5c180d980a4b08d99f8cc0c97290ae8d/dom/xhr/XMLHttpRequestWorker.cpp#849 and https://searchfox.org/mozilla-central/rev/17756e2a5c180d980a4b08d99f8cc0c97290ae8d/dom/xhr/XMLHttpRequestWorker.cpp#986 but none of them is executed between worker.js:5 and worker.js:6. Baku, is there anybody from DOM team who can have a look at it?

Comment 4

[Andrea Marchesini [:baku]]

(In reply to Tyson Smith [:tsmith] from comment #2)

If a Pernosco session would be helpful please let me know.

Yes please. Thank you!

Comment 5

[Andrea Marchesini [:baku]]

Actually it's easy to reproduce. I don't need the Pernosco session.

The problem is that we try to perform multiple Send(), instead of throwing InvalidStateError. See:

https://xhr.spec.whatwg.org/#dom-xmlhttprequest-send
4.5.6.2. If the send() flag is set, then throw an "InvalidStateError" DOMException.

We don't check the mStateData.mFlagSend, probably here: https://searchfox.org/mozilla-central/rev/17756e2a5c180d980a4b08d99f8cc0c97290ae8d/dom/xhr/XMLHttpRequestWorker.cpp#1955

Comment 6

[Kershaw Chang [:kershaw]]

baku, I'd like to assign this to you, since it seems you already know what to do.
If you are too busy feel free to bounce this back.

Comment 7

[Andrea Marchesini [:baku]]

Attachment: Bug 1588248 - XHR.send() on workers must throw an InvalidStateError if called during another xhr.send(), r?smaug

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2956fc669361
XHR.send() on workers must throw an InvalidStateError if called during another xhr.send(), r=smaug

Comment 9

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/2956fc669361

Comment 10

[Pascal Chevrel:pascalc (PTO until April 26)]

Baku, is it worth uplifting this to 71 beta or should it ride the 72 train? Thanks.

Comment 11

[Andrea Marchesini [:baku]]

Comment on attachment 9103051 [details]
Bug 1588248 - XHR.send() on workers must throw an InvalidStateError if called during another xhr.send(), r?smaug

Beta/Release Uplift Approval Request

• User impact if declined: A crash can occur in debug build. In release build the behavior would be wrong.
• Is this code covered by automated tests?: Yes
• 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): It's just a missing check. The spec enforces a check in the send-state, but in XHR for workers that check was missing.
• String changes made/needed: none

Comment 12

[Pascal Chevrel:pascalc (PTO until April 26)]

Comment on attachment 9103051 [details]
Bug 1588248 - XHR.send() on workers must throw an InvalidStateError if called during another xhr.send(), r?smaug

Low risk stability fix, has tests, verified on nighty, uplift approved for 71 beta 4, thanks.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/6252f0b78cf5

Comment 15

[Jason Kratzer [:jkratzer]]

This testcase still reproduces on mozilla-central rev 5647ec4ba6f2.

Comment 16

[Tyson Smith [:tsmith]]

A Pernosco session is available here: https://pernos.co/debug/7Rtn_SF00MS4gyBSKv6nhA/index.html
The session will expire in 7 days.

Comment 17

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

Comment on attachment 9103051 [details]
Bug 1588248 - XHR.send() on workers must throw an InvalidStateError if called during another xhr.send(), r?smaug

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: This correctness fix for Sync XHR makes it a lot easier to reason about what's going on and effectively is a precondition for uplifting other correctness fixes from fuzzers. It's also a general performance win because the syncloop is guaranteed to fail when the runnable to gets to the main thread, but in the process it blocks the worker.
• User impact if declined: Slower workers, may preclude uplifting other important fixes.
• Fix Landed on Version: 71
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): This was a straightforward fix with a patch that has been in the tree since 72 and its uplift to 71.
• String or UUID changes made by this patch: None.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

(In reply to Jason Kratzer [:jkratzer] from comment #15)

This testcase still reproduces on mozilla-central rev 5647ec4ba6f2.

Please file a new bug for this for the sake of better tracking of what landed here.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9103051 [details]
Bug 1588248 - XHR.send() on workers must throw an InvalidStateError if called during another xhr.send(), r?smaug

Fixes some sync XHR issues. Approved for 68.6esr.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr68/rev/a34778097505

Comment 21

[Jason Kratzer [:jkratzer]]

Created new bug 1618675 to track remaining fixes.

Comment 23

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

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