Closed Bug 1204254 Opened 9 years ago Closed 7 years ago

service workers should stream response bodies when intercepting a channel

Categories

(Core :: DOM: Service Workers, defect)

Product:
Core
Component:
DOM: Service Workers
Version:
32 Branch
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla58
Tracking Flags:
Tracking Status
firefox58 --- fixed

People

(Reporter: bkelly, Assigned: bkelly)

References

(Blocks 4 open bugs)

Details

Attachments

(15 files, 42 obsolete files)

P4 Dispatch the StartResponse runnable when body copying begins. r=asuth
6.31 KB, patch
bkelly
: review+
Details | Diff | Splinter Review
P5 Don't rely on nsIInputStream::Available() during interception to get the total stream length. r=asuth
1.85 KB, patch
bkelly
: review+
Details | Diff | Splinter Review
P6 Wait to start copying data until after we start the response synthesis. r=asuth
10.39 KB, patch
asuth
: review+
Details | Diff | Splinter Review
P1 Add an nsIInterceptedChannel::StartSynthesizedResponse() method. r=asuth
6.40 KB, patch
bkelly
: review+
Details | Diff | Splinter Review
P10 Expect to pass fetch-event-respond-with-response-body-with-invalid-chunk.https.html. r=asuth
899 bytes, patch
asuth
: review+
Details | Diff | Splinter Review
P11 Notify "service-worker-synthesized-response" topic when synthesis starts so devtools can trace the channel. r=asuth
2.94 KB, patch
asuth
: review+
Details | Diff | Splinter Review
P12 Remove nsIInterceptedChannel.responseBody and backing nsPipe code. r=asuth
9.14 KB, patch
asuth
: review+
Details | Diff | Splinter Review
P15 Try to make test_devtools_serviceworker_interception.html handle --verify runs again. r=asuth
4.67 KB, patch
asuth
: review+
Details | Diff | Splinter Review
P8 Improve error handling and cancelation for intercepted HttpChannelChild. r=asuth
7.51 KB, patch
asuth
: review+
Details | Diff | Splinter Review
P14 Stop faking responseStart/End directly and make PerformanceTiming map SW specific timings instead. r=asuth
9.18 KB, patch
asuth
: review+
Details | Diff | Splinter Review
P2 Move StartSynthesizedResponse() out from FinishSynthesizedResponse(). r=asuth
11.41 KB, patch
bkelly
: review+
Details | Diff | Splinter Review
P13 Add a WPT test that verifies data streams through respondWith(). r=asuth
7.15 KB, patch
bkelly
: review+
Details | Diff | Splinter Review
P3 Move logic into StartFinish() runnable separate from FinishRunnable() in ServiceWorkerEvents. r=asuth
6.38 KB, patch
bkelly
: review+
Details | Diff | Splinter Review
P7 Allow the body nsIInputStream to be passed in StartSynthesizeResponse(). r=asuth
8.51 KB, patch
bkelly
: review+
Details | Diff | Splinter Review
P9 Pass the body stream directly to StartSynthesizedResponse() instead of copying. r=asuth
41.90 KB, patch
bkelly
: review+
Details | Diff | Splinter Review 
Currently the service worker buffers the entire Response body when intercepting a channel.

https://dxr.mozilla.org/mozilla-central/source/dom/workers/ServiceWorkerEvents.cpp#321

Currently this only really effects fetch() and Cache.add() calls that have been intercepted.  In particular, it makes the Cache.add() case much less memory efficient as the body does not really stream to disk.

We will especially need to fix this once we support true streams on fetch() Response.body.  Otherwise streaming will just break when interception is in use.
Note, we might be able to bypass the current buffering completely today if chunked encoding is in use.  In that case I don't think code expects a fixed content-length anyway.
In order to handle the e10s case we need to support streaming from parent-to-child.
Depends on: 1274343
Blocks: 1283197
As long as I'm looking at streams stuff this week while baku is away I think I might try to figure this one out.  It seems the tricky part will be getting the InterceptedChannel stuff to trigger OnStartRequest and OnDataAvailable() before we FinishSynthesizedResponse().
Assignee: nobody → bkelly
Status: NEW → ASSIGNED
Here is a demo for verifying if this works or not:

https://html-sw-stream.glitch.me

It streams a line of html to an iframe once a second; both from a server and a service worker.  If this bug is successful we should display data at equivalent times for both of these cases.  Currently we only incrementally display the server streamed frame.
Note, the demo in comment 4 takes about 25 seconds to start showing data in firefox when streamed from the server.  Its not clear why.  I wrote bug 1388227 about that.
See Also: → 1388227
This passes a mochitests locally.  Need to test more to see if it even does what I am hoping it does.
The patches also allow us to incrementally update the second iframe in:

https://html-sw-stream.glitch.me/

We still have an initial delay on first paint, though, so I think there must be some minimum buffer size in the path or something.  I'll investigate that in bug 1388227.
When combined with bug 1128959 this seems to make us pass an additional WPT test as well.  So that's good...
I don't think the dependency on bug 1274343 makes sense.

The Response provided to respondWith() must be one of:

1. A fetch() provided network Response
2. A Cache API provided disk Response
3. A js created synthetic Response

In case (1) we get incremental OnDataAvailable() calls from the parent via nsIChannel.  In case (2) we are reading a file stream.  Case (3) does not transition IPC at all.

None of these cases require the IPC parent-to-child mechanism.  Not sure what I was thinking.
No longer depends on: 1274343
I guess we'll need to use it once parent-side intercept lands.  (Andrew, is this what you were getting at on IRC?)
Try without the ReadableStream patches:

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

Looks like I will need to fix an xpcshell test as well...
The necko xpcshell tests use the nsIInterceptedChannel interface directly.  So I need to update them to call startSynthesizedResponse() explicitly.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=407cd4b0f54b86ec7f51015a2e307b12c85fc155
Attachment #8895103 - Attachment is obsolete: true
I had incorrectly put one of the telemetry probes at the start of synthesis.  Update the patch to compete it at completion.  We already have a probe for when the Response is resolved and copying begins.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c9b1bd16c8cbe95bc9a735c5e6ce5771352a95c1
Attachment #8895104 - Attachment is obsolete: true
Comment on attachment 8895101 [details] [diff] [review]
P1 Add an nsIInterceptedChannel::StartSynthesizedResponse() method. r=asuth

In order to get OnStartRequest() and OnDataAvailable() to fire from our synthesized channel before copying is complete, we need to do a bunch of things earlier.

This patch starts down that road by separating most of the code in FinishSynthesizedResponse() out into a StartSynthesizedResponse().  For now Finish just calls it directly so its mostly a no-op.

Note, I add better .idl comments in a later patch.
Attachment #8895101 - Flags: review?(bugmail)
Comment on attachment 8895192 [details] [diff] [review]
P2 Move StartSynthesizedResponse() out from FinishSynthesizedResponse(). r=asuth

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

Next, we move the StartSynthesizedResponse() call out of FinishSynthesizedResponse().  We just call it immediately before finish from the various call sites for now.  Again, this is a functional no-op.
Attachment #8895192 - Flags: review?(bugmail)
Comment on attachment 8895194 [details] [diff] [review]
P3 Move logic into StartFinish() runnable separate from FinishRunnable() in ServiceWorkerEvents. r=asuth

Now we need to move the StartSynthesizedResponse() to take place sooner in ServiceWorkerEvents.  (I am ignoring the ParentHttpListener case as we determined its not really important to do start any sooner then.)

Ultimately I want to do StartSynthesizedResponse() when copying begins.  It needs to happen on the main thread, though, so I can't just call it directly from RespondWithHandler::ResolvedCallback().  So I will need to dispatch a main thread runnable for start.

This patch takes a step in that direction by moving most of the code out of the FinishResponse runnable and into a StartResponse runnable.  To keep it a no-op for now we just dispatch StartResponse when FinishResponse is dispatched.
Attachment #8895194 - Flags: review?(bugmail)
Comment on attachment 8895105 [details] [diff] [review]
P4 Dispatch the StartResponse runnable when body copying begins. r=asuth

Finally, we can actually make our functional change.  This patch makes us dispatch the StartResponse runnable when the NS_AsyncCopy() begins.

Note, in order to guarantee that StartResponse is executed before FinishResponse we must avoid the WorkerPrivate main thread target ThrottledEventQueue.  Since the copying can complete off the worker and dispatch FinishResponse directly to main thread we must use a direct dispatch for StartResponse as well.  Otherwise its possible for the StartResponse runnable to get delayed in the worker main thread event target long enough for Finish to run first.  That's obviously bad.  (And I ran into, so its a real problem.)
Attachment #8895105 - Flags: review?(bugmail)
Blocks: 1388768
Blocks: 1388774
Comment on attachment 8895101 [details] [diff] [review]
P1 Add an nsIInterceptedChannel::StartSynthesizedResponse() method. r=asuth

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

Fantastic patch splitting!
Attachment #8895101 - Flags: review?(bugmail) → review+
Comment on attachment 8895192 [details] [diff] [review]
P2 Move StartSynthesizedResponse() out from FinishSynthesizedResponse(). r=asuth

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

::: netwerk/base/nsINetworkInterceptController.idl
@@ +57,3 @@
>      /**
> +     * Instruct a channel that has been intercepted that a response is
> +     * starting to be synthesized.  No further header modification is

nit: alt-D betrayal: "modification is allowed" lost the "allowed".
Attachment #8895192 - Flags: review?(bugmail) → review+
Attachment #8895194 - Flags: review?(bugmail) → review+
These patches seem to interact with bug 1128959 to trigger an assertion in necko on debug android.  Lets try to avoid this by waiting to start copying until after start synthesis.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=222d91ac4def9de8da925128d6b833d59e9de8b6

I don't love this, but I hope to get rid of some of the copying in bug 1388768 anyway.
Comment on attachment 8895105 [details] [diff] [review]
P4 Dispatch the StartResponse runnable when body copying begins. r=asuth

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

::: dom/workers/ServiceWorkerEvents.cpp
@@ +704,5 @@
> +    // ensure the start runnable fires before the finish runnable.  The finish
> +    // runnable, though, sometimes gets dispatched from places other than the
> +    // worker thread (like at the end of copying).  Therefore it does not
> +    // use the worker main thread event target either.
> +    MOZ_ALWAYS_SUCCEEDS(SystemGroup::Dispatch(TaskCategory::Other,

This seemed unsound, but is now mooted by P6.  Woo!
Attachment #8895105 - Flags: review?(bugmail) → review+
Comment on attachment 8895425 [details] [diff] [review]
P5 Don't rely on nsIInputStream::Available() during interception to get the total stream length. r=asuth

This patch gets rid of the Available() call in our e10s child channel code.  We were using this to set the maximum size for progress, etc.  Now that we are incrementally piping the body this is no long safe to do.

Instead I call GetContentLength() instead.  This will try to use the content-length header if its present.  If not, we fall back to -1 meaning unknown max size.

I believe this is similar to what the non-e10s case does since it goes through the cache entry.
Attachment #8895425 - Flags: review?(bugmail)
Attachment #8895425 - Flags: review?(bugmail) → review+
Hmm.  These patches don't actually get us streaming in the non-e10s case.  Thats bad because it means once parent intercept lands we will lose this behavior.  Not only is it something we want for incremental render, but it has web compat implications.

Andrew suggests there is a way to fix InterceptedChannelChrome.  I'll try to figure out what I am doing wrong.
And comment 29 suggests I need to write a test.
Josh, Honza, do either of you have suggestions on how I could I get the synthesized channel's OnStartRequest() and OnDataAvailable() to start firing before I complete writing the entire body into the cache entry?

I believe the problem is that after OpenCacheEntry() the AwaitCacheCallbacks() method is returning true.  So in non-e10s I never get ContinueConnect() called this these patches and the demo in comment 4.  I tried making it ContinueConnect() anyway, but then I hit this assertion:

Assertion failure: !mCachedContentIsValid || mRaceCacheWithNetwork (We should not be hitting the network if we have valid cached content unless we are racing the network and cache), at /srv/mozilla-central/netwerk/protocol/http/nsHttpChannel.cpp:2339

Does necko support reading out from the cache entry while we are writing into it?
Flags: needinfo?(josh)
Flags: needinfo?(honzab.moz)
Did you see my comment on IRC about doing cacheEntry->SetValid()?
(In reply to Andrew Sutherland [:asuth] from comment #32)
> Did you see my comment on IRC about doing cacheEntry->SetValid()?

Nope.  Let me try that.
(In reply to Ben Kelly [:bkelly] from comment #31)
> Josh, Honza, do either of you have suggestions on how I could I get the
> synthesized channel's OnStartRequest() and OnDataAvailable() to start firing
> before I complete writing the entire body into the cache entry?
> 
> I believe the problem is that after OpenCacheEntry() the
> AwaitCacheCallbacks() method is returning true.  So in non-e10s I never get
> ContinueConnect() called this these patches and the demo in comment 4.  I
> tried making it ContinueConnect() anyway, but then I hit this assertion:

Not a good idea.  If AwaitCacheCallbacks() returns false it means the entry has not been obtained yet (OnCacheEntryAvaialble was not called) and you must not continue connecting.

> 
> Assertion failure: !mCachedContentIsValid || mRaceCacheWithNetwork (We
> should not be hitting the network if we have valid cached content unless we
> are racing the network and cache), at
> /srv/mozilla-central/netwerk/protocol/http/nsHttpChannel.cpp:2339
> 
> Does necko support reading out from the cache entry while we are writing
> into it?

It definitely does, but the entry must be marked as MetadataReady(), and NOT!! SetValid()!!, but the channel or consumer that writes to it.  MetadataReady() call says that the necessary metadata, like response headers etc. were all stored on the entry and data are now about to be written to it.  This allows the concurrency to go forward, so all other consumer will then get their OnCacheEntryCheck/Available calls.  In your case, it will make your cache-reading channel to go on.
Flags: needinfo?(honzab.moz)
Flags: needinfo?(josh)
Fix a crash.  Some devtools failures around reporting errors I will have to fix in a later patch as well.
Attachment #8895534 - Attachment is obsolete: true
So, interception is completely broken in non-e10s after P6.  I think I may need to roll back to P5 here and just focus on e10s for now.  Then in a separate bug I can remove the http cache entry and try to get this working as well.
Summary: service workers should stream response bodies when intercepting a channel → service workers should stream response bodies when intercepting a channel in e10s mode
Blocks: 1390638
I'm going to rebase these patches on bug 1391693.
Depends on: 1391693
Summary: service workers should stream response bodies when intercepting a channel in e10s mode → service workers should stream response bodies when intercepting a channel
Rather than create a bunch of copying code that will be ripped out shortly, this just makes both e10s and non-e10s avoid the extra copy altogether.  The respondWith() body stream is passed to the channel pump in both cases.

Still going to be a lot of orange here, but curious if anything new breaks:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=67d36cee8d8ebd5412f386518cebce111c84ebe4
Attachment #8913911 - Attachment is obsolete: true
I moved the test change from P11 into this patch.  The change to fire the http-on-stop-request notification was moved into bug 1391693 where InterceptedHttpChannel is added.
Attachment #8914511 - Attachment is obsolete: true
Attachment #8914560 - Attachment is obsolete: true
I'm still hitting this:

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

It seems to reproduce locally with:

xvfb-run -a -s '-screen 0 1600x1200x24' ./mach mochitest dom/push/test/test_serviceworker_lifetime.html --verify
Comment on attachment 8916029 [details] [diff] [review]
P13 Make test_devtools_serviceworker_interception.html expect synthesized responseStart/End times. r=bgrins

Brian,

In order to report meaningful responseStart/End values in PerformanceRequestTiming data I need to synthesize these timings in the channel.  I actually added this in a previous bug, but the test did not see it until the P10 patch in this bug which causes us to check the channel after OnStopRequest.

I manually checked devtools and the service worker timings shown are unchanged.  They still show zero currently.  (It would be nice to fix that to show something, though.)

This patch makes the test at least accept that we synthesize these two values which are normally "network only".
Attachment #8916029 - Flags: review?(bgrinstead)
Comment on attachment 8916029 [details] [diff] [review]
P13 Make test_devtools_serviceworker_interception.html expect synthesized responseStart/End times. r=bgrins

I need to fix something here.
Attachment #8916029 - Flags: review?(bgrinstead)
No longer blocks: 1388768
No longer blocks: 1390638
Seems my latest changes here broke:

dom/workers/test/serviceworkers/test_fetch_integrity.html

Also hitting assertion in:

/service-workers/service-worker/opaque-response-preloaded.https.html 

ASAN M-e10s(4) shows a leak as well, but hoping thats fallout from one of the above problems.
This fixes the test_fetch_integrity.html.  It was using the same service worker script that I modified here.
Attachment #8916138 - Attachment is obsolete: true
Looks like I still need to track down a leak.
A lot of the recent fallout and fixes in this bug are due to the HttpChannelChild::Cancel() changes.  These are necessary because we are now more likely to need to stop the channel in the middle of the pump.  In the past we could get away with AsyncAbort() all the time because the pump only ran for a brief moment.

I think I will restructure the patches tomorrow to make the Cancel/AsyncAbort changes before the current P8 patch to perform the stream consumption in the channel.

        Attachment #8914510 -
        Attachment is obsolete: true

        Attachment #8914902 -
        Attachment is obsolete: true

        Attachment #8914930 -
        Attachment is obsolete: true

        Attachment #8915743 -
        Attachment is obsolete: true

        Attachment #8915744 -
        Attachment is obsolete: true

        Attachment #8916029 -
        Attachment is obsolete: true

        Attachment #8916059 -
        Attachment is obsolete: true

        Attachment #8916745 -
        Attachment is obsolete: true

        Attachment #8916782 -
        Attachment is obsolete: true

        Attachment #8916857 -
        Attachment is obsolete: true

    
    

    
  
Comment on attachment 8913909 [details] [diff] [review]
P6 Wait to start copying data until after we start the response synthesis. r=asuth

I'm want to see try green again on some of the later patch, but I think I'm confident of P6 and P7 patches.  Going to flag these for review.

This patch moves our use of the RespondWithClosure to the point where we call StartSynthesizedResponse().  The copying code will disappear in P9 where we will start passing the response body stream instead.

        Attachment #8913909 -
        Flags: review?(bugmail)

    
    

    
  
Comment on attachment 8914400 [details] [diff] [review]
P7 Allow the body nsIInputStream to be passed in StartSynthesizeResponse(). r=asuth

This adds the interface bits to pass the response body stream and a callback to StartSynthesizedResponse().  It has not functional change.

Note, I did consider writing an nsIInputStream wrapper that fires a callback when its closed.  That would be possible more reliable than piping a separate argument through the channel.  I chose not to go this way, though, because streams tend to do a lot of QI to mix-in different capabilities.  I worried about losing things with a wrapper class.  (If someone else writes it, though, it would be super useful.)

        Attachment #8914400 -
        Flags: review?(bugmail)

    
    

    
  
Comment on attachment 8917562 [details] [diff] [review]
P10 Expect to pass fetch-event-respond-with-response-body-with-invalid-chunk.https.html. r=asuth

We get this for free, apparently.  I didn't set out to fix this failure but apparently propagating things through the interception better addresses whatever our problem was.

        Attachment #8917562 -
        Flags: review?(bugmail)

    
    

    
  
Comment on attachment 8917563 [details] [diff] [review]
P11 Notify "service-worker-synthesized-response" topic when synthesis starts so devtools can trace the channel. r=asuth

The devtools network monitor code wants to see all the callbacks from the intercepted channel.  It only starts tracing the channel when it sees the "service-worker-synthesized-response" observer topic, though.  So we need to fire this when we start synthesizing, not when we finish.

A side consequence of this is that test_devtools_serviceworker_interception.html can't use this observer topic to wait for a complete channel.  Instead make it use "http-on-stop-request".

        Attachment #8917563 -
        Flags: review?(bugmail)

    
    

    
  
Comment on attachment 8917564 [details] [diff] [review]
P12 Remove nsIInterceptedChannel.responseBody and backing nsPipe code. r=asuth

This removes the nsIInterceptedHttpChannel.responseBody output stream getter.  This won't be necessary after P9 (which I haven't flagged for review yet).

        Attachment #8917564 -
        Flags: review?(bugmail)

    
    

    
  
Comment on attachment 8917567 [details] [diff] [review]
P15 Try to make test_devtools_serviceworker_interception.html handle --verify runs  again. r=asuth

This patch tries to make test_devtools_serviceworker_interception.html capable of running repeatedly again.  The previous fix was not adequate because the SW was not completely removed before restart.

The test is trying to use the "wait to be controlled" method of activation which is running afoul of:

https://github.com/w3c/ServiceWorker/issues/1204

I changed this test to use a message event to trigger the claim instead of relying on the activate event.  This also effects test_fetch_integrity.html which uses the same service worker.

        Attachment #8917567 -
        Flags: review?(bugmail)

    
    

    
  
State of the remaining few patches:

P8: Waiting for another green try.
P9: Waiting for another green try.
P13: I want to tweak this test a bit more.
P14: I may rewrite how we get PerformanceRequestTiming.responseEnd so we don't have to fake a network response time.

    
    

    
  


  
This patch adds a WPT that tries to verify data streams through the interception.  I'm not 100% sure the spec *requires* this, but I asked folks in #whatwg IRC and they thought it would be reasonable to include this in WPT.  I tried to make the test flexible enough so browsers can rebuffer if they choose, etc.

        Attachment #8917565 -
        Attachment is obsolete: true

        Attachment #8917578 -
        Flags: review?(bugmail)

    
    

    
  


  
This is one of the greenest try runs I have gotten, so I'm going to go ahead and flag for review.

Updating this patch since I noticed the bug number was wrong, though.

Andrew, this patch refactors HttpChannelChild interception code to use Cancel() instead of AsyncAbort().  Cancel() will trigger an AsyncAbort() if we're being intercepted, but the pump has not been created yet.

This allows us to more cleanly stop an intercepted HttpChannelChild throughout its various states.  We have a single error call to make that works whether the pump exists or not.  Also, Cancel() de-duplicates calls while AsyncAbort() does not.

This patch also includes some additional error checking in places I found we were potentially doing work on a canceled channel.

        Attachment #8917559 -
        Attachment is obsolete: true

        Attachment #8917617 -
        Flags: review?(bugmail)

    
    

    
  
Comment on attachment 8917560 [details] [diff] [review]
P9 Pass the body stream directly to StartSynthesizedResponse() instead of copying. r=asuth

Andrew, this is perhaps the most complex patch in the series.  I'm sorry I couldn't split it up more.

The main goal of this patch is to make ServiceWorkerEvents code pass the respondWith() Response body stream directly to StartSynthesizedResponse().  Previously this code got an nsIOutputStream and wrote into it using NS_AsyncCopy().

Internally I have made HttpChannelChild and InterceptedHttpChannel pass the response body stream directly into their nsInputStreamPumps.  This avoids at least one copy we were paying for before.

The main complication here is managing the nsIInterceptedBodyCallback.  We *must* ensure that the callback is always invoked once its passed to StartSynthesizedResponse.  If its not called then we will end up leaking the channel.

I used ScopeExit closures during the initialization of the pump to handle callback on error.  Once the pump is started we are reasonably assured OnStopRequest() will be called one way or another.  The callback will be invoked there as well.

        Attachment #8917560 -
        Flags: review?(bugmail)

    
    

    
  


  
Andrew, this patch makes us use the FetchEvent timings we track on the channel in PerformanceTiming instead of faking responseStart/End.  I finally realized we track network and cache timings separately in PerformanceTiming.  It seems like we should do the same for SW timings.

This lets us pass test_devtools_serviceworker_interception.html's current expectation that responseStart/End are not modified.

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

        Attachment #8917566 -
        Attachment is obsolete: true

        Attachment #8917898 -
        Flags: review?(bugmail)

    
    

    
  
(In reply to Andrew Sutherland [:asuth] from comment #25)
> Comment on attachment 8895192 [details] [diff] [review]
> P2 Move StartSynthesizedResponse() out from FinishSynthesizedResponse().
> r=asuth
> 
> Review of attachment 8895192 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: netwerk/base/nsINetworkInterceptController.idl
> @@ +57,3 @@
> >      /**
> > +     * Instruct a channel that has been intercepted that a response is
> > +     * starting to be synthesized.  No further header modification is
> 
> nit: alt-D betrayal: "modification is allowed" lost the "allowed".

nit nit: Although this patch got obsoleted, I don't think this ever got fixed, or the fix got lost.  The try in comment 101 is still missing it on the tip[1].  It's fine to fix it anywhere in the stack.

1: https://hg.mozilla.org/try/file/3cc3e1731e381a9fad10cbd311f76ea97d0319cc/netwerk/base/nsINetworkInterceptController.idl#l70

        Attachment #8913909 -
        Flags: review?(bugmail) → review+

    
    

    
  
Comment on attachment 8914400 [details] [diff] [review]
P7 Allow the body nsIInputStream to be passed in StartSynthesizeResponse(). r=asuth

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

Thank you for the design rationale in comment 92 and the excellent comment in nsINetworkInterceptController.idl!

        Attachment #8914400 -
        Flags: review?(bugmail) → review+

    
    

    
  
Comment on attachment 8917617 [details] [diff] [review]
P8 Improve error handling and cancelation for intercepted HttpChannelChild. r=asuth

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

## Restating:

Previously, cancellation-handling logic was a bit more haphazardly distributed,
now Cancel() is idempotent and consistently:
- (Pre-existing) Is the only location mCanceled is set, enabling the various
  mCanceled checks to do their no-op returns.  (With OnTransportAndData's
  diversion handling being a special case where we bounce the event back to the
  parent rather than eating it.)
- Causes OnStopRequest to be triggered (if we're post-AsyncOpen) by ensuring
  the pump is cancelled (if pumping), or triggering intercept listener cleanup
  and AsyncAbort() (if not yet pumping).
- Is the only place the non-idempotent AsyncAbort() is invoked by

Cancel() calls are more reliably and cleanly called through judicious use of
MakeScopeExit() that closes over the rv (in the part 9 patch as well.)

InterceptedChannelContent::CancelInterception()'s reentrancy story is improved.

        Attachment #8917617 -
        Flags: review?(bugmail) → review+

    
    

    
  
(In reply to Andrew Sutherland [:asuth] from comment #104)
> - Is the only place the non-idempotent AsyncAbort() is invoked by
interception-related code, outside of the FailedAsyncOpen path that
invokes HandleAsyncAbort() itself (after setting mStatus, which is
what primarily differentiates it from AsyncAbort()).

    
    

    
  
Comment on attachment 8917560 [details] [diff] [review]
P9 Pass the body stream directly to StartSynthesizedResponse() instead of copying. r=asuth

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

## Restating (simple diff falls down here):
- RespondWithClosure moved up to its forward declaration and is unchanged.
- FinishResponse moved up so BodyCopyHandle (formerly RespondWithCopyComplete)
  could move up to where its (forward) declaration was.
- RespondWithCopyComplete (previously called by StartResponse::Run) is now
  BodyCopyHandle::BodyComplete().
  - It now asserts it is invoked on the main thread instead of conditionally
    dispatching its created runnable to the main thread if not already there.
- The BodyCopyHandle class cleans up RespondWithClosure ownership.  This is
  simplified by the elimination of the use of NS_AsyncCopy and the explicitly
  typed nsIInterceptedCallback interface.

        Attachment #8917560 -
        Flags: review?(bugmail) → review+

        Attachment #8917562 -
        Flags: review?(bugmail) → review+

        Attachment #8917563 -
        Flags: review?(bugmail) → review+

    
    

    
  
Comment on attachment 8917564 [details] [diff] [review]
P12 Remove nsIInterceptedChannel.responseBody and backing nsPipe code. r=asuth

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

Good riddance to bad writers.

        Attachment #8917564 -
        Flags: review?(bugmail) → review+

    
    

    
  
Comment on attachment 8917578 [details] [diff] [review]
P13 Add a WPT test that verifies data streams through respondWith(). r=asuth

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

::: testing/web-platform/tests/service-workers/service-worker/fetch-event-respond-with-partial-stream.https.html
@@ +38,5 @@
> +  while (received.length < expected.length) {
> +    let chunk = await reader.read();
> +    assert_false(chunk.done, 'partial body stream should not be closed yet');
> +
> +    received += decoder.decode(chunk.value);

idiom nit:  If you're allowing arbitrary re-buffering by the browser, then you need to do the decoding after you've accumulated the data.  This works fine for your ASCII payload here, but if someone naively propagates this code or changes the payload to involve the cool jack-o-lantern emoji, this can break.  I feel like Henri Sivonen has raised discussion points on this behavior in the past, but I'm having trouble finding them.  (And obviously you'll want the loop invariant to be on encodedExpected.length, not expected.length at that point).

Ex:
encPumpkin = new TextEncoder('utf-8').encode('\u{1f383}')
> Uint8Array [ 240, 159, 142, 131 ]
dec = new TextDecoder()
> TextDecoder { encoding: "utf-8", fatal: false }
'\u{1f383}' == dec.decode(encPumpkin.slice(0, 2)) + dec.decode(encPumpkin.slice(2))
> false

        Attachment #8917578 -
        Flags: review?(bugmail) → review+

        Attachment #8917898 -
        Flags: review?(bugmail) → review+

    
    

    
  


  
Fixing encoding issues with the rebuffering loop in the WPT test.

        Attachment #8917578 -
        Attachment is obsolete: true

        Attachment #8919409 -
        Flags: review+

        Attachment #8919395 -
        Flags: review+

    
    

    
  
Comment on attachment 8917567 [details] [diff] [review]
P15 Try to make test_devtools_serviceworker_interception.html handle --verify runs  again. r=asuth

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

In terms of the `registration.installing || registration.active` expressions, I'm a little worried about that idiom spreading throughout our tests without explanation.  It seems like in https://github.com/w3c/ServiceWorker/issues/1204#issuecomment-336172533 there's agreement that we can make resurrection less of a frustrating edge case by creating a new registration... maybe file a bug tracking our desire to implement that, and include a comment like the following above both of those uses:

If run repeatedly using --repeat, registration resurrection can occur, resulting in the registration existing on "active" and never existing on "installing".  Bug NNNNNN tracks removal of resurrection so that we only need to access "installing".

::: dom/workers/test/serviceworkers/test_devtools_serviceworker_interception.html
@@ +45,5 @@
>      });
>    });
>  }
>  
> +// Using utils.js doesn't seem to work in this chrome test, so redefine

You need to add it to chrome.ini under support-files; it's not there yet.  (I tested this locally.)

@@ +62,5 @@
> +    });
> +  });
> +}
> +
> +function waitForControlled(win) {

maybe this can go in utils.js then too?

        Attachment #8917567 -
        Flags: review?(bugmail) → review+

    
    

    
  
Kudos on an amazing cleanup/refactor/overhaul!

        Attachment #8919411 -
        Attachment description: bug1204254_p3_startrunnable.patch → P3 Move logic into StartFinish() runnable separate from FinishRunnable() in ServiceWorkerEvents. r=asuth

    
    

    
  
(In reply to Andrew Sutherland [:asuth] from comment #111)
> In terms of the `registration.installing || registration.active`
> expressions, I'm a little worried about that idiom spreading throughout our
> tests without explanation.  It seems like in
> https://github.com/w3c/ServiceWorker/issues/1204#issuecomment-336172533
> there's agreement that we can make resurrection less of a frustrating edge
> case by creating a new registration... maybe file a bug tracking our desire
> to implement that, and include a comment like the following above both of
> those uses:
> 
> If run repeatedly using --repeat, registration resurrection can occur,
> resulting in the registration existing on "active" and never existing on
> "installing".  Bug NNNNNN tracks removal of resurrection so that we only
> need to access "installing".

I'm adding a comment, but I think we need to resolve the spec issue before we seriously start talking about removing the resurrection behavior.

> > +// Using utils.js doesn't seem to work in this chrome test, so redefine
> 
> You need to add it to chrome.ini under support-files; it's not there yet. 
> (I tested this locally.)

Oh, duh.  Thanks!

    
    

    
  
Pushed by bkelly@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f7d794a0ad9d
P1 Add an nsIInterceptedChannel::StartSynthesizedResponse() method. r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/ea38f259983e
P2 Move StartSynthesizedResponse() out from FinishSynthesizedResponse(). r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/876207de29e4
P3 Move logic into StartFinish() runnable separate from FinishRunnable() in ServiceWorkerEvents. r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/eeb40cbe9e15
P4 Dispatch the StartResponse runnable when body copying begins. r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/eb6f84505bb6
P5 Don't rely on nsIInputStream::Available() during interception to get the total stream length. r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/72cb1e671644
P6 Wait to start copying data until after we start the response synthesis. r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/2562b0e40a9f
P7 Allow the body nsIInputStream to be passed in StartSynthesizeResponse(). r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/5d3c295e9764
P8 Improve error handling and cancelation for intercepted HttpChannelChild. r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/2da2c0e9186f
P9 Pass the body stream directly to StartSynthesizedResponse() instead of copying. r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/024f2b76fc69
P10 Expect to pass fetch-event-respond-with-response-body-with-invalid-chunk.https.html. r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/e38a6e3ea992
P11 Notify "service-worker-synthesized-response" topic when synthesis starts so devtools can trace the channel. r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/ad671af01276
P12 Remove nsIInterceptedChannel.responseBody and backing nsPipe code. r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/d99f28271f4c
P13 Add a WPT test that verifies data streams through respondWith(). r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/80ef5c73059b
P14 Stop faking responseStart/End directly and make PerformanceTiming map SW specific timings instead. r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/c6b3520e30fe
P15 Try to make test_devtools_serviceworker_interception.html handle --verify runs again. r=asuth

    
    

    
  
Telemetry shows that its taking us longer to get to the end of the response body now across both desktop/mobile:

https://mzl.la/2ylmcuk

Median increased from 7ms to 70ms.  Mean increased from 45ms to 140ms.

I looked a bit at the changes to try to understand this.  I believe this increase is mostly due to running FinishSynthesizeResponse() after the nsIChannel OnStopRequest() fires.  Previously we would call it immediately after the off-main-thread NS_AsyncCopy() completed.  This means we are waiting for main thread runnables to fire and actually deliver the data to the end consumer within the browser.

Also, we now follow a common path for completing the interception even when the body is missing.  We used to synchronously mark those complete which may have been overly aggressive.

I think this change is fine and more accurately represents the time spent to actually synthesize the channel.  If people disagree, please let me know.

Overall interception duration, however, did not change:

https://mzl.la/2ymOvZr

(The slight improvement in this graph is due to mobile improving due to bug 1391693.)

This supports the analysis that our response duration changes are due to measurement changes, but not an actual performance regression.  It also suggests that we are moving work around within the same time frame, which is consistent with streaming values incrementally.
Depends on: 1429542
Regressions: 1650082

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: