Closed Bug 697151 Opened 13 years ago Closed 3 months ago

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

Categories

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

defect

Tracking

()

RESOLVED FIXED
129 Branch
Webcompat Priority P2
Tracking Status
firefox129 --- fixed

People

(Reporter: fmate144, Assigned: twisniewski)

References

(Blocks 3 open bugs)

Details

(Keywords: dev-doc-complete, Whiteboard: [webcompat][necko-triaged])

Attachments

(3 files, 3 obsolete files)

Attached 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.
Attached 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
Blocks: 978757
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.
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)
Attached patch 697151-wip.diff (obsolete) — Splinter Review
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)
> - 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 → --

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?

Flags: needinfo?(dd.mozilla)

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.

Flags: needinfo?(dd.mozilla)

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

Flags: needinfo?(dd.mozilla)
Priority: -- → P2
Whiteboard: [webcompat] → [webcompat][necko-triaged]
Severity: normal → S3

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.

Flags: needinfo?(valentin.gosu)
Webcompat Priority: revisit → P2

I think it's OK to keep this S3.

Flags: needinfo?(valentin.gosu)

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

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

Assignee: nobody → twisniewski
Status: NEW → ASSIGNED
Attachment #9346933 - Attachment description: Bug 697151 - suspend all other channels in the loadgroup of a sync XHR as its syncloop begins, and resume them after its syncloop completes; r?smaug → Bug 697151 - suspend all other channels in the doctree of a sync XHR as its syncloop begins, and resume them after its syncloop completes; r?smaug
Attachment #9346933 - Attachment description: Bug 697151 - suspend all other channels in the doctree of a sync XHR as its syncloop begins, and resume them after its syncloop completes; r?smaug → 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
Pushed by twisniewski@mozilla.com: 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

Backed out for causing assertion failures on XMLHttpRequestMainThread.cpp

[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...
Flags: needinfo?(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

Flags: needinfo?(twisniewski)
Depends on: 1848036

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.

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

Flags: needinfo?(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)

Flags: needinfo?(twisniewski)

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.

Pushed by twisniewski@mozilla.com: 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
Status: ASSIGNED → RESOLVED
Closed: 3 months ago
Resolution: --- → FIXED
Target Milestone: --- → 129 Branch
Attachment #8997608 - Attachment is obsolete: true
Attachment #9346933 - Attachment is obsolete: true

(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!

Regressions: 1906444

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.

Flags: needinfo?(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.

Flags: needinfo?(twisniewski)

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

You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: