Closed Bug 1370819 Opened 3 years ago Closed 3 years ago
Assertion failure: m
Flag Synchronous, at XMLHttp Request Main Thread .cpp:2887
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!
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
Assignee: nobody → amarchesini
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-
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.
Postponing the event dispatching.
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 email@example.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/a6428e562e9c Postpone the dispatching of XHR events with opened synchronously, r=smaug
Sounds like we should let this ride the trains?
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
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-
You need to log in before you can comment on or make changes to this bug.