Closed Bug 1214752 Opened 8 years ago Closed 7 years ago

[e10s] CORS preflights are not shown in the network monitor

Categories

(DevTools :: Netmonitor, defect)

Product:
DevTools
Component:
Netmonitor
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(e10s+, firefox49 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 49
Tracking Flags:
Tracking Status
e10s + ---
firefox49 --- fixed

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

Details

Attachments

(2 files, 1 obsolete file)

Fix CORS preflights to provide a useful nsILoadContext, so they show up in our devtools network monitor properly
4.76 KB, patch
sicking
: review+
Details | Diff | Splinter Review
Fix CORS preflights to provide a useful nsILoadContext, so they show up in our devtools network monitor properly - test
6.03 KB, patch
Honza
: review+
Details | Diff | Splinter Review 
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.
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?
I second Boris.  This makes debugging a huge pain.  :(
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
> 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?
(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.
Blocks: 1270096
> 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...
Flags: needinfo?(jonas)
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.
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...
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
And I could really use help with a test.
Added a devtools netmonitor mochitest. Fails in e10s before the patch (OPTION request not shown), passes after the patch.
Attachment #8755190 - Flags: review?(odvarko)
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.
Attachment #8755434 - Flags: review?(odvarko)
Attachment #8755190 - Attachment is obsolete: true
Attachment #8755190 - Flags: review?(odvarko)
Try run considered OK, the many failures on Win7 are all caused by clipboard problems - bug 1270962.
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
Attachment #8755434 - Flags: review?(odvarko) → review+
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.
Flags: needinfo?(jonas)
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?
Attachment #8754924 - Flags: review?(jonas) → review+
> Or are we setting this object both as streamlistener and notificationCallbacks?

We are, yes.
(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
https://hg.mozilla.org/mozilla-central/rev/baee93983a1a
https://hg.mozilla.org/mozilla-central/rev/1f3bc3649264
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox49: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.