Ensure collisions between simultaneous, identical intercepted requests can't happen

RESOLVED FIXED in Firefox 40

Status

()

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: jdm, Assigned: Ehsan)

Tracking

unspecified
mozilla40
Points:
---

Firefox Tracking Flags

(firefox40 fixed)

Details

Attachments

(2 attachments)

(Reporter)

Description

4 years ago
I believe right now it's possible for two simultaneous requests to the same URL to be intercepted and collide if their response is synthesized. This would probably lead to one of them being treated as a network error when we truncate the cache entry, rather than both of them being synthesized and read independently.

One way to avoid this might be to share a monotonically-increasing counter in nsHttpChannel and append the value stored at channel creation to the cache key, instead of just the common 'u' prefix.
Is there any way we can detect if this happening in the wild?  Its kind of a scary intermittent.

Can we assert that the entry does not currently exist when we create it?
(Assignee)

Comment 2

4 years ago
I'm investigating whether this causes bug 1136780...
Assignee: nobody → ehsan
(Assignee)

Comment 3

4 years ago
This is indeed the root cause.
Blocks: 1136780
(Assignee)

Comment 4

4 years ago
Created attachment 8595685 [details] [diff] [review]
Part 1: Ensure that each channel uses a unique ID to compute its cache entry extension

This makes sure that concurrent synthesized HTTP channels for the same
URL do not try to use the same cache entry accidentally.  We have so far
observed this issue as an intermittent test failure (see bug 1136780),
and it's hard to test this standalone, so enabling that test will serve
as an automated test for this patch as well.
Attachment #8595685 - Flags: review?(mcmanus)
(Assignee)

Comment 5

4 years ago
Created attachment 8595686 [details] [diff] [review]
Part 2: Assert that the cache entry for the intercepted doesn't exist in the cache storage before we try to open it for the first time
Attachment #8595686 - Flags: review?(mcmanus)
Comment on attachment 8595685 [details] [diff] [review]
Part 1: Ensure that each channel uses a unique ID to compute its cache entry extension

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

is there a concern here about inter-session conflicts?
Attachment #8595685 - Flags: review?(mcmanus) → review?(michal.novotny)
Attachment #8595686 - Flags: review?(mcmanus) → review?(michal.novotny)
Comment on attachment 8595685 [details] [diff] [review]
Part 1: Ensure that each channel uses a unique ID to compute its cache entry extension

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

::: netwerk/protocol/http/nsHttpChannel.h
@@ +419,5 @@
>          MAYBE_INTERCEPT,   // interception in progress, but can be cancelled
>          INTERCEPTED,       // a synthesized response has been provided
>      } mInterceptCache;
> +    // Unique ID of this channel for the interception purposes.
> +    const uint64_t mInterceptionID;

If you're going to use a non-atomic, it seems like you should put a thread assertion with a comment in the constructor where this is incremented.
(Assignee)

Comment 8

4 years ago
(In reply to Patrick McManus [:mcmanus] from comment #6)
> Comment on attachment 8595685 [details] [diff] [review]
> Part 1: Ensure that each channel uses a unique ID to compute its cache entry
> extension
> 
> Review of attachment 8595685 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> is there a concern here about inter-session conflicts?

Yes, please see bug 1136780.  That test opens intercepted channels for the same URL from both the main thread and a worker, and sometimes the channel created through the worker script would call CacheStorage::OpenTruncate before the other channel's interception was finished, in which case the cache entry for the main-thread channel would suddenly become doomed.

(In reply to Ben Kelly [:bkelly] from comment #7)
> Comment on attachment 8595685 [details] [diff] [review]
> Part 1: Ensure that each channel uses a unique ID to compute its cache entry
> extension
> 
> Review of attachment 8595685 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: netwerk/protocol/http/nsHttpChannel.h
> @@ +419,5 @@
> >          MAYBE_INTERCEPT,   // interception in progress, but can be cancelled
> >          INTERCEPTED,       // a synthesized response has been provided
> >      } mInterceptCache;
> > +    // Unique ID of this channel for the interception purposes.
> > +    const uint64_t mInterceptionID;
> 
> If you're going to use a non-atomic, it seems like you should put a thread
> assertion with a comment in the constructor where this is incremented.

Channels can only be created from the main thread, so I don't think this is needed.
(Assignee)

Comment 9

4 years ago
ping?
Attachment #8595685 - Flags: review?(michal.novotny) → review+
Attachment #8595686 - Flags: review?(michal.novotny) → review+
https://hg.mozilla.org/mozilla-central/rev/e0f7cbf55fdf
https://hg.mozilla.org/mozilla-central/rev/b0480e99fa38
Status: NEW → RESOLVED
Last Resolved: 4 years ago
status-firefox40: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
You need to log in before you can comment on or make changes to this bug.