While fixing up Blink's fetch-canvas-taining.https.html, I came up against the indisputable fact that our network interception model is backwards. Specifically, the HTTP channel code looks at the channel's notification callbacks to see if interception should occur during AsyncOpen, but in the case that a CORS preflight request is required, this is too late. In a case where an <img crossorigin="anonymous" src="http://some.other.origin"> is used, this potentially manifests in the CORS preflight failing and the service worker never being given the chance to provide a same-origin response, for example. What _should_ happen instead is that before any CORS preflight request is made, the service worker should have a chance to intercept. This almost works in our current model, except that when the interception occurs we are intercepting the CORS preflight, rather than the original request. I have no idea how to fix this without moving CORS checks into the networking layer, unfortunately. This also ruins part of the offline story, since any cross-origin request while offline will potentially run afoul of CORS checks that can't complete.

I guess we could special case all of the places that use nsCORSListenerProxy right now, since it's not as big a list as I feared: http://mxr.mozilla.org/mozilla-central/search?string=corslistenerproxy

I think we should explore moving it into the networking layer. It seems the better long term solution.

We are to some extent already moving it into the network stack with the work happening in bug 1182535. But the work happening there is still using a listener which does the CORS checks, which sounds like that is tripping up the SW code? And the changes there does nothing for preflights. So it doesn't solve things, but it'd mean that we have much less code to change to fix this.

The original testcase described is incorrect; I was mistaken and <img src="http://cross.origin" crossOrigin="anonymous"> doesn't cause a preflight. However, an XHR that causes a preflight (such as an unusual method) will hit this same problem that I described above.

I and Josh talked about how to fix this in detail. The approach below is what we agreed to do. It may seem insane after reading it for the first time, but we believe that this is the least intensive change compared to how interception works today, and it keeps as much of the current preflight flow as we can. The basic idea is that instead of ForceNoIntercept()ing the preflight channel and do the interception after the preflight has been finished, we need to intercept the preflight channel itself but create a Request object reflecting the properties of the original request, and if the SW decided to respondWith() something, we would need to cancel the preflight before it goes to the network, and pipe that information on to the original request channel. 1. We start by telling the preflight HTTP channel that this is a preflight which may get intercepted. This will happen by changing the ForceNoIntercept() call to InterceptPreflightChannel() which would set a separate boolean indicating that this is a preflight which may get intercepted. 2. In addition, we will need a way to read the properties of the original request channel at the time when the interception actually happens (nsDocShell::ChannelIntercepted). Therefore, we need to pass a pointer to the original request channel as the context argument to AsyncOpen when opening the preflight channel, so that we can retrieve it later. 3. Now, we will create an instance of a new implementation of nsIInterceptedChannel called InterceptedPreflightChannelChrome, and store a pointer to it in a new field added to nsHttpChannel/HttpChannelChild. Let's call this field mInterceptedChannel. That implementation of nsIInterceptedChannel will have a pointer to the preflight channel object, and will also remember the information that it will eventually need to propagate to the original request channel inside it. From there, we would intercept the channel as usual. In OpenCacheEntry, instead of just checking ShouldIntercept(), we need to first check if we're dealing with a preflight by checking the flag we have set in #1 above. If we're dealing with a preflight channel, we create an instance of InterceptedPreflightChannelChrome, and intercept as usual. This new implementation will need to differ from InterceptedChannelChrome in that it shouldn't set anything on the preflight channel. 4. In InterceptedPreflightChannelChrome::FinishSynthesizedResponse, after making sure that the information that we need to propagate to the original request channel has been stored, we cancel the preflight channel with a special error value indicating that the preflight was intercepted. 5. In nsCORSPreflightListener::OnStartRequest(), we need to check whether the channel has been canceled with the special status code in step #4. In that case, we mark the original channel's mInterceptCache to ALREADY_INTERCEPTED, and AsyncOpen the channel as usual. We also need to read the mInterceptedChannel field added in step #3, and set its value on mOuterChannel, so that the original request channel has access to the InterceptedPreflightChannelChrome object. 6. In OpenCacheEntry for the original request channel, if mInterceptCache == ALREADY_INTERCEPTED, we get mInterceptedChannel, and use it to propagate all of the stored information in step #4 to the original request channel, and then we call FinishSynthesizedResponseForCORSChannel() which is similar to FinishSynthesizedResponse except that it sets stuff on the original request channel rather than mChannel, and it should also call ResurrectInfoOnChannel() on the original request channel. After this step, the interception has successfully completed, without us having done a preflight as far as the network is concerned. 7. If the service worker chooses to let the request go to the network in step #4, we never call InterceptedPreflightChannelChrome::FinishSynthesizedResponse() and instead call ResetInterception() which behaves exactly like InterceptedChannelChrome::ResetInterception(). That should cause the preflight to continue normally and everything else will behave as it does today. 8. We would need to have a IntercpetedPreflightChannelContent variant as well which will be very similar to InterceptedChannelContent, plus the same differences that Intercepted{,Preflight}ChannelChrome do.

Take a look at fetch-cors-xhr.https.html; it may cover all of the requisite test cases.

Honza, please see comment 5!

Before we go down this crazy short term solution, can we at least outline what would be required to do this the right way? We have so much technical debt in this area already, it really worries me to add more. Is anyone going to understand how this works in 6 months if we go with the plan in comment 5?

I don't fully understand all the problems involved here, so what I propose might not work. I think it should be possible make various DOM code create a single nsIChannel when preflights are needed, rather than create two (one for preflight and one for actual request) as we do now. Basically it would look like: 1. XHR (for example) would create a single nsIChannel, but add a DO_CORS_PREFLIGHT flag as one of the security-flags in the nsILoadInfo 2. XHR code calls AsyncOpen2 on the channel. 3. Channel doesn't hit the network but instead creates a second nsIChannel for the preflight. 4. We wait until the preflight either succeeds or fails. 5. If the preflight fail we call OnStartRequest/OnStopRequest with an error code. 6. If the preflight succeed, we hit the network like normal. The ServiceWorker code could then inject itself between step 2 and step 3. One of the harder parts here is that I don't know how to pause an nsIChannel that has been AsyncOpen'ed before it hits the network. But I assume that the SW code does that already, so that seems doable. This is all actually relatively easy to do nowadays. Happy to guide someone through. I think there are only three places in the DOM code that does preflights, so only three places that needs to be changed. Plus the code in the http channel of course.

I think Jonas's suggestion in comment 9 sounds much better. Right now we have a layering problem. CORS only makes sense in the case of http protocol, but we have the logic sitting further up the stack in code that has to deal with all protocols. Lets not make this worse.

Comment 9 sounds nice on paper, but it doesn't actually make things much easier for the purposes of this bug since currently the way that the SW is notified is tied to opening the channel (more specifically tied to opening a cache entry for the channel), so even if the CORS preflight was initiated somewhere inside netwerk/, we would still need to do everything in comment 5. I don't think that going after changing how the interception happens inside necko at this point is very wise, and it is definitely not something that I'm signing up to do right now. :-) In the future, once we have a way to invoke the SW that is not tied so much to opening a channel, we can fix a lot of this insanity.

Ehsan, note that the comment 9 proposes to do the SW work *before* the additional preflight channel is created or opened. But like I said, I don't know the SW code well enough to know that my proposal solves the problem here. If it doesn't please ignore the proposal for the purposes of this discussion.

Yea, I don't understand comment 11. Ehsan, if the CORS checks are moved into the channel then they would by necessity be occurring after the channel is opened, right?

(In reply to Jonas Sicking (:sicking) from comment #12) > Ehsan, note that the comment 9 proposes to do the SW work *before* the > additional preflight channel is created or opened. Yes, I understand that. > But like I said, I don't know the SW code well enough to know that my > proposal solves the problem here. If it doesn't please ignore the proposal > for the purposes of this discussion. The issue here is that the interception happens in the middle of AsyncOpen when necko opens a cache entry for the request. One problem that comment 5 tries to solve is to get the information required to create a Request object for the original request to pass to the network request before the preflight goes out, which your idea fixes. But there is the other issue with how the interception happens in the middle of AsyncOpen, and that is the part I'm not sure we can break into the bits you suggested in comment 9. Basically, there is no clean point between the steps 2 and 3 in that comment for us to do the preflight based on. It's possible to rework things in a way that would let us start the preflight after intercepting the channel and have everything else work well with it, but it's far from obvious to me that it has any chance of working. But in the interest of not arguing over something neither of us know for a fact, what I can do is to try to see if that can be made to work reasonably easily and fall back to comment 5 if that doesn't work (since that solution will at least work.)

(In reply to Ehsan Akhgari (don't ask for review please) from comment #11) > Comment 9 sounds nice on paper, but it doesn't actually make things much > easier for the purposes of this bug since currently the way that the SW is > notified is tied to opening the channel (more specifically tied to opening a > cache entry for the channel), so even if the CORS preflight was initiated > somewhere inside netwerk/, we would still need to do everything in comment > 5. I don't think that going after changing how the interception happens > inside necko at this point is very wise, and it is definitely not something > that I'm signing up to do right now. :-) Is it possible to open the cache entry manually without going through AsyncOpen?

(In reply to Nikhil Marathe [:nsm] (please needinfo?) from comment #15) > (In reply to Ehsan Akhgari (don't ask for review please) from comment #11) > > Comment 9 sounds nice on paper, but it doesn't actually make things much > > easier for the purposes of this bug since currently the way that the SW is > > notified is tied to opening the channel (more specifically tied to opening a > > cache entry for the channel), so even if the CORS preflight was initiated > > somewhere inside netwerk/, we would still need to do everything in comment > > 5. I don't think that going after changing how the interception happens > > inside necko at this point is very wise, and it is definitely not something > > that I'm signing up to do right now. :-) > > Is it possible to open the cache entry manually without going through > AsyncOpen? I don't think we need to do that. I just finished the work on bug 1199049. :-)

I don't remember why I filed a new bug for this, but this is done in bug 1199049.