1367742 - Crash in mozilla::net::nsHttpChannel::ContinueProcessRedirection

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect)

Description

[Marcia Knous [:marcia]]

This bug was filed from the Socorro interface and is 
report bp-d33b4d1c-ad70-4721-8e30-f00a70170525.
=============================================================

Seen while looking at nightly crash stats - crashes started using 20170525030225: http://bit.ly/2qjJlcB

Possible regression range based on Build ID: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=291a11111bdd05c5cd55dd552da4b1285ceba9b2&tochange=0ed0fd886134b6698f593edbf0d362ac9e12fe07

Not sure if Bug 1365306 is involved, ni on honza

Comment 1

[Marcia Knous [:marcia]]

Also seeing these crashes, so far 1 of each:

mozilla::net::nsHttpTransaction::RemoveDispatchedAsBlocking
mozilla::net::nsHttpConnectionMgr::TryDispatchTransaction
mozilla::net::Http2Stream::UpdateTransportReadEvents

Comment 2

[Honza Bambas (:mayhemer)]

(In reply to Marcia Knous [:marcia - use ni] from comment #0)
> Not sure if Bug 1365306 is involved, ni on honza

It's a backout of a code relatively far from this one.

I would more suspect bug 1366224, michal?

Comment 3

[Michal Novotny [:michal]]

It's possible, I'll check it.

Comment 4

[(no longer active)]

I got this crash when restoring my session today a few times but unfortunately I had turned off the crash reporter by mistake so only caught one instance here: bp-c9a76abe-4cef-4bb0-ba4e-03a600170525

Comment 5

[[:philipp]]

[Tracking Requested - why for this release]:
this signature accounts for >70% of browser crashes from today's nightly build so far.

Comment 6

[Marcia Knous [:marcia]]

Tracking 55+ for this crash.

Comment 7

[Marcia Knous [:marcia]]

(In reply to Marcia Knous [:marcia - use ni] from comment #6)
> Tracking 55+ for this crash.

Adding topcrash in the whiteboard since this has quickly moved up to #11 in browser crashes.

Comment 8

[Itiel]

(In reply to :Ehsan Akhgari (needinfo please, extremely long backlog) from comment #4)
> I got this crash when restoring my session today a few times but
> unfortunately I had turned off the crash reporter by mistake so only caught
> one instance here: bp-c9a76abe-4cef-4bb0-ba4e-03a600170525

I also got this crash when restoring my session, I have a session with about 270~ tabs.
Nightly crashed again after the first crash, the third time was okay.

My crash reports:
https://crash-stats.mozilla.com/report/index/e9f0f515-1c69-47e7-b9b8-f9e5d0170526
https://crash-stats.mozilla.com/report/index/3b162389-29e0-4616-a7e4-ba2000170526

Comment 9

[Honza Bambas (:mayhemer)]

Note that I was able to reproduce this on try (regularly):
https://treeherder.mozilla.org/logviewer.html#?job_id=102042324&repo=try

If [1] doesn't suffer from this crash, then it's definitely RCWN, since on [1] it's been disabled (just to avoid the crash).  If the crash is even there, the cause is elsewhere!


[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=325ac293772a6e865468e9a9d5758928ab0b9c7b

Comment 10

[Honza Bambas (:mayhemer)]

(In reply to Honza Bambas (:mayhemer) from comment #9)
> Note that I was able to reproduce this on try (regularly):
> https://treeherder.mozilla.org/logviewer.html#?job_id=102042324&repo=try
> 

Confirmed, this is a RCWN crash.

Comment 11

[Mike Taylor [:miketaylr]]

Michal, any idea why crashes stopped on the 25th? Do we think we can get to this in 55 window?

Comment 12

[Valentin Gosu [:valentin] (he/him)]

We turned off racing in bug 1367365.

Comment 13

[Michal Novotny [:michal]]

Attachment: patch v1

The problem is that we're trying to do the redirect twice because for the cache response this is initiated from ReadFromCache() and not from OnStartRequest(). The sequence is as follows:

- nsHttpChannel::ReadFromCache  -> nsHttpChannel::AsyncCall(&nsHttpChannel::HandleAsyncRedirect)
- nsHttpChannel::OnStartRequest -> nsHttpChannel::AsyncProcessRedirection()
- nsHttpChannel::HandleAsyncRedirect -> AsyncProcessRedirection()
- nsHttpChannel::ContinueProcessRedirection
- AutoRedirectVetoNotifier::ReportRedirectResult - nulls out mRedirectChannel
- nsHttpChannel::ContinueProcessRedirection 

I moved setting mFirstResponseSource = RESPONSE_FROM_CACHE from OnStartRequest() to ReadFromCache(). When testing the patch I experienced yet another problem:

- ReadFromCache  -> AsyncCall(&nsHttpChannel::HandleAsyncRedirect)
- OnStartRequest - ignores the redirect but the mTransactionPump will end up in IDLE state
- nsHttpChannel::HandleAsyncRedirect
- nsHttpChannel::WaitForRedirectCallback - mTransactionPump->Suspend() fails because it is idle
- nsHttpChannel::OnRedirectVerifyCallback - asserts on mWaitingForRedirectCallback

I solved this by ignoring mTransactionPump when the cache wins. I think similar problem can happen in case of SuspendInternal/ResumeInternal, but I'm not sure if ReadFromCache() could be called when the channel is suspended. In this case SuspendInternal() might be called when mFirstResponseSource is still RESPONSE_PENDING and ResumeInternal() when it is already RESPONSE_FROM_CACHE. I guess this shouldn't happen. Honza, am I right?

Comment 14

[Valentin Gosu [:valentin] (he/him)]

Comment on attachment 8874516 [details] [diff] [review]
patch v1

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

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +4881,5 @@
> +
> +            // XXX: Consider cancelling H2 transactions  or H1 transactions
> +            // that are not keep-alive.
> +        } else {
> +            MOZ_ASSERT(mFirstResponseSource != RESPONSE_FROM_CACHE);

We already do this assertion once. Let's check that mFirstResponseSource == RESPONSE_FROM_NETWORK

@@ +8149,5 @@
>      nsresult rv;
>      LOG(("nsHttpChannel::WaitForRedirectCallback [this=%p]\n", this));
>  
> +    if (mTransactionPump &&
> +        !(mRaceCacheWithNetwork && mFirstResponseSource == RESPONSE_FROM_NETWORK)) {

> I solved this by ignoring mTransactionPump when the cache wins.
So shouldn't you check if it's equal to RESPONSE_FROM_CACHE ?

@@ +8219,5 @@
>      // called we need OnStopRequest to be triggered, and if we broke out of the
>      // loop above (and are thus waiting for a new callback) the suspension
>      // count must be balanced in the pumps.
> +    if (mTransactionPump &&
> +        !(mRaceCacheWithNetwork && mFirstResponseSource == RESPONSE_FROM_NETWORK)) {

Also here, it's unclear why we are ignoring the transaction pump when the response is coming from the network. Please add some comments if this is correct.

Comment 15

[Michal Novotny [:michal]]

Attachment: patch v2

(In reply to Valentin Gosu [:valentin] from comment #14)
> @@ +8149,5 @@
> >      nsresult rv;
> >      LOG(("nsHttpChannel::WaitForRedirectCallback [this=%p]\n", this));
> >  
> > +    if (mTransactionPump &&
> > +        !(mRaceCacheWithNetwork && mFirstResponseSource == RESPONSE_FROM_NETWORK)) {
> 
> > I solved this by ignoring mTransactionPump when the cache wins.
> So shouldn't you check if it's equal to RESPONSE_FROM_CACHE ?

Of course, you're right.

Comment 16

[Honza Bambas (:mayhemer)]

(In reply to Michal Novotny (:michal) from comment #13)
> Created attachment 8874516 [details] [diff] [review]
> patch v1
> 
> The problem is that we're trying to do the redirect twice because for the
> cache response this is initiated from ReadFromCache() and not from
> OnStartRequest(). The sequence is as follows:
> 
> - nsHttpChannel::ReadFromCache  ->
> nsHttpChannel::AsyncCall(&nsHttpChannel::HandleAsyncRedirect)
> - nsHttpChannel::OnStartRequest -> nsHttpChannel::AsyncProcessRedirection()
> - nsHttpChannel::HandleAsyncRedirect -> AsyncProcessRedirection()
> - nsHttpChannel::ContinueProcessRedirection
> - AutoRedirectVetoNotifier::ReportRedirectResult - nulls out mRedirectChannel
> - nsHttpChannel::ContinueProcessRedirection 
> 
> I moved setting mFirstResponseSource = RESPONSE_FROM_CACHE from
> OnStartRequest() to ReadFromCache(). 

Only when you redirect, right?  (you can behave as if you already knew the result of the response, OK.)  You should probably also cancel the transaction - is there a reason why not to?  Apparently you no longer need the network response.  When the redirect is vetoed, we then load the cached content, right?

> When testing the patch I experienced
> yet another problem:
> 
> - ReadFromCache  -> AsyncCall(&nsHttpChannel::HandleAsyncRedirect)
> - OnStartRequest - ignores the redirect but the mTransactionPump will end up
> in IDLE state

How?  You return a failure?  or how that happens?

> - nsHttpChannel::HandleAsyncRedirect
> - nsHttpChannel::WaitForRedirectCallback - mTransactionPump->Suspend() fails
> because it is idle

It should probably be nullified before (as suggested to cancel and drop it)

> - nsHttpChannel::OnRedirectVerifyCallback - asserts on
> mWaitingForRedirectCallback
> 
> I solved this by ignoring mTransactionPump when the cache wins. I think
> similar problem can happen in case of SuspendInternal/ResumeInternal, but
> I'm not sure if ReadFromCache() could be called when the channel is
> suspended. 

I think it can, according the code at the bottom of that method where we are suspending the pump when the channel is suspended.

> In this case SuspendInternal() might be called when
> mFirstResponseSource is still RESPONSE_PENDING and ResumeInternal() when it
> is already RESPONSE_FROM_CACHE. I guess this shouldn't happen. Honza, am I
> right?

I'm afraid it can happen with the change you are adding.

Comment 17

[Honza Bambas (:mayhemer)]

Comment on attachment 8875274 [details] [diff] [review]
patch v2

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

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +6921,5 @@
>      if (mRaceCacheWithNetwork) {
>          LOG(("  racingNetAndCache - mFirstResponseSource:%d fromCache:%d fromNet:%d\n",
>               mFirstResponseSource, request == mCachePump, request == mTransactionPump));
> +        if (mFirstResponseSource == RESPONSE_PENDING) {
> +            MOZ_ASSERT(request == mTransactionPump);

maybe say why (that you set RESPONSE_CACHE in ReadFromCache (before OnStart can be called by the cache pump)

@@ +8154,2 @@
>          rv = mTransactionPump->Suspend();
>          NS_ENSURE_SUCCESS(rv, rv);

this seems wrong to me.. actually quite a bit, because when we resume, the conditions may already be different and the trans will never resume and I thin won't cancel as well.  I think you should throw the transaction away when reading from cache.

BTW, is the delay between starting the pump (in ReadFromCache) and OnStart from that pump?  I believe the data should already be there according how the cache is written, so that when you say you "cache won" in ReadFromCache it doesn't disadvantage network.

Comment 18

[Michal Novotny [:michal]]

(In reply to Honza Bambas (:mayhemer) from comment #16)
> > I moved setting mFirstResponseSource = RESPONSE_FROM_CACHE from
> > OnStartRequest() to ReadFromCache(). 
> 
> Only when you redirect, right?  (you can behave as if you already knew the
> result of the response, OK.)

I set it always in ReadFromCache now, but the problem was with redirect. It seems some similar problem might be when calling nsHttpChannel::HandleAsyncNotModified too.

> You should probably also cancel the transaction - is there a reason why not
> to?  Apparently you no longer need the network response.  When the redirect
> is vetoed, we then load the cached content, right?

As the comment says, there is a plan to cancel H2 and non keep-alive H1 transactions. Valentin, is the only problem with canceling keep-alive H1 transactions that the connection must be closed?

> > - ReadFromCache  -> AsyncCall(&nsHttpChannel::HandleAsyncRedirect)
> > - OnStartRequest - ignores the redirect but the mTransactionPump will end up
> > in IDLE state
> 
> How?  You return a failure?  or how that happens?

The pump also called ODA and OnStop and ended up in IDLE state.

> > - nsHttpChannel::OnRedirectVerifyCallback - asserts on
> > mWaitingForRedirectCallback
> > 
> > I solved this by ignoring mTransactionPump when the cache wins. I think
> > similar problem can happen in case of SuspendInternal/ResumeInternal, but
> > I'm not sure if ReadFromCache() could be called when the channel is
> > suspended. 
> 
> I think it can, according the code at the bottom of that method where we are
> suspending the pump when the channel is suspended.
> 
> > In this case SuspendInternal() might be called when
> > mFirstResponseSource is still RESPONSE_PENDING and ResumeInternal() when it
> > is already RESPONSE_FROM_CACHE. I guess this shouldn't happen. Honza, am I
> > right?
> 
> I'm afraid it can happen with the change you are adding.

What change do you mean? Change in this patch or RCWN change in general?

Comment 19

[Honza Bambas (:mayhemer)]

(In reply to Michal Novotny (:michal) from comment #18)
> (In reply to Honza Bambas (:mayhemer) from comment #16)
> > > I moved setting mFirstResponseSource = RESPONSE_FROM_CACHE from
> > > OnStartRequest() to ReadFromCache(). 
> > 
> > Only when you redirect, right?  (you can behave as if you already knew the
> > result of the response, OK.)
> 
> I set it always in ReadFromCache now, but the problem was with redirect. 

I didn't look into the patch in this bug before I wrote this comment.

> It
> seems some similar problem might be when calling
> nsHttpChannel::HandleAsyncNotModified too.

Didn't check.

> 
> > You should probably also cancel the transaction - is there a reason why not
> > to?  Apparently you no longer need the network response.  When the redirect
> > is vetoed, we then load the cached content, right?
> 
> As the comment says, there is a plan to cancel H2 and non keep-alive H1
> transactions. Valentin, is the only problem with canceling keep-alive H1
> transactions that the connection must be closed?

Maybe you should discard the response when you are loading from the network and not suspend the transaction when loading from the cache (because it won).

> 
> > > - ReadFromCache  -> AsyncCall(&nsHttpChannel::HandleAsyncRedirect)
> > > - OnStartRequest - ignores the redirect but the mTransactionPump will end up
> > > in IDLE state
> > 
> > How?  You return a failure?  or how that happens?
> 
> The pump also called ODA and OnStop and ended up in IDLE state.

Got it.

> 
> > > - nsHttpChannel::OnRedirectVerifyCallback - asserts on
> > > mWaitingForRedirectCallback
> > > 
> > > I solved this by ignoring mTransactionPump when the cache wins. I think
> > > similar problem can happen in case of SuspendInternal/ResumeInternal, but
> > > I'm not sure if ReadFromCache() could be called when the channel is
> > > suspended. 
> > 
> > I think it can, according the code at the bottom of that method where we are
> > suspending the pump when the channel is suspended.
> > 
> > > In this case SuspendInternal() might be called when
> > > mFirstResponseSource is still RESPONSE_PENDING and ResumeInternal() when it
> > > is already RESPONSE_FROM_CACHE. I guess this shouldn't happen. Honza, am I
> > > right?
> > 
> > I'm afraid it can happen with the change you are adding.
> 
> What change do you mean? Change in this patch or RCWN change in general?

A change in this bug.  Anyway, the feedback on the patch in comment 17 might say more on this.

Comment 20

[Valentin Gosu [:valentin] (he/him)]

(In reply to Michal Novotny (:michal) from comment #18)
> (In reply to Honza Bambas (:mayhemer) from comment #16)
> > You should probably also cancel the transaction - is there a reason why not
> > to?  Apparently you no longer need the network response.  When the redirect
> > is vetoed, we then load the cached content, right?
> 
> As the comment says, there is a plan to cancel H2 and non keep-alive H1
> transactions. Valentin, is the only problem with canceling keep-alive H1
> transactions that the connection must be closed?

Yes, the problem was that the connection is closed for H1 transactions when we cancelled it.

Comment 21

[Michal Novotny [:michal]]

(In reply to Valentin Gosu [:valentin] from comment #20)
> Yes, the problem was that the connection is closed for H1 transactions when
> we cancelled it.

But this might be only performance problem, right?

Comment 22

[Michal Novotny [:michal]]

Attachment: patch v3

Please have a look at the suspend/resume conditions.

(In reply to Honza Bambas (:mayhemer) from comment #17)
> @@ +6921,5 @@
> >      if (mRaceCacheWithNetwork) {
> >          LOG(("  racingNetAndCache - mFirstResponseSource:%d fromCache:%d fromNet:%d\n",
> >               mFirstResponseSource, request == mCachePump, request == mTransactionPump));
> > +        if (mFirstResponseSource == RESPONSE_PENDING) {
> > +            MOZ_ASSERT(request == mTransactionPump);
> 
> maybe say why (that you set RESPONSE_CACHE in ReadFromCache (before OnStart
> can be called by the cache pump)

Comment added.

> @@ +8154,2 @@
> >          rv = mTransactionPump->Suspend();
> >          NS_ENSURE_SUCCESS(rv, rv);
> 
> this seems wrong to me.. actually quite a bit, because when we resume, the
> conditions may already be different and the trans will never resume and I
> thin won't cancel as well.  I think you should throw the transaction away
> when reading from cache.

Why do you think the transaction won't be resumed? I'll file a follow-up bug for closing the transaction. I'd like to land this fix as soon as possible so we can do next round of tests on nightly.
 
> BTW, is the delay between starting the pump (in ReadFromCache) and OnStart
> from that pump?  I believe the data should already be there according how
> the cache is written, so that when you say you "cache won" in ReadFromCache
> it doesn't disadvantage network.

We don't have data to answer this. If the cache was fast enough to win the race when loading metadata, it's probably fast enough to deliver data without a big delay.

Comment 23

[Honza Bambas (:mayhemer)]

(In reply to Michal Novotny (:michal) from comment #22)
> Created attachment 8875752 [details] [diff] [review]
> patch v3
> 
> Please have a look at the suspend/resume conditions.
> 
> (In reply to Honza Bambas (:mayhemer) from comment #17)
> > @@ +6921,5 @@
> > >      if (mRaceCacheWithNetwork) {
> > >          LOG(("  racingNetAndCache - mFirstResponseSource:%d fromCache:%d fromNet:%d\n",
> > >               mFirstResponseSource, request == mCachePump, request == mTransactionPump));
> > > +        if (mFirstResponseSource == RESPONSE_PENDING) {
> > > +            MOZ_ASSERT(request == mTransactionPump);
> > 
> > maybe say why (that you set RESPONSE_CACHE in ReadFromCache (before OnStart
> > can be called by the cache pump)
> 
> Comment added.
> 
> > @@ +8154,2 @@
> > >          rv = mTransactionPump->Suspend();
> > >          NS_ENSURE_SUCCESS(rv, rv);
> > 
> > this seems wrong to me.. actually quite a bit, because when we resume, the
> > conditions may already be different and the trans will never resume and I
> > thin won't cancel as well.  I think you should throw the transaction away
> > when reading from cache.
> 
> Why do you think the transaction won't be resumed? 

Because it's simply possible with the code written as this - the resume condition is not 100% guarantied to be equal when the transaction was suspended.  If you want to land this, please add a new flag that you have suspended the transaction and use it to conditionally resume it.

> I'll file a follow-up bug
> for closing the transaction. I'd like to land this fix as soon as possible
> so we can do next round of tests on nightly.
>  
> > BTW, is the delay between starting the pump (in ReadFromCache) and OnStart
> > from that pump?  I believe the data should already be there according how
> > the cache is written, so that when you say you "cache won" in ReadFromCache
> > it doesn't disadvantage network.
> 
> We don't have data to answer this. If the cache was fast enough to win the
> race when loading metadata, it's probably fast enough to deliver data
> without a big delay.

Let's go with it for now, but would be good to confirm one day.  Maybe file a low priority bug for it blocking the RCWN project so that we don't forget?

Comment 24

[Honza Bambas (:mayhemer)]

Comment on attachment 8875752 [details] [diff] [review]
patch v3

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

R-

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +4883,5 @@
> +                // mTransactionPump was suspended in SuspendInternal, but
> +                // ResumeInternal won't resume it. Do it now, data from the pump
> +                // is ignored when the cache won the race.
> +                mTransactionPump->Resume();
> +            }

I don't understand this and it's very likely completely wrong.

@@ +6929,5 @@
>          LOG(("  racingNetAndCache - mFirstResponseSource:%d fromCache:%d fromNet:%d\n",
>               mFirstResponseSource, request == mCachePump, request == mTransactionPump));
> +        if (mFirstResponseSource == RESPONSE_PENDING) {
> +            // When the cache wins mFirstResponseSource is set to RESPONSE_FROM_CACHE
> +            // earlier in ReadFromCache, so this must be response from the network.

'must be a response'

@@ +8158,5 @@
>      nsresult rv;
>      LOG(("nsHttpChannel::WaitForRedirectCallback [this=%p]\n", this));
>  
> +    if (mTransactionPump &&
> +        !(mRaceCacheWithNetwork && mFirstResponseSource == RESPONSE_FROM_CACHE)) {

after looking into this patch, I'm not sure comment 23 is the way to go either.  I am not sure I fully understand the problem this tries to solve and definitely the implementation is incorrect.

@@ +8744,5 @@
>      }
>  
>      nsresult rvTransaction = NS_OK;
> +    if (mTransactionPump &&
> +        !(mRaceCacheWithNetwork && mFirstResponseSource == RESPONSE_FROM_CACHE)) {

I don't follow this at all.  it's the key change here and there is no explanation for it at all, nowhere.

@@ +8779,5 @@
>      }
>  
>      nsresult rvTransaction = NS_OK;
> +    if (mTransactionPump &&
> +        !(mRaceCacheWithNetwork && mFirstResponseSource == RESPONSE_FROM_CACHE)) {

so, if suspendinternal was called when mFirstResponseSource was still unknown, we don't resume here, regardless of how many times resume internal was called.

this all logic you add breaks equality of the mSuspendCount vs internal suspend counts of both pumps.

Comment 25

[Michal Novotny [:michal]]

Attachment: diff-bug1367742-v4.patch

OK, I finally understand your comments. I didn't study the suspend/resume code properly. I thought the code is smarter and calls suspend/resume on the pumps only when mSuspendCount goes/returns from/to 0.

In this patch, I'm cancelling the transaction when the cache wins, but I'm not sure I did it correctly.

Comment 26

[Honza Bambas (:mayhemer)]

(In reply to Michal Novotny (:michal) from comment #25)
> Created attachment 8877211 [details] [diff] [review]
> diff-bug1367742-v4.patch
> 
> OK, I finally understand your comments. I didn't study the suspend/resume
> code properly. I thought the code is smarter and calls suspend/resume on the
> pumps only when mSuspendCount goes/returns from/to 0.

Oh!  That was the source of the confusion then.  Yes, the code calls down to pumps on every call to Suspend/ResumeInternal.

> 
> In this patch, I'm cancelling the transaction when the cache wins, but I'm
> not sure I did it correctly.

It seems correct.  If you need to fix this immediately, then let's land it (I'll r officially yet).

For a followup bug: the proper way for h1 (whether h1/2 should be hidden from nshttpchannel) is to let the transaction finish and just discard the data.  This is the idea of 'soft closing' a transaction.  To be even more perfectionist the transaction should be hard-canceled when the remaining amount of data is too large and would just waste bandwidth.  The threshold is hard to figure out, so definitely not something to be done for 57 or 58, but definitely something to be filed at least.

Comment 27

[Honza Bambas (:mayhemer)]

Comment on attachment 8877211 [details] [diff] [review]
diff-bug1367742-v4.patch

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

are there try pushes for both rcwn on and off?

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +6942,5 @@
>          LOG(("  racingNetAndCache - mFirstResponseSource:%d fromCache:%d fromNet:%d\n",
>               mFirstResponseSource, request == mCachePump, request == mTransactionPump));
> +        if (mFirstResponseSource == RESPONSE_PENDING) {
> +            // When the cache wins mFirstResponseSource is set to RESPONSE_FROM_CACHE
> +            // earlier in ReadFromCache, so this must be response from the network.

must be a response

@@ +6950,2 @@
>          } else if (WRONG_RACING_RESPONSE_SOURCE(request)) {
>              LOG(("  Early return when racing. This response not needed."));

maybe log the |request| ptr?

Comment 28

[Michal Novotny [:michal]]

Attachment: patch v5

(In reply to Honza Bambas (:mayhemer) from comment #27)
> are there try pushes for both rcwn on and off?

RCWN off: https://treeherder.mozilla.org/#/jobs?repo=try&revision=24f636c76ea7a2b8ba238b3500e0d4d5fa3ba440
RCWN on:  https://treeherder.mozilla.org/#/jobs?repo=try&revision=3d262ca4d1a4a78f7280faeb02531b53b09a8c65

There is a lot of failures, but at the first look most of them are tests expecting cached content. We need to go through the list and disable RCWN for these tests.

> > +            // earlier in ReadFromCache, so this must be response from the network.
> 
> must be a response

fixed
 
> >              LOG(("  Early return when racing. This response not needed."));
> 
> maybe log the |request| ptr?

It's already logged at the beginning of nsHttpChannel::OnStartRequest.

Comment 29

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/914899646cb8
Crash in mozilla::net::nsHttpChannel::ContinueProcessRedirection, r=valentin, r=honzab

Comment 30

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/914899646cb8