Closed Bug 1411384 Opened 2 years ago Closed 2 years ago

Assertion failure: !mEventDispatchingSuspended, at /builds/worker/workspace/build/src/dom/xhr/XMLHttpRequestMainThread.cpp:1432

Categories

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

56 Branch
defect

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)

Attached file trigger.html
Testcase found while fuzzing mozilla-central rev a80d568a417e.
Flags: in-testsuite?
Attached file log_minidump.txt
Attached file log_stderr.txt
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
Flags: needinfo?(amarchesini)
Version: 58 Branch → 56 Branch
Attached patch xhr.patch (obsolete) — Splinter Review
Assignee: nobody → amarchesini
Flags: needinfo?(amarchesini)
Attachment #8921969 - Flags: review?(bugs)
Could you explain the approach a bit?
Flags: needinfo?(amarchesini)
(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)
Why we need all this complicated setup? Why not just throw in open() and/or send() if sync XHR is already active?
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-
Attached patch xhr2.patch (obsolete) — Splinter Review
Patch updated.
Attachment #8921969 - Attachment is obsolete: true
Flags: needinfo?(bugs)
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-
Attached patch xhr2.patchSplinter Review
Ops, yes, the test must be updated as well.
Attachment #8927286 - Attachment is obsolete: true
Flags: needinfo?(bugs)
Attachment #8927319 - Flags: review?(bugs)
Flags: needinfo?(bugs)
Attachment #8927319 - Flags: review?(bugs) → review+
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
Priority: -- → P2
https://hg.mozilla.org/mozilla-central/rev/f962fb3449f4
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
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+
(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)
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 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-
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.