Boris Zbarsky [:bzbarsky] Assignee
	Description • 9 years ago

STEPS TO REPRODUCE:  Run this HTML:

  <script>
    var arr = new ArrayBuffer(1);
    var xhr = new XMLHttpRequest;
    xhr.open("POST", "http://mozilla.org");
    xhr.setRequestHeader("Content-Type", "triggering/preflight");
    xhr.send();
  </script>

EXPECTED RESULTS: Network monitor shows the OPTIONS request.

ACTUAL RESULTS: In e10s mode it doesn't.  Makes it a bit hard to debug CORS failures.

	Nick Alexander :nalexander [he/him] (PTO until Montreallhands August 21, 2023)
	Comment 1 • 9 years ago

I don't see the OPTIONS request in the network monitor when running in Fennec.  Is that the same underlying cause, or should I file a new ticket?

	(no longer active)
	Comment 2 • 8 years ago

I second Boris.  This makes debugging a huge pain.  :(

	Jarda Snajdr [:jsnajdr]
	Comment 3 • 8 years ago

When the NetworkMonitor is notified about activity on a channel, it checks whether the channel is associated with the window or topFrame it is watching [1]. If it's not, it ignores the channel.

The topFrame and/or window are properties of the nsILoadContext retrieved by NetworkHelper.getTopFrameForRequest [2].

In e10s, the load context of a CORS preflight request fails to be retrieved, because both notificationCallbacks and loadGroup are null. Therefore, the channel doesn't match and is ignored.

In non-e10s, the loadGroup is set on the preflight request, it matches the window and the request is displayed in the network monitor.

So, is it a bug that the e10s preflight request is not part of any loadGroup?

[1] https://dxr.mozilla.org/mozilla-central/source/devtools/shared/webconsole/network-monitor.js#769
[2] https://dxr.mozilla.org/mozilla-central/source/devtools/shared/webconsole/network-helper.js#249

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

> So, is it a bug that the e10s preflight request is not part of any loadGroup?

That depends.  Are you doing this in the parent process or the child process?

In the child process, the request should totally be in a loadgroup.  It's put in the same loadgroup as the main channel in nsCORSListenerProxy::StartCORSPreflight:

1368   nsCOMPtr<nsILoadGroup> loadGroup;
1369   rv = aRequestChannel->GetLoadGroup(getter_AddRefs(loadGroup));
....

1387   rv = NS_NewChannelInternal(getter_AddRefs(preflightChannel),
1388                              uri,
1389                              loadInfo,
1390                              loadGroup,
1391                              nullptr,   // aCallbacks
1392                              loadFlags);

In the parent process... I'm not sure what our setup is.  But before I go looking into that, which process is this code in?  How does it work for the non-preflight: is it finding a loadgroup or channel callbacks?

	Jarda Snajdr [:jsnajdr]
	Comment 5 • 8 years ago

(In reply to Boris Zbarsky [:bz] from comment #4)
> But before I go looking into that, which process is this code in? How does it work
> for the non-preflight: is it finding a loadgroup or channel callbacks?

It happens in the parent. For non-preflight requests, the load context is retrieved from request.notificationCallbacks (it supports nsILoadContext). For the preflight request, however, querying request.notificationCallbacks for nsILoadContext fails with NS_NOINTERFACE.

In e10s parent, the request never has the loadGroup property set, for preflight and normal requests alike.

In non-e10s, all requests on the page, including the preflight one, have loadGroup set and nsILoadContext can be reached from there. So do all requests in e10s child.

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

> For non-preflight requests, the load context is retrieved from request.notificationCallbacks
> (it supports nsILoadContext)

Hmm.  So why is this not happening for preflights?

And the answer is: we do the preflight in the PARENT PROCESS apparently.  That actually kinda sort makes sense.  But it means that it never gets a useful load context set up on it.

Jonas, would it make sense to copy the original channel's notification callbacks over to the preflight?  Seems like it would do the right thing in general...

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

The big compat worry is the non-e10s case and people (read: extensions) assuming that XHR as channel's notification callbacks means it's an actual XHR request... If we care, we could just copy over the notification callbacks when the loadgroup is null.

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

Ah, actually, that doesn't work anyway because the CORS stuff clobbers the notification callbacks on the preflight.  But that's easy to deal with.  Patch coming up, but I have no idea how to write a test for this...

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

And I could really use help with a test.

	Jarda Snajdr [:jsnajdr]
	Comment 11 • 8 years ago

Added a devtools netmonitor mochitest. Fails in e10s before the patch (OPTION request not shown), passes after the patch.

	Jarda Snajdr [:jsnajdr]
	Comment 12 • 8 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=16055c8fea59

	Jarda Snajdr [:jsnajdr]
	Comment 13 • 8 years ago

Improved version of the test - the try run failed miserably because I misplaced a "skip-if" line in browser.ini. I'll submit a new try run when the try gets opened again.

	Jarda Snajdr [:jsnajdr]
	Comment 14 • 8 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=700592df320b

	Jarda Snajdr [:jsnajdr]
	Comment 15 • 8 years ago

Try run considered OK, the many failures on Win7 are all caused by clipboard problems - bug 1270962.

	Jan Honza Odvarko [:Honza] (always need-info? me)
	Comment 16 • 8 years ago

Comment on attachment 8755434 [details] [diff] [review]
Fix CORS preflights to provide a useful nsILoadContext, so they show up in our devtools network monitor properly - test

Looks good to me! I also tested the patch and works well on my machine.

Honza

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 17 • 8 years ago

Could we make the devtools use nsILoadInfo instead of nsILoadContext to get the relevant information here?

I.e. could we change the [1] function to use request.loadInfo somehow? I guess a problem is that we don't have information about the top-level window object there. That's something we should add though.

Another problem is that for network requests coming from workers we don't have window information in loadInfo yet.

[1] https://dxr.mozilla.org/mozilla-central/source/devtools/shared/webconsole/network-helper.js#233

So yeah, I guess this is the simplest way for now. But I hate to add more "features" on nsILoadContext given how hacky it is.

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 18 • 8 years ago

Comment on attachment 8754924 [details] [diff] [review]
Fix CORS preflights to provide a useful nsILoadContext, so they show up in our devtools network monitor properly

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

Assuming the answer to the question below is "yes", r=me

::: netwerk/protocol/http/nsCORSListenerProxy.cpp
@@ +1300,5 @@
>  {
> +  if (aIID.Equals(NS_GET_IID(nsILoadContext)) && mLoadContext) {
> +    nsCOMPtr<nsILoadContext> copy = mLoadContext;
> +    copy.forget(aResult);
> +    return NS_OK;

I'm surprised this works. This is the stream-listener, not the notificationCallbacks. Or are we setting this object both as streamlistener and notificationCallbacks?

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

> Or are we setting this object both as streamlistener and notificationCallbacks?

We are, yes.

	Pulsebot
	Comment 20 • 8 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/baee93983a1a
https://hg.mozilla.org/integration/mozilla-inbound/rev/1f3bc3649264

	Jarda Snajdr [:jsnajdr]
	Comment 21 • 8 years ago

(In reply to Jonas Sicking (:sicking) from comment #17)
> Could we make the devtools use nsILoadInfo instead of nsILoadContext to get
> the relevant information here?

To figure out which network requests belong to a particular tab and should be displayed in the network monitor, devtools use quite complicated code at [1], which checks the associatedWindow, topFrameElement and appId attributes of nsILoadContext, depending on circumstances (e10s, non-e10s, b2g, ...). Any improvement that makes this code comprehensible to mere mortals is welcome, of course.

[1] https://dxr.mozilla.org/mozilla-central/source/devtools/shared/webconsole/network-monitor.js#769-835

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

https://hg.mozilla.org/mozilla-central/rev/baee93983a1a
https://hg.mozilla.org/mozilla-central/rev/1f3bc3649264