1350359 - Expose alternate data (ex: JS Bytecode) in http cache via fetch() InternalResponse object

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, enhancement, P2)

Description

[Ben Kelly [:bkelly, not reviewing]]

Bug 1231565 added a side-store for alternate cached data like compiled js bytecode.  We should allow c++ consumers of Response to access this via the InternalResponse.  As a first step this would simply connect back to the nsIChannel used in the FetchDriver.

Bug 1336199 would be a later bug to allow storing this same data in Cache API.  That would build on the APIs introduced in this bug.

Comment 1

[Luke Wagner [:luke]]

Additionally, this bug would allow wasm compilation APIs (which compile a Response object) to implicitly cache wasm in alternate data.

Comment 2

[Ben Hsu [:HoPang]]

Attachment: P1: Introduce InternalResponse::mAlternativeBody

Comment 3

[Ben Hsu [:HoPang]]

Attachment: P2: Retrieve the alternative data before the original data if applicale

Comment 4

[Ben Hsu [:HoPang]]

Hi Ben,

After working on the caching alt-data stuff for a while, I think maybe I can start making some actual progress bit by bit.

In this bug, I tried hard to keep the original data and the alternative data matching each other. However, since we must send two requests to Necko to retrieve both the original and alternative data, IMHO, we can merely mitigate the chances of them being mismatched instead of getting rid of it.

With those patched applied, a FetchDriver firstly make a request for the alternative data, since when there is no alternative data, it can still get the original data and then resolve the fetch process. On the other hand, if the alternative data does exist, then the FetchDriver would send another request with (LOAD_FORM_CACHE | LOAD_ONLY_FORM_CACHE) for the original data. However, we could still suffer from the original data and alternative data mismatching with each other when someone modifying/updating the HTTP cache within the time window between the two requests.

Similarly, in order to acquire matched pairs of original data and alternative  data, I save an nsIInputStream in InternalResponse instead of an optional nsICacheInfoChannel mentioned 900784#c66, since I think the longer time window between the two requests sends to Necko, the more chances we would have them mismatched.

If you think this plan is acceptable, I would check FetchDriver::HttpFetch() more carefully (I am deeply afraid of any undesired side effects, since this method would be entered twice), and write a testcase for it.

There is another important question is if there is a way to verify whether an original data is matched to an alternative data. If so, the implementation will be more stable.

And the most important of all, thanks as always.

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

Hmm, this is pretty different than I was thinking.  I was originally think we would have something like:

  nsICacheInfoChannel*
  InternalResponse::GetCacheInfoChannel() const
  {
    return mCacheInfoChannel;
  }

The fetch code itself would not try to read from the nsICacheInfoChannel at all.  It would simply expose the interface if a gecko consumer of InternalResponse wants to use it.

I guess I see now that nsICacheInfoChannel does not work the way I expected it to.  I thought it provided its own stream of data, but it does not.  It just changes how the main nsIChannel works.

What if we did something like this:

1. Add a string attribute to InternalRequest like mPreferAlternateDataType.
2. Script created Request objects would get mPreferAlternateDataType set to the empty string.
3. Gecko internal code could set the InternalRequest mPreferAlternateDataType to some value with a c++ API.
4. FetchDriver simply calls PreferAlternateDataType() if mPreferAlternateDataType is not empty.

Then in a later bug we can add:

5. Add a way to ask an nsIChannel if it is preferring alternate data type and what its string is.
6. When the FetchEvent.request is created from an intercepted channel we would set mPreferAlternateDataType based on if the channel is preferring an alternate data type.

I guess the problem with this is the ServiceWorker script could pass the FetchEvent.request through to fetch(), get the alternate data stream, and then inspect it instead of passing it through to respondWith().

I'm sorry, but I think I need to think about this some more.

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

I guess maybe we could make FetchDriver:

a. By default only request the main data.
b. If mPreferredDataType is set then we would request both the main data and the alternate data.

This would allow alternate data use for pass-through SW requests.  If a SW created a completely separate Request, though, then it would not get the alternate data source optimization.

If mPreferredDataType is set we would provide access to the output stream to write alternate data.

To deal with the issue where the http cache entry is changed, maybe there is some way to determine if the resulting main data source is from the same cache entry as the alternate source.  If they don't match, then we just throw away the alternate data and keep the main data.  Maybe this could be done by comparing the cacheKey and expiration time in nsICacheInfoChannel?

What do you think?

Comment 7

[Ben Kelly [:bkelly, not reviewing]]

Honza offered to add an integer identifier to nsICacheInfoChannel so we can tell if two nsIChannel objects are pulling from the same cache entry.  This would let us discard the alternate data if we detect the main data is from a different entry.

Comment 8

[Ben Kelly [:bkelly, not reviewing]]

Actually, Honza is leaving for PTO shortly.  HoPang do you think you could do buy 1395202 as well?  Valentin or Michal could probably help there if you have questions.

Comment 9

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8902646 [details] [diff] [review]
P2: Retrieve the alternative data before the original data if applicale

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

::: dom/fetch/FetchDriver.cpp
@@ +402,5 @@
>  
> +  if (mFetchingDataType == eAlternativeData) {
> +    nsCOMPtr<nsICacheInfoChannel> cic = do_QueryInterface(chan);
> +    if (cic && nsContentUtils::IsBytecodeCacheEnabled()) {
> +      cic->PreferAlternativeDataType(nsContentUtils::JSBytecodeMimeType());

Ideally I think we should let the consumer of InternalResponse pass in the type string instead of hard coding this.  I believe we are already considering storing wasm alternate data streams, etc.  We will want to support that.

Comment 10

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8902644 [details] [diff] [review]
P1: Introduce InternalResponse::mAlternativeBody

Dropping the flags here since I commented separately in the bug.  Overall I agree we need to read both streams, but I think we need to pass the type through the InternalRequest.

Comment 11

[Ben Hsu [:HoPang]]

(In reply to Ben Kelly [:bkelly] from comment #6)
> I guess maybe we could make FetchDriver:
> 
> a. By default only request the main data.
> b. If mPreferredDataType is set then we would request both the main data and
> the alternate data.

Totally agreed, but we should be careful when deciding whether to request the alternative data. At this moment, I can only think of using the source URL as the decider, which should be treated with extra care under certain cases such as ".py" is used for script resources in wpt tests.

> This would allow alternate data use for pass-through SW requests.  If a SW
> created a completely separate Request, though, then it would not get the
> alternate data source optimization.

Agreed.

> If mPreferredDataType is set we would provide access to the output stream to
> write alternate data.

Sorry, I don't quite get where and why we have to write anything. I thought the thing needed to do in this bug is creating a pipe for the data pumped out from the nsIChannel for the alternative data.

> To deal with the issue where the http cache entry is changed, maybe there is
> some way to determine if the resulting main data source is from the same
> cache entry as the alternate source.  If they don't match, then we just
> throw away the alternate data and keep the main data.  Maybe this could be
> done by comparing the cacheKey and expiration time in nsICacheInfoChannel?

Yes, I think bug 1395202 is absolutely worth doing. Let me start from it ;P

Comment 12

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Ben Hsu [:HoPang] from comment #11)
> (In reply to Ben Kelly [:bkelly] from comment #6)
> > a. By default only request the main data.
> > b. If mPreferredDataType is set then we would request both the main data and
> > the alternate data.
> 
> Totally agreed, but we should be careful when deciding whether to request
> the alternative data. At this moment, I can only think of using the source
> URL as the decider, which should be treated with extra care under certain
> cases such as ".py" is used for script resources in wpt tests.

I don't think the URL is a good discriminator here.  The place in the code that creates the nsIChannel or InternalRequest is the best place.  So for example:

1. Main thread ScriptLoader would set the JS bytecode mimetype as the preferred alternate data source because it knows it can consume it.
2. We propagate that to the FetchEvent.request's InternalRequest.
3. If the SW script does a pass-through fetch(evt.request) or a cache.match(evt.request) then we try to load both the altnerate and main streams in the InternalResponse.
4. If script accesses the body via Response API then it gets the main data stream.  If its synthesized on nsIChannel then it gets the alternate data stream.  If it goes to Cache API then we write both streams to disk.
 
> > If mPreferredDataType is set we would provide access to the output stream to
> > write alternate data.
> 
> Sorry, I don't quite get where and why we have to write anything. I thought
> the thing needed to do in this bug is creating a pipe for the data pumped
> out from the nsIChannel for the alternative data.

So, reading the alternate data stream is only half the problem.  The other side of it is populating the alternate data in http cache, Cache API, etc.  This works today like:

1. Main thread ScriptLoader creates an nsIChannel and marks that it prefers the JS bytecode mimetype.
2. Let's say there is no bytecode and the nsIChannel provides the main data instead
3. ScriptLoader compiles the main data stream as js.
4. ScriptLoader then writes the byte code back to the nsIOutputStream provided by nsICacheInfoChannel.openAlternativeOutputStream.

So we need to be able to hold on to the nsICacheInfoChannel object in our InternalResponse and proxy GetOpenAlternativeOutputStream() to get the output stream to write to the http cache.  Or provide a different implementation that writes to the Cache API.

Does that help clarify anything?

Comment 13

[Ben Hsu [:HoPang]]

Sorry for the late reply, since I am not feeling well recently, and I need some more time to understand how things work and how they should work in the future :(

The comment does help a lot. It seems that I was somehow overly focused on scenarios like a service worker tries to fetch and store all the resources which including scripts on events like `active`.

To make sure that I am on the right track, I'd like to try to summarize the plan here which should include all the content in the following table. 

| TODO                                                   | ScriptLoader Requests | Other Requests |
| ------------------------------------------------------ | ----------------------| -------------- |
| Retrieve the alternative data from HTTP Cache          | 1350359               | Follow-up      |
| Update the alternative data to HTTP Cache              | Follow-up             | Follow-up      |
| Save the alternative data from HTTP Cache to DOM Cache | 1336199               | Follow-up      |
| Retrieve the alternative data from DOM Cache           | 1336199               | Follow-up      |
| Update the alternative data to DOM Cache               | Follow-up             | Follow-up      |

Note: When updating DOM cache, we shouldn't update the HTTP cache, since the main data in HTTP Cache might no longer remains the same as the one cached in the DOM cache.

At this moment, I think I should work on 1350359 and 1336199 first, since they can stop performance regression from ScriptLoaders not using alternative data when being intercepted by service worker, and I currently have WIP patches for both of them. Besides, we can do the updating stuff together. 

To tackle this bug, I'd like to address your comment, which includes (1) creating InternalRequest::mPreferredAlternativeDataType and (2) making FetchDrivers fetch both the data if needed (Depends on whether the request is coming from ScriptLoader). For (2), `HTTPFetch` will still could be entered twice as in the WIP patch. However, I suggest focusing "retrieving the alternative data from HTTP Cache" part in this bug, and thus I think making InternalRequests holding the nsICacheInfoChannel could be done in a following bug. There is one more thing to be noted, FetchDrivers would only fetch one type of alternative data, which means that the code should be updated if we do want to fetch both JIT code and WASM.

Comment 14

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Ben Hsu [:HoPang] from comment #13)
> To tackle this bug, I'd like to address your comment, which includes (1)
> creating InternalRequest::mPreferredAlternativeDataType and (2) making
> FetchDrivers fetch both the data if needed (Depends on whether the request
> is coming from ScriptLoader). For (2), `HTTPFetch` will still could be
> entered twice as in the WIP patch. However, I suggest focusing "retrieving
> the alternative data from HTTP Cache" part in this bug, and thus I think
> making InternalRequests holding the nsICacheInfoChannel could be done in a
> following bug. There is one more thing to be noted, FetchDrivers would only
> fetch one type of alternative data, which means that the code should be
> updated if we do want to fetch both JIT code and WASM.

Hmm, I wasn't suggesting that InternalRequest should hold a ref to the nsICacheInfoChannel.  Only that it should have a string containing the prefered alternate data mime type.  Keeping this on the request lets us:

1. Know what type of alternate data to try to load.
2. Avoid the work of trying to load any alternate data if the type is not set. (Which will be often.)

I don't think we should put this off to a follow-up bug.  I think its an essential part of reading the alternate data via the fetch API primitives.

Comment 15

[Ben Hsu [:HoPang]]

> Hmm, I wasn't suggesting that InternalRequest should hold a ref to the
> nsICacheInfoChannel.  Only that it should have a string containing the
> prefered alternate data mime type.  Keeping this on the request lets us:

My bad, I meant making InternalResponse hold a ref of nsICacheInfoChannel here.

Comment 16

[Ben Hsu [:HoPang]]

Just a status update,

Previously, I was stuck in various issues such as crashing other testcases and intermittent failures. At this moment, I think I've work around/fix them, and thus I am polishing the patches now.

Comment 17

[Ben Hsu [:HoPang]]

Attachment: P1: Set alternative data type from InterceptedChannel to InternalRequest

Comment 18

[Ben Hsu [:HoPang]]

Attachment: P2: Fetch and save alterntative data to InternalResponse

Comment 19

[Ben Hsu [:HoPang]]

Attachment: P3.1: Place the alternative data to the InterceptedChannel

Comment 20

[Ben Hsu [:HoPang]]

Attachment: P3.2. Fix a crash caused by off-main-thread destruction of a HttpChannelChild

Comment 21

[Ben Hsu [:HoPang]]

Attachment: P4.1: Steal test_script_loader_js_cache.html as the base of the

testcase. r=bkelly

Comment 22

[Ben Hsu [:HoPang]]

Attachment: P4.2: Interpolate the service worker which performs pass-through fetch

r=bkelly

Comment 23

[Ben Hsu [:HoPang]]

Attachment: P4.3: Place all the tests into a single promise_test() clause

Comment 24

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8918150 [details] [diff] [review]
P4.1: Steal test_script_loader_js_cache.html as the base of the

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

::: dom/workers/test/serviceworkers/test_script_loader_intercepted_js_cache.html
@@ +43,5 @@
> +        "scriptloader_execute": "bytecode_exec"
> +      }
> +    };
> +
> +    function flushNeckoCache() {

nit: Note, this function got removed recently, as well as all the calls to it from the dom/base test case, because it was somehow leaking the CacheIOThread.

Comment 25

[Ben Hsu [:HoPang]]

(In reply to Nicolas B. Pierron [:nbp] from comment #24)
> Comment on attachment 8918150 [details] [diff] [review]
> P4.1: Steal test_script_loader_js_cache.html as the base of the
> 
> Review of attachment 8918150 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> :::
> dom/workers/test/serviceworkers/test_script_loader_intercepted_js_cache.html
> @@ +43,5 @@
> > +        "scriptloader_execute": "bytecode_exec"
> > +      }
> > +    };
> > +
> > +    function flushNeckoCache() {
> 
> nit: Note, this function got removed recently, as well as all the calls to
> it from the dom/base test case, because it was somehow leaking the
> CacheIOThread.

Thanks!

Comment 26

[Ben Hsu [:HoPang]]

Per offline discussion, I'd like hand these patches over to Eden :)

Comment 27

[Eden Chuang[:edenchuang]]

Thanks, I will continue your work.

Comment 28

[Eden Chuang[:edenchuang]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1d2b181c45727890126aacef3a13aa855a4328a6

Comment 29

[Eden Chuang[:edenchuang]]

Attachment: P1: Set alternative data type from InterceptedChannel to InternalRequest

rebase repository for the patch.

Comment 30

[Eden Chuang[:edenchuang]]

Attachment: P1: Set alternative data type from InterceptedChannel to InternalRequest. r?bkelly

P1: Set alternative data type from InterceptedChannel to InternalRequest.

Adding a new member mPreferredAlternativeDataType in InternalRequest.
Setting up mPreferredAlternativeDataType from InterceptedChannel while creating InternalRequest.

Comment 31

[Eden Chuang[:edenchuang]]

Attachment: P2: Fetch and save alternative data to InternalResponse. r?bkelly

P2: Fetch and save alternative data to InternalResponse.

Adding new members mCacheInfoChannel and mAlternativeBody in InternalResponse.
Adding new members mCurrentFetchingDataType and mAlternativeDataCacheEntryId in FetchDriver.
When FetchDriver::Fetch is called, if InternalRequest::mPreferredAlternativeDataType is not empty, we try to read data from Http catch in the first HttpFetch, then we set the second HttpFetch to save cached data into InternalResponse::mAlternativeBody and also read data from network and save it into InternalResponse.

Comment 32

[Eden Chuang[:edenchuang]]

Attachment: P3.1: Place the alternative data to the InterceptedChannel. r?bkelly

P3.1: Place the alternative data to the InterceptedChannel.

When RespondWithHandler::ResolvedCallback is called, we setup StratResponse with InternalResponse's alternative body and cacheInfoChannel, if InternalResponse's alternative body is not null, otherwise we use InternalResponse's unfiltered body.

Comment 33

[Eden Chuang[:edenchuang]]

Attachment: P3.2: Fix a crash caused by off-main-thread destruction of a HttpChannelChild. r?bkelly

P3.2: Fix a crash caused by off-main-thread destruction of a HttpChannelChild

Comment 34

[Eden Chuang[:edenchuang]]

Attachment: P4: mochitest for exposing alternate data in http cache via fetch() InternalResponse object. r?bkelly

Comment 35

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8927697 [details] [diff] [review]
P1: Set alternative data type from InterceptedChannel to InternalRequest. r?bkelly

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

::: dom/fetch/InternalRequest.h
@@ +545,5 @@
> +    return mPreferredAlternativeDataType;
> +  }
> +
> +  void
> +  SetPreferredAlternativeDataType(const nsCString& aDataType)

const nsACString&

::: dom/workers/ServiceWorkerPrivate.cpp
@@ +1612,5 @@
> +
> +    nsAutoCString alternativeDataType;
> +    nsCOMPtr<nsICacheInfoChannel> cic = do_QueryInterface(channel);
> +    if (cic &&
> +        !NS_FAILED(cic->GetPreferredAlternativeDataType(alternativeDataType)) &&

I think its more typical to write NS_SUCCEEDED() instead of !NS_FAILED().

Comment 36

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8927707 [details] [diff] [review]
P2: Fetch and save alternative data to InternalResponse. r?bkelly

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

This looks good, but I'd like to review it again with the comments addressed.  Thanks!

::: dom/fetch/FetchDriver.cpp
@@ +501,5 @@
>    // channel will call in with an errored OnStartRequest().
>  
> +  if (!mCurrentFetchingDataType.IsEmpty()) {
> +    nsAutoCString alternativeDataType;
> +    nsCOMPtr<nsICacheInfoChannel> cic = do_QueryInterface(aRequest);

A comment about what we are doing in this case would be helpful.  Something describing that we are trying to get the alternate data stream first and will load the non-alternate stream afterwards.  We then verify that cache ID is the same between the two loads.

@@ +504,5 @@
> +    nsAutoCString alternativeDataType;
> +    nsCOMPtr<nsICacheInfoChannel> cic = do_QueryInterface(aRequest);
> +    if (cic &&
> +        NS_SUCCEEDED(cic->GetAlternativeDataType(alternativeDataType)) &&
> +        mCurrentFetchingDataType.Equals(alternativeDataType) &&

Also, maybe note in a comment that we are checking to see if we actually got the alternate data stream here.  I got confused on the difference between PreferedAlternativeDataType and AlternativeDataType on my first read through here.

@@ +530,5 @@
> +    }
> +
> +    // Fallback to the main data, since the alternative data doesn't exist.
> +    MOZ_DIAGNOSTIC_ASSERT(alternativeDataType.IsEmpty());
> +    mCurrentFetchingDataType = EmptyCString();

Maybe also clear mAlternativeDataCacheEntryId?

@@ +626,5 @@
> +  response->SetCacheInfoChannel(cic);
> +
> +  uint64_t cacheEntryId = 0;
> +  if (cic && NS_SUCCEEDED(cic->GetCacheEntryId(&cacheEntryId)) &&
> +      cacheEntryId == mAlternativeDataCacheEntryId) {

Can we add an assertion that mCurrentFetchDataType is empty here?  If I understand correctly this is the second fetch where we are trying to read the non-alternative body after getting the alternate data stream already, correct?  A comment to that effect would be helpful, I think.

@@ +628,5 @@
> +  uint64_t cacheEntryId = 0;
> +  if (cic && NS_SUCCEEDED(cic->GetCacheEntryId(&cacheEntryId)) &&
> +      cacheEntryId == mAlternativeDataCacheEntryId) {
> +    response->SetAlternativeBody(mPipeAlternativeInputStream);
> +    response->SetCacheInfoChannel(mCacheInfoChannel);

Also, can we assert these two are non-null here?

@@ +629,5 @@
> +  if (cic && NS_SUCCEEDED(cic->GetCacheEntryId(&cacheEntryId)) &&
> +      cacheEntryId == mAlternativeDataCacheEntryId) {
> +    response->SetAlternativeBody(mPipeAlternativeInputStream);
> +    response->SetCacheInfoChannel(mCacheInfoChannel);
> +  }

Can we clear the alternate piped data here immediately if the cache ID does not match?  It may be quite large so we'd like to avoid storing it in memory longer than necessary.

We should also be able to drop the output stream side of the pipe in both conditions as well.

@@ +806,5 @@
>    // NB: This can be called on any thread!  But we're guaranteed that it is
>    // called between OnStartRequest and OnStopRequest, so we don't need to worry
>    // about races.
>  
> +  if (!mCurrentFetchingDataType.IsEmpty()) {

Please add a comment that we are receiving the alternate data stream here.

@@ +870,5 @@
> +      mPipeAlternativeInputStream = nullptr;
> +    }
> +
> +    // Fetch the main data.
> +    if (NS_FAILED(HttpFetch(EmptyCString()))) {

I think we need comments here and in OnStartRequest() that the listener callbacks can be re-started.  Most nsIStreamListener implementations don't do that so we should note the difference.

::: dom/fetch/FetchDriver.h
@@ +162,5 @@
>    FetchDriver(const FetchDriver&) = delete;
>    FetchDriver& operator=(const FetchDriver&) = delete;
>    ~FetchDriver();
>  
> +  nsresult HttpFetch(const nsCString& aPreferredAlternativeDataType);

const nsACString&

::: dom/fetch/InternalResponse.h
@@ +258,5 @@
> +    mAlternativeBody = aAlternativeBody;
> +  }
> +
> +  already_AddRefed<nsIInputStream>
> +  GetAlternativeBody()

It might be nice to make this more like TakeAlternativeBody() and take the reference to the pipe.  We want to let this data clear out as soon as possible.  If we know that taking this alternate data will mean we don't need the real body, maybe we should clear the body stream as well.

Ensuring the body streams are cleared promptly could be done in a follow-up patch if you don't want to rebase the current patch queue to change it here.

Comment 37

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8927713 [details] [diff] [review]
P3.1: Place the alternative data to the InterceptedChannel. r?bkelly

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

Overall looks good, but we need to handle the synthetic cache info in InterceptedHttpChannel as well.  The means making InterceptedHttpChannel implement nsICacheInfoChannel since it doesn't currently provide that interface.

::: dom/workers/ServiceWorkerEvents.cpp
@@ +339,5 @@
>  
>      auto castLoadInfo = static_cast<LoadInfo*>(loadInfo.get());
>      castLoadInfo->SynthesizeServiceWorkerTainting(mInternalResponse->GetTainting());
>  
> +    nsCOMPtr<nsIInputStream> body = mInternalResponse->GetAlternativeBody();

Can we test that the outer channel has a preferred alternative data type set matching our mInternalResponse cache control's actual alternative data type?  We want to be careful not to return the wrong type of alternate data type here.

In theory this could happen if we have overlapping FetchEvents and the code purposely tries to pass a Response created with a Request from one to the other FetchEvent.  No real good reason to do this, but its an attack we should protect against.

Also, in theory we could (should?) store the alternate data information on Request objects stored in Cache API.  That can be a follow-up bug, though.  Its not clear if there is a good use case that would make it worth going through the hassle to add it there.

@@ +695,2 @@
>  
> +  nsCOMPtr<nsIInputStream> body = ir->GetAlternativeBody();

I don't think we actually need to touch the alternate body here.  We are just getting the body to detect if a body exists or not.

::: netwerk/protocol/http/HttpChannelChild.h
@@ +302,5 @@
>    int32_t      mCacheFetchCount;
>    uint32_t     mCacheExpirationTime;
>    nsCString    mCachedCharset;
>  
> +  nsCOMPtr<nsICacheInfoChannel> mCacheInfo;

Maybe name this to make it clear the cache info has been synthesized.  Something like mSynthesizedCacheInfo?

::: netwerk/protocol/http/InterceptedHttpChannel.cpp
@@ +717,5 @@
>  NS_IMETHODIMP
>  InterceptedHttpChannel::StartSynthesizedResponse(nsIInputStream* aBody,
>                                                   nsIInterceptedBodyCallback* aBodyCallback,
> +                                                 const nsACString& aFinalURLSpec,
> +                                                 nsICacheInfoChannel* aCacheInfoChannel)

We need to do the synthesized nsICacheInfoChannel stuff in IntercepedHttpChannel as well.  Otherwise this won't work right in non-e10s.

Comment 38

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8927714 [details] [diff] [review]
P3.2: Fix a crash caused by off-main-thread destruction of a HttpChannelChild. r?bkelly

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

Can you describe the crash you ran into a little bit more?  I don't understand why this patch is better than the code it replaces.  They seem roughly equivalent to me.

Comment 39

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8927715 [details] [diff] [review]
P4: mochitest for exposing alternate data in http cache via fetch() InternalResponse object. r?bkelly

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

I only skimmed the test.  It looks like you are reusing infrastructure for nbp's testing code, so I'm mostly trusting that works.  The main issue I think you need to fix is waiting for the SW to become active to ensure it always handles the FetchEvents.

::: dom/workers/test/serviceworkers/fetch.js
@@ +3,5 @@
>      event.respondWith(fetch("hello.html", {"integrity": "abc"}));
>    } else if (event.request.url.indexOf("fake.html") !== -1) {
>      event.respondWith(fetch("hello.html"));
>    }
> +  event.respondWith(fetch(event.request));

Other tests use this service worker.  Are you sure this does not change those other tests in a meaningful way?

::: dom/workers/test/serviceworkers/test_script_loader_intercepted_js_cache.html
@@ +123,5 @@
> +      ]});
> +
> +      // Register the service worker that perform the pass-through fetch.
> +      var registration = await navigator.serviceWorker
> +                                        .register("fetch.js", {scope: "./"});

I think you need to wait for the service worker to become active here.  Right now its possible you start running the tests before the SW is ready to handle FetchEvents.

@@ +127,5 @@
> +                                        .register("fetch.js", {scope: "./"});
> +
> +      await testCheckTheJSBytecodeCache();
> +      await testSavebytecodeAfterTheInitializationOfThePage();
> +      await testDoNotSaveBytecodeOnCompilationErrors();

Is there any way to test the case where the cache ID changes during the fetch()?  I can't think of a good way to force an http cache eviction during that small time window.

@@ +158,5 @@
> +      assert_equals(await stateMachineResult, "fallback_bytecode_saved",
> +                    "[3] ScriptLoadRequest status after the SRI hash");
> +
> +      // Loading a page, which has the same SRI should verify the SRI and
> +      // continue by executing the bytecode.

Nice test case!  I would not have thought of checking that and its definitely something that could break.

Comment 40

[Ben Kelly [:bkelly, not reviewing]]

One last thought I had while trying to go to sleep:

Can you try to use a separate nsIStreamLiatener to load the alternate data stream?  This would avoid the weirdness with restarting the main FetchDriver listener.

Another big advantage would be letting the main body load start earlier.  We should be able to start the main stream load as soon as we get OnStartRequest() for the alternate data stream.  This would allow us to resolve the response to caller without always having to buffer the entire alternate stream.

I'm just thinking of how well this will work for huge 50+ MB wasm resources.

Does that make sense?  Or is there something in the nsICacheInfoChannel code that would prevent it?

Comment 41

[Eden Chuang[:edenchuang]]

(In reply to Ben Kelly [:bkelly] from comment #40)
> One last thought I had while trying to go to sleep:
> 
> Can you try to use a separate nsIStreamLiatener to load the alternate data
> stream?  This would avoid the weirdness with restarting the main FetchDriver
> listener.
> 
> Another big advantage would be letting the main body load start earlier.  We
> should be able to start the main stream load as soon as we get
> OnStartRequest() for the alternate data stream.  This would allow us to
> resolve the response to caller without always having to buffer the entire
> alternate stream.
> 
> I'm just thinking of how well this will work for huge 50+ MB wasm resources.
> 
> Does that make sense?  Or is there something in the nsICacheInfoChannel code
> that would prevent it?

That's a good improvement to do, but I need time to check if there are any issues with nsICacheInfoChannel.
How about creating a follow bug for this improvement? At least, we can keep current implementation once we found issues.

Comment 42

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Eden Chuang[:edenchuang] from comment #41)
> That's a good improvement to do, but I need time to check if there are any
> issues with nsICacheInfoChannel.
> How about creating a follow bug for this improvement? At least, we can keep
> current implementation once we found issues.

Normally I would say this could be a follow-up bug, but I worry that blocking until the entire alternate body is read into memory might actually be a de-optimization.  We've avoided having to parse js, but we are delaying when the script loader can start working on it.

My guess is that there should no problem doing overlapping loads like this.  Our http cache almost assuredly must support this because different pages can be loading at the same time today.

Could we try doing the overlapping loads as an additional patch in this bug for now?  If it turns out to be quite difficult we can maybe punt it to another bug then.  Does that sound reasonable?

Comment 43

[Ben Kelly [:bkelly, not reviewing]]

Just some other random thoughts about how to maybe implement the overlapping loads:

1. The extra nsIStreamLoader could own the alternate data pipe input and output streams.  The alternate nsIStreamLoader can then set the pipe input stream on the FetchDriver once it knows its going to keep the alternate data.
2. If the nsIChannel doesn't actually produce alternate data then the alternate nsIStreamLoader can just forward OnStartRequest(), OnDataAvailable(), and OnStopRequest() to the main FetchDriver listener.

Anyway, I'm very sorry to throw this complication in after you've been working on this for a while.  I'm just worried about implementing something like this bug as an optimization and actually having it result in a net-negative change for some workloads.

Comment 44

[Eden Chuang[:edenchuang]]

(In reply to Ben Kelly [:bkelly] from comment #42)
> (In reply to Eden Chuang[:edenchuang] from comment #41)
> > That's a good improvement to do, but I need time to check if there are any
> > issues with nsICacheInfoChannel.
> > How about creating a follow bug for this improvement? At least, we can keep
> > current implementation once we found issues.
> 
> Normally I would say this could be a follow-up bug, but I worry that
> blocking until the entire alternate body is read into memory might actually
> be a de-optimization.  We've avoided having to parse js, but we are delaying
> when the script loader can start working on it.
> 
> My guess is that there should no problem doing overlapping loads like this. 
> Our http cache almost assuredly must support this because different pages
> can be loading at the same time today.
> 
> Could we try doing the overlapping loads as an additional patch in this bug
> for now?  If it turns out to be quite difficult we can maybe punt it to
> another bug then.  Does that sound reasonable?

Of course, we can do the improvement in this bug.
I would like to complete current patches implementation first, then implement this improvement in the other patch P5.

Comment 45

[Eden Chuang[:edenchuang]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5d2f1cec91468ddd7e365a45edc6f0b1c6dd2161

Comment 46

[Eden Chuang[:edenchuang]]

Attachment: P1: Set alternative data type from InterceptedChannel to InternalRequest. r=bkelly

Update the patch according to comment 35.

Comment 47

[Eden Chuang[:edenchuang]]

Attachment: P2: Fetch and save alternative data to InternalResponse. r?bkelly

Instead of keeping whole alternative data in memory, this patch implements the overlap loading mechanism for alternative data and main data by using a new class AlternativeDataStreamListener to listen on the alternative data channel. AlternativeDataStreamListener cooperates with FetchDriver for following situations
    1. There is no preferred alternative data type in InternalRequest
       Directly using FetchDriver to listen on the opened channel              
    2. If preferred alternative data type exists in InternalRequest, but no saved data in the cache.                                                    
      AlternativeDataStreamListener is constructed to listen on the channel, but its status would be set as FALLBACK and redirect callbacks to FetchDriver.
    3. If preferred alternative data type exists in InternalRequest, and the data also exists in the cache.                                          
       AlternativeDataStreamListener is constructed to listen on the channel for loading the alternative data. And also open a channel listened by FetchDriver for loading the main data when AlternativeDataStreamListener::OnStartRequest is called.                                               
      If the cacheEntryId is different between the main data channel and the alternative data channel, we will cancel the alternative data loading.

Comment 48

[Eden Chuang[:edenchuang]]

Attachment: P3.1: Place the alternative data to the InterceptedChannel. r?bkelly

Update the patch according to comment 37. And also implement nsIcacheInfoChannel interface on InterceptedHttpChannel.

Comment 49

[Eden Chuang[:edenchuang]]

Attachment: P4: mochitest for exposing alternate data in http cache via fetch() InternalResponse object. r=bkelly

Update the patch according to comment 39.

Comment 50

[Eden Chuang[:edenchuang]]

(In reply to Ben Kelly [:bkelly] from comment #38)
> Comment on attachment 8927714 [details] [diff] [review]
> P3.2: Fix a crash caused by off-main-thread destruction of a
> HttpChannelChild. r?bkelly
> 
> Review of attachment 8927714 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Can you describe the crash you ran into a little bit more?  I don't
> understand why this patch is better than the code it replaces.  They seem
> roughly equivalent to me.

This patch is fixing the assertion crash on the wpt-test /service-workers/service-worker/clients-get-client-types.https.html with a try run, and following is the fail log.

https://treeherder.mozilla.org/logviewer.html#?job_id=144558670&repo=try&lineNumber=29664

According to the error message on the fail log, It complains InterceptStreamListener is not a threadsafe while adding InterceptStreamListener into arrayToRelease in HttpChannelChild's destructor. 
I tried to let InterceptStreamListener be threadsafe by replacing NS_DECL_ISUPPORTS with NS_DECL_THREADSAFE_ISUPPORTS, it does not crash on /service-workers/service-worker/clients-get-client-types.https.html, but it introduces leaks with InterceptStreamListener on other tests. The leaks make me consider if InterceptStreamListener could be threadsafe or not? I need more time to figure it out.

I think this patch is a workaround that separating the InterceptStreamListener destruction from arrayToRelease, but just like you mentioned they are doing the same thing.

I guess the reason that why we did not meet the crash before applying this implementation is related to creating and using two channels at the same time. And the channels' life cycles are a bit different with the original usage.

Comment 51

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8931432 [details] [diff] [review]
P2: Fetch and save alternative data to InternalResponse. r?bkelly

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

Overall this looks really good.  Thanks!

I am a bit worried about the ref-cycle here, though.  Can you make the requested changes and re-flag for review?  I just want to make sure we avoid leaks when we follow a non-standard path through the code.  Thanks again.

::: dom/fetch/FetchDriver.cpp
@@ +88,5 @@
> +
> +private:
> +  ~AlternativeDataStreamListener() = default;
> +
> +  RefPtr<FetchDriver> mFetchDriver;

This creates a strong reference cycle with FetchDriver and its mAlDataListener field.  This means we have to be extra careful that at least one of these references is cleared.  Please add a comment about the ref-cycle.

@@ +95,5 @@
> +  nsCOMPtr<nsIOutputStream> mPipeAlternativeOutputStream;
> +  uint64_t mAlternativeDataCacheEntryId;
> +  nsCOMPtr<nsICacheInfoChannel> mCacheInfoChannel;
> +  nsCOMPtr<nsIChannel> mChannel;
> +  eStatus mStatus;

Please make this Atomic<eStatus> since its accessed on a separate thread in OnDataAvailable().

@@ +112,5 @@
> +  , mChannel(aChannel)
> +  , mStatus(AlternativeDataStreamListener::LOADING)
> +{
> +  MOZ_ASSERT(mFetchDriver);
> +  MOZ_ASSERT(mChannel);

MOZ_DIAGNOSTIC_ASSERT() for these.

@@ +237,5 @@
> +AlternativeDataStreamListener::OnStopRequest(nsIRequest* aRequest,
> +                                             nsISupports* aContext,
> +                                             nsresult aStatusCode)
> +{
> +  workers::AssertIsOnMainThread();

To help ensure that the ref-cycle is broken, lets do something like this:

  RefPtr<FetchDriver> fetchDriver = mFetchDriver.forget();

And then use the stack variable in OnStopRequest().  This ensures that the ref will be dropped no matter how we leave this method.

@@ +1059,5 @@
>                             nsISupports* aContext,
>                             nsresult aStatusCode)
>  {
> +  // Note: OnStopRequest might be restart if we can get specified alternative
> +  // data from cache. This is different with general streamListener implementation

Is this still true?  Since you have a separate stream listener I think this only gets called once, right?

@@ +1065,1 @@
>    workers::AssertIsOnMainThread();

Lets do something similar to clear the ref-cycle here:

  RefPtr<AlternateDataStreamListener> altDataListener = mAltDataListener.forget();

And then use it locally here and pass it as an argument to FinishStopRequest().

@@ +1065,3 @@
>    workers::AssertIsOnMainThread();
>  
> +  mOnStopRequestCalled = true;

MOZ_DIAGNOSTIC_ASSERT(!mOnStopRequestCalled) to make sure we're only called once?

@@ +1120,5 @@
> +  return FinishOnStopRequest();
> +}
> +
> +nsresult
> +FetchDriver::FinishOnStopRequest()

Lets make this take an AlternateDataStreamListener pointer.  So FetchDriver::OnStopRequest() would pass its stack altDataListener variable.  The AlternateDataStreamListener::OnStopRequest() would just pass `this`.

This method could then MOZ_DIAGNOSTIC_ASSERT(!mAltDataListener) to ensure that the ref-cycle has been broken.  In the LOADING case the channel will be holding the mAltDataListener alive.

::: dom/fetch/InternalResponse.h
@@ +254,5 @@
> +    if (mWrappedResponse) {
> +      return mWrappedResponse->SetAlternativeBody(aAlternativeBody);
> +    }
> +    // A request's body may not be reset once set.
> +    MOZ_ASSERT(!mAlternativeBody);

MOZ_DIAGNOSTIC_ASSERT() for cheap checks like this please.

Comment 52

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Eden Chuang[:edenchuang] from comment #50)
> This patch is fixing the assertion crash on the wpt-test
> /service-workers/service-worker/clients-get-client-types.https.html with a
> try run, and following is the fail log.
> 
> https://treeherder.mozilla.org/logviewer.
> html#?job_id=144558670&repo=try&lineNumber=29664
> 
> According to the error message on the fail log, It complains
> InterceptStreamListener is not a threadsafe while adding
> InterceptStreamListener into arrayToRelease in HttpChannelChild's
> destructor. 

Can you instead try changing this:

  // To solve multiple inheritence of nsISupports in InterceptStreamListener
  already_AddRefed<nsIStreamListener> listener = mInterceptListener.forget();
  arrayToRelease.AppendElement(listener.take());

To this:

  // To solve multiple inheritence of nsISupports in InterceptStreamListener
  nsCOMPtr<nsIStreamListener> listener = mInterceptListener.forget();
  arrayToRelease.AppendElement(listener.forget());

I think the listener.take() is not doing what that code originally wanted.

Comment 53

[Eden Chuang[:edenchuang]]

Attachment: P2: Fetch and save alternative data to InternalResponse. r?bkelly

Update the patch according to the comment 51

Ensure the reference cycle is broken while the data loading finishes.

Comment 54

[Eden Chuang[:edenchuang]]

(In reply to Ben Kelly [:bkelly] from comment #52)
> (In reply to Eden Chuang[:edenchuang] from comment #50)
> > This patch is fixing the assertion crash on the wpt-test
> > /service-workers/service-worker/clients-get-client-types.https.html with a
> > try run, and following is the fail log.
> > 
> > https://treeherder.mozilla.org/logviewer.
> > html#?job_id=144558670&repo=try&lineNumber=29664
> > 
> > According to the error message on the fail log, It complains
> > InterceptStreamListener is not a threadsafe while adding
> > InterceptStreamListener into arrayToRelease in HttpChannelChild's
> > destructor. 
> 
> Can you instead try changing this:
> 
>   // To solve multiple inheritence of nsISupports in InterceptStreamListener
>   already_AddRefed<nsIStreamListener> listener = mInterceptListener.forget();
>   arrayToRelease.AppendElement(listener.take());
> 
> To this:
> 
>   // To solve multiple inheritence of nsISupports in InterceptStreamListener
>   nsCOMPtr<nsIStreamListener> listener = mInterceptListener.forget();
>   arrayToRelease.AppendElement(listener.forget());
> 
> I think the listener.take() is not doing what that code originally wanted.

Ben, I tried your suggestion, we still meet the same crash on try with the same error message

https://treeherder.mozilla.org/logviewer.html#?job_id=148078765&repo=try&lineNumber=21043

I think no matter which listener.take() or listener.forget() is used, the InterceptStreamListener::AddRef() is called while it is appended to arrayToRelease, and the thread safety will be checked at

https://searchfox.org/mozilla-central/source/xpcom/base/nsISupportsImpl.h#657

Then we hit the assertion.

I am trying to make InterceptStreamListener be thread safe and fixing the leaks I mentioned in the comment 50.
But I think the modification in the comment 52 is correct and I will apply it in the updated patch.

Comment 55

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Eden Chuang[:edenchuang] from comment #54)
> (In reply to Ben Kelly [:bkelly] from comment #52)
> > (In reply to Eden Chuang[:edenchuang] from comment #50)
> > > This patch is fixing the assertion crash on the wpt-test
> > > /service-workers/service-worker/clients-get-client-types.https.html with a
> > > try run, and following is the fail log.
> > > 
> > > https://treeherder.mozilla.org/logviewer.
> > > html#?job_id=144558670&repo=try&lineNumber=29664
> > > 
> > > According to the error message on the fail log, It complains
> > > InterceptStreamListener is not a threadsafe while adding
> > > InterceptStreamListener into arrayToRelease in HttpChannelChild's
> > > destructor. 
> > 
> > Can you instead try changing this:
> > 
> >   // To solve multiple inheritence of nsISupports in InterceptStreamListener
> >   already_AddRefed<nsIStreamListener> listener = mInterceptListener.forget();
> >   arrayToRelease.AppendElement(listener.take());
> > 
> > To this:
> > 
> >   // To solve multiple inheritence of nsISupports in InterceptStreamListener
> >   nsCOMPtr<nsIStreamListener> listener = mInterceptListener.forget();
> >   arrayToRelease.AppendElement(listener.forget());
> > 
> > I think the listener.take() is not doing what that code originally wanted.
> 
> Ben, I tried your suggestion, we still meet the same crash on try with the
> same error message
> 
> https://treeherder.mozilla.org/logviewer.
> html#?job_id=148078765&repo=try&lineNumber=21043
> 
> I think no matter which listener.take() or listener.forget() is used, the
> InterceptStreamListener::AddRef() is called while it is appended to
> arrayToRelease, and the thread safety will be checked at
> 
> https://searchfox.org/mozilla-central/source/xpcom/base/nsISupportsImpl.h#657
> 
> Then we hit the assertion.

The `forget()` should avoid the AddRef(), though.  That is what its designed to do.  We must be getting some kind of weird compiler type coercion that makes a temporary variable copy.

Does this work?

   // To solve multiple inheritence of nsISupports in InterceptStreamListener
   nsCOMPtr<nsIStreamListener> listener = mInterceptListener.forget();
   nsCOMPtr<nsISupports> supports = listener.forget();
   arrayToRelease.AppendElement(supports.forget());

Maybe also pre-allocate the size of the nsTArray to avoid any internal copying that might occur if it auto-sizes itself.

  nsTArray<nsCOMPtr<nsISupports>> arrayToRelease(5);

That is what WorkerLoadInfo::ProxyReleaseMainThreadObjects() does:

https://searchfox.org/mozilla-central/source/dom/workers/WorkerPrivate.cpp#2099

It also does a swap() call instead of forget().  You could try that as well.

Sorry, I think getting the proxy release right is probably easier than trying to make the listener thread safe.

Comment 56

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8932448 [details] [diff] [review]
P2: Fetch and save alternative data to InternalResponse. r?bkelly

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

r=me with comments addressed.  Thanks!

::: dom/fetch/FetchDriver.cpp
@@ +73,5 @@
> +  enum eStatus {
> +    LOADING = 0,
> +    COMPLETED,
> +    CANCELED,
> +    FALLBACK

Can you add a comment that describes the stats a bit?  AFAICT we can have:


  LOADING->CANCELED
  LOADING->COMPLETED
  LOADING->FALLBACK

And maybe this one, but I suggest not doing it below:

  LOADING->FALLBACK->CANCELED

I think at least mentioned FALLBACK never goes to COMPLETED and perhaps does not go to CANCELED either would be good.

@@ +137,5 @@
> +    // FetchDriver
> +    mChannel->Cancel(NS_BINDING_ABORTED);
> +    mChannel = nullptr;
> +  }
> +  mStatus = AlternativeDataStreamListener::CANCELED;

Should we really set this to CANCELED here if we are in fallback mode?  It seems this might prevent us from propagating the final OnStopRequest() to FetchDriver in that case.

@@ +260,5 @@
> +      mAlternativeDataCacheEntryId = 0;
> +      mCacheInfoChannel = nullptr;
> +      mPipeAlternativeInputStream = nullptr;
> +    }
> +    mStatus = AlternativeDataStreamListener::COMPLETED;

Do we really need the COMPLETED mStatus?  Nothing seems to use it?

@@ +265,5 @@
> +    // alternative data loading finish, call FetchDriver::FinishOnStopRequest to
> +    // continue the final step for the case FetchDriver::OnStopRequest is called
> +    // earlier than AlternativeDataStreamListener::OnStopRequest
> +    MOZ_ASSERT(fetchDriver);
> +    fetchDriver->FinishOnStopRequest(this);

Can you return NS_OK immediately after FinishOnStopRequest() here?  Its more typical style here unless you really need to fall through for some reason.

@@ +272,5 @@
> +  if (mStatus == AlternativeDataStreamListener::FALLBACK) {
> +    MOZ_ASSERT(fetchDriver);
> +    return fetchDriver->OnStopRequest(aRequest, aContext, aStatusCode);
> +  }
> +  return NS_OK;

If I understand correctly we can only have one other mStatus value here; CANCELED.

Maybe restructure this method to be like:

  if (mStatus == CANCELED) {
    // do nothing
    return NS_OK;
  }

  if (mStatus == FALLBACK) {
    MOZ_ASSERT(fetchDriver);
    return fetchDriver->OnStopRequest(aRequest, aContext, aStatusCode);
  }

  MOZ_DIAGNOSTIC_ASSERT(mStatus == LOADING);

  // insert loading code here without the extra indenting, etc.

Comment 57

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8931433 [details] [diff] [review]
P3.1: Place the alternative data to the InterceptedChannel. r?bkelly

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

Looks good with a few small changes.  r=me with comments addressed.  Thanks!

::: netwerk/base/nsINetworkInterceptController.idl
@@ +82,5 @@
>       */
>      void startSynthesizedResponse(in nsIInputStream body,
>                                    in nsIInterceptedBodyCallback callback,
> +                                  in ACString finalURLSpec,
> +                                  in nsICacheInfoChannel channel);

Since the finalURLSpec is technically optional at the moment, can you move this nsICacheInfoChannel before it in the argument list?  Also, please add to the comment describing that parameter.

::: netwerk/protocol/http/InterceptedHttpChannel.cpp
@@ +750,5 @@
>    mBodyCallback = aBodyCallback;
>    aBodyCallback = nullptr;
>  
> +  if (aSynthesizedCacheInfo) {
> +    mSynthesizedCacheInfo = aSynthesizedCacheInfo;

Why the conditional?  Can't we just do the assignment always whether its nullptr or populated?

@@ +1171,5 @@
> +  InterceptedHttpChannel does not really implement the nsICacheInfoChannel
> +  interface, we tranfers parameters to the saved nsICacheInfoChannel(mSynthesizedCacheInfo)
> +  from StartSynthesizedResponse. And we return NS_ERROR_NOT_IMPLEMENTED for all
> +  methods while the saved mSynthesizedCacheInfo does not exist.
> +*/

nit: Please use // style comments here.

Also, see comments below. I think we should try to make nsHttpChannel's behavior here instead of throwing NS_ERROR_NOT_IMPLEMENTED.  That means returning success and "false" from IsFromCache() and then NS_ERROR_NOT_AVAILABLE from the other methods.

@@ +1178,5 @@
> +{
> +  if (mSynthesizedCacheInfo) {
> +    return mSynthesizedCacheInfo->IsFromCache(value);
> +  }
> +  return NS_ERROR_NOT_IMPLEMENTED;

I think perhaps we should just return false for the value instead of throwing an error.

@@ +1187,5 @@
> +{
> +  if (mSynthesizedCacheInfo) {
> +    return mSynthesizedCacheInfo->GetCacheEntryId(aCacheEntryId);
> +  }
> +  return NS_ERROR_NOT_IMPLEMENTED;

And then return NS_ERROR_NOT_AVAILABLE like nsHttpChannel does when its not actually from cache:

https://searchfox.org/mozilla-central/source/netwerk/protocol/http/nsHttpChannel.cpp#7789

Here and for the other method implementations below.

Comment 58

[Eden Chuang[:edenchuang]]

Attachment: P1: Set alternative data type from InterceptedChannel to InternalRequest. r=bkelly

Rebase for the patch.

Comment 59

[Eden Chuang[:edenchuang]]

Attachment: P2: Fetch and save alternative data to InternalResponse. r=bkelly

Update the patch according to the comment 56.

Comment 60

[Eden Chuang[:edenchuang]]

Attachment: P3: Place the alternative data to the InterceptedChannel. r=bkelly

Update the patch according to comment 57.

Comment 61

[Eden Chuang[:edenchuang]]

Attachment: P4: Fix a crash caused by off-main-thread destruction of a HttpChannelChild. r?bkelly

I tried suggestions mentioned in the comment 55. But no one works.

After tracing the crashing code, the crash happens when destructing the InternalResponse's mCacheInfoChannel(is a HttpChannelChild) in the cases that mCacheInfoChannel is not needed.
In the original implementation, we set InternalResponse's mCacheInfoChannel in every case, even we don't need it. This makes the channel's life cycle change and releases the channel at the wrong time. This patch fixes the implementation to set the InternalResponse's mCacheInfoChannel if it is necessary.

This patch also implements the suggestion in comment 52.

I submitted a try with this patch, and the crash is fixed.

Comment 62

[Eden Chuang[:edenchuang]]

Attachment: P5: mochitest for exposing alternate data in http cache via fetch() InternalResponse object. r=bkelly

Rebase and update the patch according to P3's update.

Comment 63

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8934085 [details] [diff] [review]
P4: Fix a crash caused by off-main-thread destruction of a HttpChannelChild. r?bkelly

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

Looks reasonable.

Also, we should probably take precautions that we don't release this cache info channel on the wrong thread in other cases.  Can you change InternalResponse to have:

  nsMainThreadPtrHandle<nsICacheInfoChannel> mCacheInfoChannel;

This will cause it to proxy release on the main thread automatically.  You can do this in a separate patch, of course.  Thanks.

Comment 64

[Eden Chuang[:edenchuang]]

Attachment: P4: Fix a crash caused by off-main-thread destruction of a HttpChannelChild. r=bkelly

Comment 65

[Eden Chuang[:edenchuang]]

Attachment: P6: Make sure releasing nsICacheInfoChannel of InternalResponse on the main thread. r?bkelly

Using nsMainThreadPtrHandle to hold nsICacheInfoChannel in InternalResponse to make sure it would be released in the main thread.

Comment 66

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8934405 [details] [diff] [review]
P6: Make sure releasing nsICacheInfoChannel of InternalResponse on the main thread. r?bkelly

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

A good start, but I think we should be a bit more explicit about this by requiring the caller to pass in and consume the nsMainThreadPtrHandle.

We may have to maybe adjust this when we get to the Cache API implementation in bug 1336199.  In that case we may want the flexibility to have a non-main-thread nsICacheInfoChannel since Cache API works OMT.  Its unclear to me yet exactly how we will want to handle that yet.  Maybe a Variant<> with an nsMainThreadPtrHandle and a thread-safe nsCOMPtr<>.  We can tackle that in bug 1336199, though.

::: dom/fetch/InternalResponse.h
@@ +286,5 @@
>        return mWrappedResponse->SetCacheInfoChannel(aCacheInfoChannel);
>      }
> +    MOZ_ASSERT(!mCacheInfoChannel);
> +    mCacheInfoChannel = new nsMainThreadPtrHolder<nsICacheInfoChannel>(
> +                          "nsIInterceptedChannel", aCacheInfoChannel, false);

Rather than create this here, I think we should make SetCacheInfoChannel() take an nsMainThreadPtrHandle:

  SetCacheInfoChannel(const nsMainThreadPtrHandle<nsICacheInfoChannel>& aCacheInfoChannel)

And then the caller would do something like:

  nsMainThreadPtrHandle<nsICacheInfoChannel> handle(
    new nsMainThreadPtrHolder<nsICacheInfoChannel>(cacheInfoChannel));
  ir->SetCacheInfoChannel(handle);

@@ +298,3 @@
>      }
> +    nsCOMPtr<nsICacheInfoChannel> cacheInfo = mCacheInfoChannel.get();
> +    return cacheInfo.forget();

This should also return an nsMainThreadPtrHandle.  And I think we should try to keep it as a "Take" operation.  Maybe something like:

  nsMainThreadPtrHandle<nsICacheInfoChannel>
  TakeCacheInfoChannel()
  {
    if (mWrappedResponse) {
      return mWrappedResponse->TakeCacheInfoChannel();
    }
    nsMainThreadPtrHandle<nsICacheInfoChannel> rtn = mCacheInfoChannel;
    mCacheInfoChannel = nullptr;
    return rtn;
  }

Comment 67

[Eden Chuang[:edenchuang]]

Attachment: P6: Make sure releasing nsICacheInfoChannel of InternalResponse on the main thread. r?bkelly

Update the patch according to the comment 66.

Comment 68

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8934611 [details] [diff] [review]
P6: Make sure releasing nsICacheInfoChannel of InternalResponse on the main thread. r?bkelly

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

Thanks!

Comment 69

[Eden Chuang[:edenchuang]]

Attachment: P6: Make sure releasing nsICacheInfoChannel of InternalResponse on the main thread. r=bkelly

Comment 70

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8a56d02d0455
Part 1: Set alternative data type from InterceptedChannel to InternalRequest. r=bkelly
https://hg.mozilla.org/integration/mozilla-inbound/rev/cada18145faf
Part 2: Fetch and save alterntative data to InternalResponse. r=bkelly
https://hg.mozilla.org/integration/mozilla-inbound/rev/e588f73617b0
Part 3: Place the alternative data to the InterceptedChannel. r=bkelly
https://hg.mozilla.org/integration/mozilla-inbound/rev/3141ed00dc03
Part 4: Fix a crash caused by off-main-thread destruction of a HttpChannelChild. r=bkelly
https://hg.mozilla.org/integration/mozilla-inbound/rev/0f47dfbb1de4
Part 5: Add mochitest for exposing alternate data in http cache via fetch() InternalResponse object. r=bkelly
https://hg.mozilla.org/integration/mozilla-inbound/rev/64766497d822
Part 6: Make sure releasing nsICacheInfoChannel of InternalResponse on the main thread. r=bkelly

Comment 71

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/8a56d02d0455
https://hg.mozilla.org/mozilla-central/rev/cada18145faf
https://hg.mozilla.org/mozilla-central/rev/e588f73617b0
https://hg.mozilla.org/mozilla-central/rev/3141ed00dc03
https://hg.mozilla.org/mozilla-central/rev/0f47dfbb1de4
https://hg.mozilla.org/mozilla-central/rev/64766497d822