(no longer active) Assignee
	Description • 9 years ago

I have some patches that are starting to pass tests in order to move CORS preflights to happen inside Necko.  I've moved fetch() to use the new API and it will take me a while to finish moving XHR and beacon to use it as well, and make it work in e10s etc...

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)
	Comment 1 • 9 years ago

Ehsan, I'm working on XHR using asyncopen2, and I'm almost there, but CORS preflight is tripping tests.

At this point, XHR still opens its own NS_StartCORSPreflight() in case of preflight requests. Will your patch obviate the need for this and have DoCORSCheck (in the new asyncOpen2 code path) or something call NS_StartCORSPreflight()? If so, could you put up the patch so I can try it out with XHR?

	(no longer active) Assignee
	Comment 2 • 9 years ago

I discussed with Nikhil in person.  Looks like we agree on how the pieces will fit together.

	(no longer active) Assignee
	Comment 3 • 9 years ago

Before attaching my patches, here is the high-level picture of what I have done here.  Currently, a caller that wants to perform CORS preflights needs to do three things:

* Set up an nsCORSListenerProxy as the listener of the channel.
* Call NS_StartCORSPreflight.
* Remember to not AsyncOpen the channel, and rely on nsCORSPreflightListener to do it for them once the CORS checks pass.

I am modifying things so that callers do the following instead:

* Set up an nsCORSListenerProxy as before.
* Call nsIHttpChannelInternal::SetCorsPreflightParameters() to pass in the information to perform the CORS preflight.
* AsyncOpen(2) the channel as normal.

Once the caller does this, here is what happens inside Necko.  We start opening the channel, and if the load needs to be intercepted, the interception occurs.  After any potential interceptions, if we decide to go to the network, we check to see if we need to do a CORS preflight.  If the answer is no, we go to the network as usual.  If the answer is yes, we create a preflight channel internally, start the preflight, and if the preflight was successful we continue going to the network.  The CORS preflight now happens in the parent process in e10s mode, compared to before where it was initiated in the child process.

There is room for future improvement.  One thing that I would like us to do is to move the CORS preflight information from the channel to the load info object, and require AsyncOpen2 for CORS preflights.  I was originally trying to do that work here but when trying to update XHR to use the new API I realized that is not practical without reworking XHR to open the channel in send() when we have all of the required information (including unsafe CORS headers).  I decided to not do that refactoring work now, but Nikhil is working on that problem in bug 1182571, and once that lands, we can move this API to nsILoadInfo instead (I volunteer to do that!).  I just would like to land this now since it took me a while to get it to work, and it blocks some service worker stuff that is not depending on the said refactoring to happen.

Another longer term goal is of course getting rid of the requirement on the caller to setup an nsCORSListenerProxy, and as I understand that is part of the larger AsyncOpen2 project.

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 4 • 9 years ago

Note that "* Set up an nsCORSListenerProxy as the listener of the channel" is no longer needed by callers that use AsyncOpen2(). Look at the sendBeacon code for example.

I definitely would like to tie this to AsyncOpen2 and LoadInfo. Setting the actual preflight parameters seems fine to do through nsIHttpChannelInternal::SetCorsPreflightParameters. Especially since our XHR code requires that we set the preflight parameters separately from creating the channel (the [ChromeOnly] .channel property requires that we create the channel long before we have all preflight information).

I think the simplest way to tie SetCorsPreflightParameters to AsyncOpen2/LoadInfo would be to make SetCorsPreflightParameters fail (and assert) if the LoadInfo doesn't have the SEC_REQUIRE_CORS_DATA_INHERITS flag set.

That would require the callsite to create the channel with the CORS mode, optionally call SetCorsPreflightParameters, and then call AsyncOpen2. This since we already require AsyncOpen2 to be called when SEC_REQUIRE_CORS_DATA_INHERITS is set.

I'm ok with adding that requirement separately though. It should be trivial to do once we've converted XHR and FetchDriver to use AsyncOpen2.

	(no longer active) Assignee
	Comment 5 • 9 years ago

(In reply to Jonas Sicking (:sicking) from comment #4)
> Note that "* Set up an nsCORSListenerProxy as the listener of the channel"
> is no longer needed by callers that use AsyncOpen2(). Look at the sendBeacon
> code for example.

Yep.

> I definitely would like to tie this to AsyncOpen2 and LoadInfo. Setting the
> actual preflight parameters seems fine to do through
> nsIHttpChannelInternal::SetCorsPreflightParameters. Especially since our XHR
> code requires that we set the preflight parameters separately from creating
> the channel (the [ChromeOnly] .channel property requires that we create the
> channel long before we have all preflight information).
> 
> I think the simplest way to tie SetCorsPreflightParameters to
> AsyncOpen2/LoadInfo would be to make SetCorsPreflightParameters fail (and
> assert) if the LoadInfo doesn't have the SEC_REQUIRE_CORS_DATA_INHERITS flag
> set.

I will try this, and add a new patch to this series if it's easy.

> That would require the callsite to create the channel with the CORS mode,
> optionally call SetCorsPreflightParameters, and then call AsyncOpen2. This
> since we already require AsyncOpen2 to be called when
> SEC_REQUIRE_CORS_DATA_INHERITS is set.
> 
> I'm ok with adding that requirement separately though. It should be trivial
> to do once we've converted XHR and FetchDriver to use AsyncOpen2.

Yes, it's trivial to do on top of my work here.  I think we should pick that up after XHR and Fetch are moved to AsyncOpen2 though, since there are only these three callers for this API.

	(no longer active) Assignee
	Comment 6 • 9 years ago

This is finally going green on try, so I'm going to attach my patches: <https://treeherder.mozilla.org/#/jobs?repo=try&revision=5a76325d26c1>

I'm asking from Jason to review the Necko specific bits, and from Jonas and Christoph to review the CORS changes, but in some of the patches the lines are fuzzy, so feel free to do drive by reviews too.

	(no longer active) Assignee
	Comment 13 • 9 years ago

This is necessary because it's possible for one request triggered from
a child to start a CORS preflight in the parent process and get an entry
into the preflight cache, and then a CORS check for the same URL to fail
in the child process later on.  In this case, we need to tell the parent
process to clear its CORS preflight cache entry.

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

Comment on attachment 8656231 [details] [diff] [review]
Part 3: Add a CORS preflight result notification API

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

Looks good to me.

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 19 • 9 years ago

Comment on attachment 8656231 [details] [diff] [review]
Part 3: Add a CORS preflight result notification API

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

::: netwerk/protocol/http/nsCORSListenerProxy.cpp
@@ +1045,5 @@
>  {
>  public:
>    nsCORSPreflightListener(nsIChannel* aOuterChannel,
>                            nsIStreamListener* aOuterListener,
>                            nsISupports* aOuterContext,

You can remove aOuterContext too, right? You are removing the only user of mOuterContext.

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 20 • 9 years ago

Comment on attachment 8656231 [details] [diff] [review]
Part 3: Add a CORS preflight result notification API

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

::: netwerk/protocol/http/nsCORSListenerProxy.cpp
@@ +1212,2 @@
>      mOuterListener->OnStartRequest(mOuterChannel, mOuterContext);
>      mOuterListener->OnStopRequest(mOuterChannel, mOuterContext, rv);

Should you really be calling these? Isn't notifying OnPreflightFailed enough. In particular, won't the "real" channel also call OnStartRequest/OnStopRequest once OnPreflightFailed is called?

If these can be removed, then you can remove the nsIStreamListener argument from NS_StartCORSPreflight and the nsCORSPreflightListener ctor.

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 21 • 9 years ago

Comment on attachment 8656232 [details] [diff] [review]
Part 4: Perform CORS preflights from nsHttpChannel before connecting to the network

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

Looks good, but someone else should verify that putting these changes in ContinueConnect() is the right place to put them.

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +453,5 @@
> +    if (!mIsCorsPreflightDone && mRequireCORSPreflight &&
> +        mInterceptCache != INTERCEPTED) {
> +        nsCOMPtr<nsIChannel> preflightChannel;
> +        nsresult rv = NS_StartCORSPreflight(this, mListener, mPreflightPrincipal,
> +                                            this, mWithCredentials, mUnsafeHeaders,

80 columns, on both these lines.

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 22 • 9 years ago

Comment on attachment 8656239 [details] [diff] [review]
Part 11: Make it impossible to start CORS preflights from outside of Necko

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

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +455,5 @@
>          nsCOMPtr<nsIChannel> preflightChannel;
> +        nsresult rv =
> +            nsCORSListenerProxy::StartCORSPreflight(this, mListener, mPreflightPrincipal,
> +                                                    this, mWithCredentials, mUnsafeHeaders,
> +                                                    getter_AddRefs(preflightChannel));

80 columns

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 23 • 9 years ago

Comment on attachment 8656237 [details] [diff] [review]
Part 9: Use Necko-level CORS preflights in XHR

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

::: dom/base/nsXMLHttpRequest.cpp
@@ -1210,5 @@
>      mChannel->Cancel(NS_BINDING_ABORTED);
>    }
> -  if (mCORSPreflightChannel) {
> -    mCORSPreflightChannel->Cancel(NS_BINDING_ABORTED);
> -  }

Not sure what other patch to look in, so just wanted to double-check that when the original channel is cancelled, necko will also cancel the preflight channel?

	(no longer active) Assignee
	Comment 24 • 9 years ago

(In reply to Jonas Sicking (:sicking) from comment #19)
> ::: netwerk/protocol/http/nsCORSListenerProxy.cpp
> @@ +1045,5 @@
> >  {
> >  public:
> >    nsCORSPreflightListener(nsIChannel* aOuterChannel,
> >                            nsIStreamListener* aOuterListener,
> >                            nsISupports* aOuterContext,
> 
> You can remove aOuterContext too, right? You are removing the only user of
> mOuterContext.

No, it's still used in nsCORSPreflightListener::OnStartRequest.

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 25 • 9 years ago

Comment on attachment 8656237 [details] [diff] [review]
Part 9: Use Necko-level CORS preflights in XHR

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

Overholt said it's ok if I steal reviews. So I'll steal this one :)

	Josh Matthews [:jdm]
	Comment 26 • 9 years ago

Comment on attachment 8656237 [details] [diff] [review]
Part 9: Use Necko-level CORS preflights in XHR

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

::: dom/base/nsXMLHttpRequest.cpp
@@ +2842,5 @@
>    }
>  
>    nsCOMPtr<nsIStreamListener> listener = this;
> +  if (!IsSystemXHR() ||
> +      (mState & XML_HTTP_REQUEST_NEED_AC_PREFLIGHT)) {

Why this change?

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 27 • 9 years ago

(In reply to Ehsan Akhgari (don't ask for review please) from comment #24)
> No, it's still used in nsCORSPreflightListener::OnStartRequest.

Ah. See comment 20. I think that code needs to be removed too.

	(no longer active) Assignee
	Comment 28 • 9 years ago

(In reply to Jonas Sicking (:sicking) from comment #20)
> ::: netwerk/protocol/http/nsCORSListenerProxy.cpp
> @@ +1212,2 @@
> >      mOuterListener->OnStartRequest(mOuterChannel, mOuterContext);
> >      mOuterListener->OnStopRequest(mOuterChannel, mOuterContext, rv);
> 
> Should you really be calling these? Isn't notifying OnPreflightFailed
> enough. In particular, won't the "real" channel also call
> OnStartRequest/OnStopRequest once OnPreflightFailed is called?

I tried doing this before but it didn't work for some reason (don't remember why...)  But I tried again and it does work now!  I will submit a new part for that.

> If these can be removed, then you can remove the nsIStreamListener argument
> from NS_StartCORSPreflight and the nsCORSPreflightListener ctor.

I'm not 100% sure if this is kosher from a Necko perspective, but if Jason r+'es the patch I'm about to submit, I'll clean this up later as you suggest.

	(no longer active) Assignee
	Comment 30 • 9 years ago

(In reply to Jonas Sicking (:sicking) from comment #23)
> Comment on attachment 8656237 [details] [diff] [review]
> Part 9: Use Necko-level CORS preflights in XHR
> 
> Review of attachment 8656237 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/base/nsXMLHttpRequest.cpp
> @@ -1210,5 @@
> >      mChannel->Cancel(NS_BINDING_ABORTED);
> >    }
> > -  if (mCORSPreflightChannel) {
> > -    mCORSPreflightChannel->Cancel(NS_BINDING_ABORTED);
> > -  }
> 
> Not sure what other patch to look in, so just wanted to double-check that
> when the original channel is cancelled, necko will also cancel the preflight
> channel?

Ah, yes we should do this.  I actually don't think my patch series does this right now...  New patch coming up.

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 31 • 9 years ago

(In reply to Ehsan Akhgari (don't ask for review please) from comment #28)
> (In reply to Jonas Sicking (:sicking) from comment #20)
> > ::: netwerk/protocol/http/nsCORSListenerProxy.cpp
> > @@ +1212,2 @@
> > >      mOuterListener->OnStartRequest(mOuterChannel, mOuterContext);
> > >      mOuterListener->OnStopRequest(mOuterChannel, mOuterContext, rv);
> > 
> > Should you really be calling these? Isn't notifying OnPreflightFailed
> > enough. In particular, won't the "real" channel also call
> > OnStartRequest/OnStopRequest once OnPreflightFailed is called?
> 
> I tried doing this before but it didn't work for some reason (don't remember
> why...)  But I tried again and it does work now!  I will submit a new part
> for that.

Maybe because you hadn't converted XHR/sendBeacon/fetch to rely on the necko-provided in-channel preflight. But you have now.

> > If these can be removed, then you can remove the nsIStreamListener argument
> > from NS_StartCORSPreflight and the nsCORSPreflightListener ctor.
> 
> I'm not 100% sure if this is kosher from a Necko perspective, but if Jason
> r+'es the patch I'm about to submit, I'll clean this up later as you suggest.

The argument wouldn't be used at all, so I don't see why it wouldn't be ok to remove it...

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 32 • 9 years ago

Comment on attachment 8656776 [details] [diff] [review]
Part 12: Move the calls on the listener object to OnPreflightFailed()

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

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +7008,5 @@
>      mIsCorsPreflightDone = 1;
>  
>      Cancel(aError);
> +    mListener->OnStartRequest(this, mListenerContext);
> +    mListener->OnStopRequest(this, mListenerContext, aError);

No, you shouldn't do this. The call to Cancel() will take care of this.

	Josh Matthews [:jdm]
	Comment 33 • 9 years ago

The call to Cancel will only take care of it if there is a pump (either from the cache or the http transaction) that is active. In the case where a preflight is in progress, the original channel has no such pump and there will be no notifications forthcoming.

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 34 • 9 years ago

Comment on attachment 8656776 [details] [diff] [review]
Part 12: Move the calls on the listener object to OnPreflightFailed()

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

Ah. I should stay out of this then since I don't know the code well enough :)

Restoring original review request.

	(no longer active) Assignee
	Comment 36 • 9 years ago

(In reply to Josh Matthews [:jdm] from comment #26)
> Comment on attachment 8656237 [details] [diff] [review]
> Part 9: Use Necko-level CORS preflights in XHR
> 
> Review of attachment 8656237 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/base/nsXMLHttpRequest.cpp
> @@ +2842,5 @@
> >    }
> >  
> >    nsCOMPtr<nsIStreamListener> listener = this;
> > +  if (!IsSystemXHR() ||
> > +      (mState & XML_HTTP_REQUEST_NEED_AC_PREFLIGHT)) {
> 
> Why this change?

To ensure that we will always have an nsCORSListenerProxy listener when we'd do a preflight.  Previously, NS_StartCORSPreflight did this for us.

	(no longer active) Assignee
	Comment 37 • 9 years ago

Comment on attachment 8656231 [details] [diff] [review]
Part 3: Add a CORS preflight result notification API

Jonas, does part 12 make you reconsider your r- here?  :-)

	(no longer active) Assignee
	Comment 38 • 9 years ago

(In reply to Jonas Sicking (:sicking) from comment #31)
> (In reply to Ehsan Akhgari (don't ask for review please) from comment #28)
> > (In reply to Jonas Sicking (:sicking) from comment #20)
> > > ::: netwerk/protocol/http/nsCORSListenerProxy.cpp
> > > @@ +1212,2 @@
> > > >      mOuterListener->OnStartRequest(mOuterChannel, mOuterContext);
> > > >      mOuterListener->OnStopRequest(mOuterChannel, mOuterContext, rv);
> > > 
> > > Should you really be calling these? Isn't notifying OnPreflightFailed
> > > enough. In particular, won't the "real" channel also call
> > > OnStartRequest/OnStopRequest once OnPreflightFailed is called?
> > 
> > I tried doing this before but it didn't work for some reason (don't remember
> > why...)  But I tried again and it does work now!  I will submit a new part
> > for that.
> 
> Maybe because you hadn't converted XHR/sendBeacon/fetch to rely on the
> necko-provided in-channel preflight. But you have now.

Hmm, the ordering of the patches doesn't reflect the order in which I wrote them, I tried to reorder stuff to make them follow a logical sequence, so I don't think that was the reason, but it doesn't really matter.

> > > If these can be removed, then you can remove the nsIStreamListener argument
> > > from NS_StartCORSPreflight and the nsCORSPreflightListener ctor.
> > 
> > I'm not 100% sure if this is kosher from a Necko perspective, but if Jason
> > r+'es the patch I'm about to submit, I'll clean this up later as you suggest.
> 
> The argument wouldn't be used at all, so I don't see why it wouldn't be ok
> to remove it...

I meant if part 12 gets an r+.  :-)

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 39 • 9 years ago

Comment on attachment 8656849 [details] [diff] [review]
Part 13: Cancel the preflight channel if the original channel gets canceled when a CORS preflight is in progress

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

Seems good to me, but Jason should review.

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 40 • 9 years ago

Comment on attachment 8656231 [details] [diff] [review]
Part 3: Add a CORS preflight result notification API

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

Sure, as long as they land together.

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 41 • 9 years ago

Comment on attachment 8656776 [details] [diff] [review]
Part 12: Move the calls on the listener object to OnPreflightFailed()

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

This still looks suspect to me though given that no other place in nsHttpChannel directly calls mListener->OnStartRequest. You probably want to at least call CallOnStartRequest(). And looking at nsHttpChannel::OnStopRequest it seems like we want to do at least some of the stuff that it does, like ReleaseListeners() and mIsPending = false.

How do we handle this stuff when a SW directly cancels a request? Or when it creates an inline response?

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 42 • 9 years ago

> > > +  if (!IsSystemXHR() ||
> > > +      (mState & XML_HTTP_REQUEST_NEED_AC_PREFLIGHT)) {
> > 
> > Why this change?
> 
> To ensure that we will always have an nsCORSListenerProxy listener when we'd
> do a preflight.  Previously, NS_StartCORSPreflight did this for us.

We don't need a nsCORSListenerProxy if IsSystemXHR is true. When that's true we don't run any CORS logic at all. NS_StartCORSPreflight never installed a nsCORSListenerProxy on the main channel. It only installed one on the preflight channel.

In any case, XML_HTTP_REQUEST_NEED_AC_PREFLIGHT is never set when IsSystemXHR() returns true.

	(no longer active) Assignee
	Comment 43 • 9 years ago

(In reply to Jonas Sicking (:sicking) from comment #41)
> This still looks suspect to me though given that no other place in
> nsHttpChannel directly calls mListener->OnStartRequest. You probably want to
> at least call CallOnStartRequest(). And looking at
> nsHttpChannel::OnStopRequest it seems like we want to do at least some of
> the stuff that it does, like ReleaseListeners() and mIsPending = false.

Yeah, I'm not sure whether this is what we want either.  This really needs input from Jason.  But this is more or less what the current code does, so at least it doesn't change the behavior that much.

> How do we handle this stuff when a SW directly cancels a request?

We do an internal redirect to redirect the channel to the same URL, this time making it go to the network without interception.

> Or when it creates an inline response?

We synthesize the headers and status code manually and let the channel operate as if it's being responded to from a cache entry containing the synthesized body of the Response object.  None of these cases are similar to this one.

(In reply to Jonas Sicking (:sicking) from comment #42)
> > > > +  if (!IsSystemXHR() ||
> > > > +      (mState & XML_HTTP_REQUEST_NEED_AC_PREFLIGHT)) {
> > > 
> > > Why this change?
> > 
> > To ensure that we will always have an nsCORSListenerProxy listener when we'd
> > do a preflight.  Previously, NS_StartCORSPreflight did this for us.
> 
> We don't need a nsCORSListenerProxy if IsSystemXHR is true. When that's true
> we don't run any CORS logic at all. NS_StartCORSPreflight never installed a
> nsCORSListenerProxy on the main channel. It only installed one on the
> preflight channel.
> 
> In any case, XML_HTTP_REQUEST_NEED_AC_PREFLIGHT is never set when
> IsSystemXHR() returns true.

Yeah, OK it all makes sense now.  I'll take out this hunk.

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 44 • 9 years ago

(In reply to Ehsan Akhgari (don't ask for review please) from comment #43)
> > How do we handle this stuff when a SW directly cancels a request?
> 
> We do an internal redirect to redirect the channel to the same URL, this
> time making it go to the network without interception.

So a SW can't make a network request fail? It can only say "go to network" or "here's a response"? The SW can't produce a network error?

	(no longer active) Assignee
	Comment 45 • 9 years ago

(In reply to Jonas Sicking (:sicking) from comment #44)
> (In reply to Ehsan Akhgari (don't ask for review please) from comment #43)
> > > How do we handle this stuff when a SW directly cancels a request?
> > 
> > We do an internal redirect to redirect the channel to the same URL, this
> > time making it go to the network without interception.
> 
> So a SW can't make a network request fail? It can only say "go to network"
> or "here's a response"? The SW can't produce a network error?

It can, by calling event.respondWith(Promise.reject(...));.  In that case we just call Cancel() on the channel, but in that case we have a pump from the cache entry, I think, so that's different too.

	Ben Kelly [:bkelly, not reviewing]
	Comment 46 • 9 years ago

(In reply to Ehsan Akhgari (don't ask for review please) from comment #45)
> (In reply to Jonas Sicking (:sicking) from comment #44)
> > (In reply to Ehsan Akhgari (don't ask for review please) from comment #43)
> > > > How do we handle this stuff when a SW directly cancels a request?
> > > 
> > > We do an internal redirect to redirect the channel to the same URL, this
> > > time making it go to the network without interception.
> > 
> > So a SW can't make a network request fail? It can only say "go to network"
> > or "here's a response"? The SW can't produce a network error?
> 
> It can, by calling event.respondWith(Promise.reject(...));.  In that case we
> just call Cancel() on the channel, but in that case we have a pump from the
> cache entry, I think, so that's different too.

Just to be clear, that's not the only way.  SWs can pass Response.error() or an illegal Response to respondWith().

I believe spec is also designed to allow event.preventDefault() to cancel the network request as well.  I'm not sure we implement it, though.

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 47 • 9 years ago

I understand that it's a different codepath now, but why can't we make CORS-reject follow the same codepath as SW-reject?

	Josh Matthews [:jdm]
	Comment 48 • 9 years ago

(In reply to Ehsan Akhgari (don't ask for review please) from comment #45)
> (In reply to Jonas Sicking (:sicking) from comment #44)
> > (In reply to Ehsan Akhgari (don't ask for review please) from comment #43)
> > > > How do we handle this stuff when a SW directly cancels a request?
> > > 
> > > We do an internal redirect to redirect the channel to the same URL, this
> > > time making it go to the network without interception.
> > 
> > So a SW can't make a network request fail? It can only say "go to network"
> > or "here's a response"? The SW can't produce a network error?
> 
> It can, by calling event.respondWith(Promise.reject(...));.  In that case we
> just call Cancel() on the channel, but in that case we have a pump from the
> cache entry, I think, so that's different too.

Nope: http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/InterceptedChannel.cpp?force=1#241

	(no longer active) Assignee
	Comment 49 • 9 years ago

(In reply to Josh Matthews [:jdm] from comment #48)
> (In reply to Ehsan Akhgari (don't ask for review please) from comment #45)
> > (In reply to Jonas Sicking (:sicking) from comment #44)
> > > (In reply to Ehsan Akhgari (don't ask for review please) from comment #43)
> > > > > How do we handle this stuff when a SW directly cancels a request?
> > > > 
> > > > We do an internal redirect to redirect the channel to the same URL, this
> > > > time making it go to the network without interception.
> > > 
> > > So a SW can't make a network request fail? It can only say "go to network"
> > > or "here's a response"? The SW can't produce a network error?
> > 
> > It can, by calling event.respondWith(Promise.reject(...));.  In that case we
> > just call Cancel() on the channel, but in that case we have a pump from the
> > cache entry, I think, so that's different too.
> 
> Nope:
> http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/
> InterceptedChannel.cpp?force=1#241

Oh, I thought the Cancel() call is an a channel, not an intercepted channel object, sorry.

(In reply to Jonas Sicking (:sicking) from comment #47)
> I understand that it's a different codepath now, but why can't we make
> CORS-reject follow the same codepath as SW-reject?

So we CloseCacheEntry() in that case followed by AsyncAbort().  I just tested and that works as well, so I'll attach another patch to do that.  But again, I don't actually know which one of the three ways that we came up with for canceling a channel is the right one.  I'd like to defer to Jason to pick one, or give us a better way.

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)
	Comment 51 • 9 years ago

(In reply to Ben Kelly [:bkelly] from comment #46)
> (In reply to Ehsan Akhgari (don't ask for review please) from comment #45)
> > (In reply to Jonas Sicking (:sicking) from comment #44)
> > > (In reply to Ehsan Akhgari (don't ask for review please) from comment #43)
> > > > > How do we handle this stuff when a SW directly cancels a request?
> > > > 
> > > > We do an internal redirect to redirect the channel to the same URL, this
> > > > time making it go to the network without interception.
> > > 
> > > So a SW can't make a network request fail? It can only say "go to network"
> > > or "here's a response"? The SW can't produce a network error?
> > 
> > It can, by calling event.respondWith(Promise.reject(...));.  In that case we
> > just call Cancel() on the channel, but in that case we have a pump from the
> > cache entry, I think, so that's different too.
> 
> Just to be clear, that's not the only way.  SWs can pass Response.error() or
> an illegal Response to respondWith().
> 
> I believe spec is also designed to allow event.preventDefault() to cancel
> the network request as well.  I'm not sure we implement it, though.

Bug 1198230 will implement this.

	Christoph Kerschbaumer [:ckerschb]
	Comment 52 • 9 years ago

Comment on attachment 8656239 [details] [diff] [review]
Part 11: Make it impossible to start CORS preflights from outside of Necko

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

Looks good to me, but I am not a netwerk peer :-(

	Christoph Kerschbaumer [:ckerschb]
	Comment 53 • 9 years ago

Comment on attachment 8656232 [details] [diff] [review]
Part 4: Perform CORS preflights from nsHttpChannel before connecting to the network

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

Looks good to me, but as mentioned earlier, I am not a netwerk peer :-(

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +460,5 @@
> +    }
> +
> +    if (mRequireCORSPreflight && mInterceptCache != INTERCEPTED) {
> +        MOZ_RELEASE_ASSERT(mIsCorsPreflightDone,
> +                           "CORS preflight must have been finished by the time we do the rest of ContinueConnect");

Nit: you could merge the if-condition into the MOZ_RELEASE_ASSERT, it feels weired to have to run through an if-condition just to assert within the condition.

@@ +959,5 @@
>  nsHttpChannel::CallOnStartRequest()
>  {
> +    if (mRequireCORSPreflight && mInterceptCache != INTERCEPTED) {
> +        MOZ_RELEASE_ASSERT(mIsCorsPreflightDone,
> +                           "CORS preflight must have been finished by the time we call OnStartRequest");

same nit applies here.

	Jason Duell
	Comment 54 • 9 years ago

Comment on attachment 8656229 [details] [diff] [review]
Part 1: Move nsCORSListenerProxy.* to necko

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

From the name of this patch I expected it to include moving the actual nsCORSListenerProxy.* files, but I don't see that here, nor in the other patches.  Did you forget that part?  +r if I'm crazy, and/or if you change the patch to fix this (i.e. no re-review needed).

	Jason Duell
	Comment 55 • 9 years ago

Comment on attachment 8656230 [details] [diff] [review]
Part 2: Add a channel API for requesting CORS preflights

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

::: netwerk/protocol/http/HttpBaseChannel.cpp
@@ +2820,5 @@
> +NS_IMETHODIMP
> +HttpBaseChannel::SetCorsPreflightParameters(const nsTArray<nsCString>& aUnsafeHeaders,
> +                                            bool aWithCredentials,
> +                                            nsIPrincipal* aPrincipal)
> +{

I assume this needs to get called before AsyncOpen?  Might want to add ENSURE_CALLED_BEFORE_CONNECT() here.

	Jason Duell
	Comment 56 • 9 years ago

Comment on attachment 8656232 [details] [diff] [review]
Part 4: Perform CORS preflights from nsHttpChannel before connecting to the network

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

This looks fine, but I'd like to run it by :mcmanus to double-check whether BeginConnect() is the right place here. Also my question about AsyncAbort below (which is now mostly moot given that we switch to use it in patch 14).

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +7005,5 @@
> +{
> +    MOZ_ASSERT(mRequireCORSPreflight, "Why did a preflight happen?");
> +    mIsCorsPreflightDone = 1;
> +
> +    Cancel(aError);

I think we need AsyncAbort() here instead of Cancel, for the same reasons mentioned in comment 33: we don't have a pump yet, so nothing will call OnStart/OnStop unless we do it via AsyncAbort.  (You could do the calls manually if you want to avoid posting a new event, but that's fairly cheap and AsyncAbort also does some other useful stuff like remove the request from the LoadGroup, which you're also missing here.  AsyncAbort also handles delaying OnStart/Stop if the channel is suspended, which is important)

	Jason Duell
	Comment 57 • 9 years ago

Comment on attachment 8656235 [details] [diff] [review]
Part 7: Remove entries from the CORS preflight cache in the parent process when a CORS check in the child process fails

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

::: netwerk/protocol/http/HttpChannelChild.cpp
@@ +2186,5 @@
> +  }
> +  bool result = false;
> +  // Be careful to not attempt to send a message to the parent after the
> +  // actor has been destroyed.
> +  if (state() != PHttpChannel::__Dead) {

In HttpChannelChild we generally use "if (mIPCOpen)" to check the liveness of the IPDL channel.  Use that for consistency unless there's a reason for preferring the _Dead check syntax?

(Interesting: in HttpChannelParent we set mIPCClosed based on when ActorDestroy() is called, while HttpChannelChild is setting mIPCOpen = false when NeckoChild::DeallocPHttpChannelChild() is called.  Not sure of the reason for the different codepaths there.)

@@ +2189,5 @@
> +  // actor has been destroyed.
> +  if (state() != PHttpChannel::__Dead) {
> +    result = SendRemoveCorsPreflightCacheEntry(uri, principalInfo);
> +  }
> +  return result ? NS_OK : NS_ERROR_FAILURE;

IIRC getting false back from SendFOO in IPDL results in the child being killed, so I'm not sure you need this here.  But doesn't hurt.

::: netwerk/protocol/http/HttpChannelParent.cpp
@@ +806,5 @@
>  }
>  
> +bool
> +HttpChannelParent::RecvRemoveCorsPreflightCacheEntry(const URIParams& uri,
> +                                                     const mozilla::ipc::PrincipalInfo& requestingPrincipal)

Looks >80 chars wide?

::: netwerk/protocol/http/PHttpChannel.ipdl
@@ +80,5 @@
>    // Child has no more events/messages to divert to the parent.
>    DivertComplete();
>  
> +  // Child has detected a CORS check failure, so needs to tell the parent
> +  // to remove an entry from the CORS preflight cache.

s/an/any matching/

	Jason Duell
	Comment 58 • 9 years ago

Comment on attachment 8656776 [details] [diff] [review]
Part 12: Move the calls on the listener object to OnPreflightFailed()

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

Cancel by itself isn't enough here. Again, moot given patch 14 (so +r).

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +7008,5 @@
>      mIsCorsPreflightDone = 1;
>  
>      Cancel(aError);
> +    mListener->OnStartRequest(this, mListenerContext);
> +    mListener->OnStopRequest(this, mListenerContext, aError);

I think you want AsyncAbort() here.  Cancel isn't sufficient for the reasons jdm mentioned in comment 33 (I'm not sure you need the Cancel() call at all: AsyncAbort sets mStatus, so the only thing Cancel achieves is to also set mCanceled, which I'm not sure we need).  AsyncAbort handles delaying the OnStart/Stop calls if the channel is suspended.  (It also removes the channel from the loadGroup, which we want here too).

	Jason Duell
	Comment 59 • 9 years ago

Comment on attachment 8656849 [details] [diff] [review]
Part 13: Cancel the preflight channel if the original channel gets canceled when a CORS preflight is in progress

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

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +456,1 @@
>          nsCOMPtr<nsIChannel> preflightChannel;

Seems you don't need the local preflightChannel variable now that we have mPreflightChannel?

@@ +4906,5 @@
>          mCachePump->Cancel(status);
>      if (mAuthProvider)
>          mAuthProvider->Cancel(status);
> +    if (mPreflightChannel)
> +        mPreflightChannel->Cancel(status);

Hmm.. So my read of nsCORSListenerProxy is that once we Cancel the preflight channel, it'll get OnStart/Stop and proceed to call OnPreflightFailed, right?  It would be Very Bad if that resulted in OnStart/OnStop being called twice on the mListener (once by the pump, and once by the AsyncAbort call in OnPreflightFailed).  But I guess that can't happen since we're not opening up any pumps until the preflight completes, right?

It might be nice to start adding in some assertions to Cancel (like all pumps must be null if mPreflightChannel isn't) to guarantee this.

	Jason Duell
	Comment 60 • 9 years ago

Comment on attachment 8657226 [details] [diff] [review]
Part 14: Cancel the original channel in case a CORS preflight fails using AsyncAbort()

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

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +7019,2 @@
>      CloseCacheEntry(true);
> +    AsyncAbort(aError);

Patrick: do we need Cancel() here too?  I assume not.  (The only thing it would achieve AFACIT is to set mCanceled, and I'm not sure we need/want that).

	(no longer active) Assignee
	Comment 61 • 9 years ago

(In reply to Jason Duell [:jduell] (needinfo? me) from comment #54)
> From the name of this patch I expected it to include moving the actual
> nsCORSListenerProxy.* files, but I don't see that here, nor in the other
> patches.  Did you forget that part?  +r if I'm crazy, and/or if you change
> the patch to fix this (i.e. no re-review needed).

It does include moving them, but Splinter doesn't show file moves in its UI at all.  If you look at the raw diff, it's there.  :-)

	(no longer active) Assignee
	Comment 62 • 9 years ago

(In reply to Jason Duell [:jduell] (needinfo? me) from comment #59)
> @@ +4906,5 @@
> >          mCachePump->Cancel(status);
> >      if (mAuthProvider)
> >          mAuthProvider->Cancel(status);
> > +    if (mPreflightChannel)
> > +        mPreflightChannel->Cancel(status);
> 
> Hmm.. So my read of nsCORSListenerProxy is that once we Cancel the preflight
> channel, it'll get OnStart/Stop and proceed to call OnPreflightFailed,
> right?  It would be Very Bad if that resulted in OnStart/OnStop being called
> twice on the mListener (once by the pump, and once by the AsyncAbort call in
> OnPreflightFailed).  But I guess that can't happen since we're not opening
> up any pumps until the preflight completes, right?

Yes, that is the idea.

> It might be nice to start adding in some assertions to Cancel (like all
> pumps must be null if mPreflightChannel isn't) to guarantee this.

Will do.

	Patrick McManus [:mcmanus]
	Comment 64 • 9 years ago

Comment on attachment 8657226 [details] [diff] [review]
Part 14: Cancel the original channel in case a CORS preflight fails using AsyncAbort()

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

this should be ok

	Patrick McManus [:mcmanus]
	Comment 65 • 9 years ago

Comment on attachment 8656232 [details] [diff] [review]
Part 4: Perform CORS preflights from nsHttpChannel before connecting to the network

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

I think ContinueConnect() should be fine - it does mean the CORS check will happen pretty late (after addon on-modify-request for example, which can change some things about the request, but it looks like you store the principal and headers used for cors are set separately and not inferred from the state of the request, so this shouldn't change behavior)

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +460,5 @@
> +    }
> +
> +    if (mRequireCORSPreflight && mInterceptCache != INTERCEPTED) {
> +        MOZ_RELEASE_ASSERT(mIsCorsPreflightDone,
> +                           "CORS preflight must have been finished by the time we do the rest of ContinueConnect");

+1

@@ +7005,5 @@
> +{
> +    MOZ_ASSERT(mRequireCORSPreflight, "Why did a preflight happen?");
> +    mIsCorsPreflightDone = 1;
> +
> +    Cancel(aError);

jason is right here - but its overtaken by the events of part 14

	(no longer active) Assignee
	Comment 66 • 9 years ago

(In reply to Patrick McManus [:mcmanus] from comment #65)
> I think ContinueConnect() should be fine - it does mean the CORS check will
> happen pretty late (after addon on-modify-request for example, which can
> change some things about the request, but it looks like you store the
> principal and headers used for cors are set separately and not inferred from
> the state of the request, so this shouldn't change behavior)

Yes, those are determined by the caller.

	Pulsebot
	Comment 67 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/fd2477247c15
https://hg.mozilla.org/integration/mozilla-inbound/rev/815732d94c86
https://hg.mozilla.org/integration/mozilla-inbound/rev/712ee72787ad
https://hg.mozilla.org/integration/mozilla-inbound/rev/c0503e3636ec
https://hg.mozilla.org/integration/mozilla-inbound/rev/6710f6e565e3
https://hg.mozilla.org/integration/mozilla-inbound/rev/422cab1af909
https://hg.mozilla.org/integration/mozilla-inbound/rev/5015d235ae95
https://hg.mozilla.org/integration/mozilla-inbound/rev/c7445d6f1d2d
https://hg.mozilla.org/integration/mozilla-inbound/rev/27203d34c684
https://hg.mozilla.org/integration/mozilla-inbound/rev/f3e4bb1f935c
https://hg.mozilla.org/integration/mozilla-inbound/rev/7117493f38b2
https://hg.mozilla.org/integration/mozilla-inbound/rev/b0ee24b21b2a
https://hg.mozilla.org/integration/mozilla-inbound/rev/03449091a3a3
https://hg.mozilla.org/integration/mozilla-inbound/rev/29f4582c95e0

	Pulsebot
	Comment 68 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/0a017caf03c4

	Phil Ringnalda (:philor)
	Comment 69 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/fd2477247c15
https://hg.mozilla.org/mozilla-central/rev/815732d94c86
https://hg.mozilla.org/mozilla-central/rev/712ee72787ad
https://hg.mozilla.org/mozilla-central/rev/c0503e3636ec
https://hg.mozilla.org/mozilla-central/rev/6710f6e565e3
https://hg.mozilla.org/mozilla-central/rev/422cab1af909
https://hg.mozilla.org/mozilla-central/rev/5015d235ae95
https://hg.mozilla.org/mozilla-central/rev/c7445d6f1d2d
https://hg.mozilla.org/mozilla-central/rev/27203d34c684
https://hg.mozilla.org/mozilla-central/rev/f3e4bb1f935c
https://hg.mozilla.org/mozilla-central/rev/7117493f38b2
https://hg.mozilla.org/mozilla-central/rev/b0ee24b21b2a
https://hg.mozilla.org/mozilla-central/rev/03449091a3a3
https://hg.mozilla.org/mozilla-central/rev/29f4582c95e0
https://hg.mozilla.org/mozilla-central/rev/0a017caf03c4