Move CORS preflights to happen inside Necko

RESOLVED FIXED in Firefox 43

Status

()

Core
Networking: HTTP
RESOLVED FIXED
2 years ago
5 months ago

People

(Reporter: Away for a while, Assigned: Away for a while)

Tracking

unspecified
mozilla43
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox43 fixed)

Details

Attachments

(14 attachments)

5.12 KB, patch
jduell
: review+
Details | Diff | Splinter Review
5.90 KB, patch
jduell
: review+
Details | Diff | Splinter Review
8.78 KB, patch
sicking
: review+
jduell
: review+
Details | Diff | Splinter Review
8.02 KB, patch
jduell
: review+
mcmanus
: review+
sicking
: feedback+
ckerschb
: feedback+
Details | Diff | Splinter Review
2.27 KB, patch
jduell
: review+
Details | Diff | Splinter Review
8.81 KB, patch
jduell
: review+
Details | Diff | Splinter Review
14.32 KB, patch
jduell
: review+
Details | Diff | Splinter Review
1.85 KB, patch
jdm
: review+
Details | Diff | Splinter Review
7.95 KB, patch
sicking
: review+
Details | Diff | Splinter Review
2.43 KB, patch
sicking
: review+
Details | Diff | Splinter Review
6.64 KB, patch
jduell
: review+
sicking
: review+
ckerschb
: feedback+
Details | Diff | Splinter Review
1.67 KB, patch
jduell
: review+
Details | Diff | Splinter Review
3.82 KB, patch
jduell
: review+
sicking
: feedback+
Details | Diff | Splinter Review
926 bytes, patch
jduell
: review+
mcmanus
: review+
Details | Diff | Splinter Review
(Assignee)

Description

2 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...
(Assignee)

Updated

2 years ago
Depends on: 815299
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?
Flags: needinfo?(ehsan)
(Assignee)

Updated

2 years ago
Depends on: 1199693
(Assignee)

Comment 2

2 years ago
I discussed with Nikhil in person.  Looks like we agree on how the pieces will fit together.
Flags: needinfo?(ehsan)
(Assignee)

Comment 3

2 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.
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.
(Assignee)

Comment 5

2 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.
(Assignee)

Comment 6

2 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.
(Assignee)

Comment 7

2 years ago
Created attachment 8656229 [details] [diff] [review]
Part 1: Move nsCORSListenerProxy.* to necko
Attachment #8656229 - Flags: review?(jduell.mcbugs)
(Assignee)

Comment 8

2 years ago
Created attachment 8656230 [details] [diff] [review]
Part 2: Add a channel API for requesting CORS preflights
Attachment #8656230 - Flags: review?(jduell.mcbugs)
(Assignee)

Comment 9

2 years ago
Created attachment 8656231 [details] [diff] [review]
Part 3: Add a CORS preflight result notification API
Attachment #8656231 - Flags: review?(mozilla)
Attachment #8656231 - Flags: review?(jonas)
Attachment #8656231 - Flags: review?(jduell.mcbugs)
(Assignee)

Comment 10

2 years ago
Created attachment 8656232 [details] [diff] [review]
Part 4: Perform CORS preflights from nsHttpChannel before connecting to the network
Attachment #8656232 - Flags: review?(mozilla)
Attachment #8656232 - Flags: review?(jonas)
Attachment #8656232 - Flags: review?(jduell.mcbugs)
(Assignee)

Comment 11

2 years ago
Created attachment 8656233 [details] [diff] [review]
Part 5: Preserve the CORS preflight information when setting up a replacement channel
Attachment #8656233 - Flags: review?(jduell.mcbugs)
(Assignee)

Comment 12

2 years ago
Created attachment 8656234 [details] [diff] [review]
Part 6: Transfer the preflight parameters to the parent process in e10s mode
Attachment #8656234 - Flags: review?(jduell.mcbugs)
(Assignee)

Comment 13

2 years ago
Created 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

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.
Attachment #8656235 - Flags: review?(jduell.mcbugs)
(Assignee)

Comment 14

2 years ago
Created attachment 8656236 [details] [diff] [review]
Part 8: Use Necko-level CORS preflights in fetch
Attachment #8656236 - Flags: review?(josh)
(Assignee)

Comment 15

2 years ago
Created attachment 8656237 [details] [diff] [review]
Part 9: Use Necko-level CORS preflights in XHR
Attachment #8656237 - Flags: review?(josh)
(Assignee)

Comment 16

2 years ago
Created attachment 8656238 [details] [diff] [review]
Part 10: Use Necko-level CORS preflights in sendBeacon
Attachment #8656238 - Flags: review?(josh)
(Assignee)

Comment 17

2 years ago
Created attachment 8656239 [details] [diff] [review]
Part 11: Make it impossible to start CORS preflights from outside of Necko
Attachment #8656239 - Flags: review?(mozilla)
Attachment #8656239 - Flags: review?(jonas)
Attachment #8656239 - Flags: review?(jduell.mcbugs)
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.
Attachment #8656231 - Flags: review?(jonas) → review+
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.
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.
Attachment #8656231 - Flags: review+ → review-
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.
Attachment #8656232 - Flags: review?(jonas) → feedback+
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
Attachment #8656239 - Flags: review?(jonas) → review+
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?
Attachment #8656237 - Flags: review+
(Assignee)

Comment 24

2 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.

Updated

2 years ago
Attachment #8656236 - Flags: review?(josh) → review+
Attachment #8656238 - Flags: review?(josh) → review+
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 :)
Attachment #8656237 - Flags: review?(josh)
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?
(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.
(Assignee)

Comment 28

2 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.
(Assignee)

Comment 29

2 years ago
Created attachment 8656776 [details] [diff] [review]
Part 12: Move the calls on the listener object to OnPreflightFailed()
Attachment #8656776 - Flags: review?(jduell.mcbugs)
(Assignee)

Comment 30

2 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.
(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...
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.
Attachment #8656776 - Flags: review?(jduell.mcbugs) → review-
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.
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.
Attachment #8656776 - Flags: review- → review?(jduell.mcbugs)
(Assignee)

Comment 35

2 years ago
Created attachment 8656849 [details] [diff] [review]
Part 13: Cancel the preflight channel if the original channel gets canceled when a CORS preflight is in progress
Attachment #8656849 - Flags: review?(jonas)
Attachment #8656849 - Flags: review?(jduell.mcbugs)
(Assignee)

Comment 36

2 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.
(Assignee)

Updated

2 years ago
Attachment #8656849 - Attachment description: Part 12: Cancel the preflight channel if the original channel gets canceled when a CORS preflight is in progress → Part 13: Cancel the preflight channel if the original channel gets canceled when a CORS preflight is in progress
(Assignee)

Comment 37

2 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?  :-)
Attachment #8656231 - Flags: review- → review?(jonas)
(Assignee)

Comment 38

2 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+.  :-)
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.
Attachment #8656849 - Flags: review?(jonas) → feedback+
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.
Attachment #8656231 - Flags: review?(mozilla)
Attachment #8656231 - Flags: review?(jonas)
Attachment #8656231 - Flags: review?(jduell.mcbugs)
Attachment #8656231 - Flags: review+
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?
> > > +  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.
(Assignee)

Comment 43

2 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.
(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?
(Assignee)

Comment 45

2 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.

Comment 46

2 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.
I understand that it's a different codepath now, but why can't we make CORS-reject follow the same codepath as SW-reject?
(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
(Assignee)

Comment 49

2 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.
(Assignee)

Comment 50

2 years ago
Created attachment 8657226 [details] [diff] [review]
Part 14: Cancel the original channel in case a CORS preflight fails using AsyncAbort()
Attachment #8657226 - Flags: review?(jduell.mcbugs)
(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.
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 :-(
Attachment #8656239 - Flags: review?(mozilla) → feedback+
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.
Attachment #8656232 - Flags: review?(mozilla) → feedback+
Blocks: 1182571
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).
Attachment #8656229 - Flags: review?(jduell.mcbugs) → review-
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.
Attachment #8656230 - Flags: review?(jduell.mcbugs) → review+
Attachment #8656231 - Flags: review+
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)
Attachment #8656232 - Flags: review?(mcmanus)
Attachment #8656232 - Flags: review?(jduell.mcbugs)
Attachment #8656232 - Flags: review+
Attachment #8656233 - Flags: review?(jduell.mcbugs) → review+
Attachment #8656234 - Flags: review?(jduell.mcbugs) → review+
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/
Attachment #8656235 - Flags: review?(jduell.mcbugs) → review+
Attachment #8656239 - Flags: review?(jduell.mcbugs) → review+
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).
Attachment #8656776 - Flags: review?(jduell.mcbugs) → review+
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.
Attachment #8656849 - Flags: review?(jduell.mcbugs) → review+
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).
Attachment #8657226 - Flags: review?(mcmanus)
Attachment #8657226 - Flags: review?(jduell.mcbugs)
Attachment #8657226 - Flags: review+
(Assignee)

Comment 61

2 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.  :-)
(Assignee)

Comment 62

2 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.
Attachment #8656229 - Flags: review- → review+
(Assignee)

Updated

2 years ago
Blocks: 1059784, 1194848
(Assignee)

Updated

2 years ago
No longer blocks: 1194384
Duplicate of this bug: 1194384
Status: NEW → ASSIGNED
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
Attachment #8657226 - Flags: review?(mcmanus) → review+
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
Attachment #8656232 - Flags: review?(mcmanus) → review+
(Assignee)

Comment 66

2 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.

Comment 67

2 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

Comment 68

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/0a017caf03c4
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
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox43: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla43

Updated

2 years ago
Blocks: 1206084

Updated

2 years ago
No longer blocks: 1206084
Depends on: 1206084
Depends on: 1207556
(Assignee)

Updated

2 years ago
No longer depends on: 1207556

Updated

5 months ago
See Also: → bug 1367810
You need to log in before you can comment on or make changes to this bug.