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)
Tracking
()
RESOLVED
FIXED
mozilla58
Tracking | Status | |
---|---|---|
firefox58 | --- | fixed |
People
(Reporter: bkelly, Assigned: bkelly)
References
(Blocks 4 open bugs)
Details
Attachments
(15 files, 42 obsolete files)
6.31 KB,
patch
|
bkelly
:
review+
|
Details | Diff | Splinter Review |
1.85 KB,
patch
|
bkelly
:
review+
|
Details | Diff | Splinter Review |
10.39 KB,
patch
|
asuth
:
review+
|
Details | Diff | Splinter Review |
6.40 KB,
patch
|
bkelly
:
review+
|
Details | Diff | Splinter Review |
899 bytes,
patch
|
asuth
:
review+
|
Details | Diff | Splinter Review |
2.94 KB,
patch
|
asuth
:
review+
|
Details | Diff | Splinter Review |
9.14 KB,
patch
|
asuth
:
review+
|
Details | Diff | Splinter Review |
4.67 KB,
patch
|
asuth
:
review+
|
Details | Diff | Splinter Review |
7.51 KB,
patch
|
asuth
:
review+
|
Details | Diff | Splinter Review |
9.18 KB,
patch
|
asuth
:
review+
|
Details | Diff | Splinter Review |
11.41 KB,
patch
|
bkelly
:
review+
|
Details | Diff | Splinter Review |
7.15 KB,
patch
|
bkelly
:
review+
|
Details | Diff | Splinter Review |
6.38 KB,
patch
|
bkelly
:
review+
|
Details | Diff | Splinter Review |
8.51 KB,
patch
|
bkelly
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•9 years ago
|
||
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.
Assignee | ||
Comment 2•8 years ago
|
||
In order to handle the e10s case we need to support streaming from parent-to-child.
Depends on: 1274343
Assignee | ||
Updated•8 years ago
|
Blocks: ServiceWorkers-streams
Assignee | ||
Updated•7 years ago
|
Assignee | ||
Comment 3•7 years ago
|
||
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
Assignee | ||
Comment 4•7 years ago
|
||
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.
Assignee | ||
Comment 5•7 years ago
|
||
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
Assignee | ||
Comment 6•7 years ago
|
||
Assignee | ||
Comment 7•7 years ago
|
||
Assignee | ||
Comment 8•7 years ago
|
||
Assignee | ||
Comment 9•7 years ago
|
||
Assignee | ||
Comment 10•7 years ago
|
||
This passes a mochitests locally. Need to test more to see if it even does what I am hoping it does.
Assignee | ||
Comment 11•7 years ago
|
||
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.
Assignee | ||
Comment 12•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8d20ad8ba896798ddf620410c43e17463d9a75c3
Assignee | ||
Comment 13•7 years ago
|
||
When combined with bug 1128959 this seems to make us pass an additional WPT test as well. So that's good...
Assignee | ||
Comment 14•7 years ago
|
||
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
Assignee | ||
Comment 15•7 years ago
|
||
I guess we'll need to use it once parent-side intercept lands. (Andrew, is this what you were getting at on IRC?)
Assignee | ||
Comment 16•7 years ago
|
||
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...
Assignee | ||
Comment 17•7 years ago
|
||
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
Assignee | ||
Comment 18•7 years ago
|
||
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
Assignee | ||
Comment 19•7 years ago
|
||
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)
Assignee | ||
Comment 20•7 years ago
|
||
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)
Assignee | ||
Comment 21•7 years ago
|
||
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)
Assignee | ||
Comment 22•7 years ago
|
||
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)
Assignee | ||
Comment 23•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0b587ff0430884a8c44b013491b6e7a6c39c8d15
Comment 24•7 years ago
|
||
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 25•7 years ago
|
||
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+
Updated•7 years ago
|
Attachment #8895194 -
Flags: review?(bugmail) → review+
Assignee | ||
Comment 26•7 years ago
|
||
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 27•7 years ago
|
||
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+
Assignee | ||
Comment 28•7 years ago
|
||
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)
Updated•7 years ago
|
Attachment #8895425 -
Flags: review?(bugmail) → review+
Assignee | ||
Comment 29•7 years ago
|
||
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.
Assignee | ||
Comment 30•7 years ago
|
||
And comment 29 suggests I need to write a test.
Assignee | ||
Comment 31•7 years ago
|
||
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)
Comment 32•7 years ago
|
||
Did you see my comment on IRC about doing cacheEntry->SetValid()?
Assignee | ||
Comment 33•7 years ago
|
||
(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.
Comment 34•7 years ago
|
||
(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)
Comment 35•7 years ago
|
||
To be exact: https://dxr.mozilla.org/mozilla-central/rev/4c5fbf49376351679dcc49f4cff26c3c2e055ccc/netwerk/cache2/nsICacheEntry.idl#180
Updated•7 years ago
|
Flags: needinfo?(josh)
Assignee | ||
Comment 36•7 years ago
|
||
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
Assignee | ||
Comment 37•7 years ago
|
||
Assignee | ||
Comment 38•7 years ago
|
||
Work in progress...
Assignee | ||
Comment 39•7 years ago
|
||
Attachment #8896463 -
Attachment is obsolete: true
Assignee | ||
Comment 40•7 years ago
|
||
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.
Assignee | ||
Updated•7 years ago
|
Summary: service workers should stream response bodies when intercepting a channel → service workers should stream response bodies when intercepting a channel in e10s mode
Assignee | ||
Comment 41•7 years ago
|
||
I'm going to rebase these patches on bug 1391693.
Depends on: 1391693
Assignee | ||
Updated•7 years ago
|
Summary: service workers should stream response bodies when intercepting a channel in e10s mode → service workers should stream response bodies when intercepting a channel
Assignee | ||
Comment 42•7 years ago
|
||
I'll upload patches later, but this works locally: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7b79c91f61bbbb7a5c3137c55fac8c382fff5887
Assignee | ||
Comment 43•7 years ago
|
||
Attachment #8895101 -
Attachment is obsolete: true
Attachment #8913904 -
Flags: review+
Assignee | ||
Comment 44•7 years ago
|
||
Attachment #8895192 -
Attachment is obsolete: true
Attachment #8913905 -
Flags: review+
Assignee | ||
Comment 45•7 years ago
|
||
Attachment #8895194 -
Attachment is obsolete: true
Attachment #8913906 -
Flags: review+
Assignee | ||
Comment 46•7 years ago
|
||
Attachment #8895105 -
Attachment is obsolete: true
Attachment #8913907 -
Flags: review+
Assignee | ||
Comment 47•7 years ago
|
||
Attachment #8895425 -
Attachment is obsolete: true
Attachment #8913908 -
Flags: review+
Assignee | ||
Comment 48•7 years ago
|
||
Attachment #8896461 -
Attachment is obsolete: true
Assignee | ||
Comment 49•7 years ago
|
||
Attachment #8896462 -
Attachment is obsolete: true
Assignee | ||
Comment 50•7 years ago
|
||
Attachment #8897139 -
Attachment is obsolete: true
Assignee | ||
Comment 51•7 years ago
|
||
Attachment #8913910 -
Attachment is obsolete: true
Assignee | ||
Comment 52•7 years ago
|
||
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
Assignee | ||
Comment 53•7 years ago
|
||
Attachment #8914401 -
Attachment is obsolete: true
Assignee | ||
Comment 54•7 years ago
|
||
Assignee | ||
Comment 55•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=669d9d08bd186137f51192c8ff928b05680f1b29
Assignee | ||
Comment 56•7 years ago
|
||
Assignee | ||
Comment 57•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b72caedc50684a44f3e23451f32e018730dec057
Assignee | ||
Comment 58•7 years ago
|
||
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
Assignee | ||
Comment 59•7 years ago
|
||
Assignee | ||
Comment 60•7 years ago
|
||
Assignee | ||
Comment 61•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1474886c7f5012ee414bba9142165535b6afd73b
Assignee | ||
Comment 62•7 years ago
|
||
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
Assignee | ||
Comment 63•7 years ago
|
||
Attachment #8914509 -
Attachment is obsolete: true
Assignee | ||
Comment 64•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=49ff8b86d8bd32be4de807440994d5683bd2ee4b
Attachment #8914903 -
Attachment is obsolete: true
Assignee | ||
Comment 65•7 years ago
|
||
Assignee | ||
Comment 66•7 years ago
|
||
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)
Assignee | ||
Comment 67•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0d19863c175e78416b5a8afc7e12fe539495bd6e
Assignee | ||
Comment 68•7 years ago
|
||
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)
Assignee | ||
Comment 69•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9c3e68da50f32c93a473c4bd331b93b948cdda36
Assignee | ||
Comment 70•7 years ago
|
||
Attachment #8916136 -
Attachment is obsolete: true
Assignee | ||
Comment 73•7 years ago
|
||
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.
Assignee | ||
Comment 74•7 years ago
|
||
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
Assignee | ||
Comment 75•7 years ago
|
||
This fixes the assertion for me. https://treeherder.mozilla.org/#/jobs?repo=try&revision=2168dd68fc92a8c75c78f38d02f88fafddf6cf2a
Assignee | ||
Comment 76•7 years ago
|
||
Looks like I still need to track down a leak.
Assignee | ||
Comment 77•7 years ago
|
||
Attachment #8916778 -
Attachment is obsolete: true
Assignee | ||
Comment 78•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f2ea65f018525ca71f62cc284a92403df8ff297a
Assignee | ||
Comment 79•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=401095d8c91a8bd9883b5186b5b7ff1ea849236f
Assignee | ||
Comment 80•7 years ago
|
||
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.
Assignee | ||
Comment 81•7 years ago
|
||
Re-base on top of bug 1407186.
Attachment #8913904 -
Attachment is obsolete: true
Attachment #8917498 -
Flags: review+
Assignee | ||
Comment 82•7 years ago
|
||
Attachment #8913905 -
Attachment is obsolete: true
Attachment #8917501 -
Flags: review+
Assignee | ||
Comment 83•7 years ago
|
||
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
Assignee | ||
Comment 84•7 years ago
|
||
Assignee | ||
Comment 85•7 years ago
|
||
Assignee | ||
Comment 86•7 years ago
|
||
Assignee | ||
Comment 87•7 years ago
|
||
Assignee | ||
Comment 88•7 years ago
|
||
Assignee | ||
Comment 89•7 years ago
|
||
Assignee | ||
Comment 90•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d02d6a9446d23dfe8662883f134c87789f5ff2c2
Assignee | ||
Comment 91•7 years ago
|
||
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)
Assignee | ||
Comment 92•7 years ago
|
||
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)
Assignee | ||
Comment 93•7 years ago
|
||
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)
Assignee | ||
Comment 94•7 years ago
|
||
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)
Assignee | ||
Comment 95•7 years ago
|
||
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)
Assignee | ||
Comment 96•7 years ago
|
||
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)
Assignee | ||
Comment 97•7 years ago
|
||
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.
Assignee | ||
Comment 98•7 years ago
|
||
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)
Assignee | ||
Comment 99•7 years ago
|
||
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)
Assignee | ||
Comment 100•7 years ago
|
||
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)
Assignee | ||
Comment 101•7 years ago
|
||
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)
Comment 102•7 years ago
|
||
(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
Updated•7 years ago
|
Attachment #8913909 -
Flags: review?(bugmail) → review+
Comment 103•7 years ago
|
||
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 104•7 years ago
|
||
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+
Comment 105•7 years ago
|
||
(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 106•7 years ago
|
||
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+
Updated•7 years ago
|
Attachment #8917562 -
Flags: review?(bugmail) → review+
Updated•7 years ago
|
Attachment #8917563 -
Flags: review?(bugmail) → review+
Comment 107•7 years ago
|
||
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 108•7 years ago
|
||
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+
Updated•7 years ago
|
Attachment #8917898 -
Flags: review?(bugmail) → review+
Assignee | ||
Comment 110•7 years ago
|
||
Fixing encoding issues with the rebuffering loop in the WPT test.
Attachment #8917578 -
Attachment is obsolete: true
Attachment #8919409 -
Flags: review+
Assignee | ||
Updated•7 years ago
|
Attachment #8919395 -
Flags: review+
Comment 111•7 years ago
|
||
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+
Assignee | ||
Comment 112•7 years ago
|
||
Rebase.
Attachment #8913906 -
Attachment is obsolete: true
Attachment #8919411 -
Flags: review+
Comment 113•7 years ago
|
||
Kudos on an amazing cleanup/refactor/overhaul!
Assignee | ||
Updated•7 years ago
|
Attachment #8919411 -
Attachment description: bug1204254_p3_startrunnable.patch → P3 Move logic into StartFinish() runnable separate from FinishRunnable() in ServiceWorkerEvents. r=asuth
Assignee | ||
Comment 114•7 years ago
|
||
Rebase.
Attachment #8914400 -
Attachment is obsolete: true
Attachment #8919412 -
Flags: review+
Assignee | ||
Comment 115•7 years ago
|
||
Attachment #8917560 -
Attachment is obsolete: true
Attachment #8919413 -
Flags: review+
Assignee | ||
Comment 116•7 years ago
|
||
(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!
Comment 117•7 years ago
|
||
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
Comment 118•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/f7d794a0ad9d https://hg.mozilla.org/mozilla-central/rev/ea38f259983e https://hg.mozilla.org/mozilla-central/rev/876207de29e4 https://hg.mozilla.org/mozilla-central/rev/eeb40cbe9e15 https://hg.mozilla.org/mozilla-central/rev/eb6f84505bb6 https://hg.mozilla.org/mozilla-central/rev/72cb1e671644 https://hg.mozilla.org/mozilla-central/rev/2562b0e40a9f https://hg.mozilla.org/mozilla-central/rev/5d3c295e9764 https://hg.mozilla.org/mozilla-central/rev/2da2c0e9186f https://hg.mozilla.org/mozilla-central/rev/024f2b76fc69 https://hg.mozilla.org/mozilla-central/rev/e38a6e3ea992 https://hg.mozilla.org/mozilla-central/rev/ad671af01276 https://hg.mozilla.org/mozilla-central/rev/d99f28271f4c https://hg.mozilla.org/mozilla-central/rev/80ef5c73059b https://hg.mozilla.org/mozilla-central/rev/c6b3520e30fe
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox58:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Assignee | ||
Comment 119•6 years ago
|
||
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.
You need to log in
before you can comment on or make changes to this bug.
Description
•