Closed Bug 1370819 Opened 4 years ago Closed 4 years ago

Assertion failure: mFlagSynchronous, at XMLHttpRequestMainThread.cpp:2887

Categories

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

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox-esr52 --- wontfix
firefox54 --- wontfix
firefox55 --- wontfix
firefox56 --- fixed

People

(Reporter: cbook, Assigned: baku)

References

()

Details

(Keywords: assertion)

Attachments

(3 files, 2 obsolete files)

Attached file crash stack
Found via bughunter and reproduced on latest m-c tinderbox debug windows build on windows 7

Steps to reproduce:
-> Load https://customer.bnsf.com/Pages/Home.aspx
---> Assertion failure after about 30 seconds
baku could you take a look ? thanks!
Flags: needinfo?(amarchesini)
I cannot reproduce it. Do you have a crash report?
Flags: needinfo?(amarchesini) → needinfo?(cbook)
(In reply to Andrea Marchesini [:baku] from comment #2)
> I cannot reproduce it. Do you have a crash report?

like a stack, its here https://bug1370819.bmoattachments.org/attachment.cgi?id=8875198
Flags: needinfo?(cbook)
Attached file test case
Assignee: nobody → amarchesini
Attached patch patch (obsolete) — Splinter Review
I don't see why we should call abort() here and not in the other cases.
The issue is that abort() ends up dispatching an event and in this event we can call xhr.open() again changing the sync flag value.

We can also remove the assertion, but if we are closing the window, we don't need to call abort().
Attachment #8877492 - Flags: review?(bugs)
Comment on attachment 8877492 [details] [diff] [review]
patch

Doesn't our mState stay in odd state with this patch, and channel isn't cancelled so we keep network connection open?
Attachment #8877492 - Flags: review?(bugs) → review-
Attached patch xhr_crash.patch (obsolete) — Splinter Review
Attachment #8877492 - Attachment is obsolete: true
Attachment #8877892 - Flags: review?(bugs)
Comment on attachment 8877892 [details] [diff] [review]
xhr_crash.patch

I still can't see how mState handling is correct.
Attachment #8877892 - Flags: review?(bugs) → review-
But feel free to correct me and explain how the state gets changed.
Perhaps OnStopRequest gets called after all and it sets state to done... but if that is the case, readystatechange event would get fired, and it would have the same issue as abort event.
Attached patch xhr_crash.patchSplinter Review
Postponing the event dispatching.
Attachment #8877892 - Attachment is obsolete: true
Attachment #8878045 - Flags: review?(bugs)
Comment on attachment 8878045 [details] [diff] [review]
xhr_crash.patch

Yeah, something like this is good.
Scary, but let's try. But this isn't something I'd use for bet, though perhaps we don't need to fix this for beta.
Make sure to run this through tryserver! I'm a bit surprised if this doesn't break some tests.
Attachment #8878045 - Flags: review?(bugs) → review+
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a6428e562e9c
Postpone the dispatching of XHR events with opened synchronously, r=smaug
https://hg.mozilla.org/mozilla-central/rev/a6428e562e9c
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Sounds like we should let this ride the trains?
Flags: needinfo?(amarchesini)
Comment on attachment 8878045 [details] [diff] [review]
xhr_crash.patch

Approval Request Comment
[Feature/Bug causing the regression]:
[User impact if declined]: When a sync XHR is running, we still dispatch events. These events can be used to modify that XHR in such a way that we can trigger unexpected behavior. Some of them can cause crashes. The solution is to suspend all the events until the sync operation is completed.
[Is this code covered by automated tests?]: no
[Has the fix been verified in Nightly?]: not yet.
[Needs manual test from QE? If yes, steps to reproduce]: there is a testcase attached.
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: not too much, considering that XHR sync is deprecated.
[Why is the change risky/not risky?]: we are changing the timing of event dispatching. This was a relatively easy fix: we store them and when the sync step is completed, we dispatch those events 1 by 1. If we want to keep the events as they are we need to do a big refactoring of XHR code.
[String changes made/needed]: none
Flags: needinfo?(amarchesini)
Attachment #8878045 - Flags: approval-mozilla-beta?
Comment on attachment 8878045 [details] [diff] [review]
xhr_crash.patch

sounds kinda risky, I'd prefer to let this ride with 56.
Attachment #8878045 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
Depends on: 1411384
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.