1370819 - Assertion failure: mFlagSynchronous, at XMLHttpRequestMainThread.cpp:2887

Status: RESOLVED FIXED

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

Description

[Carsten Book [:Tomcat]]

Attachment: 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

Comment 1

[Carsten Book [:Tomcat]]

baku could you take a look ? thanks!

Comment 2

[Andrea Marchesini [:baku]]

I cannot reproduce it. Do you have a crash report?

Comment 3

[Carsten Book [:Tomcat]]

(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

Comment 4

[Andrea Marchesini [:baku]]

Attachment: test case

Comment 5

[Andrea Marchesini [:baku]]

Attachment: patch

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

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

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?

Comment 7

[Andrea Marchesini [:baku]]

Attachment: xhr_crash.patch

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8877892 [details] [diff] [review]
xhr_crash.patch

I still can't see how mState handling is correct.

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

But feel free to correct me and explain how the state gets changed.

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 11

[Andrea Marchesini [:baku]]

Attachment: xhr_crash.patch

Postponing the event dispatching.

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a6428e562e9c
Postpone the dispatching of XHR events with opened synchronously, r=smaug

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/a6428e562e9c

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Sounds like we should let this ride the trains?

Comment 16

[Andrea Marchesini [:baku]]

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

Comment 17

[Julien Cristau [:jcristau]]

Comment on attachment 8878045 [details] [diff] [review]
xhr_crash.patch

sounds kinda risky, I'd prefer to let this ride with 56.