1382831 - Race condition in nsHttpChannel::OnCacheEntryCheck when racing cache with network

Status: RESOLVED FIXED

(Core :: Networking: Cache, defect)

Description

[Michal Novotny [:michal]]

When TriggerNetwork is called to send an unconditional request, OnCacheEntryCheck could be called on another thread at the same time and it can add conditional headers before the transaction is set up in ContinueConnect. Conditional request would be sent without having mCacheEntry.

Comment 1

[Michal Novotny [:michal]]

Attachment: diff-bug1382831-v1.patch

AFAICS OnCacheEntryCheck must not be called at the same time as SetupTransaction. It should be safe if OnCacheEntryCheck is called before or after SetupTransaction.

I included few other fixes in this patch:
 - mFirstResponseSource needs to be atomic to avoid data race
 - mOnCacheAvailableCalled doesn't need to be atomic
 - if we don't have the entry in OnNormalCacheEntryAvailable, we need to remove byte range headers if they were added in OnCacheEntryCheck

Comment 2

[Honza Bambas (:mayhemer)]

Comment on attachment 8892907 [details] [diff] [review]
diff-bug1382831-v1.patch

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

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +4404,5 @@
>          return mStatus;
>      }
>  
> +    if (mIgnoreCacheEntry) {
> +        if (!entry) {

we always have an entry here (unless I miss something).  I think you can do with MOZ_ASSERT(entry) here.

@@ +4449,5 @@
>          return NS_OK;
>      }
>  
> +    if (mRaceCacheWithNetwork && ((mCacheEntry && !mCachedContentIsValid &&
> +        (mDidReval || mCachedContentIsPartial)) || mIgnoreCacheEntry)) {

if each statement, by bracing, were on a separate line, this would be readable a bit better.

@@ +4487,5 @@
> +            LOG(("  Removing byte range request headers"));
> +            UntieByteRangeRequest();
> +            mCachedContentIsPartial = false;
> +        }
> +

yep, this was missing!

::: netwerk/protocol/http/nsHttpChannel.h
@@ +691,5 @@
> +        RESPONSE_PENDING      = 0,  // response is pending
> +        RESPONSE_FROM_CACHE   = 1,  // response coming from cache. no network.
> +        RESPONSE_FROM_NETWORK = 2   // response coming from the network
> +    };
> +    Atomic<ResponseSource> mFirstResponseSource;

would only Relaxed do for you?

@@ +713,5 @@
>      Atomic<bool> mRaceCacheWithNetwork;
>      uint32_t mRaceDelay;
>      bool mCacheAsyncOpenCalled;
> +    bool mIgnoreCacheEntry;
> +    mozilla::Mutex mRCWNLock;

comment what's this used for

Comment 3

[Michal Novotny [:michal]]

(In reply to Honza Bambas (:mayhemer) from comment #2)
> ::: netwerk/protocol/http/nsHttpChannel.cpp
> @@ +4404,5 @@
> >          return mStatus;
> >      }
> >  
> > +    if (mIgnoreCacheEntry) {
> > +        if (!entry) {
> 
> we always have an entry here (unless I miss something).  I think you can do
> with MOZ_ASSERT(entry) here.

True, we return an empty new one if we can't find entry on disk. But what about appcache? Does it behaves the same way?
Btw, we don't want to report NotSent also when aNew == true, I will fix this.

> ::: netwerk/protocol/http/nsHttpChannel.h
> @@ +691,5 @@
> > +        RESPONSE_PENDING      = 0,  // response is pending
> > +        RESPONSE_FROM_CACHE   = 1,  // response coming from cache. no network.
> > +        RESPONSE_FROM_NETWORK = 2   // response coming from the network
> > +    };
> > +    Atomic<ResponseSource> mFirstResponseSource;
> 
> would only Relaxed do for you?

yes

Comment 4

[Michal Novotny [:michal]]

Attachment: patch v2 - addressed reviewer's comments

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/dd577e997eb3
Race condition in nsHttpChannel::OnCacheEntryCheck when racing cache with network. r=valentin, r=honzab

Comment 6

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/dd577e997eb3

Comment 7

[Honza Bambas (:mayhemer)]

(In reply to Michal Novotny (:michal) from comment #3)
> (In reply to Honza Bambas (:mayhemer) from comment #2)
> > ::: netwerk/protocol/http/nsHttpChannel.cpp
> > @@ +4404,5 @@
> > >          return mStatus;
> > >      }
> > >  
> > > +    if (mIgnoreCacheEntry) {
> > > +        if (!entry) {
> > 
> > we always have an entry here (unless I miss something).  I think you can do
> > with MOZ_ASSERT(entry) here.
> 
> True, we return an empty new one if we can't find entry on disk. But what
> about appcache? Does it behaves the same way?
> Btw, we don't want to report NotSent also when aNew == true, I will fix this.


Ah!  My mistake!  I missed the method context - here you are in OnCacheEntryAvailable, not Check.  Sorry.  Then this code is perfectly correct.  Appcache behaves the same way, yes.