697151 - Synchronous XMLHttpRequest (XHR) does not block readyState events for async XHR

Status: RESOLVED FIXED

(Core :: DOM: Networking, defect, P2)

Description

[FARKAS Máté]

Attachment: Code sample

User Agent: Mozilla/5.0 (Windows NT 6.1; rv:7.0.1) Gecko/20100101 Firefox/7.0.1
Build ID: 20110928134238

Steps to reproduce:

Send an asyncronous AJAX, wait 1 second, then send a syncronous AJAX.
The server waits 2 seconds before reply.


Actual results:

The asyncronous response fired while I was waiting for the response of the syncronous request.


Expected results:

Wait for the syncronous response, then receive the asynchron response.

In other browsers (IE9, Opera11, Chrome 14, Safari 5.1) it is:
async ---> (0 sec)
SYNC ----> (1 sec)
<---- SYNC (3 sec)
<--- async (3 sec)

But in XULRunner and Firefox it is:
async ---> (0 sec)
SYNC ----> (1 sec)
<--- async (2 sec)
<---- SYNC (3 sec)

I think, it breaks the JavaScript philosophy.

Comment 1

[FARKAS Máté]

Attachment: Server side sample

Comment 2

[Boris Zbarsky [:bzbarsky]]

This has nothing to do with JavaScript.

"Synchronous" XHR is not actually a blocking synchronous call: it spins the event loop before it returns (you can tell because the page doesn't stop painting, videos keep playing, the page can be scrolled, etc; not spinning the event loop would break the web).  So the only question is which events delivery is suppressed for....

Comment 3

[FARKAS Máté]

(In reply to Boris Zbarsky (:bz) from comment #2)
> This has nothing to do with JavaScript.
We use XULRunner, and we got an event from our components while we were waiting for a synchronous XHR response.

JavaScript runs in only one thread. Which means if I run a function, there will be no "context-switch". But with syncronous XHR my function go sleep and an another "thread" wakes up. When that dies, my original function comes, and when it finishes it's work, it returns.

But because JS is single threaded, I think, when I go into a function there will be nothing else until I came out from it. Am I wrong?

Sorry my english.

Comment 4

[Boris Zbarsky [:bzbarsky]]

There is only one thread.  That thread spins the event loop while waiting on the sync XHR.  That has nothing to do with JS the language, which doesn't even have a concept of event loop.  It's a pure browser construct.

Comment 5

[4esn0k]

I have the same problem

test case:

(function () {
  var s = [];
  var url = "#";

  setTimeout(function () {
    s.push('? - setTimeout');
  }, 0);

  window.addEventListener("message", function (event) {
    s.push('? - postMessage');
  });

  s.push(1);
  window.postMessage("", "*");
  s.push(2);

  var x = new XMLHttpRequest();
  x.open("GET", url, true);
  x.onreadystatechange = function () {
    if (x.readyState===4) {
      s.push('? - XMLHttpRequest');
    }
  };
  x.send(null);
  s.push(3);

  var x1 = new XMLHttpRequest();
  x1.open("GET", url, false);
  x1.onreadystatechange = function () {
    if (x1.readyState===4) {
      s.push(4);
    }
  };
  x1.send(null);
  s.push(5);
  setTimeout(function () {
    console.log(s.join(", "));
  }, 500);
}());

in Chrome, Opera, IE 9+ console.log will output:

1, 2, 3, 4, 5, ? - postMessage, ? - setTimeout, ? - XMLHttpRequest 

in Firefox:

1, 2, 3, ? - postMessage, ? - XMLHttpRequest, 4, 5, ? - setTimeout

it is surprising, that async callbacks are executed before the script finished

Comment 7

[Hallvord R. M. Steen [:hallvors]]

Oh my. I had no idea authors would rely on something like that :-o
This needs a test in the W3C suite.. Adding now as XMLHttpRequest/send-sync-blocks-async.htm

Comment 8

[Thomas Wisniewski]

Attachment: 697151-suspend-sync-xhrs-loadgroup-while-it-runs.diff

The sync XHR code currently does suppress timeouts as well as its related document's event handling (while the XHR runs). But of course events still get queued up in the wrong order that way, and so when event handling resumes they still don't match the desired output.

Here's a patch that instead suspends the rest of the sync XHR's load group until it completes. This passes the web platform test, though it still doesn't match what Chrome outputs for the code in comment 5:
  1, 2, 3, ? - postMessage, 4, 5, ? - setTimeout, ? - XMLHttpRequest

It appears that's because postMessage needs to also be suspended somehow, but before I investigate that I thought I'd check whether this patch is even close to a desirable fix.

Comment 9

[Boris Zbarsky [:bzbarsky]]

The right "real" fix here is to do khuey's event queue split and shut down all events except the ones for the sync XHR.

That _might_ require keeping all necko events going in general, though, so the "suspend all but this thing" bit on the loadgroup might still make sense.  But even then, we'd want to do that for all loadgroups in the sync-reachable set of documents, not just the one loadgroup for the one document.

In any case, the first step here is doing the event queue split.  That's planned to be done in the next few months; I'm not sure it's worth doing bandaids in the meantime.

Comment 10

[Thomas Wisniewski]

Sure, I see no reason to rush on this.

Comment 12

[Malte Wedel]

As there seems to be no reason to rush on making Firefox compliant to the XHR spec and compatible with the rest of the browsers after 7 years - wait that is just the age of this bug report, as far I remember it has always been like this in Firefox, so more than 14 years - here is a little "polyfix", which delays execution of promises, timeouts and async XHR callbacks and solved all execution order problems we had with Firefox synchronous XHR:

https://github.com/SAP/openui5/blob/master/src/sap.ui.core/src/sap/ui/base/syncXHRFix.js

Please be aware that this is not complete - there are still some other events that may cause JS execution while waiting for responses of synchronous requests, they just were not relevant for our use case.

Comment 13

[Thomas Wisniewski [:twisniewski]]

This is also causing breakage at www.chicme.com (I've added a see-also link to the relevant webcompat.com issue where I diagnosed this).

The polyfix in comment 12 seems to fix the problem there; thanks for that!

I've conjured up a feature-detection function to see if the polyfix is necessary, in case anyone else wants to copy-paste it until this bug is fixed:

>function needSyncXHRFix() {
>  var async = new XMLHttpRequest();
>  var sync = new XMLHttpRequest();
>  async.open("get", "data:text/html,");
>  sync.open("get", "data:text/html,", false);
>  async.onloadend = function() {
>    hackNeeded = true;
>  }
>  var hackNeeded = false;
>  async.send();
>  sync.send();
>  return hackNeeded;
>}

This will return false in Safari, Chrome and Edge, but true in Firefox (and should return false in Firefox as well once this bug is fixed).

Comment 14

[Thomas Wisniewski [:twisniewski]]

Just a quick update for anyone considering using a polyfix here, I've created a simplified drop-in polyfix here with feature detection included: https://github.com/wisniewskit/FirefoxSynchronousXHRPolyfix

I've confirmed that it works with IE11, Edge, Chrome, Safari and Firefox, and fixes the postMessage ordering issue mentioned above as well.

Comment 15

[Thomas Wisniewski [:twisniewski]]

bz, what's the status of khuey's event queue split that you mention here in comment #9? Is there another way forward here at this stage?

Comment 16

[Boris Zbarsky [:bzbarsky]]

I don't know that there's any work on that happening right now.  Chances are, there won't be work along those lines until Fission is done....

Comment 17

[Thomas Wisniewski [:twisniewski]]

Then do you feel it be worth doing something here before that stage? This is causing real compat issues, and people are investigating polyfixes as workarounds, so I think a bandaid may actually be appropriate.

Comment 18

[Boris Zbarsky [:bzbarsky]]

The problem is that it's not clear what the "something" would be.  Is there a concrete proposal?

Comment 19

[Thomas Wisniewski [:twisniewski]]

Attachment: 697151-wip.diff

For the live issue I've seen, it would be enough to just pass the expectation of the WPT xhr/send-sync-blocks-async.htm, which this new patch does (in an obviously incorrect way, but just as a quick example). Essentially we would:

- capture all sync and async XHR events on the same window while a sync XHR is going on.
- also record any interesting bits like the readyState at the time, so when we finally fire them, we can have the XHR to reflect what they were supposed to be.
- only fire the events after-the-fact (sync first, then async), while overriding the readyState/etc as appropriate during that event.

Of course, it would not be good enough for sites expecting other event types to be similarly blocked (like window.postMessage). I fully expect that I'm also missing other details. But it could at least bring us closer to interop. Thoughts?

Comment 21

[Boris Zbarsky [:bzbarsky]]

> - capture all sync and async XHR events on the same window while a sync XHR is going on.

So "events" here is notifications the XHR sends to script, not the notifications it receives from necko, right?

> also record any interesting bits like the readyState at the time, so when we finally fire them,
> we can have the XHR to reflect what they were supposed to be.

Unfortunately, just faking the readyState is not good enough.

For example, say I have an async XHR that plans to call overrideMimeType() when the readyState ends up HEADERS_RECEIVED.  If we just queue up the readystatechange event dispatch but go ahead and process the data with the wrong MIME type, what effect, exactly will the overrideMimeType call have?

It seems like it would "fix" run-to-completion for sync XHR (except for workers, perhaps, though maybe those already manage through the use of control runnables?), at the cost of the pending async XHRs misbehaving in weird ways like the overrideMimeType example above.  I'm not sure that's the right tradeoff...

What we _could_ do is queue up the actual necko notifications (OnStartRequest, OnDataAvailable, OnStopRequest, progress notifications, etc) in some form and re-deliver them once the sync XHR finishes.  As long as XHRs don't expect to mess with necko channels in OnStartRequest this would more or less work, I suspect.  As you say, other event sources would not be blocked by the sync XHR, but the interaction with async XHR would be fixed.

Comment 22

[Thomas Wisniewski [:twisniewski]]

>So "events" here is notifications the XHR sends to script, not the notifications it receives from necko, right?

Yes, just the Progress Events and ReadyStateChanges for "blocked" async XHRs.

>Unfortunately, just faking the readyState is not good enough.

Agreed, but I was hoping we could "fake" any other properties we might need to. You're probably correct that it's more trouble than just queueing up the necko notifications, however. In fact that's what my previous patch was doing (or so I presume) with loadGroup->BlockAllBut(mChannel). However, that doesn't seem to exist anymore, so how would we go about doing that now?

Comment 23

[Boris Zbarsky [:bzbarsky]]

Worth checking with the necko people, but the other option would be to just queue them up in the relevant XHR object by snapshotting whatever data is needed to "replay" the notifications.

Comment 24

[Thomas Wisniewski [:twisniewski]]

Dragana, do you have and insights which could help here? It would be nice if sync XHRs could simply tell the window to queue up Necko notifications for other XHRs that are currently running in the same window, as per comment 21. Is there a reasonable way to do that already, or would it be necessary for the XHR code to queue up the events itself?

Comment 25

[Firefox Bug Husbandry Bot]

Migrating Webcompat whiteboard priorities to project flags. See bug 1547409.

Comment 26

[Firefox Bug Husbandry Bot]

See bug 1547409. Migrating whiteboard priority tags to program flags.

Comment 27

[Dragana Damjanovic [:dragana]]

Dragana, do you have and insights which could help here? It would be nice if sync XHRs could simply tell the window to queue up Necko notifications for other XHRs that are currently running in the same window, as per comment 21. Is there a reasonable way to do that already, or would it be necessary for the XHR code to queue up the events itself?

wouls suspending all other httpChannels work?

Comment 28

[Thomas Wisniewski [:twisniewski]]

Would that actually suspend the network fetches for those requests, or just the resulting notifications? I don't believe that other engines block the actual fetches, they just don't process their related notifications/events until after the sync XHR completes.

Comment 29

[Dragana Damjanovic [:dragana]]

(In reply to Thomas Wisniewski [:twisniewski] from comment #28)

Would that actually suspend the network fetches for those requests, or just the resulting notifications? I don't believe that other engines block the actual fetches, they just don't process their related notifications/events until after the sync XHR completes.

this will not suspend actual network fetch. Data will be collected in a stream listener of the socket or in tcp layer. Probably it is not ideal.

Comment 30

[BugBot [:suhaib / :marco/ :calixte]]

The severity field for this bug is relatively low, S3. However, the bug has 3 duplicates.
:valentin, could you consider increasing the bug severity?

For more information, please visit auto_nag documentation.

Comment 31

[Valentin Gosu [:valentin] (he/him)]

I think it's OK to keep this S3.

Comment 32

[Malte Wedel]

No one dares to touch Firefox core eventing? Better waste web developers time for another 10 years, while they try to figure out why their app/page/site only breaks on Firefox. Sure, it is their own fault, why do they still use Sync XHR, it has been deprecated for so long :-)

Comment 33

[Thomas Wisniewski [:twisniewski]]

Speaking as someone who has already spent a lot of time on improving the webcompat story here (across browsers), if this is really more vital than it currently appears to be, then I would appreciate for web developers to let us know. Vote on the issue, report your experiences to us here or on webcompat.com, etc. Otherwise I'm afraid that it will have to continue to wait its turn on being fixed, along with all of the other issues causing web developers headaches :(

Comment 34

[Thomas Wisniewski [:twisniewski]]

Attachment: Bug 697151 - suspend all other (non-navigation) channels in the docgroup of a sync XHR as its syncloop begins, and resume them after its syncloop completes; r?smaug

Comment 35

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ddf986b83075
suspend all other (non-navigation) channels in the docgroup of a sync XHR as its syncloop begins, and resume them after its syncloop completes; r=smaug,necko-reviewers,valentin

Comment 36

[Natalia Csoregi [:nataliaCs]]

Backed out for causing assertion failures on XMLHttpRequestMainThread.cpp

• backout: https://hg.mozilla.org/integration/autoland/rev/8543c2a7eb517e27978f4148a90de286ed31eed0
• push: https://treeherder.mozilla.org/jobs?repo=autoland&group_state=expanded&revision=ddf986b830754349f8ebe48e93d301bb39d0055e
• failure log: https://treeherder.mozilla.org/logviewer?job_id=425406435&repo=autoland&lineNumber=4452

[task 2023-08-09T15:16:48.497Z] 15:16:48     INFO - GECKO(2350) | Assertion failure: aLoadGroup, at /builds/worker/checkouts/gecko/dom/xhr/XMLHttpRequestMainThread.cpp:3039
[task 2023-08-09T15:16:48.505Z] 15:16:48     INFO -  Initializing stack-fixing for the first stack frame, this may take a while...
[task 2023-08-09T15:16:58.349Z] 15:16:58     INFO - GECKO(2350) | #01: mozilla::dom::(anonymous namespace)::GatherChannelsInLoadGroup(nsCOMPtr<nsILoadGroup>&, nsCOMPtr<nsIChannel>&, nsTObserverArray<nsCOMPtr<nsIChannel> >&) [dom/xhr/XMLHttpRequestMainThread.cpp:3039]
[task 2023-08-09T15:16:58.350Z] 15:16:58     INFO - GECKO(2350) | #02: mozilla::dom::XMLHttpRequestMainThread::SuspendOtherChannels(nsTObserverArray<nsCOMPtr<nsIChannel> >&) [dom/xhr/XMLHttpRequestMainThread.cpp:3090]
[task 2023-08-09T15:16:58.351Z] 15:16:58     INFO - GECKO(2350) | #03: mozilla::dom::XMLHttpRequestMainThread::SendInternal(mozilla::dom::BodyExtractorBase const*, bool, mozilla::ErrorResult&) [dom/xhr/XMLHttpRequestMainThread.cpp:3273]
[task 2023-08-09T15:16:58.351Z] 15:16:58     INFO - GECKO(2350) | #04: mozilla::dom::XMLHttpRequest_Binding::send(JSContext*, JS::Handle<JSObject*>, void*, JSJitMethodCallArgs const&) [s3:gecko-generated-sources:6e8d2533591f5db4df99c2580cc93402ed265ab824d23db7675f4b38e72e81c977bc17345a85b44d9e5975124d35541db025f1d1b4160f591ca202efe9a38b1f/dom/bindings/XMLHttpRequestBinding.cpp::1664]
[task 2023-08-09T15:16:58.351Z] 15:16:58     INFO - GECKO(2350) | #05: mozilla::dom::binding_detail::GenericMethod<mozilla::dom::binding_detail::NormalThisPolicy, mozilla::dom::binding_detail::ThrowExceptions>(JSContext*, unsigned int, JS::Value*) [dom/bindings/BindingUtils.cpp:3329]
[task 2023-08-09T15:16:58.352Z] 15:16:58     INFO - GECKO(2350) | #06: CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), js::CallReason, JS::CallArgs const&) [js/src/vm/Interpreter.cpp:486]
[task 2023-08-09T15:16:58.353Z] 15:16:58     INFO - GECKO(2350) | #07: js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct, js::CallReason) [js/src/vm/Interpreter.cpp:580]
[task 2023-08-09T15:16:58.354Z] 15:16:58     INFO - GECKO(2350) | #08: js::Interpret(JSContext*, js::RunState&) [js/src/vm/Interpreter.cpp:0]
[task 2023-08-09T15:16:58.356Z] 15:16:58     INFO - GECKO(2350) | #09: js::RunScript(JSContext*, js::RunState&) [js/src/vm/Interpreter.cpp:458]
[task 2023-08-09T15:16:58.357Z] 15:16:58     INFO - GECKO(2350) | #10: js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct, js::CallReason) [js/src/vm/Interpreter.cpp:612]
[task 2023-08-09T15:16:58.358Z] 15:16:58     INFO - GECKO(2350) | #11: js::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, js::AnyInvokeArgs const&, JS::MutableHandle<JS::Value>, js::CallReason) [js/src/vm/Interpreter.cpp:679]
[task 2023-08-09T15:16:58.359Z] 15:16:58     INFO - GECKO(2350) | #12: js::fun_apply(JSContext*, unsigned int, JS::Value*) [js/src/vm/JSFunction.cpp:1007]
[task 2023-08-09T15:16:58.360Z] 15:16:58     INFO - GECKO(2350) | #13: CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), js::CallReason, JS::CallArgs const&) [js/src/vm/Interpreter.cpp:486]
[task 2023-08-09T15:16:58.361Z] 15:16:58     INFO - GECKO(2350) | #14: js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct, js::CallReason) [js/src/vm/Interpreter.cpp:580]
[task 2023-08-09T15:16:58.362Z] 15:16:58     INFO - GECKO(2350) | #15: js::Interpret(JSContext*, js::RunState&) [js/src/vm/Interpreter.cpp:0]
[task 2023-08-09T15:16:58.363Z] 15:16:58     INFO - GECKO(2350) | #16: js::RunScript(JSContext*, js::RunState&) [js/src/vm/Interpreter.cpp:458]
[task 2023-08-09T15:16:58.363Z] 15:16:58     INFO - GECKO(2350) | #17: js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct, js::CallReason) [js/src/vm/Interpreter.cpp:612]
[task 2023-08-09T15:16:58.367Z] 15:16:58     INFO - GECKO(2350) | #18: js::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, js::AnyInvokeArgs const&, JS::MutableHandle<JS::Value>, js::CallReason) [js/src/vm/Interpreter.cpp:679]
[task 2023-08-09T15:16:58.368Z] 15:16:58     INFO - GECKO(2350) | #19: JS::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::HandleValueArray const&, JS::MutableHandle<JS::Value>) [js/src/vm/CallAndConstruct.cpp:119]
[task 2023-08-09T15:16:58.369Z] 15:16:58     INFO - GECKO(2350) | #20: mozilla::dom::EventHandlerNonNull::Call(mozilla::dom::BindingCallContext&, JS::Handle<JS::Value>, mozilla::dom::Event&, JS::MutableHandle<JS::Value>, mozilla::ErrorResult&) [s3:gecko-generated-sources:ad20126262f563698aa490e895713edd44479aa56e7bebd3ff19658c25d739c0aef0da660321ce37cf403ea2940ca3b194a181356014de1a8189c0a772e8b106/dom/bindings/EventHandlerBinding.cpp::65]
[task 2023-08-09T15:16:58.371Z] 15:16:58     INFO - GECKO(2350) | #21: mozilla::dom::EventHandlerNonNull::Call<nsCOMPtr<mozilla::dom::EventTarget> >(nsCOMPtr<mozilla::dom::EventTarget> const&, mozilla::dom::Event&, JS::MutableHandle<JS::Value>, mozilla::ErrorResult&, char const*, mozilla::dom::CallbackObject::ExceptionHandling, JS::Realm*) [s3:gecko-generated-sources:4dd708197ad8e4f8d8d458496bc451a5b681d33062ec17ee182fb1b4c2f6131ac197ee5144e58d18bbd01de2307334ee7e48bf0a15d3a53b253e2003c89318ef/dist/include/mozilla/dom/EventHandlerBinding.h::0]
[task 2023-08-09T15:16:58.371Z] 15:16:58     INFO - GECKO(2350) | #22: mozilla::JSEventHandler::HandleEvent(mozilla::dom::Event*) [dom/events/JSEventHandler.cpp:199]
[task 2023-08-09T15:16:58.372Z] 15:16:58     INFO - GECKO(2350) | #23: mozilla::EventListenerManager::HandleEventSubType(mozilla::EventListenerManager::Listener*, mozilla::dom::Event*, mozilla::dom::EventTarget*) [dom/events/EventListenerManager.cpp:1259]
[task 2023-08-09T15:16:58.373Z] 15:16:58     INFO - GECKO(2350) | #24: mozilla::EventListenerManager::HandleEventInternal(nsPresContext*, mozilla::WidgetEvent*, mozilla::dom::Event**, mozilla::dom::EventTarget*, nsEventStatus*, bool) [dom/events/EventListenerManager.cpp:1455]
[task 2023-08-09T15:16:58.373Z] 15:16:58     INFO - GECKO(2350) | #25: mozilla::EventTargetChainItem::HandleEvent(mozilla::EventChainPostVisitor&, mozilla::ELMCreationDetector&) [dom/events/EventDispatcher.cpp:345]
[task 2023-08-09T15:16:58.374Z] 15:16:58     INFO - GECKO(2350) | #26: mozilla::EventTargetChainItem::HandleEventTargetChain(nsTArray<mozilla::EventTargetChainItem>&, mozilla::EventChainPostVisitor&, mozilla::EventDispatchingCallback*, mozilla::ELMCreationDetector&) [dom/events/EventDispatcher.cpp:563]
[task 2023-08-09T15:16:58.375Z] 15:16:58     INFO - GECKO(2350) | #27: mozilla::EventDispatcher::Dispatch(nsISupports*, nsPresContext*, mozilla::WidgetEvent*, mozilla::dom::Event*, nsEventStatus*, mozilla::EventDispatchingCallback*, nsTArray<mozilla::dom::EventTarget*>*) [dom/events/EventDispatcher.cpp:0]
[task 2023-08-09T15:16:58.375Z] 15:16:58     INFO - GECKO(2350) | #28: nsDocumentViewer::LoadComplete(nsresult) [layout/base/nsDocumentViewer.cpp:0]
[task 2023-08-09T15:16:58.376Z] 15:16:58     INFO - GECKO(2350) | #29: nsDocShell::EndPageLoad(nsIWebProgress*, nsIChannel*, nsresult) [docshell/base/nsDocShell.cpp:0]
[task 2023-08-09T15:16:58.376Z] 15:16:58     INFO - GECKO(2350) | #30: nsDocShell::OnStateChange(nsIWebProgress*, nsIRequest*, unsigned int, nsresult) [docshell/base/nsDocShell.cpp:5806]
[task 2023-08-09T15:16:58.377Z] 15:16:58     INFO - GECKO(2350) | #31: {virtual override thunk({offset(-448)}, nsDocShell::OnStateChange(nsIWebProgress*, nsIRequest*, unsigned int, nsresult))} [docshell/base/nsDocShell.cpp:0]
[task 2023-08-09T15:16:58.377Z] 15:16:58     INFO - GECKO(2350) | #32: nsDocLoader::DoFireOnStateChange(nsIWebProgress*, nsIRequest*, int&, nsresult) [uriloader/base/nsDocLoader.cpp:0]
[task 2023-08-09T15:16:58.378Z] 15:16:58     INFO - GECKO(2350) | #33: nsDocLoader::doStopDocumentLoad(nsIRequest*, nsresult) [uriloader/base/nsDocLoader.cpp:975]
[task 2023-08-09T15:16:58.379Z] 15:16:58     INFO - GECKO(2350) | #34: nsDocLoader::DocLoaderIsEmpty(bool, mozilla::Maybe<nsresult> const&) [uriloader/base/nsDocLoader.cpp:797]
[task 2023-08-09T15:16:58.379Z] 15:16:58     INFO - GECKO(2350) | #35: nsDocLoader::OnStopRequest(nsIRequest*, nsresult) [uriloader/base/nsDocLoader.cpp:0]
[task 2023-08-09T15:16:58.380Z] 15:16:58     INFO - GECKO(2350) | #36: nsDocShell::OnStopRequest(nsIRequest*, nsresult) [docshell/base/nsDocShell.cpp:13901]
[task 2023-08-09T15:16:58.381Z] 15:16:58     INFO - GECKO(2350) | #37: mozilla::net::nsLoadGroup::NotifyRemovalObservers(nsIRequest*, nsresult) [netwerk/base/nsLoadGroup.cpp:631]
[task 2023-08-09T15:16:58.381Z] 15:16:58     INFO - GECKO(2350) | #38: mozilla::net::nsLoadGroup::RemoveRequest(nsIRequest*, nsISupports*, nsresult) [netwerk/base/nsLoadGroup.cpp:0]
[task 2023-08-09T15:16:58.382Z] 15:16:58     INFO - GECKO(2350) | #39: mozilla::dom::Document::DoUnblockOnload() [dom/base/Document.cpp:0]
[task 2023-08-09T15:16:58.383Z] 15:16:58     INFO - GECKO(2350) | #40: mozilla::dom::Document::DispatchContentLoadedEvents() [dom/base/Document.cpp:0]
[task 2023-08-09T15:16:58.383Z] 15:16:58     INFO - GECKO(2350) | #41: mozilla::dom::Document::EndLoad() [dom/base/Document.cpp:8212]
[task 2023-08-09T15:16:58.384Z] 15:16:58     INFO - GECKO(2350) | #42: mozilla::dom::PrototypeDocumentContentSink::DoneWalking() [dom/prototype/PrototypeDocumentContentSink.cpp:688]
[task 2023-08-09T15:16:58.385Z] 15:16:58     INFO - GECKO(2350) | #43: mozilla::dom::PrototypeDocumentContentSink::MaybeDoneWalking() [dom/prototype/PrototypeDocumentContentSink.cpp:641]
[task 2023-08-09T15:16:58.385Z] 15:16:58     INFO - GECKO(2350) | #44: mozilla::dom::PrototypeDocumentContentSink::ResumeWalkInternal() [dom/prototype/PrototypeDocumentContentSink.cpp:0]
[task 2023-08-09T15:16:58.386Z] 15:16:58     INFO - GECKO(2350) | #45: mozilla::dom::PrototypeDocumentContentSink::ResumeWalk() [dom/prototype/PrototypeDocumentContentSink.cpp:451]
[task 2023-08-09T15:16:58.386Z] 15:16:58     INFO - GECKO(2350) | #46: mozilla::dom::PrototypeDocumentContentSink::OnScriptCompileComplete(js::frontend::CompilationStencil*, nsresult) [dom/prototype/PrototypeDocumentContentSink.cpp:946]
[task 2023-08-09T15:16:58.387Z] 15:16:58     INFO - GECKO(2350) | #47: NotifyOffThreadScriptCompletedTask::Run() [dom/xul/nsXULElement.cpp:1945]
[task 2023-08-09T15:16:58.388Z] 15:16:58     INFO - GECKO(2350) | #48: mozilla::TaskController::DoExecuteNextTaskOnlyMainThreadInternal(mozilla::detail::BaseAutoLock<mozilla::Mutex&> const&) [xpcom/threads/TaskController.cpp:886]
[task 2023-08-09T15:16:58.389Z] 15:16:58     INFO - GECKO(2350) | #49: mozilla::TaskController::ExecuteNextTaskOnlyMainThreadInternal(mozilla::detail::BaseAutoLock<mozilla::Mutex&> const&) [xpcom/threads/TaskController.cpp:0]
[task 2023-08-09T15:16:58.390Z] 15:16:58     INFO - GECKO(2350) | #50: mozilla::TaskController::ProcessPendingMTTask(bool) [xpcom/threads/TaskController.cpp:495]
[task 2023-08-09T15:16:58.391Z] 15:16:58     INFO - GECKO(2350) | #51: mozilla::detail::RunnableFunction<mozilla::TaskController::TaskController()::$_0>::Run() [xpcom/threads/nsThreadUtils.h:549]
[task 2023-08-09T15:16:58.392Z] 15:16:58     INFO - GECKO(2350) | #52: nsThread::ProcessNextEvent(bool, bool*) [xpcom/threads/nsThread.cpp:1203]
[task 2023-08-09T15:16:58.392Z] 15:16:58     INFO - GECKO(2350) | #53: NS_ProcessNextEvent(nsIThread*, bool) [xpcom/threads/nsThreadUtils.cpp:480]
[task 2023-08-09T15:16:58.393Z] 15:16:58     INFO - GECKO(2350) | #54: mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) [ipc/glue/MessagePump.cpp:85]
[task 2023-08-09T15:16:58.394Z] 15:16:58     INFO - GECKO(2350) | #55: MessageLoop::Run() [ipc/chromium/src/base/message_loop.cc:346]
[task 2023-08-09T15:16:58.395Z] 15:16:58     INFO - GECKO(2350) | #56: nsBaseAppShell::Run() [widget/nsBaseAppShell.cpp:150]
[task 2023-08-09T15:16:58.395Z] 15:16:58     INFO - GECKO(2350) | #57: nsAppStartup::Run() [toolkit/components/startup/nsAppStartup.cpp:296]
[task 2023-08-09T15:16:58.396Z] 15:16:58     INFO - GECKO(2350) | #58: XREMain::XRE_mainRun() [toolkit/xre/nsAppRunner.cpp:5672]
[task 2023-08-09T15:16:58.397Z] 15:16:58     INFO - GECKO(2350) | #59: XREMain::XRE_main(int, char**, mozilla::BootstrapConfig const&) [toolkit/xre/nsAppRunner.cpp:5873]
[task 2023-08-09T15:16:58.397Z] 15:16:58     INFO - GECKO(2350) | #60: XRE_main(int, char**, mozilla::BootstrapConfig const&) [toolkit/xre/nsAppRunner.cpp:5929]
[task 2023-08-09T15:16:58.398Z] 15:16:58     INFO - GECKO(2350) | #61: ??? [/builds/worker/workspace/build/application/firefox/firefox + 0x45c68]
[task 2023-08-09T15:16:58.399Z] 15:16:58     INFO - GECKO(2350) | #62: __libc_start_main [/lib/x86_64-linux-gnu/libc.so.6 + 0x21b97]
[task 2023-08-09T15:16:58.399Z] 15:16:58     INFO - GECKO(2350) | #63: ??? [/builds/worker/workspace/build/application/firefox/firefox + 0x45809]
[task 2023-08-09T15:16:58.400Z] 15:16:58     INFO - GECKO(2350) | #64: ??? (???:???)
[task 2023-08-09T15:16:58.401Z] 15:16:58     INFO - GECKO(2350) | ExceptionHandler::GenerateDump cloned child 2467
[task 2023-08-09T15:16:58.401Z] 15:16:58     INFO - GECKO(2350) | ExceptionHandler::SendContinueSignalToChild sent continue signal to child
[task 2023-08-09T15:16:58.402Z] 15:16:58     INFO - GECKO(2350) | ExceptionHandler::WaitForContinueSignal waiting for continue signal...

Comment 37

[Thomas Wisniewski [:twisniewski]]

It looks like I just need a couple of more checks that I had missed:
https://treeherder.mozilla.org/jobs?repo=try&revision=faca7a81c3e78bef7e52b68642f7d75ac2dffb58

Comment 38

[Nika Layzell [:nika] (ni? for response)]

After looking into this a bit at :twisniewski's request, I came across this existing logic in ChannelEventQueue: https://searchfox.org/mozilla-central/rev/e9b338c2d597067f99e96d5f20769f41f312fa8f/netwerk/ipc/ChannelEventQueue.cpp#179-224. This appears to be existing logic in Necko which suspends and unsuspends network event callbacks for XHR channels if events are currently suppressed by the corresponding document.

This sounds like it ought to have been handling this situation already, however, there is a cut-out in the logic to not suspend if the document is in a "synchronous operation that might require XHR events to be processed (such as a synchronous XHR)", meaning that it will do nothing for sync XHRs.

I wonder whether it would make sense for us to build upon this existing logic for this bug by making the event suspension code more precise. If we could distinguish between channels which were opened for an async and sync XHR at this point, we could potentially remove the !document->IsInSyncOperation() check, meaning we always suspend requests for async XHRs.

I currently don't see any way that the XHR is telling the channel whether or not it is sync upon creation. As far as I can tell the only communication is that the channel has the internal content policy TYPE_INTERNAL_XMLHTTPREQUEST, and the initiator type u"xmlhttprequest"_ns.

The channel is created after mFlagSynchronous is set, so the information is available at this point. There are various potential approaches to getting the information into the channel's LoadInfo such that it's available in the ChannelEventQueue logic.

One option, which is a bit gross but might work, is to split the TYPE_INTERNAL_XMLHTTPREQUEST internal content policy type into 2 separate internal content policy types - TYPE_INTERNAL_XMLHTTPREQUEST_ASYNC and TYPE_INTERNAL_XMLHTTPREQUEST_SYNC. This would definitely make the information known in the ChannelEventQueue code, as that's how we're telling if the channel is an XHR in the first place: https://searchfox.org/mozilla-central/rev/e9b338c2d597067f99e96d5f20769f41f312fa8f/netwerk/ipc/ChannelEventQueue.cpp#202-204. If we took this approach, we'd need to check if it's for an async XHR rather than for any kind of XHR, and remove the IsInSyncOperation check.

This might be a bit invasive though, so perhaps there's some other mechanism we can/should be using - it's probably worth chatting with necko folks about what seems the most appealing to them.

Comment 39

[Raul Bucata]

Not really sure how to test this issue. Tom, any suggestions?

Comment 40

[Thomas Wisniewski [:twisniewski]]

It's still an issue we have to fix, Raul. The easiest way to test it right now is probably to just check if we're passing this web platform test: https://wpt.fyi/results/xhr/send-sync-blocks-async.htm (which can be run directly at https://wpt.live/xhr/send-sync-blocks-async.htm)

Comment 41

[Thomas Wisniewski [:twisniewski]]

Attachment: Bug 697151 - distinguish between SYNC and ASYNC XMLHttpRequests in nsIContentPolicy types, and have ChannelEventQueue::MaybeSuspendIfEventsAreSuppressed only suspend async ones; r?nika

Comment 42

[boghyon]

It would be great if the fix could be included in Firefox ESR — still lots of legacy enterprise applications are out there relying on sync XHRs running either in other browsers or in FF but with the "polyfix" shared by Malte.

Comment 43

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/65a8caf91784
distinguish between SYNC and ASYNC XMLHttpRequests in nsIContentPolicy types, and have ChannelEventQueue::MaybeSuspendIfEventsAreSuppressed only suspend async ones; r=nika,necko-reviewers,peterv,dom-storage-reviewers,asuth,kershaw

Comment 44

[pstanciu]

https://hg.mozilla.org/mozilla-central/rev/65a8caf91784

Comment 45

[Thomas Wisniewski [:twisniewski]]

(In reply to boghyon from comment #42)

It would be great if the fix could be included in Firefox ESR — still lots of legacy enterprise applications are out there relying on sync XHRs running either in other browsers or in FF but with the "polyfix" shared by Malte.

If this patch seems to working without issues for a release or two, I will strongly consider nominating it for ESR. Right now we can't be sure that it will actually fix more sites than it will break, since synchronous XMLHttpRequests are rather notorious and there may be other subtle issues I have unintentionally missed. To that end it would be very helpful for folks to let us know if this does fix their sites or apps, so please let us know!

Comment 46

[Hamish Willee]

Hi Thomas, my understanding of this is that

• prior to FF129 if you had a worker make an async then a sync XMLHttpRequest users would expect the async requests to block until the sync fetch completes - and more specifically not send the readyState events (by which I presume you mean https://developer.mozilla.org/en-US/docs/Web/API/XMLHttpRequest/readystatechange_event)?
• However what actually happens is that your code still gets those events, and in some cases code that relies on the sync call having completed might return different result? Right?
• With this patch the async event queues are suspended so the events for the async requests are sent after the sync request completes - right?

We probably should include this for the MDN release notes (and probably the more official ones) so that people know to test this. WOuld you like to provide your own words for this? Essentially the change, the value added, and what you might like people to do in a paragraph.

Comment 47

[Thomas Wisniewski [:twisniewski]]

Yes. Basically, it's possible to "nest" XMLHttpRequests by starting one while another is still in progress. And now if you nest XMLHttpRequests, with the innermost one being a synchronous one, Firefox will effectively wait for that innermost synchronous one to complete, firing its events first, before firing the other outer XHR's events. This is to match the behavior of other browsers for web compatibility.

Comment 48

[Hamish Willee]

FF129 Docs work for this can be tracked in https://github.com/mdn/content/issues/34726 (just a release note, as discussed above - thanks Thomas).