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

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect)

Description

[Josh Matthews [:jdm]]

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.

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

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?

Comment 2

[(no longer active)]

I'm investigating whether this causes bug 1136780...

Comment 3

[(no longer active)]

This is indeed the root cause.

Comment 4

[(no longer active)]

Attachment: 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.

Comment 5

[(no longer active)]

Attachment: 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

Comment 6

[Patrick McManus [: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?

Comment 7

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 8

[(no longer active)]

(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.

Comment 9

[(no longer active)]

ping?

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e0f7cbf55fdf
https://hg.mozilla.org/integration/mozilla-inbound/rev/b0480e99fa38

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e0f7cbf55fdf
https://hg.mozilla.org/mozilla-central/rev/b0480e99fa38