Bug 24089 – ThreadableLoader::loadResourceSynchronously should do callbacks like the async code.

• Component: Page Loading
• Status: RESOLVED FIXED

[David Levin]

The xhr sync send code path needs to be fixed up to allow it to work the same for both Documents and Workers.

[David Levin]

Created attachment 27876 [details]
Proposed fix.

[Alexey Proskuryakov]

Comment on attachment 27876 [details]
Proposed fix.

I don't see where identifier changes from std::numeric_limits<unsigned long>::max() to a real one now. I think that the fix is just to assign the return value of FrameLoader::loadResourceSynchronously() to it.

+ function log(htmlMessage)

This looks like overkill. You don't ever pass HTML-formatted messages to this function, do you?

r=me with two conditions:
1) Please test that Web Inspector still displays XHR responses correctly.
2) Please verify that the change in behavior matches other browsers, not just the spec.

[David Levin]

Comment on attachment 27876 [details]
Proposed fix.

The behavior doesn't match other browsers (namely Firefox), so I'll revise it.

[David Levin]

Created attachment 27928 [details]
Proposed fix.

For now, I've change the patch to not throw an exception when there is a redirect failure due to a security issue which seemed consistent with IE and FF, but I've sent an email about this issue and can easy change this aspect of the behavior in the future.


I left the log message as html format not because I needed it but because it seemed simpler.  (I didn't find a built in js function to escape text and (to my memory) innerText didn't work in FF.)

[David Levin]

btw, I also verified:  Please test that Web Inspector still displays XHR responses correctly.

[David Levin]

Comment on attachment 27928 [details]
Proposed fix.

ap suggested "You can use document.createTextNode() and appendChild() to add text to the newly created span."  
So I'll fix that.

[David Levin]

Created attachment 27961 [details]
Proposed fix.

After further review, FF3 doesn't throw an exception in the sync redirect failed case, but the nightlies of FF do.  Also IE throw an exception as well (I made a mistake in my previous testing of IE because the test didn't work in IE for reasons I'd rather not discuss here.)

[Alexey Proskuryakov]

Comment on attachment 27961 [details]
Proposed fix.

> +    var logElement = document.getElementById('log');
> +    logElement.appendChild(document.createTextNode(message));
> +    logElement.appendChild(document.createElement("p"));

My suggestion was actually a bit different:

  var p = document.createElement("p");
  p.appendChild(document.createTextNode(message));
  logElement.appendChild(p);

But let's keep it as is - regression tests are all about testing unusual coding practices and edge cases ;)

> +        Changes the behavior of sync xhr for insecure redirects in two ways:
> +        + Sends an error event instead of an abort event (which is the same as async xhr's behavior).
> +        + Throws a network exception which is what the spec says to do (http://www.w3.org/TR/XMLHttpRequest/).

Please mention that it's what other browsers do.

> -    changeState(HEADERS_RECEIVED);

Does this happen in didFinishLoading()? I'm worried that this (not just this change, but refactoring in general) may subtly clash with work Julien Chaffraix is doing in bug 22475.

r=me

[David Levin]

Commited in r41215.


> > -    changeState(HEADERS_RECEIVED);
> Does this happen in didFinishLoading()? 
Yep, both didReceiveData and didFinishLoading advance the state to HEADERS_RECEIVED (if needed).

> I'm worried that this (not just this change, but refactoring in general) may subtly clash with
> work Julien Chaffraix is doing in bug 22475.
I looked at that patch and as far as this change is concerned, it should be fine.

I did notice differences in readyStateChange between WebKit and other browsers.  However, in this patch I didn't change WebKit's behavior for readyStateChange, so the differences in this regard remained the same with this change.

> My suggestion was actually a bit different...
Thanks for the info!