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

NEW
Unassigned

Status

()

defect
8 years ago
4 days ago

People

(Reporter: fmate144, Unassigned, NeedInfo)

Tracking

(Blocks 3 bugs, {dev-doc-needed})

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Webcompat Priority:revisit)

Details

(Whiteboard: [webcompat])

Attachments

(3 attachments, 1 obsolete attachment)

Posted file 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.
Posted file Server side sample
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....
Assignee: general → nobody
Component: JavaScript Engine → DOM
QA Contact: general → general
Summary: Synchronous AJAX doesn't block the JavaScript event queue → Synchronous XMLHttpRequest (XHR) does not block readyState events for async XHR
(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.
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.
OS: Windows 7 → All
Hardware: x86 → All
Version: unspecified → Trunk
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
Status: UNCONFIRMED → NEW
Ever confirmed: true
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
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.
Flags: needinfo?(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.
Flags: needinfo?(bzbarsky)
Sure, I see no reason to rush on this.
Duplicate of this bug: 521894
Blocks: xhr
No longer blocks: xhr2pass
Priority: -- → P5
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.
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).
Whiteboard: [webcompat]
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.
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?
Flags: needinfo?(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....
Flags: needinfo?(bzbarsky)
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.
Flags: needinfo?(bzbarsky)
The problem is that it's not clear what the "something" would be.  Is there a concrete proposal?
Flags: needinfo?(bzbarsky)
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?
Attachment #8769993 - Attachment is obsolete: true
Flags: needinfo?(bzbarsky)
Duplicate of this bug: 1366979
> - 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.
Flags: needinfo?(bzbarsky)
>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?
Flags: needinfo?(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.
Flags: needinfo?(bzbarsky)
Component: DOM → DOM: Core & HTML

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?

Flags: needinfo?(dd.mozilla)

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

Webcompat Priority: --- → ?

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

Blocks: 1528800
Webcompat Priority: ? → revisit
Component: DOM: Core & HTML → DOM: Networking
Priority: P5 → --
You need to log in before you can comment on or make changes to this bug.