Closed Bug 1588248 Opened 1 year ago Closed 8 months ago

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

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla72
Tracking Status
firefox-esr68 74+ fixed
firefox67 --- wontfix
firefox68 --- wontfix
firefox69 --- wontfix
firefox70 --- wontfix
firefox71 --- fixed
firefox72 --- fixed

People

(Reporter: tsmith, Assigned: baku)

References

(Blocks 1 open bug)

Details

(Keywords: assertion, regression, testcase, Whiteboard: [necko-triaged])

Attachments

(3 files)

Attached file 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
Flags: in-testsuite?
Attached file worker.js

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

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?

Flags: needinfo?(amarchesini)

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

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

Yes please. Thank you!

Flags: needinfo?(amarchesini) → needinfo?(twsmith)

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

Flags: needinfo?(twsmith)

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.

Assignee: nobody → amarchesini
Priority: -- → P2
Whiteboard: [necko-triaged]
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2956fc669361
XHR.send() on workers must throw an InvalidStateError if called during another xhr.send(), r=smaug
Status: NEW → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla72

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

Flags: needinfo?(amarchesini)

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
Flags: needinfo?(amarchesini)
Attachment #9103051 - Flags: approval-mozilla-beta?

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.

Attachment #9103051 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Duplicate of this bug: 1228890

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

Status: RESOLVED → REOPENED
Resolution: FIXED → ---

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

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.
Attachment #9103051 - Flags: approval-mozilla-esr68?

(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.

Status: REOPENED → RESOLVED
Closed: 1 year ago8 months ago
Flags: needinfo?(jkratzer)
Resolution: --- → FIXED

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.

Attachment #9103051 - Flags: approval-mozilla-esr68? → approval-mozilla-esr68+

Created new bug 1618675 to track remaining fixes.

Flags: needinfo?(jkratzer)
Duplicate of this bug: 1228890

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

Keywords: regression
You need to log in before you can comment on or make changes to this bug.