Bug 72154 – Synchronous XHR in window context should not support new XHR responseTypes for HTTP(S) requests

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[James Robinson]

Synchronous XMLHttpRequest in the window (not worker) context is an abomination, but we have to support it for compat reasons.  For new response types, though, we don't have this legacy constraint. Mozilla has floated the idea of not supporting any new response types in sync XHR as a way to encourage authors to stick to asynchronous mode and I think it's a great idea.

[Olli Pettay (:smaug)]

https://bugzilla.mozilla.org/show_bug.cgi?id=701787
http://www.w3.org/Bugs/Public/show_bug.cgi?id=14773

[Dimitri Glazkov (Google)]

Big fan of this.

[Alexey Proskuryakov]

Good idea.

[Darin Fisher (:fishd, Google)]

Yes, +1

[Jarred Nicholls]

brilliant

[Jarred Nicholls]

Created attachment 119801 [details]
Patch

[WebKit Review Bot]

Comment on attachment 119801 [details]
Patch

Attachment 119801 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10921502

New failing tests:
http/tests/xmlhttprequest/cross-origin-no-authorization.html
http/tests/xmlhttprequest/cross-origin-cookie-storage.html
http/tests/xmlhttprequest/cross-origin-authorization.html

[Jarred Nicholls]

Created attachment 119854 [details]
Patch

[Alexey Proskuryakov]

WebKit and other engines have been shipping withCredentials for a while now. It seems unlikely that we can remove it. What research did you perform that demonstrates that it won't break the Web?

I suggest keeping this bug for new response types as originally filed - even so, it may not be easy to decide which types are sufficiently new at this point.

[Olli Pettay (:smaug)]

withCredentials is reasonable new thing and using it with
sync XHR (in window context) has been disabled in Gecko.
https://bugzilla.mozilla.org/show_bug.cgi?id=701787

There has been talk about disabling all CORS when using
sync XHR (in window context), but that sounds too risky to me.
See
https://www.w3.org/Bugs/Public/show_bug.cgi?id=14773

[Jarred Nicholls]

(In reply to comment #9)
> WebKit and other engines have been shipping withCredentials for a while now. It seems unlikely that we can remove it. What research did you perform that demonstrates that it won't break the Web?

For sync requests in window context? I discovered that Firefox and Opera have or are in the process of doing the same.  If you have doubts or reservations about this, you may be too late for that argument :) I'd like to think it's not breaking the web but instead moving the web forward.

Considering no version of IE supports it...well need I say more.

> 
> I suggest keeping this bug for new response types as originally filed - even so, it may not be easy to decide which types are sufficiently new at this point.

Going for the whole tamale here...

[Alexey Proskuryakov]

Comment on attachment 119854 [details]
Patch

Discouraging use doesn't mean that we should break existing callers. Unlike Mozilla and Opera engines, WebKit is widely used for non-Web content (and that's an explicit goal of the project). It's also more commonly used with walled garden content.

As far as web content is concerned, I did a quick Google search, and withCredentials is widely used, including by libraries such as jQuery and mooTools. Said libraries use it for decisions that would be icky to make based on disabled functionality, even if that doesn't directly render results invalid. It just adds too much complexity to the Web when you study and debug code like the below:

// Does this browser support crossDomain XHR requests
jQuery.support.cors = testXHR && ( "withCredentials" in testXHR );

In fact, looking at those search results, I'm slightly less supportive of this idea than before. We engine vendors have little say in whether to discourage sync requests, it's library vendors who do. Since new responseTypes are basically syntactic sugar, they can work with or without engine support. It might be good to demonstrate our position, however with understanding that benefits will be indirect if any, while damage will be direct.

> Going for the whole tamale here...

Changes that have the potential of breaking sites should be done separately to ease finding the culprit later. That's the case for most kinds of changes, in fact.

[James Robinson]

Supporting walled garden and non-Web content is a more important goal for this project than supporting the future of the web platform? Unless you have some evidence that making this change will cause nontrivial compatibility concerns, I think your objections are out of line. By delaying this you are increasing the chance that we will need to maintain this behavior forever.

[Alexey Proskuryakov]

> Supporting walled garden and non-Web content is a more important goal for this project than supporting the future of the web platform?

I don't think that this question is relevant to the discussion. Sync XHRs are not the future of the Web platform. This bug is a about a very far fetched attempt to influence authors in order to make user experience better, not about building the future of the platform.

In this case, we don't have to choose between explicitly stated goals of the project to make the decision, or to admit that our goals are not exactly the same as for other browser vendors whose opinion has been mentioned.

> By delaying this you are increasing the chance that we will need to maintain this behavior forever. 

That's not scary at all, the existing behavior easy to support. Blocking it is what takes work, and has long term costs in the complexity of the platform.

[Jarred Nicholls]

(In reply to comment #12)
> (From update of attachment 119854 [details])
> Discouraging use doesn't mean that we should break existing callers. Unlike Mozilla and Opera engines, WebKit is widely used for non-Web content (and that's an explicit goal of the project). It's also more commonly used with walled garden content.

Alexey, I love your conservatism around changing the status quo and all of the same scenarios that run through your head also run through mine; but then I wake up and realize I'm not only over reacting to an *extreme* edge case, but also hamstringing the web by not following through with its natural progression.  With the noted chance of sounding presumptuous, I'll continue...

Forget about Mozilla and Opera, we should be holding the torch and leading the way on this change and I'm shocked we are dead last in the race.  What is equally worse for WebKit is to deliberately choose to fragment the web platform to satisfy a use case that *no one wants or cares about*.

I agree that supporting the status quo is easier - what I don't want is for the hemorrhaging of this wound to continue until so much time has passed that we've reached a point of no turning back.  But rather I see a future of the opposite occurring: the future where synchronous XHR in the window context is so utterly abandoned that it can be turned off completely; we've now just made the web platform a simpler place.

> 
> As far as web content is concerned, I did a quick Google search, and withCredentials is widely used, including by libraries such as jQuery and mooTools. 

I work for and represent a major JS library vendor - dare I say our mobile app framework is the most popular, targeting WebKit mobile browsers.  Our XHR wrapper (Ext.Ajax / Ext.data.Connection) doesn't publicly support synchronous requests *at all*.  It's possible, but purposefully undocumented.

> Said libraries use it for decisions that would be icky to make based on disabled functionality, even if that doesn't directly render results invalid. It just adds too much complexity to the Web when you study and debug code like the below:
> 
> // Does this browser support crossDomain XHR requests
> jQuery.support.cors = testXHR && ( "withCredentials" in testXHR );

I know all of the code you speak of - I added CORS support to Ext JS and Sencha Touch.  The above is simply a detection mechanism that CORS is supported.  But your facts aren't complete: jQuery, after detecting CORS support, explicitly disables synchronous CORS requests.  From their docs:

"async (Boolean)
Default: true
By default, all requests are sent asynchronously (i.e. this is set to true by default). If you need synchronous requests, set this option to false. Cross-domain requests and dataType: "jsonp" requests do not support synchronous operation. Note that synchronous requests may temporarily lock the browser, disabling any actions while the request is active."

The functionality we're trying to disable here is not even allowed in jQuery and not even documented in Ext JS or Sencha Touch.  I'm not sure what further data is needed to prove that no one wants this functionality, nor will anyone miss it.

> 
> In fact, looking at those search results, I'm slightly less supportive of this idea than before. We engine vendors have little say in whether to discourage sync requests, it's library vendors who do. 

See above...

> Since new responseTypes are basically syntactic sugar, they can work with or without engine support. 

That's not true.  .response is polymorphic and .responseType determines the type of .response.  It's not just syntactic sugar, it has a real purpose in particular for binary types such as ArrayBuffer and Blob where .response is the only means to accessing that data.

> It might be good to demonstrate our position, however with understanding that benefits will be indirect if any, while damage will be direct.

I agree - we're in demolition mode.  But the benefit isn't seen today, it's seen tomorrow.  Imagine a world as I described above: sync requests in the window context are completely turned off, leading to a simpler web platform.  That future takes time by making choices like this one today.

> 
> > Going for the whole tamale here...
> 
> Changes that have the potential of breaking sites should be done separately to ease finding the culprit later. That's the case for most kinds of changes, in fact.

I think this particular change would be very easy to find (and reverse if necessary) whether or not it's separate - but in principle I am totally with you.

> > By delaying this you are increasing the chance that we will need to maintain this behavior forever. 
> 
> That's not scary at all, the existing behavior easy to support. Blocking it is what takes work, and has long term costs in the complexity of the platform.

See above.  I disagree, I think this is to make the platform simpler in the long term, not more complex.  A minor prick of the finger today...

[Olli Pettay (:smaug)]

(In reply to comment #15)
> I agree - we're in demolition mode.  But the benefit isn't seen today, it's seen tomorrow.
> Imagine a world as I described above: sync requests in the window context are completely turned off, leading to a simpler web platform.
> That future takes time by making choices like this one today.

Since we're anyway in a mode where specs like DOM4 remove features from web platform, I think it is a good time to try to get rid of
possibly the most annoying feature in the platform. It may take time, or it may not succeed, but if all or most of the browser vendors try to do it
actively, we can hopefully remove sync XHR (in window context) at some point.
That would be a clear victory to user experience.
(Gecko developer hat on)

[Anne van Kesteren]

Alexey, while you are right that there is increased complexity for implementations, for end users and developers (as they will have to use the right way API) this is a win that I think outweighs the fairly minimal implementation cost.

In any event, if you really think this is the wrong behavior you should say so on the public list and not block someone trying to implement the specification.

[Adam Barth]

> Unlike Mozilla and Opera engines, WebKit is widely used for non-Web content (and that's an explicit goal of the project).

Can you show me where on <http://www.webkit.org/projects/goals.html> it says that "wall garden" considerations should take precedence over standards?  Here's the text that I see:

---8<---
The project's primary focus is content deployed on the World Wide Web, using standards-based technologies such as HTML, CSS, JavaScript and the DOM. However, we also want to make it possible to embed WebKit in other applications, and to use it as a general-purpose display and interaction engine.
--->8---

That says that WebKit's primary focus is content deployed on the World Wide Web.  We want to make it *possible* to embed WebKit in other applications, but that's not the primary focus of the project.

The goals page also says:

---8<---
WebKit aims for compliance with relevant web standards, and support for new standards In addition to improving compliance, we participate in the web standards community to bring new technologies into standards, and to make sure new standards are practical to implement in our engine.
--->8---

It seems that if you are opposed to this change, you should take your point of view to the web standards community instead of trying to end-run the process in this bug.

[Alexey Proskuryakov]

Jarred: I did not know that jQuery disabled sync CORS. This is great news, as well as what you say about Ext.Ajax. I'm not sure if that has direct effect on any decision of ours, but it's great to consider in this context. I think that this is an order of magnitude more important than what engines can do, however disruptive we're willing to be.

Anne: Is there a specific e-mail that I could respond to, saying that WebKit is rather unlikely to make changes that are not backwards compatible? I'm not an ultimate decision maker, but think that I have a reasonable share of authority over XHR in WebKit.

Adam: I am not going to show you where in project goals "wall garden" content is said to take precedence. I never claimed or implied anything even remotely close to that, so you are arguing with a straw man. Also, we should be able to discuss the strategy of getting rid of sync XHR without a holy war.

Everyone: I think that we're largely missing step 2 here.
1. Remove some newer features from XHR. Ones that we think almost no one is using yet.
2. ???
3. Authors stop using sync XHR.

Sync XHR has been used before these features were added. Not only we can't change old unmaintained code, but even most authors of new code won't notice that something is missing. Enlightenment can occur in cases where code evolves and hits a wall, but I don't see that as a very common scenario, or one where lack of support for features would be the primary driver for change (when starting to use CORS, you're quite likely to be hugely redesigning your application, and will switch to a more modern library anyway).

Getting rid of sync XHR is a goal I wholeheartedly support. Some approaches are more helpful (JS library changes and engines logging console warnings seem to be the best ones at this point). Some are nearly useless for achieving step 3, yet safe and easy, like not enabling any new functionality. Disabling already shipping functionality is both nearly useless, and fairly dangerous. I think that this is quite clearly not the change we should make.

[Jarred Nicholls]

(In reply to comment #19)
> Jarred: I did not know that jQuery disabled sync CORS. This is great news, as well as what you say about Ext.Ajax. I'm not sure if that has direct effect on any decision of ours, but it's great to consider in this context. I think that this is an order of magnitude more important than what engines can do, however disruptive we're willing to be.

Maybe sync [CORS] XHR requests in the window context is an exception because user experience is so utterly bad, but you would be surprised how JS libraries are willing and wanting to follow along and/or expose the features of the engines to the T; mostly for the sake of claiming "completeness".  I think there are times where the standards community and the UAs should take the lead on an issue, and this is one of those times where they all have; except we are the only ones spinning wheels.

> Disabling already shipping functionality is both nearly useless, and fairly dangerous. I think that this is quite clearly not the change we should make.

So are you reneging on the idea of disabling responseType for sync XHR in the window context?  It's already shipped.  What about responseType "json" that I'm working on?  Are you suggesting we should disable "json" responseType but leave "text" "document" and "arraybuffer" turned on because they're shipped?  I don't want to make the stubborn mistake of sticking to certain principles for the sake of the principles, and would rather consider each change we make individually.  When I consider only the change of disabling responseType and withCredentials for sync requests in the window context, given the facts and positions and the minimal impact on real-world use, I have to support disabling them because it lays the ground work for future simplification.  I don't want to make a mountain out of a mole hill.  If we were talking about removing <table> then this would be an entirely different conversation.

[Alexey Proskuryakov]

> Maybe sync [CORS] XHR requests in the window context is an exception

It certainly appears so!

> Are you suggesting we should disable "json" responseType but leave "text" "document" and "arraybuffer" turned on because they're shipped?

Yes, that formally follows from what I said, and I think that this would be the responsible approach. However that would be offensively inconsistent. Maybe preserving our sanity as engine developers outweighs being responsible. I'm not sure, so I'm willing to agree with vocal requests that are being made.

I don't think that withCredentials deserves getting such (counter?)preferential treatment.

[Jarred Nicholls]

> However that would be offensively inconsistent.

Totally, you can't have it both ways...

> Maybe preserving our sanity as engine developers outweighs being responsible.

...because it's not about our sanity, it's about the sanity of web developers and the confusion it would cause having Firefox & Opera all in one direction but WebKit being scatterbrained and indecisive like a toddler.

> I'm not sure, so I'm willing to agree with vocal requests that are being made.

So what are the next steps?  Do we take a democratic vote to r+ this?

[Darin Adler]

(In reply to comment #22)
> Do we take a democratic vote to r+ this?

Absolutely not, that is not how we run the project.

[Jarred Nicholls]

(In reply to comment #23)
> (In reply to comment #22)
> > Do we take a democratic vote to r+ this?
> 
> Absolutely not, that is not how we run the project.

Good to hear!  Conflicts of interest galore...

[Jarred Nicholls]

Alexey, I appreciate your responsible level-headed stance and will leave the ball in your court.  If you have any questions you know where to find me.

If you would be so kind as to give a nod of approval or object to removing the block on bug #73648 (with the notion that bug #73648 can go in prior to a resolution on this bug, so no one's time is wasted) I would appreciate it - just so I know where I can focus next.

Thanks!

[Alexey Proskuryakov]

One more thought: do we want to disable new(-ish) functionality in sync requests to file: URLs? What about data: ones? Custom URL handlers implemented by applications embedding WebKit?

These don't appear like something we should discourage.

>> I'm not sure, so I'm willing to agree with vocal requests that are being made.
> So what are the next steps?  Do we take a democratic vote to r+ this?

Decisions are generally made and explained by people who do the work, and are then approved by reviewers.

I think that you are in a good position to decide whether to only disable "json", or all functionality available through responseType, so I'd r+ an otherwise good patch that does either.

> If you would be so kind as to give a nod of approval or object to removing the block on bug #73648

If you and others feel that this moves in the direction of WONTFIX, then it is appropriate to unblock "json" implementation. I still think that disabling new functionality was a good idea, but inconsistencies - including the new one with dependency on URL scheme - appear to be killing it.

[Jarred Nicholls]

> I think that you are in a good position to decide whether to only disable "json", or all functionality available through responseType, so I'd r+ an otherwise good patch that does either.

Sounds good Alexey, thanks.  Patch on the way.

[Jarred Nicholls]

Created attachment 120361 [details]
Patch

[Jarred Nicholls]

For those who oppose following the spec for withCredentials (http://dvcs.w3.org/hg/xhr/raw-file/tip/Overview.html#the-withcredentials-attribute) should hit up public-webapps and dispute it accordingly.  Thanks.

[Alexey Proskuryakov]

Thanks. So what are the opinions about file, data and custom URL schemes? There isn't anything wrong about sync requests to those, and Anne's spec doesn't cover non-HTTP protocols anyway.

[Jarred Nicholls]

(In reply to comment #30)
> Thanks. So what are the opinions about file, data and custom URL schemes? There isn't anything wrong about sync requests to those, and Anne's spec doesn't cover non-HTTP protocols anyway.

I think those being synchronous are okay.  If between you and I we're good with that idea, I can modify the patch to only reject HTTP requests.  Thanks for bringing that up.

[Alexey Proskuryakov]

Comment on attachment 120361 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=120361&action=review

> If between you and I we're good with that idea, I can modify the patch to only reject HTTP requests.

Thanks, let's limit this to http and https then. Comments about the current version below.

> Source/WebCore/xml/XMLHttpRequest.cpp:286
> -    if (m_state != OPENED || m_loader) {
> +    if (m_state > OPENED || m_loader) {

This change is not explained in ChangeLog, and doesn't match the spec (one should also be able to set this property in HEADERS_RECEIVED state). Please address this, and add a separate test case.

> Source/WebCore/xml/XMLHttpRequest.cpp:293
> +        ec = INVALID_ACCESS_ERR;
> +        return;

I think that we owe developers a console message here.

> Source/WebCore/xml/XMLHttpRequest.cpp:-418
> -    m_responseTypeCode = ResponseTypeDefault;

In fact, this should not be only tested and mentioned in ChangeLog, but be done in a separate bug.

> Source/WebCore/xml/XMLHttpRequest.cpp:447
> +    // Synchronous requests from a window context should not be able to use responseType per W3C spec.

You have a comment here, but not in setResponseType(). The comment could say more about "why", e.g. "Newer functionality is not available to synchronous requests in window contexts, as a spec-mandated attempt to discourage synchronous XHR use."

> Source/WebCore/xml/XMLHttpRequest.cpp:449
> +    if (!async && scriptExecutionContext()->isDocument()
> +        && m_responseTypeCode != ResponseTypeDefault) {

I wouldn't have wrapped this line.

> Source/WebCore/xml/XMLHttpRequest.cpp:451
> +        ec = INVALID_ACCESS_ERR;
> +        return;

I think that we owe developers a console message here.

> LayoutTests/fast/xmlhttprequest/xmlhttprequest-responsetype-sync-request.html:6
> +        description('This tests that the XMLHttpRequest responseType attribute is not usable with synchronous requests.');

We can still read it, so I would say "not modifiable".

[Jarred Nicholls]

Comment on attachment 120361 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=120361&action=review

Thanks for the review!

>> Source/WebCore/xml/XMLHttpRequest.cpp:286
>> +    if (m_state > OPENED || m_loader) {
> 
> This change is not explained in ChangeLog, and doesn't match the spec (one should also be able to set this property in HEADERS_RECEIVED state). Please address this, and add a separate test case.

This is explained in the ChangeLog: "responseType can be set before or directly after a call to open()", meaning the request doesn't need to be exactly in the OPENED state to be set.  I can make that more clear.  And you're right, per the spec, HEADERS_RECEIVED is a valid state to set it; however, HEADERS_RECEIVED is set in didReceiveData() and then choices are made directly after that regarding text decoding etc. etc., which need the responseType set in stone in order for those decisions to be made.  The LOADING state isn't set until until after those decisions have been made.  Maybe I'm missing something, but I assume the ThreadableLoaderClient is called asynchronously as data loads in, allowing someone to (with wicked timing) set the responseType after the decoder decisions have been made, but before the state is changed to LOADING.

>> Source/WebCore/xml/XMLHttpRequest.cpp:293
>> +        return;
> 
> I think that we owe developers a console message here.

Fair enough.

>> Source/WebCore/xml/XMLHttpRequest.cpp:-418
>> -    m_responseTypeCode = ResponseTypeDefault;
> 
> In fact, this should not be only tested and mentioned in ChangeLog, but be done in a separate bug.

Again I can make this more clear, but this is to support the notion that "responseType can be set before or directly after a call to open()", otherwise a call to open() will trump any value set to responseType before the call to open() was made.

I'm not sure I totally agree that this is to be done in a separate bug because it's all related to denying access to responseType.  The decision to "deny access" obviously depends upon the value of responseType when the call to open() is made.  A separate bug would likely be called "responseType should be modifiable <= OPENED" but it would have to additionally include the "deny access" snippet further down in open(), which is then arguably unrelated to the bug.  It's better coupled together I think then split out over X amount of time.

>> Source/WebCore/xml/XMLHttpRequest.cpp:447
>> +    // Synchronous requests from a window context should not be able to use responseType per W3C spec.
> 
> You have a comment here, but not in setResponseType(). The comment could say more about "why", e.g. "Newer functionality is not available to synchronous requests in window contexts, as a spec-mandated attempt to discourage synchronous XHR use."

Ok.

>> Source/WebCore/xml/XMLHttpRequest.cpp:449
>> +        && m_responseTypeCode != ResponseTypeDefault) {
> 
> I wouldn't have wrapped this line.

Yeah I will hoist it.

>> LayoutTests/fast/xmlhttprequest/xmlhttprequest-responsetype-sync-request.html:6
>> +        description('This tests that the XMLHttpRequest responseType attribute is not usable with synchronous requests.');
> 
> We can still read it, so I would say "not modifiable".

Agreed.

[Alexey Proskuryakov]

> This is explained in the ChangeLog

My apologies, I looked at (missing) per-function comments, and didn't notice that there was an explanation above.

>  Maybe I'm missing something, but I assume the ThreadableLoaderClient is called asynchronously as data loads in, allowing someone to (with wicked timing) set the responseType after the decoder decisions have been made, but before the state is changed to LOADING.

This should be easy to do in a readystatechange event handler. Probably not a very practical use case, but we could just as well do what the spec says.

> I'm not sure I totally agree that this is to be done in a separate bug because it's all related to denying access to responseType.

The parts of the change I'd like split into a separate bug affect async requests and make it permissible to change responseType when it didn't use to be allowed, so they feel quite out of place in a patch for a bug entitled "Synchronous XHR in window context should not support new XHR responseTypes".

I'd like to have the potentially disruptive part where we block stuff that used to be allowed in a completely isolated patch, so that it would be easier to investigate and address potential regressions.

[Jarred Nicholls]

Sure I'm with you, makes good sense.  I'll work on it this evening.

[Jarred Nicholls]

Created attachment 120434 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 120434 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=120434&action=review

r- because of inappropriate use of reportUnsafeUsage. I suggest just using addConsoleMessage directly (you can consider abstracting it out when adding the second check later).

> Source/WebCore/xml/XMLHttpRequest.cpp:301
> +    // attempt to discourage synchronous XHR use. responseType is one such function.

responseType is not a function :)

> Source/WebCore/xml/XMLHttpRequest.cpp:305
> +        reportUnsafeUsage(scriptExecutionContext(), "XMLHttpRequest cannot set responseType for synchronous HTTP(S) requests made from the window context.");

This is not unsafe usage, so calling a function named "reportUnsafeUsage" is not appropriate.

The comment appears OK, although grammar seems slightly off (it's not XMLHttpRequest who does the setting). I'd have said "XMLHttpRequest.responseType cannot be changed ... from a window context".

[Jarred Nicholls]

Comment on attachment 120434 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=120434&action=review

>> Source/WebCore/xml/XMLHttpRequest.cpp:301
>> +    // attempt to discourage synchronous XHR use. responseType is one such function.
> 
> responseType is not a function :)

I didn't mean function as in "method", but function as in "a piece of functionality", i.e., "responseType is one such [piece of functionality] that I was just talking about in the previous sentence". Programmers are likely to misinterpret this, so I'll make it more clear :)

>> Source/WebCore/xml/XMLHttpRequest.cpp:305
>> +        reportUnsafeUsage(scriptExecutionContext(), "XMLHttpRequest cannot set responseType for synchronous HTTP(S) requests made from the window context.");
> 
> This is not unsafe usage, so calling a function named "reportUnsafeUsage" is not appropriate.
> 
> The comment appears OK, although grammar seems slightly off (it's not XMLHttpRequest who does the setting). I'd have said "XMLHttpRequest.responseType cannot be changed ... from a window context".

I'll rename reportUnsafeUsage to something more generic, like "reportErrorToConsole" or "(write|log)ConsoleError".

Thanks I'll clean up the console message.

[Jarred Nicholls]

Created attachment 120458 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 120458 [details]
Patch

OK.

I'm now thinking that we should add "Please use asynchronous requests" to console message. But that can be considered later.

[Jarred Nicholls]

Comment on attachment 120458 [details]
Patch

Clearing flags on attachment: 120458

Committed r103629: <http://trac.webkit.org/changeset/103629>

[Jarred Nicholls]

All reviewed patches have been landed.  Closing bug.

[Darin Fisher (:fishd, Google)]

It looks like this change broke Mandreel WebGL-based apps.

For example:
http://chrome.monsterdashgame.com/

Generates this error:
"XMLHttpRequest.responseType cannot be changed for synchronous HTTP(S) requests made from the window context."

[Darin Fisher (:fishd, Google)]

It looks like Mandreel is using this to simulate fopen.  In their case, they use it to load local files from the WebFileSystem.  Given that they are trying to support fopen (for emulating an OS runtime), I don't see any other way for them to implement this.

[Olli Pettay (:smaug)]

(In reply to comment #44)
> It looks like Mandreel is using this to simulate fopen.  In their case, they use it to load local files from the WebFileSystem.  Given that they are trying to support fopen (for emulating an OS runtime), I don't see any other way for them to implement this.

That is not a reason to make web platform worse, IMO.
There should not be any UI blocking APIs in the window context.

Mandreel could just wait for async XHR load before doing anything else.

[Jarred Nicholls]

(In reply to comment #44)
> It looks like Mandreel is using this to simulate fopen.  In their case, they use it to load local files from the WebFileSystem.  Given that they are trying to support fopen (for emulating an OS runtime), I don't see any other way for them to implement this.

I don't know the internals of their compiler, but since they are in control of it, they could work around this by translating any sync call stack that invokes fopen() into an async pattern that doesn't continue execution until the async XHR request has completed.  I'm quite certain this is easier said than done, since their fopen() translation use to be self-contained but will now have to be context sensitive to where it is used...but it is definitely doable and would yield a better experience for fully web-based games that are loading large resources (e.g. textures) from remote sources.

[Darin Fisher (:fishd, Google)]

Olli, Jarred:  I certainly have no love for synchronous XHR.  It is an abomination to be sure.  I'm more concerned about the fact that this behavior has been in WebKit / Chrome for nearly a year, and we have changed it suddenly without warning and broken real web pages as a result.  It seems like bad form to make breaking changes in this fashion.  (I'm in contact with the Mandreel devs.)

[Jarred Nicholls]

(In reply to comment #47)
> Olli, Jarred:  I certainly have no love for synchronous XHR.  It is an abomination to be sure.  I'm more concerned about the fact that this behavior has been in WebKit / Chrome for nearly a year, and we have changed it suddenly without warning and broken real web pages as a result.  It seems like bad form to make breaking changes in this fashion.  (I'm in contact with the Mandreel devs.)

Let us know what Mandreel devs come back with.  I agree it is bad form; we all had to of expected someone, somewhere, would be affected by this change, but I always had in the back of my mind that the switch to async XHR would be trivial for those edge cases.  Mandreel is one that may not be so trivial.

If they can work around it, great, but if they cannot, we should do another kumbaya/huddle with those who have implemented this already (Mozilla, Opera) and public-webapps.

[Miguel Angel Pastor]

(In reply to comment #48)
> (In reply to comment #47)
> > Olli, Jarred:  I certainly have no love for synchronous XHR.  It is an abomination to be sure.  I'm more concerned about the fact that this behavior has been in WebKit / Chrome for nearly a year, and we have changed it suddenly without warning and broken real web pages as a result.  It seems like bad form to make breaking changes in this fashion.  (I'm in contact with the Mandreel devs.)
> 
> Let us know what Mandreel devs come back with.  I agree it is bad form; we all had to of expected someone, somewhere, would be affected by this change, but I always had in the back of my mind that the switch to async XHR would be trivial for those edge cases.  Mandreel is one that may not be so trivial.
> 
> If they can work around it, great, but if they cannot, we should do another kumbaya/huddle with those who have implemented this already (Mozilla, Opera) and public-webapps.

Hi,

I'm Miguel Angel Pastor, Mandreel developer

when porting native apps to web we encountered the issue with fopen. The solution we figured out was using application cache + synchronous XHR. Mandreel games wait for application cache to be ready with all the files game is using, so we know all the fopen are gonna open files stored in the application cache. 
We understand synchronous XHR in the web context are evil, but using Application Cache we know the fopen is gonna be always a hit
We recently added a new way to load files in Mandreel, having an ArrayBuffer with all the files in memory, it works for most of the games, but we have 2 games having more than 100 megabytes in resources, memory usage for those games is gonna be big.
From next week we are gonna fix the already published games to address this issue, but we don't see any problem with using synchronous XHR for CWS installed apps or when opening files in the application cache, the XHR is safe

Thanks,
Miguel

[Olli Pettay (:smaug)]

(In reply to comment #49)
> when porting native apps to web we encountered the issue with fopen. The solution we figured out was using application cache + synchronous XHR. Mandreel games wait for application cache to be ready with all the files game is using, so we know all the fopen are gonna open files stored in the application cache. 
> We understand synchronous XHR in the web context are evil, but using Application Cache we know the fopen is gonna be always a hit
But if application cache is stored on disk, sync XHR may block UI for a long time. So, in general case, I don't see how using Application Cache helps here.


> We recently added a new way to load files in Mandreel, having an ArrayBuffer with all the files in memory, it works for most of the games, but we have 2 games having more than 100 megabytes in resources, memory usage for those games is gonna be big.
> From next week we are gonna fix the already published games to address this issue, but we don't see any problem with using synchronous XHR for CWS installed apps or when opening files in the application cache, the XHR is safe
Sync XHR is not about "safety" but about blocking UI thread.
Have you tested Mandreel when the application cache data is stored somewhere in NFS?

[Miguel Angel Pastor]

in all the tests we did and so far clients haven't complained, using synchronous XHR from application cache files is fast enough

reading from disk it has been fast enough, we never got a "this script is taking too long" message
and we haven't tested on NFS, didn't know application cache can be saved there

(In reply to comment #50)
> (In reply to comment #49)
> > when porting native apps to web we encountered the issue with fopen. The solution we figured out was using application cache + synchronous XHR. Mandreel games wait for application cache to be ready with all the files game is using, so we know all the fopen are gonna open files stored in the application cache. 
> > We understand synchronous XHR in the web context are evil, but using Application Cache we know the fopen is gonna be always a hit
> But if application cache is stored on disk, sync XHR may block UI for a long time. So, in general case, I don't see how using Application Cache helps here.
> 
> 
> > We recently added a new way to load files in Mandreel, having an ArrayBuffer with all the files in memory, it works for most of the games, but we have 2 games having more than 100 megabytes in resources, memory usage for those games is gonna be big.
> > From next week we are gonna fix the already published games to address this issue, but we don't see any problem with using synchronous XHR for CWS installed apps or when opening files in the application cache, the XHR is safe
> Sync XHR is not about "safety" but about blocking UI thread.
> Have you tested Mandreel when the application cache data is stored somewhere in NFS?

[Olli Pettay (:smaug)]

(In reply to comment #49)
> but we don't see any problem with using synchronous XHR for CWS installed apps
Oh, and if CWS means Chrome Web Apps, that is a walled garden thing, so, not really about this bug, IMO.
(I'm a Gecko developer who cares about the open web ;) )

[Miguel Angel Pastor]

(In reply to comment #52)
> (In reply to comment #49)
> > but we don't see any problem with using synchronous XHR for CWS installed apps
> Oh, and if CWS means Chrome Web Apps, that is a walled garden thing, so, not really about this bug, IMO.
> (I'm a Gecko developer who cares about the open web ;) )

yeah, CWS mean Chrome Web Store

[Olli Pettay (:smaug)]

(In reply to comment #51)
> in all the tests we did and so far clients haven't complained, using synchronous XHR from application cache files is fast enough
> 
> reading from disk it has been fast enough, we never got a "this script is taking too long" message
> and we haven't tested on NFS, didn't know application cache can be saved there

Web browsers may or may not store application cache somewhere in disk. And the disk is possibly 
NFS. Or even if it is not NFS, reading from disk might cause fsyncs.
What I mean that there is no guarantee that application cache is really fast. There is only (reasonable good)
guaranteed that the application cached data is really there.

[Jarred Nicholls]

Miguel, sync XHR still works in Workers (where it belongs).  Can you utilize workers and run Mandreel's processing in a worker, and post message notifications on UI updates?  Let's at least attempt to analyze and resolve this as it is now before deciding to renege on a very favorable change to the web platform (one that's drafted in the spec).

[Alexey Proskuryakov]

I disagree with the view that we should treat network loading and local cache access as equivalent in terms of expected performance characteristics. If local cache is configured in a way that does not provide seamless experience, it's not an API design issue.

[Miguel Angel Pastor]

(In reply to comment #55)
> Miguel, sync XHR still works in Workers (where it belongs).  Can you utilize workers and run Mandreel's processing in a worker, and post message notifications on UI updates?  Let's at least attempt to analyze and resolve this as it is now before deciding to renege on a very favorable change to the web platform (one that's drafted in the spec).

If we can get access to the WebGL context and Web Audio context in the Web Worker, we are more than happy and that's what we really want.

[James Robinson]

(In reply to comment #57)
> (In reply to comment #55)
> > Miguel, sync XHR still works in Workers (where it belongs).  Can you utilize workers and run Mandreel's processing in a worker, and post message notifications on UI updates?  Let's at least attempt to analyze and resolve this as it is now before deciding to renege on a very favorable change to the web platform (one that's drafted in the spec).
> 
> If we can get access to the WebGL context and Web Audio context in the Web Worker, we are more than happy and that's what we really want.

I think that'd be the best long-term way to fix this particular problem.

Workers have some other tricky issues - such as how to handle events - but I think they are tractable.

[Miguel Angel Pastor]

> I think that'd be the best long-term way to fix this particular problem.
> 
> Workers have some other tricky issues - such as how to handle events - but I think they are tractable.

so we agreed with this solution, we just need to know when it's gonna be ready :) for the moment we are just happy with only WebGL context and being exclusive, we don't need WebGL on the UI

[Alexey Proskuryakov]

This caused bug 80991.

[Gordon Williams]

Hi, Please can somebody clarify the situation here?

This change has just broken our WebGL-based website: www.morphyre.com/scenedesign/go

Can WebGL be accessed from a Worker thread currently? A quick search seems to suggest not.

Is there any way around this change for a WebGL website currently, other than to convert JavaScript to asynchronous?

We're in a similar situation to Mandreel where we are converting code that was designed to be synchronous. Our compilation process works from binaries, not source, so trying to automatically convert the code to be asynchronous is amazingly difficult.

I think realistically our workaround will have to be to use a server-side script to create JSON, pass this back as a string and then evaluate it with eval - which will result in SIGNIFICANTLY worse user experience than if this had just been left alone.

[Jarred Nicholls]

(In reply to comment #61)
> Hi, Please can somebody clarify the situation here?
> 
> This change has just broken our WebGL-based website: www.morphyre.com/scenedesign/go
> 
> Can WebGL be accessed from a Worker thread currently? A quick search seems to suggest not.
> 
> Is there any way around this change for a WebGL website currently, other than to convert JavaScript to asynchronous?
> 
> We're in a similar situation to Mandreel where we are converting code that was designed to be synchronous. Our compilation process works from binaries, not source, so trying to automatically convert the code to be asynchronous is amazingly difficult.
> 
> I think realistically our workaround will have to be to use a server-side script to create JSON, pass this back as a string and then evaluate it with eval - which will result in SIGNIFICANTLY worse user experience than if this had just been left alone.

If everyone doesn't mind: let's continue this conversation on public-webapps and not on 3 different bug tickets in 2 different bug tracking systems.  It's becoming redundant.