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

RESOLVED FIXED in Firefox 59

Status

()

Core
DOM
P2
normal
RESOLVED FIXED
28 days ago
3 days ago

People

(Reporter: jkratzer, Assigned: baku)

Tracking

(Blocks: 1 bug, {assertion, testcase})

56 Branch
mozilla59
assertion, testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox56 wontfix, firefox57 wontfix, firefox58 fix-optional, firefox59 fixed)

Details

Attachments

(4 attachments, 2 obsolete attachments)

(Reporter)

Description

28 days ago
Created attachment 8921596 [details]
trigger.html

Testcase found while fuzzing mozilla-central rev a80d568a417e.
Flags: in-testsuite?
(Reporter)

Comment 1

28 days ago
Created attachment 8921597 [details]
log_minidump.txt
(Reporter)

Comment 2

28 days ago
Created attachment 8921598 [details]
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
status-firefox56: --- → wontfix
status-firefox57: --- → wontfix
status-firefox58: affected → fix-optional
status-firefox-esr52: --- → unaffected
Flags: needinfo?(amarchesini)
Version: 58 Branch → 56 Branch
(Assignee)

Comment 4

27 days ago
Created attachment 8921969 [details] [diff] [review]
xhr.patch
Assignee: nobody → amarchesini
Flags: needinfo?(amarchesini)
Attachment #8921969 - Flags: review?(bugs)

Comment 5

27 days ago
Could you explain the approach a bit?

Updated

26 days ago
Flags: needinfo?(amarchesini)
(Assignee)

Comment 6

26 days 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

26 days 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

26 days 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

11 days ago
Created attachment 8927286 [details] [diff] [review]
xhr2.patch

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-
(Assignee)

Comment 11

11 days ago
Created attachment 8927319 [details] [diff] [review]
xhr2.patch

Ops, yes, the test must be updated as well.
Attachment #8927286 - Attachment is obsolete: true
Flags: needinfo?(bugs)
(Assignee)

Updated

11 days ago
Attachment #8927319 - Flags: review?(bugs)

Updated

11 days ago
Flags: needinfo?(bugs)
Attachment #8927319 - Flags: review?(bugs) → review+

Comment 12

11 days ago
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/35bb5af0f317
nested sync XHR should throw, r=smaug
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

9 days ago
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f962fb3449f4
nested sync XHR should throw, r=smaug
Priority: -- → P2

Comment 15

8 days ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/f962fb3449f4
Status: NEW → RESOLVED
Last Resolved: 8 days ago
status-firefox59: --- → fixed
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+
(Assignee)

Comment 17

5 days 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

5 days 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 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-
You need to log in before you can comment on or make changes to this bug.