Closed
Bug 1411384
Opened 7 years ago
Closed 7 years ago
Assertion failure: !mEventDispatchingSuspended, at /builds/worker/workspace/build/src/dom/xhr/XMLHttpRequestMainThread.cpp:1432
Categories
(Core :: DOM: Core & HTML, defect, P2)
Tracking
()
RESOLVED
FIXED
mozilla59
Tracking | Status | |
---|---|---|
firefox-esr52 | --- | unaffected |
firefox56 | --- | wontfix |
firefox57 | --- | wontfix |
firefox58 | --- | wontfix |
firefox59 | --- | fixed |
People
(Reporter: jkratzer, Assigned: baku)
References
(Blocks 1 open bug)
Details
(Keywords: assertion, testcase)
Attachments
(4 files, 2 obsolete files)
612 bytes,
text/html
|
Details | |
101.95 KB,
text/plain
|
Details | |
9.78 KB,
text/plain
|
Details | |
11.07 KB,
patch
|
smaug
:
review+
gchang
:
approval-mozilla-beta-
|
Details | Diff | Splinter Review |
Testcase found while fuzzing mozilla-central rev a80d568a417e.
Flags: in-testsuite?
Reporter | ||
Comment 1•7 years ago
|
||
Reporter | ||
Comment 2•7 years ago
|
||
Comment 3•7 years ago
|
||
INFO: Last good revision: 955e237fc290e79eecface60d9b1af4d2abe293b INFO: First bad revision: a6428e562e9c6510e48eaecfa4d74269928d75e6 INFO: Pushlog: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=955e237fc290e79eecface60d9b1af4d2abe293b&tochange=a6428e562e9c6510e48eaecfa4d74269928d75e6
Blocks: 1370819
Has Regression Range: --- → yes
status-firefox56:
--- → wontfix
status-firefox57:
--- → wontfix
status-firefox-esr52:
--- → unaffected
Flags: needinfo?(amarchesini)
Version: 58 Branch → 56 Branch
Assignee | ||
Comment 4•7 years ago
|
||
Assignee: nobody → amarchesini
Flags: needinfo?(amarchesini)
Attachment #8921969 -
Flags: review?(bugs)
Comment 5•7 years ago
|
||
Could you explain the approach a bit?
Updated•7 years ago
|
Flags: needinfo?(amarchesini)
Assignee | ||
Comment 6•7 years ago
|
||
(In reply to Olli Pettay [:smaug] from comment #5) > Could you explain the approach a bit? This patch does a few things: 1. it detects a sync loop into sync loop. This is done creating a unique ID per loop (mSyncLoopId). When the spinning loop is completed, XHR checks if the current sync loop is what started, and if not, it returns an error. This means that if we have 1 XHR, starting a sync send, and here, another sync XHR.send() is called, when both are completed, the first loop throws an error. The spec says that calling open(), we need to terminate the existing operations, but this cannot be directly done if we are into a sync send(). Using mSyncLoopId, we make the sync send() able to throw when completed. 2. each sync XHR send() must block and restore the dispatching of the input events and timers of the current document. This is done extending UnsuppressEventHandlingAndResume. 3. a test to check 2 sync nested send()s. and a non-sync send() into a sync send().
Flags: needinfo?(amarchesini)
Comment 7•7 years ago
|
||
Why we need all this complicated setup? Why not just throw in open() and/or send() if sync XHR is already active?
Comment 8•7 years ago
|
||
Comment on attachment 8921969 [details] [diff] [review] xhr.patch I don't really see reason for this setup, given what kind of sync XHR implementation we have atm. Or am I missing something?
Attachment #8921969 -
Flags: review?(bugs) → review-
Assignee | ||
Comment 9•7 years ago
|
||
Patch updated.
Attachment #8921969 -
Attachment is obsolete: true
Flags: needinfo?(bugs)
Comment 10•7 years ago
|
||
Comment on attachment 8927286 [details] [diff] [review] xhr2.patch I don't understand the test. test_syncVsSync and test_syncVsAsync are doing exactly the same things. I assume test_syncVsAsync was supposed to use async XHR, but it isn't.
Flags: needinfo?(bugs)
Attachment #8927286 -
Flags: review-
Assignee | ||
Comment 11•7 years ago
|
||
Ops, yes, the test must be updated as well.
Attachment #8927286 -
Attachment is obsolete: true
Flags: needinfo?(bugs)
Assignee | ||
Updated•7 years ago
|
Attachment #8927319 -
Flags: review?(bugs)
Updated•7 years ago
|
Flags: needinfo?(bugs)
Attachment #8927319 -
Flags: review?(bugs) → review+
Comment 12•7 years ago
|
||
Pushed by amarchesini@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/35bb5af0f317 nested sync XHR should throw, r=smaug
Comment 13•7 years ago
|
||
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/d138e03573ff for intermittently finding a path through the test code which doesn't involve actually doing anything, https://treeherder.mozilla.org/logviewer.html#?job_id=143856625&repo=mozilla-inbound
Comment 14•7 years ago
|
||
Pushed by amarchesini@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/f962fb3449f4 nested sync XHR should throw, r=smaug
Updated•7 years ago
|
Priority: -- → P2
Comment 15•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/f962fb3449f4
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox59:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Comment 16•6 years ago
|
||
Is there a user impact which justifies backport consideration here or can this patch ride the 59 train?
Flags: needinfo?(amarchesini)
Flags: in-testsuite?
Flags: in-testsuite+
Assignee | ||
Comment 17•6 years ago
|
||
(In reply to Ryan VanderMeulen [:RyanVM] from comment #16) > Is there a user impact which justifies backport consideration here or can > this patch ride the 59 train? This is nice to have in beta. We are not talking of crashing, but wrong behavior in sync XHR.
Flags: needinfo?(amarchesini)
Assignee | ||
Comment 18•6 years ago
|
||
Comment on attachment 8927319 [details] [diff] [review] xhr2.patch Approval Request Comment [Feature/Bug causing the regression]: sync XHR [User impact if declined]: Wrong behavior of nested sync XHR. [Is this code covered by automated tests?]: yes [Has the fix been verified in Nightly?]: we have tests. [Needs manual test from QE? If yes, steps to reproduce]: no. [List of other uplifts needed for the feature/fix]: none. [Is the change risky?]: no. [Why is the change risky/not risky?]: Just making assertions when methods are used in nested sync XHR. [String changes made/needed]: none
Attachment #8927319 -
Flags: approval-mozilla-beta?
Comment 19•6 years ago
|
||
Comment on attachment 8927319 [details] [diff] [review] xhr2.patch Per comment #16 & #17, this is nice to have. So, we can let this ride the 59 train. Beta58-.
Attachment #8927319 -
Flags: approval-mozilla-beta? → approval-mozilla-beta-
Updated•6 years ago
|
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•