Created attachment 8921596 [details] trigger.html 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
Created attachment 8921969 [details] [diff] [review] xhr.patch
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?
Created attachment 8927286 [details] [diff] [review] xhr2.patch Patch updated.
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.
Created attachment 8927319 [details] [diff] [review] xhr2.patch Ops, yes, the test must be updated as well.
Pushed by email@example.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
Pushed by firstname.lastname@example.org: 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
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-.