Hallvord R. M. Steen [:hallvors] Reporter
	Description • 11 years ago

Test cases that throw wrong exceptions:
http://www2.w3c-test.org/web-platform-tests/master/XMLHttpRequest/send-after-setting-document-domain.htm
http://w3c-test.org/web-platform-tests/master/XMLHttpRequest/send-network-error-sync-events.htm
http://w3c-test.org/web-platform-tests/master/XMLHttpRequest/send-redirect-bogus-sync.htm
http://w3c-test.org/web-platform-tests/master/XMLHttpRequest/send-redirect-infinite-sync.htm
http://w3c-test.org/web-platform-tests/master/XMLHttpRequest/xmlhttprequest-network-error-sync.htm

	Hallvord R. M. Steen [:hallvors] Reporter
	Comment 1 • 11 years ago

and http://w3c-test.org/web-platform-tests/master/XMLHttpRequest/send-non-same-origin.htm

	Thomas Wisniewski Assignee
	Comment 2 • 8 years ago

Here's a patch that fixes things so that NetworkErrors are thrown where expected (though the test "send-non-same-origin.sub.htm" still fails for unrelated reasons).

A try run seems clean: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b667caddd6dc

Note that there was a complication for the XHR web platform test "send-after-setting-document-domain.htm": it tries to access window.opener after modifying document.location (to change over to a subdomain before running its tests). This isn't allowed in Firefox, so the test harness reports an error, despite the things actually being tested passing fine.

I don't think a decision has been made as to whether Firefox is correct to block window.opener in this case. Chrome allows it, but both approaches seem reasonable to me. Since the test isn't really meant to test that anyway, I opted to remove its reliance on window.opener; now it passes the data it needs as URL params while it changes location.href. I'm needinfo'ing :annevk in case that's sub-optimal.

	Thomas Wisniewski Assignee
	Comment 3 • 8 years ago

:annevk is on vacation until next week, so I'll change the needinfo on the WPT change to :jgraham.

	James Graham [:jgraham]
	Comment 4 • 8 years ago

Uh, yeah I need to review that change, but I'm in a f2f meeting this week, so I don't know if I will get around to it.

	Boris Zbarsky [:bzbarsky]
	Comment 5 • 8 years ago

Comment on attachment 8770399 [details] [diff] [review]
918751-throw_NetworkErrors_instead_of_failures_where_XHR_WPTs_expect_them.diff

>+    ErrorResult rv;
>+    rv.Throw(NS_ERROR_DOM_NETWORK_ERR);
>+    return rv.StealNSResult();

How about:

  return NS_ERROR_DOM_NETWORK_ERR;

?  That said, what's a web-observable way we hit this line of code?  I guess we could fail CSP checks, but I don't see where that leads to a synchronous exception from send() in the spec for async XHR (as opposed to an async network error response).  What do other UAs do for a failing CSP check on async XHR?  Unsetting review request pending this being sorted out.

>   if (!mChannel) {
>-    return NS_ERROR_FAILURE;

This corresponds to the "failed sync XHR after succeeding AsyncOpen" case, I guess?  The change here makes sense, but again just:

  return NS_ERROR_DOM_NETWORK_ERR;

>+  var val = location.href.match(RegExp("[?&]" + name + "=([^&]*)"));

Please use a |new URLSearchParams(location.search.substr(1))| to do this stuff instead of using regexps.

>+if (timeout_multiplier !== undefined) props["timeout_multiplier"] = Number(timeout_multiplier);

Please put the if body on a separate line, and inside curlies.

	Thomas Wisniewski Assignee
	Comment 6 • 8 years ago

>Please use a |new URLSearchParams(location.search.substr(1))| to do this stuff instead of using regexps.

I considered that, but it would break all of the tests on browsers which don't support URLSearchParams yet, and its MDN entry implies that Safari and IE/Edge don't yet support it.

	Thomas Wisniewski Assignee
	Comment 7 • 8 years ago

(In reply to Boris Zbarsky [:bz] from comment #5)
> That said, what's a web-observable way we hit this line of code?  I guess
> we could fail CSP checks, but I don't see where that leads to a synchronous
> exception from send() in the spec for async XHR (as opposed to an async
> network error response).  What do other UAs do for a failing CSP check on
> async XHR?  Unsetting review request pending this being sorted out.
> 

That line is hit if we simply do a GET to invalid URLs, like a mailto: or javascript: URI (giving NS_ERROR_DOM_BAD_URI), or a data: URI (giving NS_ERROR_ILLEGAL_VALUE). These WPTs cover those cases:
  http://w3c-test.org/XMLHttpRequest/send-non-same-origin.sub.htm
  http://w3c-test.org/XMLHttpRequest/send-network-error-sync-events.sub.htm

Chrome currently passes those tests and passes back a NetworkError instead. Firefox will still fail them even with this fix, but for reasons unrelated to the exception type. The former isn't CSP-blocking a GET access to a tel: URI I think (which Chrome does), and the latter just isn't firing all the expected ProgressEvents.

	Boris Zbarsky [:bzbarsky]
	Comment 8 • 8 years ago

> These WPTs cover those cases:

Those tests both use sync XHR.  For sync XHR, throwing here makes sense, because it's observably the same as synthesizing a network error fetch response: such a response causes sync XHR to throw.

My question was about async XHR.

	Thomas Wisniewski Assignee
	Comment 9 • 8 years ago

>My question was about async XHR.

Oh right, duh.

That said, when I modify the tests to be async, the mailto: and javascript: URIs still trigger an NS_ERROR_DOM_BAD_URI on that line... the data URI case doesn't throw.

Would that implicate the CSP code instead of this line?

	Boris Zbarsky [:bzbarsky]
	Comment 10 • 8 years ago

> the mailto: and javascript: URIs still trigger an NS_ERROR_DOM_BAD_URI on that line...

Yes, I know.  My point is that this is not spec-compliant as far as I can tell, and neither would be throwing a NetworkError.  And the question is what other browsers do, so we can figure out whether the spec needs to change or whether our implementation does.  We could push that off to a followup if you'd prefer, I guess.  Certainly throwing NetworkError here is no _worse_ than what we do now...

	Thomas Wisniewski Assignee
	Comment 11 • 8 years ago

I see, thanks for explaining things so patiently :)

Well I certainly don't mind waiting if you'd prefer, but since this seems like it's only going to nudge Firefox closer to the spec (and other implementations do the same), then I'd veer toward committing it and following up as you suggest.

Another thing I could change would be to continue throwing the failures as-is for system XHRs, only changing them for non-sys-XHRs. Do you suspect that would be better for overall compat?

	Boris Zbarsky [:bzbarsky]
	Comment 12 • 8 years ago

Comment on attachment 8770399 [details] [diff] [review]
918751-throw_NetworkErrors_instead_of_failures_where_XHR_WPTs_expect_them.diff

OK, so modulo the shorter return statements, this is fine, but please do file a bug on sorting out async XHR behavior here.

	Thomas Wisniewski Assignee
	Comment 13 • 8 years ago

Alright, thanks. Here's a new version with the shorter return statements and curly-bracket changes. Carrying over r+.

Prior to check-in I'll leave this open for :jgraham to take a final look at the WPT, unless you think that's unnecessary.

I'll also file a follow-up bug before I request check-in.

	Boris Zbarsky [:bzbarsky]
	Comment 14 • 8 years ago

I do think it's good for jgraham to take a look at the wpt changes.

	James Graham [:jgraham]
	Comment 15 • 8 years ago

Comment on attachment 8770711 [details] [diff] [review]
918751-throw_NetworkErrors_instead_of_failures_where_XHR_WPTs_expect_them.diff

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

This doesn't quite work. You can't put functions for the test to call into testharnessreport.js because that's explicitly only for browser-implementation-specific functions. It could be reasonable to write a more generic implementation and put it in testharness.js, but I think that my inclination would be to rewrite the test to first window.open a new window in a subdomain and write the document.domain parts in that window.

	Thomas Wisniewski Assignee
	Comment 16 • 8 years ago

I'm having trouble getting the window.open idea working, and figured I'd ask for suggestions before I went overboard on something that's probably the wrong approach.

This patch is what I'm basically attempting. I just cloned the existing document.domain-manupulating test into a "helper" file, and then ran it through window.open as you suggested. At that point I should be able to simply process the result messages that the child postMessages back to its parent (which is now us).

However, simply forwarding those messages to our own parent (the harness) seems to do nothing. As such I figured I'd just run shadow-tests mirroring the results that the child window gives instead. Unfortunately, the harness doesn't seem to realize that these tests have all ended, and hangs there waiting for all but the first one.

I'm guessing this is because the tests are run from inside of an event handler, but trying to defer them with a setTimeout yields the same results (even if I try async_test with test.done() instead). And so I figured I'd ask for insight before I try digging into the harness code to see what's holding things up.

	Thomas Wisniewski Assignee
	Comment 17 • 8 years ago

Alright, I had some time to look over this again. Is this what you had in mind, James?

	Thomas Wisniewski Assignee
	Comment 18 • 8 years ago

Actually, here's a simpler version of the same idea for that test.

	Thomas Wisniewski Assignee
	Comment 19 • 8 years ago

Comment on attachment 8780920 [details] [diff] [review]
918751-throw_NetworkErrors_instead_of_failures_where_XHR_WPTs_expect_them.diff

It seems as though James is busy lately, so would you mind taking a look at the web platform test change in this patch, Anne?

Basically I have send-after-setting-document-domain.htm run its tests from a window now (send-after-setting-document-domain-window.htm), because of what I mentioned in comment 2 in this bug.

I believe this is what James had in mind, but I'm not 100% sure if there's a better way to do it (given that postMessage is necessary due to the domain in the window differing).

	Anne (:annevk)
	Comment 20 • 8 years ago

Here's some feedback/questions:

* send-after-setting-document-domain-window.htm is not annotated as a support resource; perhaps it should be in a subdirectory or have "support" added to its name?
* "The spec doesn't seem to explicitly cover this case (as of June 2013)" is that actually true? I think which origin to use is covered.
* By the time the second run_async_test is invoked, document.domain was already set. Is that not problematic? Also, how is this testing fetches from the other origin? The URLs appear identical and your expected results are identical.
* Also, why does the support file have <div id=log>?

	Anne (:annevk)
	Comment 21 • 8 years ago

(Just to be clear, I greatly appreciate you trying to clean this all up. That's huge.)

	Thomas Wisniewski Assignee
	Comment 22 • 8 years ago

>send-after-setting-document-domain-window.htm is not annotated as a support resource; perhaps it should be in a subdirectory or have "support" added to its name?

I ended up just moving the support files into the existing resource subfolder in this version, if that's ok.


>"The spec doesn't seem to explicitly cover this case (as of June 2013)" is that actually true? I think which origin to use is covered.

I think the tests are valid according to spec now, so I've removed the comments that expressed uncertainty. But it's always good to have a second pair of eyes on this kind of thing, so let me know if you think they might be off.


>By the time the second run_async_test is invoked, document.domain was already set. Is that not problematic? Also, how is this testing fetches from the other origin? The URLs appear identical and your expected results are identical.

You're right, there was the potential for racing here. I've split the tests up into their own separate window.opened runs in this version, which should be safer and more straightforward.

Another big problem was that I wasn't actually running the correct second test at all (it was the same test twice). This version actually runs the correct test.


>Also, why does the support file have <div id=log>?

It shouldn't; it's gone in this version.


>(Just to be clear, I greatly appreciate you trying to clean this all up. That's huge.)

No problem. I'm still getting the hang of all of this, so thanks for being patient, too.

	Anne (:annevk)
	Comment 23 • 8 years ago

Comment on attachment 8783730 [details] [diff] [review]
918751-throw_NetworkErrors_instead_of_failures_where_XHR_WPTs_expect_them.diff

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

send-after-setting-document-domain-window.htm appears unused. That can probably be removed now? Seems fine otherwise. (I'm assuming you have tried running them.)

	Thomas Wisniewski Assignee
	Comment 24 • 8 years ago

Thanks Anne! Here's a version without the stray send-after-setting-document-domain-window.htm.

And yes, I've checked the tests, including a try-run, and it all seems fine: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8ada19c75826

As such, I'm carrying over r+ and requesting check-in.

	Pulsebot
	Comment 25 • 8 years ago

Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b64d6f5b63db
Throw NetworkErrors instead of failures where appropriate for some XHR web platform tests. r=bz, r=annevk

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 26 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/b64d6f5b63db

	Phil Ringnalda (:philor)
	Comment 27 • 8 years ago

Backed out in https://hg.mozilla.org/mozilla-central/rev/26e22af660e543ebb69930f082188b69ec756185 - despite the way we filed it as though it was intermittent, this caused a permanent "TEST-UNEXPECTED-PASS | /XMLHttpRequest/send-non-same-origin.sub.htm | XMLHttpRequest: send() - non same-origin (tel:+31600000000) - expected FAIL" on OS X.

	Thomas Wisniewski Assignee
	Comment 29 • 8 years ago

Thanks. I don't actually have an OSX machine handy, but I'm not sure if it's a problem if the test is just passing. bz, what do you think? Would it be alright to just mark it as passing with something like this, or is this a worrying sign that requires more investigating?

   [XMLHttpRequest: send() - non same-origin (tel:+31600000000)]
     expected:
       if (os != "osx"): FAIL

(The try-run in question shows that both OSX opt and debug are permapassing: https://treeherder.mozilla.org/#/jobs?repo=mozilla-central&revision=6c0d74730bd5)

	Boris Zbarsky [:bzbarsky]
	Comment 30 • 8 years ago

A better question is why it's failing on non-mac...  That should definitely be a cross-domain load, so I'd expect it to end up with a CORS/CheckMayLoad failure.  Comment 7 mentions CSP, but there shouldn't be any CSP here I don't think.

	Thomas Wisniewski Assignee
	Comment 31 • 8 years ago

Right now, we throw an NS_ERROR_UNKNOWN_PROTOCOL when trying to NS_NewChannel during XHR.open().

Per spec, we should of course be throwing NS_ERROR_DOM_NETWORK_ERR instead of NS_ERROR_UNKNOWN_PROTOCOL, but also we should not be failing during open(), but during send(). That's because the spec wants us to create the channel in send() step 5, not in open().

I've recently refactored the XHR code so it will be easy to defer creating the channel until the specified time in send()... at least for web content. Privileged/system XHRs cannot defer the channel creation until send(), because users of the internal XHR.channel API expect it to be created on open().

But if we're ok with just fixing non-privileged XHRs for now, this additional patch should fix the WPT. Will that be acceptable? If so I'll merge the patches, mark the WPT as passing, and do another try-run.

	Boris Zbarsky [:bzbarsky]
	Comment 32 • 8 years ago

Comment on attachment 8786940 [details] [diff] [review]
918751-defer-channel-creation-for-web-content-until-send-and-throw-appropriate-NetworkError.diff

Looks reasonable.  r=me

	Thomas Wisniewski Assignee
	Comment 33 • 8 years ago

Unfortunately this rabbit hole was deeper than expected.

There is code in the tree which creates a XHR that is not IsSystemXHR(), and then SpecialPowers.wraps it to access its channel after calling XHR.open(). This will of course get a null channel with my earlier patch, so that version of the patch is a no-go.

I was hoping to instead create the channel on-demand when an XHR.GetChannel() is issued after open (but before send), but that's a no-go as well, as SpecialPowers.wrap().channel doesn't actually call XHR.GetChannel().

As such there's not much I can see us doing except to continue creating the channel in open(), but ignore any exceptions that happen at that stage and instead throw them as a NetworkError (as per spec) at send()-time if the channel wasn't created (ie, it's null).

That approach works, but of course there is some code in the tree which relies on XHR.open() being the place where the exception is thrown. That implies there may be more code that does the same in addons/Thunderbird.

I suppose we could always just continue throwing during open() instead of send(), for IsSystemXHRs. That would limit any exception-handling changes to only those cases where they're not IsSystemXHRs for some reason (presumably that's only in our in-tree test code?)

What do you think? Is this reasonable to you, one way or the other? At least it passes try in its current form, with what appears to be a typical slew of unrelated intermittents: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9316db092ed6

	Boris Zbarsky [:bzbarsky]
	Comment 34 • 8 years ago

> as SpecialPowers.wrap().channel doesn't actually call XHR.GetChannel()

Uh... why would it not?  I would very much expect it to call it!

You just have to be careful because there are two GetChannels: <http://searchfox.org/mozilla-central/rev/3582398bc9db5a83e25c2a8299cc780a52ad7ca8/dom/xhr/XMLHttpRequestMainThread.cpp#433> and <http://searchfox.org/mozilla-central/rev/3582398bc9db5a83e25c2a8299cc780a52ad7ca8/dom/xhr/XMLHttpRequestMainThread.h#488>.  The latter one is the one that will get called.

A bigger concern is whether we have callers that expect open() to throw, as opposed to the .channel getter.  But I doubt we do for our tests (where SpecialPowers happens) and actual system code would still create the channel up front, I would hope.  I'm pretty sure that changing that last would be hard.

	Boris Zbarsky [:bzbarsky]
	Comment 35 • 8 years ago

Anyway, does the fact that you really _can_ lazy-create in GetChannel if you want to make you want to?

	Thomas Wisniewski Assignee
	Comment 36 • 8 years ago

>A bigger concern is whether we have callers that expect open() to throw.

Yes, there are at least the examples I found in my patch that caused tests to fail (including one non-test case). It's not the channel getter that's expected to fail, but open(). There may be other cases I haven't found yet, but I'll do one more look through the code to see.

>does the fact that you really _can_ lazy-create in GetChannel if you want to make you want to?

I don't really think it matters which of the two approaches we use, as they both change when the exception is thrown. The one in my latest patch is less code and probably a bit easier to follow, so I'd be inclined to stick with it, unless you suspect that the lazy-getter approach is better/safer for some reason I'm not seeing yet.

	Boris Zbarsky [:bzbarsky]
	Comment 37 • 8 years ago

Comment on attachment 8787705 [details] [diff] [review]
918751-defer_throwing_xhr_channel_creation_NetworkErrors_until_send_is_called.diff

> dom/system/NetworkGeolocationProvider.js

The change to this file doesn't seem to be needed.

r=me

	Thomas Wisniewski Assignee
	Comment 40 • 8 years ago

>The change to this file doesn't seem to be needed.

If it's omitted then the xpcshell test dom/tests/unit/test_geolocation_position_unavailable.js fails, because it formerly relied on xhr.open() throwing (but now xhr.channel will be null instead, and an exception will be thrown on that line rather than the one will open).

There were also a few more places in the codebase that I saw try/catching on an XHR.open() call, which might now fail in a similar way, so I've adjusted them as well to be on safe side (even though they weren't tripping any existing tests).

Here's a (hopefully) final patch that rolls up all the changes. A try run seems fine: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f167dd1d481d

Thanks a bunch for the patient reviews! Here's hoping this can be the last one (sans if you feel anyone else should also review some of these changes).

	Boris Zbarsky [:bzbarsky]
	Comment 41 • 8 years ago

Comment on attachment 8787924 [details] [diff] [review]
918751-throw_NetworkErrors_instead_of_failures_where_XHR_WPTs_expect_them.diff

>+++ b/browser/experiments/Experiments.jsm

Please get review from someone who knows this code.  The old code didn't modify this._networkRequest unless open() succeeded; your code will modify it.  That seems wrong to me, but it's not clear to me what behavior this code really wants.

>+++ b/devtools/shared/gcli/commands/jsb.js

Again, needs review from someone who knows this code.  In particular, does context.defer() have side-effects that need to be undone if we fail to open() or send()?

>+++ b/security/manager/tools/getHSTSPreloadList.js

This needs review from someone who knows this code too.

r=me on the rest.

	Thomas Wisniewski Assignee
	Comment 42 • 8 years ago

Comment on attachment 8787924 [details] [diff] [review]
918751-throw_NetworkErrors_instead_of_failures_where_XHR_WPTs_expect_them.diff

Thanks Boris!

Dave, Paul, Ehsan: would you all mind reviewing parts of this patch as per comment 41 (or helping me find more suitable reviewers)? I'm hoping that Dave can help with getHSTSPreloadList.js, Ehsan with Experiments.jsm, and Paul with the change to devtools/shared/gcli/commands/jsb.js.

Note that this patch will be changing when certain XHR exceptions are thrown, so that open() will no longer throw on AsyncOpen failure, but an exception will instead not be thrown until the subsequent send() call or access of the .channel property (which would be null on failure).

Thanks in advance!

	(no longer active)
	Comment 43 • 8 years ago

Comment on attachment 8787924 [details] [diff] [review]
918751-throw_NetworkErrors_instead_of_failures_where_XHR_WPTs_expect_them.diff

I have no experience with Experiments.jsm, sorry.

	Thomas Wisniewski Assignee
	Comment 44 • 8 years ago

Comment on attachment 8787924 [details] [diff] [review]
918751-throw_NetworkErrors_instead_of_failures_where_XHR_WPTs_expect_them.diff

mossop, how about you? Do you feel comfortable reviewing this patch's change to Experiments.jsm? The change was made because with this patch, XHR.open() will no longer throw on AsyncOpen anymore, but will just leave the channel null and defer throwing until send().

	Dana Keeler (she/her) (use needinfo) (:keeler for reviews)
	Comment 45 • 8 years ago

Comment on attachment 8787924 [details] [diff] [review]
918751-throw_NetworkErrors_instead_of_failures_where_XHR_WPTs_expect_them.diff

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

Speaking specifically about getHSTSPreloadList.js, in what cases would it actually throw if we left it as-is? There's sort-of a preexisting problem in that if it throws in that try/catch block, the overall script may eventually hang (if it happens enough).

	Dave Townsend [:mossop]
	Comment 46 • 8 years ago

Comment on attachment 8787924 [details] [diff] [review]
918751-throw_NetworkErrors_instead_of_failures_where_XHR_WPTs_expect_them.diff

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

r+ for the Experiments.jsm change.

	Thomas Wisniewski Assignee
	Comment 47 • 8 years ago

>in what cases would it actually throw if we left it as-is

Basically anytime the underlying AsyncOpen call fails and throws (ie, the channel could not be created). Unfortunately I'm still new to those parts of the code, so I'm not quite sure if that will include anything other than sanity-checks (the URL being valid, the port not being blocked, etc).

That said, I'm not opposed to leaving the change out and letting a subsequent ticket deal with any fallout and pre-existing issues, if you'd rather do that. I don't even mind waiting until that's dealt with first, though I'd of course like to land this as soon as possible, before it bitrots away.

	Paul Rouget [:paul]
	Comment 48 • 8 years ago

Comment on attachment 8787924 [details] [diff] [review]
918751-throw_NetworkErrors_instead_of_failures_where_XHR_WPTs_expect_them.diff

r+ for devtools code.

	Dana Keeler (she/her) (use needinfo) (:keeler for reviews)
	Comment 49 • 8 years ago

Comment on attachment 8787924 [details] [diff] [review]
918751-throw_NetworkErrors_instead_of_failures_where_XHR_WPTs_expect_them.diff

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

Ok - I think this is the right thing to do for getHSTSPreloadList.js, so let's go with it. There may be some follow-up work, but we can take care of that independent of this.

	Thomas Wisniewski Assignee
	Comment 50 • 8 years ago

Alright, thanks for the reviews everyone!

The patch still applies cleanly on inbound, compiles, and passes the tests that it changes, so I'm requesting checkin.

	Pulsebot
	Comment 51 • 8 years ago

Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5892d029f637
Throw NetworkErrors instead of failures where appropriate for some XHR web platform tests. r=bz, r=keeler, r=Mossop, r=paul

	Carsten Book [:Tomcat]
	Comment 52 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/5892d029f637