Closed Bug 969671 Opened 6 years ago Closed 6 years ago

Warn about use of sync XHR in the main thread

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla30

People

(Reporter: smaug, Assigned: smaug)

References

Details

(Keywords: dev-doc-complete, site-compat)

Attachments

(1 file, 1 obsolete file)

http://xhr.spec.whatwg.org/#sync-warning

We should warn at least when it is used outside various unload event listeners.
Attached patch something like this (obsolete) — Splinter Review
this should warn when we're not handling pagehide/beforeunload/unload listeners.
Attachment #8372677 - Flags: review?(jonas)
Comment on attachment 8372677 [details] [diff] [review]
something like this

Review of attachment 8372677 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/base/src/nsXMLHttpRequest.h
@@ +747,5 @@
> +
> +  static bool sDontWarnAboutSyncXHR;
> +};
> +
> +class AutoDontWarnAboutSyncXHR

Please annotate with MOZ_STACK_CLASS.

::: dom/locales/en-US/chrome/dom/dom.properties
@@ +144,5 @@
>  ShowModalDialogWarning=Use of window.showModalDialog() is deprecated. Use window.open() instead. For more help https://developer.mozilla.org/en-US/docs/Web/API/Window.open
>  # LOCALIZATION NOTE: Do not translate "window._content" or "window.content"
>  Window_ContentWarning=window._content is deprecated.  Please use window.content instead.
> +# LOCALIZATION NOTE: Do not translate "XMLHttpRequest"
> +SyncXMLHttpRequestWarning=Please don't use synchronous XMLHttpRequest on the main thread. It is bad for the user experience.

It would be nice if we could link to a page on MDN which explains why this is bad and what authors can do about it...
Comment on attachment 8372677 [details] [diff] [review]
something like this

Review of attachment 8372677 [details] [diff] [review]:
-----------------------------------------------------------------

Also mention that it's been deprecated in the latest standard. r=me with that and Ehsan's comments.
Not sure about MDN links. Those tend to change more often than links to specs. And a link to a spec makes it more clear that this isn't just about FF, but all the browsers.

Perhaps
"Synchronous XMLHttpRequest on the main thread is deprecated because of its detrimental effects to the end user's experience. For more help http://xhr.spec.whatwg.org/"
For Deprecated.jsm, the de-facto convention is to provide either MDN links or bugzilla links. The latter doesn't really apply, but I'd go for the former.
I'm fine with a link to any useful resource, doesn't have to be MDN (good point about the issue not being Firefox specific.)
Attached patch v2Splinter Review
Attachment #8372677 - Attachment is obsolete: true
Attachment #8372677 - Flags: review?(jonas)
Attachment #8373492 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/dff86b5bbfa6
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
Blocks: 980902
Duplicate of this bug: 558972
We have a large client written in XUL and it relies heavily on synchronous requests.  The reason is the server presents itself as a collection of SOAP services which then call API calls.  When communicating with the server we may have to make several SOAP calls to the server in order to get all the data and we need those to happen in a certain order and the user not being able to interact with the client until the data is loaded isn't a bug, it's a feature.  This could be worked around in the client but would be a total pain in a client that zipped up is over 1M.  

Is there any way that synchronous requests can stick around?  Maybe if the client is launched using the -chrome argument or through a preference?
(In reply to comment #12)
> Is there any way that synchronous requests can stick around?  Maybe if the
> client is launched using the -chrome argument or through a preference?

I think all of the downsides of synchronous XHR apply to chrome and content equally, so I see no reason to favor chrome loaded content here, especially since that would mean that add-ons can trigger this behavior and cause bad user experience no matter which web page our users visit.  So I suggest you consider moving away from using sync XHR.
(In reply to Tony Wells from comment #12)
> Is there any way that synchronous requests can stick around?  Maybe if the
> client is launched using the -chrome argument or through a preference?

A possible solution: Promise + async requests
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Promise
Hi there, please do NOT deprecate sync-loading of a resource, it *is* useful in some situations, where, as has been pointed out, using async with post-load callbacks is a PAIN IN THE A**. I load pieces of my program as-needed, from multiple locations (urls). Within a single function, I do this multiple times, for ex:

loadFile(fileA);
// do something with its contents, eg. parse, eval parts of it..
loadFile(fileB);
// do something with fileB's contents
..

I need loadFile to BLOCK while fileA is being fetched, and later, for fileB to do likewise. I do not want to load fileA asynchronously, then place the rest of the code in a callback, and within THAT callback, do another nested async-load callback, etc. I prefer the simplicity of what I indicated above, it has been working beautifully well, until, starting with FF30, there is the deprecation warning starting to appear. How a developer chooses to load (sync vs async) is up to them, and to the users (who will complain if the page becomes quite unresponsive) - the browser should not heavy-handedly dictate "best practices" (in my case, async-loading is NOT best case, it is a needless work-around). Please clean up the warning to remove the word 'deprecated', and, please DO NOT DEPRECATE this quite useful capability!!

Thanks,
Saty
Meant to say above, '...in my case, async-loading is NOT best PRACTICE' (not 'best case'), sorry.
saty, you should take a look at comment 14 + http://taskjs.org/ and at comment 15. This is much better than using the synchronous API.
Thanks for your reply, David. I did look at comments 14 and 15 (and played around with web workers a bit, even prior to that) before posting. I hadn't come across taskjs.org, thanks for that - looks 'promising' (pun).

I am in the middle of creating an application, and am busy with writing higher level code (compared to sync-fetching, which I have encapsulated into a blockAndFetch() call that has been working 100% well so far), and would like what I already have, to "just work" (don't want to be distracted by re-writing the loading code). 

Also, I'm aware of LAB.js etc (for guaranteed loading of files in specific order), but again, I just want <myXHRObj>.open("GET", <myURL>, false) to just work in the main thread!! 

So, question for you - will this sync-loading functionality be eventually deprecated completely?

Thanks,
Saty
We do want to remove this API at some point in the future, yes.
The aim is to remove sync XHR from the main thread once the usage of it will be rare enough.
I guess it will take anything from one to five years.

Bug 558972 has some more background to this, see for example Bug 558972 comment 7.
Blocks: 721781
Sorry, I come to this discussion late.

What statistics and metrics do you have that show end users experience detrimental effects?
Please provide a link to the discussion of detrimental effects from "sync XHR" function.

I would like tools to monitor something, anything, that reveals to me that my clients experience detrimental effects.

Is this an arbitrary decision to remove "sync XHR" function in the future?  By what means do you determine that this functionality needs to be removed?  What is the harm of maintaining this functionality?
See the link in Bug 558972 comment 7.

And all the major browser vendors have agree that sync XHR in the main thread should be
removed if just possible.
Hi Olli, I read contents of your link.  Did I get this correctly:

a) the page locks up when "synch XHR" fails.

So, the page is worthless anyway if the sync fails.  Client moves on.

b) some browsers fail if the "sync XHR" fails.

So, the page is worthless; browser should handle any fail state without crashing.

c) browser processes stop until sync is resolved as either complete or fail.

So, let the browser process stop.  If detrimental to user's experience then web developers have an option to re-write their code; no need to have code re-write forced by removal of functionality. Concern is being focused only on browser efficiency, not efficiency of the the whole client - server - web developer picture. Without web development resources or server resources, who cares if the browser is efficient? 

My anecdote: six years using "sync XHR" ( started with IE6 and Firefox 2 )  with acceptably short browser lag time in displaying results.  My option will be to have lag time and increased resource use on my server instead of the client's browser.  For my sites, programmer resources needed to engineer an "async XHR" are unavailable.  My clients will experience detrimental effects when I have to shift work from their browser to my server.
> Not sure about MDN links. Those tend to change more often than links to specs. And a link to a spec makes it more clear that this isn't just about FF, but all the browsers.

I would recommend against pointing developers to the specification. It's not a document that is focused on describing more detail about this brand new console warning they're looking at. Plus, the moment any developer sees a block of IDL, you've probably lost them. 

I would recommend you point developers to the really well-crafted https://developer.mozilla.org/en-US/docs/Web/API/XMLHttpRequest/Synchronous_and_Asynchronous_Requests
Is the deprecated message being generated for sync XHR from chrome?  I'm reading a local file in an addon using synchronous XMLHttpRequest and I'm not seeing the message.
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.