Closed Bug 1411384 Opened 2 years ago Closed 2 years ago
Assertion failure: !m
Event Dispatching Suspended, at /builds/worker/workspace/build/src/dom/xhr/XMLHttp Request Main Thread .cpp:1432
Testcase found while fuzzing mozilla-central rev a80d568a417e.
INFO: Last good revision: 955e237fc290e79eecface60d9b1af4d2abe293b INFO: First bad revision: a6428e562e9c6510e48eaecfa4d74269928d75e6 INFO: Pushlog: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=955e237fc290e79eecface60d9b1af4d2abe293b&tochange=a6428e562e9c6510e48eaecfa4d74269928d75e6
Assignee: nobody → amarchesini
Attachment #8921969 - Flags: review?(bugs)
Could you explain the approach a bit?
(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().
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-
Attachment #8921969 - Attachment is obsolete: true
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.
Attachment #8927286 - Flags: review-
Ops, yes, the test must be updated as well.
Attachment #8927286 - Attachment is obsolete: true
Attachment #8927319 - Flags: review?(bugs) → review+
Pushed by firstname.lastname@example.org: 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
Pushed by email@example.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/f962fb3449f4 nested sync XHR should throw, r=smaug
Is there a user impact which justifies backport consideration here or can this patch ride the 59 train?
(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.
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.