1184607 - Implement new redirect logic and restrict opaqueredirect Response objects correctly

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Ben Kelly [:bkelly, not reviewing]]

The fetch spec changed redirect handling recently:

  https://github.com/whatwg/fetch/issues/66

It seems we may want this sooner rather than later since redirects tend to show up in security issues.

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

I'll do this as I need it in order to land bug 1184967.

Comment 2

[Noemí Freire (:noemi)]

Per comment 1 blocking bug 1184967. Thanks!

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Implement fetch opaqueredirect and associated logic. r=ehsan

Work-in-progress.  Still needs:

* Implement checks for manual redirect in HTTP fetch the return opaqueredirect response
* Implement Cache database schema migration
* write tests

Note, this fixes some other problems I saw with opaque responses.  We were not properly passing on or persisting the status code for the opaque response, etc.

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Implement fetch opaqueredirect and associated logic. r=ehsan

I believe this is fully working now, but I need to write a number of tests to verify things.

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Implement fetch opaqueredirect and associated logic. r=ehsan

We can't use nsCORSListenerProxy for same-origin requests.  Per spec we need to check the manual redirect flag even for cross-origin redirects in this case and the proxy cancels the channel in those cases.

Still to do:

1) Cache schema migration.
2) Tests

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Implement fetch opaqueredirect and associated logic. r=ehsan

Updated to actually pass the redirect flag through the nsIChannel.  Also, the nsCORSListenerProxy should not cancel the original channel if an outer nsIChannelEventSink vetoes the redirect while leaving the channel open.

Comment 7

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Add a wpt test to verify service worker redirect logic. r=ehsan

Beginnings of a wpt test file to verify redirects with interception.  Lots more test cases to add.

Comment 8

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Add a wpt test to verify service worker redirect logic. r=ehsan

That was the wrong test patch.

Comment 9

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Add a wpt test to verify service worker redirect logic. r=ehsan

One more version with a working expected_type check for opaqueredirect.

Comment 10

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Add a wpt test to verify service worker redirect logic. r=ehsan

Add support for testing URLs with username:password credentials in the redirection location.

Comment 11

[Ben Kelly [:bkelly, not reviewing]]

Attachment: bug1184607_p2_test.patch

Fleshed out with a wide number of combinations now.  Some failures I need to track down.  Some other test cases to add as well.

Comment 12

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Implement fetch opaqueredirect and associated logic. r=ehsan

Comment 13

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Add a wpt test to verify service worker redirect logic. r=ehsan

Add test cases for redirecting to a cross-origin resource that has CORS headers.

Comment 14

[Ben Kelly [:bkelly, not reviewing]]

Also, it seems the blink team has implemented some tests for navigation redirects:

  https://codereview.chromium.org/1286153003/

So far I think this is complementary to my tests since I've focused on the non-navigation pieces.

Comment 15

[Ben Kelly [:bkelly, not reviewing]]

A possible spec issue I ran into while writing the tests:

  https://github.com/whatwg/fetch/issues/112

Comment 16

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Implement fetch opaqueredirect and associated logic. r=ehsan

Some FetchDriver refactoring in order to get response tainting to work right during redirects.

Comment 17

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Add a wpt test to verify service worker redirect logic. r=ehsan

Fix test to expect correct Response types.  Also add comments why we expect rejections in each case.

Comment 18

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Add a wpt test to verify service worker redirect logic. r=nsm

I'm going to try to land what I have so far.  There are more things to test and possibly fix, but this is a pretty bug set of patches already and I don't want them to rot.

Here is a cleaned up test that can be reviewed while I split the P1 patch up and write the Cache migration code.

Comment 19

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Update webidl for RequestRedirect and 'opaqueredirect' in ResponseType. r=ehsan

Break out the webidl changes for DOM peer review.  Spec:

  https://fetch.spec.whatwg.org/#requestredirect
  https://fetch.spec.whatwg.org/#responsetype

Comment 20

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Update Request and Response DOM objects for new redirect model. r=nsm

This adds the bits for RequestRedirect and the 'opaqueredirect' ResponseType.

Also, I expose the unfiltered status as I realized we were not correctly storing that in Cache or using it in opaque interception.

Comment 21

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P3 Add a RedirectMode flag to nsIHttpChannelInternal. r=nsm

Add a flag to the internal http channel to propagate the RequestRedirect mode.  In addition, make sure CorsMode and RedirectMode are propagated when a redirect actually happens.

Comment 22

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P4 Handle the RequestRedirect mode during service worker interception. r=nsm

Set RequestRedirect correctly during interception and handle 'opaqueredirect' correctly in RespondWith().

Comment 23

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P5 Set RequestRedirect to "manual" for navigations. r=nsm

Set the RequestRedirect to "manual" in the places we produce network channels with a navigation content policy type.

Comment 24

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P6 Set RequestRedirect and fix various redirect bugs in FetchDriver. r=nsm

This patch does a few things:

 - Make sure RequestRedirect is set by fetch()
 - Make sure RequestMode is set by fetch(). This is necessary for RequestMode to set correctly when intercepting fetch(url, { mode: 'same-origin' }).
 - Make sure we perform main fetch checks and response tainting on redirect
 - Support opaqueredirect tainting

Comment 25

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P7 Allow the original channel to complete when redirects are vetoed in "manual" mode. r=nsm

We need to exempt "manual" redirect mode from the normal CORS proxy redirect protections in order to get the Response back correctly in FetchDriver.

I believe this is safe because the only things setting manual mode are fetch() and navigations.  We want the behavior in fetch() and navigations should not be using CORS.

I spoke with Jonas about this.  He seemed to think it made sense, but stopped short of recommending it since he wasn't sure of the fetch/service worker spec.

Comment 26

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P8 Support RequestRedirect in Cache API. r=ehsan

Add RequestRedirect to Cache API.  This is not ready for review yet since I need to add migration code and tests.

Comment 27

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P9 Add wpt tests to verify service worker redirect logic. r=nsm

Renumber the test patch.

Comment 28

[Ben Kelly [:bkelly, not reviewing]]

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

Comment 29

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P4 Handle the RequestRedirect mode during service worker interception. r=nsm

Loosen the assert for manual redirect mode on navigations to only apply to http channels.  We get this wrong for app:// protocol channels.  I could fix it there, but my impression is app:// interception was a short term solution that will be removed.  Right?

Comment 30

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P6 Set RequestRedirect and fix various redirect bugs in FetchDriver. r=nsm

Fix opt build.

New try:

  https://treeherder.mozilla.org/#/jobs?repo=try&revision=917e32ed7c7d

Comment 31

[Ben Kelly [:bkelly, not reviewing]]

I'm fixing the mochitest failures in a new patch.  They are currently failing because they are calling respondWith() using a CORS response which we currently block for iframes (until bug 1184967).  To be honest, I don't see how these tests could have been passing previously unless there was another bug somewhere I accidentally fixed with my patches here.

I don't think fixing the mochitests should require changing any of the existing patches under review.

Comment 32

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P8 Support RequestRedirect in Cache API. r=ehsan

Add a missing break statement in a switch.

Comment 33

[Ben Kelly [:bkelly, not reviewing]]

I have a partial fix for the mochitests, but it seems fetching a manual redirect request fails if the same URL has been previously redirected and cached.  Need to investigate further.

Comment 34

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Comment on attachment 8650113 [details] [diff] [review]
P2 Update Request and Response DOM objects for new redirect model. r=nsm

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

::: dom/fetch/Request.cpp
@@ +347,5 @@
>        return nullptr;
>      }
>    }
>  
> +

Nit: blank line.

Comment 35

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Comment on attachment 8650116 [details] [diff] [review]
P3 Add a RedirectMode flag to nsIHttpChannelInternal. r=nsm

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

::: netwerk/protocol/http/HttpBaseChannel.cpp
@@ +2389,5 @@
>               "[this=%p] transferring chain of redirect cache-keys", this));
>          httpInternal->SetCacheKeysRedirectChain(mRedirectedCachekeys.forget());
>      }
> +
> +    // Preserve any skip-serviceworker-flag

Nit: Period at the end, here and in the other two.

::: netwerk/protocol/http/nsIHttpChannelInternal.idl
@@ +242,5 @@
> +    const unsigned long REDIRECT_MODE_MANUAL = 2;
> +    /**
> +     * Set to indicate Request.redirect mode exposed during ServiceWorker
> +     * interception.  No policy enforcement is performed by the channel for this
> +     * value.  Defaults to REDIRECT_MODE_FOLLOW.

Nit: Double space after periods in this paragraph.

I'm not sure it is wise to mention the default value here, perhaps point the reader to HttpBaseChannel?

Comment 36

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Comment on attachment 8650132 [details] [diff] [review]
P5 Set RequestRedirect to "manual" for navigations. r=nsm

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

I can't find anything in the spec stating when "manual" is supposed to be used. I also think Tanvi or Christoph or someone else should take a look at this to make sure there are the only callsites that need updating.

Comment 37

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Nikhil Marathe [:nsm] (please needinfo?) from comment #36)
> Comment on attachment 8650132 [details] [diff] [review]
> P5 Set RequestRedirect to "manual" for navigations. r=nsm
> 
> Review of attachment 8650132 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I can't find anything in the spec stating when "manual" is supposed to be
> used. I also think Tanvi or Christoph or someone else should take a look at
> this to make sure there are the only callsites that need updating.

Nikhil, please see this spec issue:

  https://github.com/whatwg/fetch/issues/101

Basically it should be defined in http spec, but it hasn't been updated yet.

I would wait to apply the manual policy, but we must have it set for navigations in order for the other security checks in fetch to work.

Comment 38

[Ben Kelly [:bkelly, not reviewing]]

More specifically:

  https://github.com/whatwg/fetch/issues/101#issuecomment-127944574

Comment 39

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

(In reply to Ben Kelly [:bkelly] from comment #37)
> (In reply to Nikhil Marathe [:nsm] (please needinfo?) from comment #36)
> > Comment on attachment 8650132 [details] [diff] [review]
> > P5 Set RequestRedirect to "manual" for navigations. r=nsm
> > 
> > Review of attachment 8650132 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > I can't find anything in the spec stating when "manual" is supposed to be
> > used. I also think Tanvi or Christoph or someone else should take a look at
> > this to make sure there are the only callsites that need updating.
> 
> Nikhil, please see this spec issue:
> 
>   https://github.com/whatwg/fetch/issues/101
> 
> Basically it should be defined in http spec, but it hasn't been updated yet.
> 
> I would wait to apply the manual policy, but we must have it set for
> navigations in order for the other security checks in fetch to work.

Sounds reasonable, but someone else should review the change to make sure we got all callsites.

Comment 40

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Comment on attachment 8650233 [details] [diff] [review]
P4 Handle the RequestRedirect mode during service worker interception. r=nsm

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

::: dom/workers/ServiceWorkerEvents.cpp
@@ +269,3 @@
>    }
>  
>    // Section 4.2, step 2.2:

Could you update this to section "HTTP Fetch", step 2.2.

@@ +271,5 @@
>    // Section 4.2, step 2.2:
>    //  If one of the following conditions is true, return a network error:
>    //    * response's type is "error".
>    //    * request's mode is not "no-cors" and response's type is "opaque".
>    //    * request is a client request and response's type is neither "basic"

This comment should be removed too in Bug 1184967.

::: dom/workers/ServiceWorkerManager.cpp
@@ +3735,5 @@
> +      uint32_t redirectMode;
> +      internalChannel->GetRedirectMode(&redirectMode);
> +      switch (redirectMode) {
> +        case nsIHttpChannelInternal::REDIRECT_MODE_FOLLOW:
> +          mRequestRedirect = RequestRedirect::Follow;

There are static_asserts at the top of this file for cors mode constant equality checks, please add the same for these.

Comment 41

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8650132 [details] [diff] [review]
P5 Set RequestRedirect to "manual" for navigations. r=nsm

Christoph, can you review this short patch to set a new redirect flag for all navigations?

The html spec has not been updated yet, but there is agreement to this in:

  https://github.com/whatwg/fetch/issues/101

A later patch in this bug uses a MOZ_ASSERT() to verify that the navigation has the "manual" redirect correctly.

Note, this does not work for app:// protocol navigations, but I believe we are not going to allow service workers to intercept those in the long term.  The current support for app:// interceptions is only temporary and on non-release channels.

Comment 42

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Comment on attachment 8650236 [details] [diff] [review]
P6 Set RequestRedirect and fix various redirect bugs in FetchDriver. r=nsm

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

bkelly asked to stop review since this patch needs to be updated.

::: dom/fetch/FetchDriver.cpp
@@ +513,5 @@
> +        internalChan->SetCorsMode(
> +          nsIHttpChannelInternal::CORS_MODE_CORS_WITH_FORCED_PREFLIGHT);
> +        break;
> +      default:
> +        MOZ_CRASH("unexpected redirect mode");

Nit: cors mode.

Comment 43

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8650132 [details] [diff] [review]
P5 Set RequestRedirect to "manual" for navigations. r=nsm

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

(In reply to Ben Kelly [:bkelly] from comment #41)
> Comment on attachment 8650132 [details] [diff] [review]
> P5 Set RequestRedirect to "manual" for navigations. r=nsm
> 
> Christoph, can you review this short patch to set a new redirect flag for
> all navigations?

Ben, I think you have to find someone else to review that patch, I am neither a dom-, docshell-, or necko-peer :-(

Comment 44

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8650132 [details] [diff] [review]
P5 Set RequestRedirect to "manual" for navigations. r=nsm

Ollie, please see comment 41 for what this patch is about.  Basically I need a "manual" redirect flag set for navigations (as defined by content policy type).

As far as I can tell these are the only places that produce channels with the TYPE_DOCUMENT or TYPE_SUBDOCUMENT (or the corresponding mapped TYPE_INNER_* types).

Comment 45

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P6 Set RequestRedirect and fix various redirect bugs in FetchDriver. r=nsm

Modify our approach to stopping the redirect and getting an opaqueredirect.  The "return an error and not cancel the channel" approach was too fragile.  Instead, simulate calls to OnStartRequest() and OnStopRequst() directly to generate the opaqueredirect Response.  Then cancel the channel (which generates a silently failing OnStartRequest).

Comment 46

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8650139 [details] [diff] [review]
P7 Allow the original channel to complete when redirects are vetoed in "manual" mode. r=nsm

With the changes to P6, we no longer have to modify the nsCORSListenerProxy at all.

Comment 47

[Ben Kelly [:bkelly, not reviewing]]

I hope to post the mochitest fix later tonight.  Passing locally but I need to clean it up a bit.

Comment 48

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P7 Support RequestRedirect in Cache API. r=ehsan

Renumber.

Comment 49

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P8 Fix mochitests to store opaqueredirect responses in Cache for navigation URLs. r=nsm

Fix the mochitests.  We should never have been able to intercept a navigation with a CORS response like this.  Use opaqueredirect responses instead.

Comment 50

[Ben Kelly [:bkelly, not reviewing]]

I see I have a bad implicit constructor, but I'm going to let this try run over night:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=38bc03e282da

Comment 51

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8650702 [details] [diff] [review]
P6 Set RequestRedirect and fix various redirect bugs in FetchDriver. r=nsm

Sorry for the flag spam.  Try shows there is still a problem in e10s mode.

Comment 52

[Ben Kelly [:bkelly, not reviewing]]

Hmm, I think maybe I'm just seeing bug 1172992.  Updating my tree to verify.

Comment 53

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P10 Expose channel security info during e10s redirect. Support security info in redirecting interceptions. r=jduell

Jason, this patch does two things:

1) It makes sure the security info is set in the child while performing the redirect callback.  This matches the non-e10s behavior.
2) It sets the correct flag to allow security info to be set via an interception that redirects.  I think this was just an untested path previously due to (1).

Comment 54

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P4 interdiff 001 fix manual redirect assertion for navigations r=nsm

The way I was checking for http-vs-jar channel for asserting redirect==manual for navigations was not safe in e10s mode.  Switch the logic around a bit to be thread safe.

Comment 55

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8650702 [details] [diff] [review]
P6 Set RequestRedirect and fix various redirect bugs in FetchDriver. r=nsm

Reflag for review.  Turns out nothing changed here since I fixed the e10s issue in necko.

Comment 56

[Ben Kelly [:bkelly, not reviewing]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5e9cfd22e52f

Comment 57

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8650132 [details] [diff] [review]
P5 Set RequestRedirect to "manual" for navigations. r=nsm

I don't think we want the flag in ImportLoader case. That isn't about navigation,
it is more like about XHR loading some document or so.
but r+ for the docshell part.

(also, HTML imports are pretty much dead I think)

Comment 58

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Olli Pettay [:smaug] (high review load) from comment #57)
> Comment on attachment 8650132 [details] [diff] [review]
> P5 Set RequestRedirect to "manual" for navigations. r=nsm
> 
> I don't think we want the flag in ImportLoader case. That isn't about
> navigation,
> it is more like about XHR loading some document or so.
> but r+ for the docshell part.
> 
> (also, HTML imports are pretty much dead I think)

Yea, I think html imports is using the wrong nsContentPolicyType.  Given bug 1197401, though, I don't think I will try to fix it.  Instead I'll just leave the assertion to catch if someone tries using html imports with service workers.  And on release the worst thing that could happen is that the import redirection will fail with interception.  There won't be any security concerns as far as I can see.

Thanks for the review.

Comment 59

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Comment on attachment 8650702 [details] [diff] [review]
P6 Set RequestRedirect and fix various redirect bugs in FetchDriver. r=nsm

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

I'd like to see this again with some explanation so I can understand it better.

::: dom/fetch/FetchDriver.cpp
@@ +163,3 @@
>      corsPreflight = true;
> +
> +    // TODO: we have to clear CORS preflight cache entries if we fail after this

Why do we have to do this? The cache won't be populated till the nsCORSListenerProxy is actually used right. Also if this isn't fixed in this patch, please put the bug number.

@@ +583,5 @@
>  
> +  // Only use nsCORSListenerProxy if we are in CORS mode.  Otherwise it
> +  // will overwrite the CorsMode flag unconditionally to "cors" or
> +  // "cors-with-forced-preflight".
> +  if (mRequest->Mode() == RequestMode::Cors) {

I don't understand how this will deal with the case when a same-origin redirect then redirects to a cross-origin? In that case there will be no CORS proxy to follow the CORS protocol. Is this impossible with the new architecture where we are tracking redirects in FetchDriver?

@@ +889,5 @@
>    NS_PRECONDITION(aNewChannel, "Redirect without a channel?");
>  
>    nsresult rv;
>  
> +  if (NS_WARN_IF(mRequest->GetRedirectMode() == RequestRedirect::Error)) {

Could you add comments about spec bits you are implementing throughout this method?

@@ +890,5 @@
>  
>    nsresult rv;
>  
> +  if (NS_WARN_IF(mRequest->GetRedirectMode() == RequestRedirect::Error)) {
> +    aOldChannel->Cancel(NS_BINDING_FAILED);

Why aren't On{Start,Stop}Request simulated here?

@@ +904,5 @@
>    mRequest->UnsetSameOriginDataURL();
>  
> +  if (mRequest->GetRedirectMode() == RequestRedirect::Manual) {
> +    // Ideally we would simply not cancel the old channel and allow it to
> +    // be processed as normal.  Unfortunately this is quite fragile and

Fragile how? A small explanation or an example of how it breaks would be best for future reference.

@@ +914,5 @@
> +    unused << OnStopRequest(aOldChannel, nullptr, NS_OK);
> +
> +    aOldChannel->Cancel(NS_BINDING_FAILED);
> +
> +    return NS_BINDING_FAILED;

This sounds like you are vetoing the redirect, where the spec just says to set the response tainting and continue. Could you resolve the discrepancy between the spec and the implementation with a comment?

Specifically, won't failing here lead to the new channel not being opened at all? In that case how do we finish processing the redirect?

Comment 60

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

(In reply to Nikhil Marathe [:nsm] (please needinfo?) from comment #59)
> @@ +914,5 @@
> > +    unused << OnStopRequest(aOldChannel, nullptr, NS_OK);
> > +
> > +    aOldChannel->Cancel(NS_BINDING_FAILED);
> > +
> > +    return NS_BINDING_FAILED;
> 
> This sounds like you are vetoing the redirect, where the spec just says to
> set the response tainting and continue. Could you resolve the discrepancy
> between the spec and the implementation with a comment?
> 
> Specifically, won't failing here lead to the new channel not being opened at
> all? In that case how do we finish processing the redirect?


I think I understand this bit now. Since it is a manual redirect, we don't want to continue with the redirect. But why cancel the old channel? IIUC that is the channel that redirected, so shouldn't it be allowed to continue and call FetchDriver::OnStartRequest so that we can resolve the fetch() with a opaqueredirect Response? Sorry, I'm having trouble understanding who calls ::OnStartRequest the other time with the correct parameters and not the simulated call above.

Comment 61

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Comment on attachment 8650787 [details] [diff] [review]
P8 Fix mochitests to store opaqueredirect responses in Cache for navigation URLs. r=nsm

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

::: dom/workers/test/serviceworkers/fetch/origin/origin_test.js
@@ +6,5 @@
>        .then(c => {
>          return Promise.all(
>            [
> +            c.add(new Request(prefix + 'index.sjs',
> +                              { redirect: 'manual' } )),

Just to clarify, before this change:
During "install", Cache fetches index.sjs which redirects to "realindex.html", so cache now actually stores the contents of "realindex.html" (which needed the CORS header so it could actually be retrieved).

After this change:
During "install", Cache fetches index.sjs, which is delivered as a opaqueredirect, internally cache reads out the headers and stores those in the Response. In onfetch, the Cache delivers the same Response, at which point Gecko sees the Location headers and goes and fetches "realindex.html". No CORS needed since it is a navigation request (and not a fetch as it was previously when it was caching on install).

Am I correct? In that case, if the user is offline, won't this redirect fail since "realindex" won't be reachable. I'm not saying that is a problem during the test, just wondering if I have all my mental models right.

Comment 62

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

(In reply to Nikhil Marathe [:nsm] (please needinfo?) from comment #61)
> Comment on attachment 8650787 [details] [diff] [review]
> P8 Fix mochitests to store opaqueredirect responses in Cache for navigation
> URLs. r=nsm
> 
> Review of attachment 8650787 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/workers/test/serviceworkers/fetch/origin/origin_test.js
> @@ +6,5 @@
> >        .then(c => {
> >          return Promise.all(
> >            [
> > +            c.add(new Request(prefix + 'index.sjs',
> > +                              { redirect: 'manual' } )),
> 
> Just to clarify, before this change:
> During "install", Cache fetches index.sjs which redirects to
> "realindex.html", so cache now actually stores the contents of
> "realindex.html" (which needed the CORS header so it could actually be
> retrieved).
> 
> After this change:
> During "install", Cache fetches index.sjs, which is delivered as a
> opaqueredirect, internally cache reads out the headers and stores those in
> the Response. In onfetch, the Cache delivers the same Response, at which
> point Gecko sees the Location headers and goes and fetches "realindex.html".
> No CORS needed since it is a navigation request (and not a fetch as it was
> previously when it was caching on install).
> 
> Am I correct? In that case, if the user is offline, won't this redirect fail
> since "realindex" won't be reachable. I'm not saying that is a problem
> during the test, just wondering if I have all my mental models right.

On IRC:
<bkelly> nsm: basically you are correct there
<bkelly> nsm: when an opaquredirect is returned to a navigation it will execute the redirect and can be intercepted again, though
<bkelly> nsm: and that secondary intercept can be offlined separately
<nsm> right, the test doesn't do it, but that is how production code would
<bkelly> the navigation redirect could be cross-origin.. .so it may be itnercepted by a completely different SW
<nsm> but that redirect will only happen in onfetch right?
<nsm> so at install it can't be offlined
<bkelly> nsm: right... I actually plan to switch the test back to do the CORS follow redirect after I land bug         1184967
<bkelly> nsm: correct... the redirect is only executed when the navigation actually gets the opaqueredirect back
<bkelly> nsm: its kind of a wart of the spec... but its unclear how to fix that

Comment 63

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Comment on attachment 8650142 [details] [diff] [review]
P9 Add wpt tests to verify service worker redirect logic. r=nsm

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

I wish there was a nicer way to express the tests when enumerating the possibilities. Like some state machine that ran the fetch algorithm according to spec and ensured our implementation matched. This will have to do for now.

::: testing/web-platform/mozilla/tests/service-workers/service-worker/resources/fetch-event-redirect-iframe.html
@@ +9,5 @@
> +    port.postMessage({result: 'reject', detail: e.toString()});
> +  });
> +});
> +
> +function executeFetch(data) {

Why not

| return fetch(new Request(data.url, data.request_init))|

::: testing/web-platform/mozilla/tests/service-workers/service-worker/resources/redirect.py
@@ +1,1 @@
> +def main(request, response):

I have a bugfree version of this entire file in Bug 1180861. I would recommend using that...

@@ +19,5 @@
> +        if query in request.GET:
> +            headers.append((header, request.GET[query]))
> +
> +    if "ACEHeaders" in request.GET:
> +        headers.append(("Access-Control-Expose-Headers", request.GET[query]))

...because there were bugs like this here |query| not being defined.

::: testing/web-platform/mozilla/tests/service-workers/service-worker/resources/success.py
@@ +4,5 @@
> +    if "ACAOrigin" in request.GET:
> +        for item in request.GET["ACAOrigin"].split(","):
> +            headers.append(("Access-Control-Allow-Origin", item))
> +
> +    return 200, headers, "{ \"result\": \"success\" }"

Nit: You don't need the 200.

Comment 64

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Nikhil Marathe [:nsm] (please needinfo?) from comment #59)
> Comment on attachment 8650702 [details] [diff] [review]
> P6 Set RequestRedirect and fix various redirect bugs in FetchDriver. r=nsm
> 
> Review of attachment 8650702 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'd like to see this again with some explanation so I can understand it
> better.
> 
> ::: dom/fetch/FetchDriver.cpp
> @@ +163,3 @@
> >      corsPreflight = true;
> > +
> > +    // TODO: we have to clear CORS preflight cache entries if we fail after this
> 
> Why do we have to do this? The cache won't be populated till the
> nsCORSListenerProxy is actually used right. Also if this isn't fixed in this
> patch, please put the bug number.

This was mainly me trying to account for:

  "The result of performing an HTTP fetch using request with the CORS flag and CORS-preflight flag set. If the result is a network error, clear cache entries using request. "

In step 8 of main fetch.  On IRC, you noted that it appears we do this, though, by calling this when CheckRequestApproved() fails:

  https://dxr.mozilla.org/mozilla-central/source/dom/security/nsCORSListenerProxy.cpp#327

So I will remove the TODO comment.
 
> @@ +583,5 @@
> > +  // Only use nsCORSListenerProxy if we are in CORS mode.  Otherwise it
> > +  // will overwrite the CorsMode flag unconditionally to "cors" or
> > +  // "cors-with-forced-preflight".
> > +  if (mRequest->Mode() == RequestMode::Cors) {
> 
> I don't understand how this will deal with the case when a same-origin
> redirect then redirects to a cross-origin? In that case there will be no
> CORS proxy to follow the CORS protocol. Is this impossible with the new
> architecture where we are tracking redirects in FetchDriver?

If RequestMode is "same-origin", we should NOT follow CORS protocol for cross-origin requests.

For the case of sameOriginURL resulting in a 30x to crossOriginURL we follow these spec steps:

1) Enter Main Fetch
2) Main Fetch step 8 goes to Basic Fetch (sameOriginURL is same origin and CORS flag not set)
3) Basic Fetch goes to HTTP Fetch
4) HTTP Fetch step 5 gets a 30x result
5) HTTP Fetch step 5 "redirect status" switch step 11.6 goes to Main Fetch with CORS flag not set
6) Main Fetch step 8 does not follow basic fetch because crossOriginURL is not same-origin
7) Main Fetch step 8 fails with Network Error because request's mode is "same-origin"

This is implemented in the patch by performing the SetTaintingAndGetNextOp() method in both ContinueFetch() and the redirect path.

> @@ +889,5 @@
> > +  if (NS_WARN_IF(mRequest->GetRedirectMode() == RequestRedirect::Error)) {
> 
> Could you add comments about spec bits you are implementing throughout this
> method?

Sure.  This particular bit is for HTTP Fetch step 5 "redirect status" step 1.

> 
> @@ +890,5 @@
> >  
> >    nsresult rv;
> >  
> > +  if (NS_WARN_IF(mRequest->GetRedirectMode() == RequestRedirect::Error)) {
> > +    aOldChannel->Cancel(NS_BINDING_FAILED);
> 
> Why aren't On{Start,Stop}Request simulated here?

I think we discussed on IRC and later here, but we only simulate On{Start,Stop}Request in order to produce an opaqueresult Response.  Here, trying to redirect with RequestRedirect == "error" should actually result in failing the entire fetch.

> @@ +904,5 @@
> >    mRequest->UnsetSameOriginDataURL();
> >  
> > +  if (mRequest->GetRedirectMode() == RequestRedirect::Manual) {
> > +    // Ideally we would simply not cancel the old channel and allow it to
> > +    // be processed as normal.  Unfortunately this is quite fragile and
> 
> Fragile how? A small explanation or an example of how it breaks would be
> best for future reference.

I'll expand the comment, but other code in the tree is not designed to allow it.  The two cases I ran into are:

1) nsCORSListenerProxy explicitly cancels the starting channel if an outer nsIEventSink vetoes the redirect.
2) The http cache fails the nsIChannel if the redirect is vetoed and it previously cached a redirect for that same URI.

It seemed quite hard to fix all the possible cases these might touch on, so I thought it would be more robust just to create the opaqueredirect Response immediately when I knew the redirect was stopped due to being "manual".

> @@ +914,5 @@
> > +    unused << OnStopRequest(aOldChannel, nullptr, NS_OK);
> > +
> > +    aOldChannel->Cancel(NS_BINDING_FAILED);
> > +
> > +    return NS_BINDING_FAILED;
> 
> This sounds like you are vetoing the redirect, where the spec just says to
> set the response tainting and continue. Could you resolve the discrepancy
> between the spec and the implementation with a comment?
> 
> Specifically, won't failing here lead to the new channel not being opened at
> all? In that case how do we finish processing the redirect?

We are supposed to stop the redirect.  See HTTP fetch step 5 "redirect status" step 10:

  "If request's redirect mode is "manual", set response to an opaque-redirect filtered response whose internal response is actualResponse."

We don't actually loop back into the main fetch in this case.  So when we hit a redirect with "manual" mode we just need to synthesize the opaqueredirect Response and stop the redirect.

Comment 65

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Nikhil Marathe [:nsm] (please needinfo?) from comment #63)
> I wish there was a nicer way to express the tests when enumerating the
> possibilities. Like some state machine that ran the fetch algorithm
> according to spec and ensured our implementation matched. This will have to
> do for now.

Yea, I don't know how to implement that.  Particularly with a recursive algorithm like fetch.  The redirect logic is insanely complex and our implementation is modeled completely different from the fetch spec.  It makes it hard to verify we are hitting all the cases.

I tried to put in TODO comments for the cases I think we are missing.  Mainly navigations, although I think our mochitests help there.  Also there is a new blink test I can import to test some navigation code.

> :::
> testing/web-platform/mozilla/tests/service-workers/service-worker/resources/
> redirect.py
> @@ +1,1 @@
> > +def main(request, response):
> 
> I have a bugfree version of this entire file in Bug 1180861. I would
> recommend using that...

You will probably land your version before me, so I will just use yours.  I had my version in my patch before you started bug 1180861.

Comment 66

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Update Request and Response DOM objects for new redirect model. r=nsm

Comment 67

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P3 Add a RedirectMode flag to nsIHttpChannelInternal. r=nsm

Comment 68

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P4 Handle the RequestRedirect mode during service worker interception. r=nsm

Comment 69

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P5 Set RequestRedirect to "manual" for navigations. r=nsm

Comment 70

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P6 Set RequestRedirect and fix various redirect bugs in FetchDriver. r=nsm

Updated for review feedback and to remove a couple TODO items I had left in regarding the CORS flag.  I also handle the preflight redirect block as the spec requires using the error redirect flag.  This broke a couple of the mochitests that were incorrect, but I will post another patch to fix them.

Comment 71

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P11 Fix fetch CORS tests to not expect same-origin requests that redirect to preflight. r=nsm

This fixes a couple redirect cases in test_fetch_cors.html.

Basically, the tests were incorrectly preventing fetch() calls with POST or DELETE with unsafe headers to same-origin URLs that redirect.  This is not actually what the spec says.

Consider a fetch() with non-safe headers and POST method to sameOriginURL that redirects to CORS-enabled crossOriginURL.  The spec follows this path:

1) Enter Main Fetch with CORS flag unset.
2) Main Fetch step 8 "same origin and CORS flag unset" matches.
3) Enter Basic Fetch
4) Enter HTTP Fetch
5) Receive 30x status code and enter step 5 "redirect status" with crossOriginURL in Location
6) Reach step 11.6 and recurse into Main Fetch
7) Main Fetch step 8 "request's unsafe-request flag is set and method is not simple" matches.
8) Set redirect mode to error
9) Call HTTP fetch with cors and preflight flags set
10) crossOriginURL is CORS enabled and loads with a 200
11) return result CORS Response

We actually need the URL that redirects to be cross-origin itself in order to trigger preflight+redirect failure.

This test marks the existing tests as expected pass.  It also adds two new tests with additional cross-origin hops that will hit the preflight+redirect restriction.

Comment 72

[Ben Kelly [:bkelly, not reviewing]]

I still need to address action items in P8 and P9.  I am also working on the migration code for P7.

Comment 73

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Ben,

If you look at the XHR redirect tests https://dxr.mozilla.org/mozilla-central/source/dom/security/test/cors/test_CrossSiteXHR.html?offset=200#1046
(the XHR tests run in a frame loaded from example.org, which is why the first hop is example.org), then it is expected to fail. Does XHR need to be modified per the fetch spec, or is this because XHR is "cors" mode by default?

Comment 74

[Ben Kelly [:bkelly, not reviewing]]

> If you look at the XHR redirect tests
> https://dxr.mozilla.org/mozilla-central/source/dom/security/test/cors/
> test_CrossSiteXHR.html?offset=200#1046
> (the XHR tests run in a frame loaded from example.org, which is why the
> first hop is example.org), then it is expected to fail. Does XHR need to be
> modified per the fetch spec, or is this because XHR is "cors" mode by
> default?

It appears that might be a known bug.  See bug 918767.

Comment 75

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P7.1 Move Cache schema SQL into separate constants. r=ehsan

I'm posting some initial Cache modification patches.  I've tried to make these as small as possible for easy review.

This one moves all of the schema SQL definitions into static constants.  This lets me use them in multiple places (validation/migration) in the code in later patches.

This also removes the trailing semicolons from the SQL as sqlite strips them anyway.

No other changes to the content of the SQL.

Comment 76

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P7.2 Validate Cache schema in debug builds. r=ehsan

Validate the final database schema against our SQL constants before proceeding.  This will catch inadvertent changes to the schema where a migration hasn't been written.

Comment 77

[Jason Duell]

Comment on attachment 8651222 [details] [diff] [review]
P10 Expose channel security info during e10s redirect. Support security info in redirecting interceptions. r=jduell

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

::: netwerk/protocol/http/PHttpChannel.ipdl
@@ +121,5 @@
>    Redirect1Begin(uint32_t           newChannelId,
>                   URIParams          newUri,
>                   uint32_t           redirectFlags,
> +                 nsHttpResponseHead responseHead,
> +                 nsCString           securityInfoSerialization);

nit: indentation should match other params

Comment 78

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P7.3 Rename Cache "max wipe version" constant to "first shipped version". r=ehsan

The name of the kMaxWipeSchemaVersion was hurting my brain.  Its not actually the maximum version we will wipe.  Its the first version we won't wipe.

So rename it to kFirstShippedSchemaVersion.

Comment 79

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P7.4 Infrastructure for running Cache schema migrations. r=ehsan

This adds infrastructure to run migration functions when the schema is old.  I'm not flagging for review yet because I still have to test it.

Comment 80

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P7.5 Add RequestRedirect to Cache API schema with migration. r=ehsan

This expands on the earlier P7 patch by adding a migration.  I will add a test tomorrow to verify this works and then flag for review.

Comment 81

[(no longer active)]

Comment on attachment 8653108 [details] [diff] [review]
P7.2 Validate Cache schema in debug builds. r=ehsan

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

mMind->Blown("wow!");

Comment 82

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P7.6 Expose CacheStorage .caches property on xpcshell global. r=smaug

Expose the CacheStorage .caches attribute on the xpcshell global.

Comment 83

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P7.7 Allow new Response() to be used in xpcshell tests. r=ehsan

Allow Response::Constructor() to be used in chrome script when there is no window.  This is necessary to write cache xpcshell tests.

Comment 84

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8653820 [details] [diff] [review]
P7.6 Expose CacheStorage .caches property on xpcshell global. r=smaug

>+    rv.SuppressException();
>+    return false;

This will throw an uncatchable exception, right?  Please "return ThrowMethodFailed(cx, rv);" unless you want the uncatchable exception, in which case add a comment explaining why.

>+  if (NS_WARN_IF(!GetOrCreateDOMReflector(aCx, storage, &caches))) {

May be better to use ToJSValue here instead.

r=me

Comment 85

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P7.4 Infrastructure for running Cache schema migrations. r=ehsan

Fixed some problems revealed by testing.  Also, run a VACUUM after changing the structure of the database.

Comment 86

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P7.5 Add RequestRedirect to Cache API schema with migration. r=ehsan

Tested working version of the migration.

Comment 87

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P7.8 Test Cache API schema verison migrations. r=ehsan

Add an xpcshell test to verify migration works.

Also, include a dummy xpcshell test for making new profile zips in case we need to test different database contents in the future.  This "make_profile.js" test is marked skip-if=true.

Comment 88

[Ben Kelly [:bkelly, not reviewing]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4373f89a86eb

Comment 89

[(no longer active)]

Comment on attachment 8654290 [details] [diff] [review]
P7.8 Test Cache API schema verison migrations. r=ehsan

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

Very nice!

::: dom/cache/test/xpcshell/make_profile.js
@@ +106,5 @@
> +
> +    dump('### ### call getUsageForPrincipal()\n');
> +    // use getUsageForPrincipal() to get a callback when the reset() is done
> +    try{
> +    qm.getUsageForPrincipal(principal, function(principal, usage, fileUsage) {

Nit: indentation.

::: dom/cache/test/xpcshell/test_migration.js
@@ +9,5 @@
> +var Cc = Components.classes;
> +var Ci = Components.interfaces;
> +var Cu = Components.utils;
> +
> +function create_test_profile() {

I think that if this function takes a zip file name as an argument, it would be a valuable utility function for future migration tests.  Do you mind changing it to accept the argument and move it to a head.js file in the same directory?  The xpcshell framework automatically evaluates head.js files before running these test scripts, so this function would become available to all tests in the same directory.

Comment 90

[(no longer active)]

Comment on attachment 8653821 [details] [diff] [review]
P7.7 Allow new Response() to be used in xpcshell tests. r=ehsan

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

::: dom/fetch/ChannelInfo.cpp
@@ +77,5 @@
> +
> +  MOZ_RELEASE_ASSERT(
> +    nsContentUtils::IsSystemPrincipal(aGlobal->PrincipalOrNull()));
> +
> +  mSecurityInfo = EmptyCString();

Nit: mSecurityInfo.Truncate();

Comment 91

[(no longer active)]

Comment on attachment 8654284 [details] [diff] [review]
P7.5 Add RequestRedirect to Cache API schema with migration. r=ehsan

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

::: dom/cache/DBSchema.cpp
@@ +2433,5 @@
> +  MOZ_ASSERT(aConn);
> +
> +  // Add the request_redirect column with a default value of "follow".  Note,
> +  // we only use a default value here because its required by ALTER TABLE.  We
> +  // don't actually want it in the schema.

Please extend the comment to also say that this default value is desirable for existing entries in the cache, otherwise the migration would have needed to deal with existing data as well.

Comment 92

[Ben Kelly [:bkelly, not reviewing]]

Addressed action items and new try build:

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

I'm considering splitting a couple patches and re-ordering things before landing.  Right now there is no rev you can check out to get a schema 15 database again.  If I re-order the patches, though, I should be able to permit that.  Seems like it could be useful.

Comment 93

[Ben Kelly [:bkelly, not reviewing]]

The last try had a few xpcshell failures on b2g.  I've disabled these them on b2g since this is always what IDB does for its migration tests.

Comment 94

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/991f30d04649
https://hg.mozilla.org/integration/mozilla-inbound/rev/3595c533caac
https://hg.mozilla.org/integration/mozilla-inbound/rev/5783ae461579
https://hg.mozilla.org/integration/mozilla-inbound/rev/c704a0a42901
https://hg.mozilla.org/integration/mozilla-inbound/rev/0e09cff1b46c
https://hg.mozilla.org/integration/mozilla-inbound/rev/42abe24004fd
https://hg.mozilla.org/integration/mozilla-inbound/rev/9a054173a13e
https://hg.mozilla.org/integration/mozilla-inbound/rev/35262b99f9c7
https://hg.mozilla.org/integration/mozilla-inbound/rev/95fed4c06038
https://hg.mozilla.org/integration/mozilla-inbound/rev/4ab4edf1f730
https://hg.mozilla.org/integration/mozilla-inbound/rev/5f09a49ac1a9
https://hg.mozilla.org/integration/mozilla-inbound/rev/84cb3ede78ef
https://hg.mozilla.org/integration/mozilla-inbound/rev/ed9da75e6f6e
https://hg.mozilla.org/integration/mozilla-inbound/rev/e330761d6058
https://hg.mozilla.org/integration/mozilla-inbound/rev/0f3779b36cb2
https://hg.mozilla.org/integration/mozilla-inbound/rev/96ee097b78c1
https://hg.mozilla.org/integration/mozilla-inbound/rev/a01c5c8209a8

Comment 95

[Ben Kelly [:bkelly, not reviewing]]

And this one which had the wrong bug number in the commit message:

  https://hg.mozilla.org/integration/mozilla-inbound/rev/066e84750afd

Comment 96

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/daf3c7087ab2

Comment 97

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/991f30d04649
https://hg.mozilla.org/mozilla-central/rev/3595c533caac
https://hg.mozilla.org/mozilla-central/rev/5783ae461579
https://hg.mozilla.org/mozilla-central/rev/c704a0a42901
https://hg.mozilla.org/mozilla-central/rev/0e09cff1b46c
https://hg.mozilla.org/mozilla-central/rev/42abe24004fd
https://hg.mozilla.org/mozilla-central/rev/9a054173a13e
https://hg.mozilla.org/mozilla-central/rev/35262b99f9c7
https://hg.mozilla.org/mozilla-central/rev/95fed4c06038
https://hg.mozilla.org/mozilla-central/rev/4ab4edf1f730
https://hg.mozilla.org/mozilla-central/rev/5f09a49ac1a9
https://hg.mozilla.org/mozilla-central/rev/84cb3ede78ef
https://hg.mozilla.org/mozilla-central/rev/ed9da75e6f6e
https://hg.mozilla.org/mozilla-central/rev/e330761d6058
https://hg.mozilla.org/mozilla-central/rev/0f3779b36cb2
https://hg.mozilla.org/mozilla-central/rev/96ee097b78c1
https://hg.mozilla.org/mozilla-central/rev/a01c5c8209a8
https://hg.mozilla.org/mozilla-central/rev/daf3c7087ab2