Closed Bug 1400001 Opened 7 years ago Closed 7 years ago

Use AwaitingCacheCallbacks when deciding if racing instead of mCacheAsyncOpenCalled

Categories

(Core :: Networking, enhancement, P1)

enhancement

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox58 --- fixed

People

(Reporter: valentin, Assigned: valentin)

References

(Blocks 1 open bug)

Details

(Whiteboard: [necko-active])

Attachments

(1 file)

We have a bug in the RCWN implementation where if AsyncOpenURI fails mCacheAsyncOpenCalled stays true, but mOnCacheAvailableCalled is false, thus leading the code to think that we are racing.

Michal pointed out that we'd be better off using AwaitingCacheCallbacks() since it mostly has the same function
Comment on attachment 8908435 [details]
Bug 1400001 - Use AwaitingCacheCallbacks when deciding if racing instead of mCacheAsyncOpenCalled

https://reviewboard.mozilla.org/r/180076/#review185614

::: netwerk/protocol/http/nsHttpChannel.cpp:3987
(Diff revision 1)
>                  MOZ_ASSERT(NS_IsMainThread(), "Should be called on the main thread");
> -                self->mCacheAsyncOpenCalled = true;
>                  if (self->mNetworkTriggered) {
>                      self->mRaceCacheWithNetwork = true;
>                  }
> -                nsresult rv = cacheStorage->AsyncOpenURI(openURI, extension, cacheEntryOpenFlags, self);
> +                cacheStorage->AsyncOpenURI(openURI, extension, cacheEntryOpenFlags, self);

In case of delayed AsyncOpenURI, AwaitingCacheCallbacks() will return true before AsyncOpenURI is called. Not sure if it is really a problem.
(In reply to Michal Novotny (:michal) from comment #2)
> ::: netwerk/protocol/http/nsHttpChannel.cpp:3987
> (Diff revision 1)
> >                  MOZ_ASSERT(NS_IsMainThread(), "Should be called on the main thread");
> > -                self->mCacheAsyncOpenCalled = true;
> >                  if (self->mNetworkTriggered) {
> >                      self->mRaceCacheWithNetwork = true;
> >                  }
> > -                nsresult rv = cacheStorage->AsyncOpenURI(openURI, extension, cacheEntryOpenFlags, self);
> > +                cacheStorage->AsyncOpenURI(openURI, extension, cacheEntryOpenFlags, self);
> 
> In case of delayed AsyncOpenURI, AwaitingCacheCallbacks() will return true
> before AsyncOpenURI is called. Not sure if it is really a problem.

That should be OK - technically it should have been that way from the beginning, to better simulate how the real cache works.
Comment on attachment 8908435 [details]
Bug 1400001 - Use AwaitingCacheCallbacks when deciding if racing instead of mCacheAsyncOpenCalled

https://reviewboard.mozilla.org/r/180076/#review186024
Attachment #8908435 - Flags: review?(michal.novotny) → review+
Pushed by valentin.gosu@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/1942972c6b53
Use AwaitingCacheCallbacks when deciding if racing instead of mCacheAsyncOpenCalled r=michal
https://hg.mozilla.org/mozilla-central/rev/1942972c6b53
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: