Closed Bug 1382831 Opened 3 years ago Closed 3 years ago

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

Categories

(Core :: Networking: Cache, defect, major)

defect
Not set
major

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox57 --- fixed

People

(Reporter: michal, Assigned: michal)

References

(Blocks 1 open bug)

Details

(Whiteboard: [necko-active])

Attachments

(1 file, 1 obsolete file)

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.
Attached patch diff-bug1382831-v1.patch (obsolete) — Splinter Review
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
Attachment #8892907 - Flags: review?(valentin.gosu)
Attachment #8892907 - Flags: review?(honzab.moz)
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
Attachment #8892907 - Flags: review?(honzab.moz) → review+
(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
Attachment #8892907 - Flags: review?(valentin.gosu) → review+
Attachment #8892907 - Attachment is obsolete: true
Attachment #8893343 - Flags: review+
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/dd577e997eb3
Race condition in nsHttpChannel::OnCacheEntryCheck when racing cache with network. r=valentin, r=honzab
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/dd577e997eb3
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
(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.
You need to log in before you can comment on or make changes to this bug.