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

RESOLVED FIXED in Firefox 59

Status

()

P2
normal
RESOLVED FIXED
2 years ago
11 months ago

People

(Reporter: bkelly, Assigned: edenchuang)

Tracking

(Blocks: 1 bug)

unspecified
mozilla59
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox59 fixed)

Details

Attachments

(6 attachments, 23 obsolete attachments)

7.32 KB, patch
edenchuang
: review+
Details | Diff | Splinter Review
25.50 KB, patch
edenchuang
: review+
Details | Diff | Splinter Review
30.81 KB, patch
edenchuang
: review+
Details | Diff | Splinter Review
21.02 KB, patch
edenchuang
: review+
Details | Diff | Splinter Review
9.13 KB, patch
edenchuang
: review+
Details | Diff | Splinter Review
6.43 KB, patch
edenchuang
: review+
Details | Diff | Splinter Review
(Reporter)

Description

2 years ago
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

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

Updated

2 years ago
Blocks: 1350364
Priority: -- → P2

Updated

a year ago
Assignee: nobody → bhsu

Comment 2

a year ago
Created attachment 8902644 [details] [diff] [review]
P1: Introduce InternalResponse::mAlternativeBody
Attachment #8902644 - Flags: feedback?(bkelly)

Comment 3

a year ago
Created attachment 8902646 [details] [diff] [review]
P2: Retrieve the alternative data before the original data if applicale
Attachment #8902646 - Flags: feedback?(bkelly)

Comment 4

a year ago
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.
(Reporter)

Comment 5

a year ago
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.
Flags: needinfo?(bkelly)
(Reporter)

Comment 6

a year ago
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?
Flags: needinfo?(bkelly) → needinfo?(bhsu)
(Reporter)

Updated

a year ago
Depends on: 1395202
(Reporter)

Comment 7

a year ago
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.
(Reporter)

Comment 8

a year ago
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.
(Reporter)

Comment 9

a year ago
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.
Attachment #8902646 - Flags: feedback?(bkelly)
(Reporter)

Comment 10

a year ago
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.
Attachment #8902644 - Flags: feedback?(bkelly)

Comment 11

a year ago
(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
Flags: needinfo?(bhsu)
(Reporter)

Comment 12

a year ago
(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

a year ago
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.
(Reporter)

Comment 14

a year ago
(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

a year ago
> 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

a year ago
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

a year ago
Created attachment 8918146 [details] [diff] [review]
P1: Set alternative data type from InterceptedChannel to InternalRequest

Comment 18

a year ago
Created attachment 8918147 [details] [diff] [review]
P2: Fetch and save alterntative data to InternalResponse

Comment 19

a year ago
Created attachment 8918148 [details] [diff] [review]
P3.1: Place the alternative data to the InterceptedChannel

Comment 20

a year ago
Created attachment 8918149 [details] [diff] [review]
P3.2. Fix a crash caused by off-main-thread destruction of a HttpChannelChild

Comment 21

a year ago
Created attachment 8918150 [details] [diff] [review]
P4.1: Steal test_script_loader_js_cache.html as the base of the

testcase. r=bkelly

Comment 22

a year ago
Created attachment 8918152 [details] [diff] [review]
P4.2: Interpolate the service worker which performs pass-through fetch

r=bkelly

Comment 23

a year ago
Created attachment 8918153 [details] [diff] [review]
P4.3: Place all the tests into a single promise_test() clause
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.

Updated

a year ago
Attachment #8902644 - Attachment is obsolete: true

Updated

a year ago
Attachment #8902646 - Attachment is obsolete: true

Comment 25

a year ago
(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

a year ago
Per offline discussion, I'd like hand these patches over to Eden :)
Assignee: bhsu → echuang
Flags: needinfo?(echuang)
(Assignee)

Comment 27

a year ago
Thanks, I will continue your work.
Flags: needinfo?(echuang)
(Assignee)

Comment 29

a year ago
Created attachment 8927696 [details] [diff] [review]
P1: Set alternative data type from InterceptedChannel to InternalRequest

rebase repository for the patch.
Attachment #8918146 - Attachment is obsolete: true
Attachment #8927696 - Flags: review+
(Assignee)

Comment 30

a year ago
Created attachment 8927697 [details] [diff] [review]
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.
Attachment #8927696 - Attachment is obsolete: true
Attachment #8927697 - Flags: review?(bkelly)
(Assignee)

Comment 31

a year ago
Created attachment 8927707 [details] [diff] [review]
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.
Attachment #8918147 - Attachment is obsolete: true
Attachment #8927707 - Flags: review?(bkelly)
(Assignee)

Comment 32

a year ago
Created attachment 8927713 [details] [diff] [review]
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.
Attachment #8918148 - Attachment is obsolete: true
Attachment #8927713 - Flags: review?(bkelly)
(Assignee)

Comment 33

a year ago
Created attachment 8927714 [details] [diff] [review]
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
Attachment #8918149 - Attachment is obsolete: true
Attachment #8927714 - Flags: review?(bkelly)
(Assignee)

Comment 34

a year ago
Created attachment 8927715 [details] [diff] [review]
P4: mochitest for exposing alternate data in http cache via fetch() InternalResponse object. r?bkelly
Attachment #8918150 - Attachment is obsolete: true
Attachment #8918152 - Attachment is obsolete: true
Attachment #8918153 - Attachment is obsolete: true
Attachment #8927715 - Flags: review?(bkelly)
(Reporter)

Comment 35

11 months ago
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().
Attachment #8927697 - Flags: review?(bkelly) → review+
(Reporter)

Comment 36

11 months ago
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.
Attachment #8927707 - Flags: review?(bkelly) → feedback+
(Reporter)

Comment 37

11 months ago
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.
Attachment #8927713 - Flags: review?(bkelly) → feedback+
(Reporter)

Comment 38

11 months ago
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.
Attachment #8927714 - Flags: review?(bkelly)
(Reporter)

Comment 39

11 months ago
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.
Attachment #8927715 - Flags: review?(bkelly) → review+
(Reporter)

Comment 40

11 months ago
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?
(Assignee)

Comment 41

11 months ago
(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.
(Reporter)

Comment 42

11 months ago
(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?
(Reporter)

Comment 43

11 months ago
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.
(Assignee)

Comment 44

11 months ago
(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.
(Assignee)

Comment 46

11 months ago
Created attachment 8931430 [details] [diff] [review]
P1: Set alternative data type from InterceptedChannel to InternalRequest. r=bkelly

Update the patch according to comment 35.
Attachment #8927697 - Attachment is obsolete: true
Attachment #8931430 - Flags: review+
(Assignee)

Comment 47

11 months ago
Created attachment 8931432 [details] [diff] [review]
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.
Attachment #8927707 - Attachment is obsolete: true
Attachment #8931432 - Flags: review?(bkelly)
(Assignee)

Comment 48

11 months ago
Created attachment 8931433 [details] [diff] [review]
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.
Attachment #8927713 - Attachment is obsolete: true
Attachment #8931433 - Flags: review?(bkelly)
(Assignee)

Comment 49

11 months ago
Created attachment 8931434 [details] [diff] [review]
P4: mochitest for exposing alternate data in http cache via fetch() InternalResponse object. r=bkelly

Update the patch according to comment 39.
Attachment #8927715 - Attachment is obsolete: true
Attachment #8931434 - Flags: review+
(Assignee)

Comment 50

11 months ago
(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.
Flags: needinfo?(bkelly)
(Reporter)

Comment 51

11 months ago
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.
Attachment #8931432 - Flags: review?(bkelly) → review-
(Reporter)

Comment 52

11 months ago
(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.
Flags: needinfo?(bkelly)
(Assignee)

Comment 53

11 months ago
Created attachment 8932448 [details] [diff] [review]
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.
Attachment #8931432 - Attachment is obsolete: true
Attachment #8932448 - Flags: review?(bkelly)
(Assignee)

Comment 54

11 months ago
(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.
Flags: needinfo?(bkelly)
(Reporter)

Comment 55

11 months ago
(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.
Flags: needinfo?(bkelly)
(Reporter)

Comment 56

11 months ago
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.
Attachment #8932448 - Flags: review?(bkelly) → review+
(Reporter)

Comment 57

11 months ago
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.
Attachment #8931433 - Flags: review?(bkelly) → review+
(Assignee)

Comment 58

11 months ago
Created attachment 8934078 [details] [diff] [review]
P1: Set alternative data type from InterceptedChannel to InternalRequest. r=bkelly

Rebase for the patch.
Attachment #8931430 - Attachment is obsolete: true
Attachment #8934078 - Flags: review+
(Assignee)

Comment 59

11 months ago
Created attachment 8934080 [details] [diff] [review]
P2: Fetch and save alternative data to InternalResponse. r=bkelly

Update the patch according to the comment 56.
Attachment #8932448 - Attachment is obsolete: true
Attachment #8934080 - Flags: review+
(Assignee)

Comment 60

11 months ago
Created attachment 8934081 [details] [diff] [review]
P3: Place the alternative data to the InterceptedChannel. r=bkelly

Update the patch according to comment 57.
Attachment #8931433 - Attachment is obsolete: true
Attachment #8934081 - Flags: review+
(Assignee)

Comment 61

11 months ago
Created attachment 8934085 [details] [diff] [review]
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.
Attachment #8927714 - Attachment is obsolete: true
Attachment #8934085 - Flags: review?(bkelly)
(Assignee)

Comment 62

11 months ago
Created attachment 8934086 [details] [diff] [review]
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.
Attachment #8931434 - Attachment is obsolete: true
Attachment #8934086 - Flags: review+
(Reporter)

Comment 63

11 months ago
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.
Attachment #8934085 - Flags: review?(bkelly) → review+
(Assignee)

Comment 64

11 months ago
Created attachment 8934404 [details] [diff] [review]
P4: Fix a crash caused by off-main-thread destruction of a HttpChannelChild. r=bkelly
Attachment #8934085 - Attachment is obsolete: true
Attachment #8934404 - Flags: review+
(Assignee)

Comment 65

11 months ago
Created attachment 8934405 [details] [diff] [review]
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.
Attachment #8934405 - Flags: review?(bkelly)
(Reporter)

Comment 66

11 months ago
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;
  }
Attachment #8934405 - Flags: review?(bkelly)
(Assignee)

Comment 67

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

Update the patch according to the comment 66.
Attachment #8934405 - Attachment is obsolete: true
Attachment #8934611 - Flags: review?(bkelly)
(Reporter)

Comment 68

11 months ago
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!
Attachment #8934611 - Flags: review?(bkelly) → review+
(Assignee)

Comment 69

11 months ago
Created attachment 8934623 [details] [diff] [review]
P6: Make sure releasing nsICacheInfoChannel of InternalResponse on the main thread. r=bkelly
Attachment #8934611 - Attachment is obsolete: true
Attachment #8934623 - Flags: review+
(Assignee)

Updated

11 months ago
Keywords: checkin-needed

Comment 70

11 months ago
Pushed by ryanvm@gmail.com:
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
Keywords: checkin-needed
(Reporter)

Updated

11 months ago
Blocks: 1423623
(Reporter)

Updated

11 months ago
No longer blocks: 1350364
Duplicate of this bug: 1350364
You need to log in before you can comment on or make changes to this bug.