Closed
Bug 1110814
Opened 10 years ago
Closed 10 years ago
support streaming nsPipe data from child to parent in Service Worker Cache
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla39
Tracking | Status | |
---|---|---|
firefox39 | --- | fixed |
People
(Reporter: bkelly, Assigned: bkelly)
References
(Blocks 1 open bug)
Details
Attachments
(3 files, 7 obsolete files)
50.23 KB,
patch
|
Details | Diff | Splinter Review | |
6.41 KB,
patch
|
khuey
:
review+
|
Details | Diff | Splinter Review |
2.46 KB,
patch
|
nsm
:
review+
|
Details | Diff | Splinter Review |
The Service Worker Cache must serialize Request and Response body streams from the child process to the parent process. We use SerializeInputStream() for this.
Unfortunately, though, if the Response is being read from the network then we get an nsIPipe stream which cannot be serialized.
To handle this case the Cache needs to use the new CrossProcessPipe to copy data across in chunks.
In the future, it might be nice to open a writable fd in the parent process, dup() it to the child, and then have the child write the data directly. I do not believe this is currently possible with the QuotaManager file streams, though.
Assignee | ||
Comment 1•10 years ago
|
||
Assignee | ||
Comment 2•10 years ago
|
||
Assignee: nobody → bkelly
Status: NEW → ASSIGNED
Assignee | ||
Comment 3•10 years ago
|
||
Attachment #8537400 -
Attachment is obsolete: true
Assignee | ||
Comment 4•10 years ago
|
||
Work around current CrossProcessPipe problems on non-e10s.
Assignee | ||
Comment 5•10 years ago
|
||
Now with actual source files.
Attachment #8560634 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Summary: integrate CrossProcessPipe into Service Worker Cache → support streaming nsPipe data from child to parent in Service Worker Cache
Assignee | ||
Comment 6•10 years ago
|
||
Work-in-progress patch for implementing this without CrossProcessPipe.
This patch creates a new Cache actor type. The child simply reads directly from an nsIAsyncInputStream in a non-blocking fashion and calls SendBuffer(). The parent side writes each buffer into a pipe when RecvBuffer() is called.
I now need to integrate this with the rest of the code and then see if it works.
Attachment #8559411 -
Attachment is obsolete: true
Attachment #8559412 -
Attachment is obsolete: true
Attachment #8560635 -
Attachment is obsolete: true
Assignee | ||
Comment 7•10 years ago
|
||
This pushes data from child to parent successfully in both mochitest and by browsing trained-to-thrill.
Kyle, do you mind reviewing since you're familiar with the problem space?
Attachment #8577688 -
Attachment is obsolete: true
Attachment #8577777 -
Flags: review?(khuey)
Assignee | ||
Comment 8•10 years ago
|
||
Updated patch:
1) Make test_cache_put.js verify the stored body matches the original.
2) Treat an Available() value of 0 bytes as NS_BASE_STREAM_WOULD_BLOCK.
3) Properly clear the actor from the Callback object.
Also, I'm switching the review to Ehsan as he's more heavily involved in the Cache work right now.
Ehsan, if you're not comfortable reviewing the IPC elements here, then Kyle is probably a good person to ask or flag for review.
Attachment #8577777 -
Attachment is obsolete: true
Attachment #8577777 -
Flags: review?(khuey)
Attachment #8577818 -
Flags: review?(ehsan)
Assignee | ||
Updated•10 years ago
|
Attachment #8577818 -
Attachment description: Implement Cache IPC actor for streaming data from child to parent. r=khuey → Implement Cache IPC actor for streaming data from child to parent.
FWIW, I will actually clear off my schedule to review this tomorrow if you want me to.
Assignee | ||
Comment 10•10 years ago
|
||
Comment on attachment 8577818 [details] [diff] [review]
Implement Cache IPC actor for streaming data from child to parent.
I talked to Ehsan in IRC and he was happy to pass this review on. Thanks Kyle!
Attachment #8577818 -
Flags: review?(ehsan) → review?(khuey)
Assignee | ||
Comment 11•10 years ago
|
||
Try build including a patch to re-enable unified builds in dom/cache:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d6a5ebaf2661
Assignee | ||
Comment 12•10 years ago
|
||
New try build to fix static analysis failure:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2a9e28cdc80d
Assignee | ||
Comment 13•10 years ago
|
||
According to me try build I messed up my test when I added body comparing. Its getting a 404 on the file I'm trying to fetch.
Assignee | ||
Comment 14•10 years ago
|
||
New try that actually loads a real file with fetch() and also properly handle the parallel mode in the cache tests. The parallel mode thing was what was causing the problems before, I think.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=01e14e32f524
Comment on attachment 8577818 [details] [diff] [review]
Implement Cache IPC actor for streaming data from child to parent.
Review of attachment 8577818 [details] [diff] [review]:
-----------------------------------------------------------------
This patch doesn't actually compile, so I'm somewhat skeptical. But this is a hell of a lot better than CrossProcessPipe.
Please provide an interdiff with the changes below and whatever is needed to make this compile and work.
::: dom/cache/CachePushStreamChild.cpp
@@ +26,5 @@
> + {
> + MOZ_ASSERT(mActor);
> + }
> +
> + NS_METHOD
NS_IMETHOD
@@ +45,5 @@
> +
> + return NS_OK;
> + }
> +
> + NS_METHOD
NS_IMETHOD
@@ +55,5 @@
> + }
> + return NS_OK;
> + }
> +
> + NS_METHOD
NS_IMETHOD
@@ +227,5 @@
> + NS_ASSERT_OWNINGTHREAD(CachePushStreamChild);
> + MOZ_ASSERT(mCallback);
> + MOZ_ASSERT(aCallback == mCallback);
> + mCallback->ClearActor();
> + mCallback = nullptr;
It seems kind of wasteful to create and destroy the Callback every time we AsyncWait. Can we wait until OnEnd?
::: dom/cache/CachePushStreamChild.h
@@ +44,5 @@
> + void OnEnd(nsresult aRv);
> +
> + nsCOMPtr<nsIAsyncInputStream> mStream;
> + nsRefPtr<Callback> mCallback;
> + bool mClosed = false;
I'm 99.99999% sure that doesn't compile.
::: dom/cache/CachePushStreamParent.h
@@ +25,5 @@
> +
> + ~CachePushStreamParent();
> +
> + already_AddRefed<nsIInputStream>
> + ExtractReader();
I'd probably prefer TakeReader, but this is nitpicking.
::: dom/cache/PCache.ipdl
@@ -36,5 @@
> PutResponse(RequestId requestId, nsresult aRv);
> DeleteResponse(RequestId requestId, nsresult aRv, bool success);
> KeysResponse(RequestId requestId, nsresult aRv, PCacheRequest[] requests);
> -
> -both:
Is this actually related to this bug?
::: dom/cache/TypeUtils.cpp
@@ +106,5 @@
>
> +void
> +SerializeNormalStream(nsIInputStream* aStream, PCacheReadStream& aReadStreamOut)
> +{
> + nsAutoTArray<FileDescriptor, 4> fds;
Seems like this might as well be nsTArray, although it doesn't matter much.
Attachment #8577818 -
Flags: review?(khuey)
Assignee | ||
Comment 16•10 years ago
|
||
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #15)
> Comment on attachment 8577818 [details] [diff] [review]
> Implement Cache IPC actor for streaming data from child to parent.
>
> Review of attachment 8577818 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> This patch doesn't actually compile, so I'm somewhat skeptical.
I left my temp change to use non-unified builds in by accident. Reverting that in moz.build allows it to compile. The non-unified bustage fix is also in inbound from another bug, but not in central yet.
> @@ +227,5 @@
> > + NS_ASSERT_OWNINGTHREAD(CachePushStreamChild);
> > + MOZ_ASSERT(mCallback);
> > + MOZ_ASSERT(aCallback == mCallback);
> > + mCallback->ClearActor();
> > + mCallback = nullptr;
>
> It seems kind of wasteful to create and destroy the Callback every time we
> AsyncWait. Can we wait until OnEnd?
I think this micro optimization would make the code harder reason about with little benefit.
If the data is flowing quickly, then we will not be doing any AsyncWait()s. If the data is flowing slowly, then the cost of allocating the callback is going to occur infrequently. Also, allocating the callback will pale in comparison to the cost of allocating the buffer and copying data to it.
I say it would be harder to reason about because there would be an additional state to track for the Callback. It also seems having simpler life cycles is beneficial for any code interacting with an Actor.
I guess I'd like to wait to do this until a profile shows it's relevant. If you really want it, though, I will of course do it.
> ::: dom/cache/CachePushStreamChild.h
> @@ +44,5 @@
> > + void OnEnd(nsresult aRv);
> > +
> > + nsCOMPtr<nsIAsyncInputStream> mStream;
> > + nsRefPtr<Callback> mCallback;
> > + bool mClosed = false;
>
> I'm 99.99999% sure that doesn't compile.
Try server says otherwise, but yes, I will fix.
> ::: dom/cache/CachePushStreamParent.h
> @@ +25,5 @@
> > +
> > + ~CachePushStreamParent();
> > +
> > + already_AddRefed<nsIInputStream>
> > + ExtractReader();
>
> I'd probably prefer TakeReader, but this is nitpicking.
Agreed.
> ::: dom/cache/PCache.ipdl
> @@ -36,5 @@
> > PutResponse(RequestId requestId, nsresult aRv);
> > DeleteResponse(RequestId requestId, nsresult aRv, bool success);
> > KeysResponse(RequestId requestId, nsresult aRv, PCacheRequest[] requests);
> > -
> > -both:
>
> Is this actually related to this bug?
Unrelated. I will remove it for now.
> ::: dom/cache/TypeUtils.cpp
> @@ +106,5 @@
> >
> > +void
> > +SerializeNormalStream(nsIInputStream* aStream, PCacheReadStream& aReadStreamOut)
> > +{
> > + nsAutoTArray<FileDescriptor, 4> fds;
>
> Seems like this might as well be nsTArray, although it doesn't matter much.
This was added per previous review comments. So I'd like to keep it.
Assignee | ||
Comment 17•10 years ago
|
||
Implemented all your suggestions except saving the Callback till OnEnd. Also some fixes from try.
New try limited to M1 where cache tests run:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=65d0bd2d55eb
Attachment #8578680 -
Flags: review?(khuey)
Comment on attachment 8578680 [details] [diff] [review]
interdiff 001 Re-enable unified builds, fix try failures, address review feedback
Review of attachment 8578680 [details] [diff] [review]:
-----------------------------------------------------------------
The test failure on try is slightly concerning. e10s opt only could be a race condition?
Attachment #8578680 -
Flags: review?(khuey) → review+
Assignee | ||
Comment 19•10 years ago
|
||
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #18)
> The test failure on try is slightly concerning. e10s opt only could be a
> race condition?
Yes. I need to investigate and fix that. I'm having trouble reproducing locally.
Assignee | ||
Comment 20•10 years ago
|
||
I'm going to work bug 1143223 first as it could explain some bad behavior on opt.
Comment 21•10 years ago
|
||
Pushed this to try with a debugging patch: <https://treeherder.mozilla.org/#/jobs?repo=try&revision=3fdf6aebbd1d>
And immediately got results:
15:24:52 INFO - XXXXehsan mStream->Available failed: -2142830590
15:24:52 INFO - XXXXehsan in OnEnd, rv: -2142830590
Ben, does this give you enough to know what's going on? If not, I can look again tomorrow.
Flags: needinfo?(bkelly)
Comment 22•10 years ago
|
||
(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #21)
> 15:24:52 INFO - XXXXehsan mStream->Available failed: -2142830590
> 15:24:52 INFO - XXXXehsan in OnEnd, rv: -2142830590
This error code is NS_BASE_STREAM_CLOSED, which suggests that the stream has been closed already. If I'm reading the code correctly, we have assertions that ensure that the stream is not being closed inside CachePushStreamChild. I think the input stream in question here is an nsPipeInputStream, so receiving this error can mean that the pipe's output stream has been closed.
But running this under the debugger, we do hit this path with the same error message even when the test successfully passes. I'm kind of lost on what to assume based on that!
Comment 23•10 years ago
|
||
Ben asked me on IRC to do a try push dumping out the total number of bytes read out of the CachePushStreamChild. Here is the try push: <https://treeherder.mozilla.org/#/jobs?repo=try&revision=6aa0dd4c73a9>
The number of bytes that we read in the failure case is 5228, which is the exact size of test_cache.js in this tree, so there is nothing more to read from the stream.
Assignee | ||
Comment 24•10 years ago
|
||
At this point I think I just need to add a ton of instrumentation and push to try.
Flags: needinfo?(bkelly)
Assignee | ||
Comment 25•10 years ago
|
||
In this try log I don't see any output for the CachePushStreamChild for the failure case at all. Not even ActorDestroy(), which must happen. This suggests to me we are getting a Response with no body from fetch().
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/bkelly@mozilla.com-f02d0258abbe/try-linux64-debug/try_ubuntu64_vm-debug_test-mochitest-1-bm54-tests1-linux64-build141.txt.gz
A new try with more instrumentation to try to prove that:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7892e848e0c5
Assignee | ||
Comment 26•10 years ago
|
||
Yea, we're getting Response without a body in the failure case:
### ### [0x7f2df70fc328] AutoChildRequestResponse::Add() response body (nil)
From this try run:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/bkelly@mozilla.com-7892e848e0c5/try-linux64-debug/try_ubuntu64_vm-debug_test-mochitest-1-bm68-tests1-linux64-build78.txt.gz
So the good news is there is nothing wrong with this patch. I just need to figure out what is happening to the Response we are getting. Its a clone(), so maybe the bug is there.
Assignee | ||
Comment 27•10 years ago
|
||
Yes, the body has not been set on the Response yet when we clone it. I think I see the race in FetchDriver. Patch forthcoming and new try build forthcoming.
Assignee | ||
Comment 28•10 years ago
|
||
Nikhil, there is a race in http FetchDriver where resolves the promise with the Response before setting the body on the Response. This is fine for main thread, since content can't run until the body is set, but for workers content can run before Response is finished being initialized.
This patch reorders things so that the body and security info is set before BeginAndGetFilteredResponse(). I believe this should fix the errors we are seeing here.
Try build to verify:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=329ee483af63
Attachment #8581159 -
Flags: review?(nsm.nikhil)
Assignee | ||
Comment 29•10 years ago
|
||
Yea, that last patch appears to fix the problem. Here's a re-based T-shaped try run to make sure I didn't break the fetch tests or anything else:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=cd053d650b61
Comment on attachment 8581159 [details] [diff] [review]
P0 Fully initialize Response before resolve Fetch promise. r=nsm
Review of attachment 8581159 [details] [diff] [review]:
-----------------------------------------------------------------
Good catch! This also makes sure all the properties are set on the underlying unfiltered response.
Attachment #8581159 -
Flags: review?(nsm.nikhil) → review+
Blocks: 1141332
Assignee | ||
Comment 31•10 years ago
|
||
After rebasing for all the final/override changes in the tree:
remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/498032321f32
remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/61cb338ad147
Assignee | ||
Comment 32•10 years ago
|
||
I botched my integration with the new test_caches.html mochitest, but Ehsan fixed my mistake for me:
https://hg.mozilla.org/integration/mozilla-inbound/rev/11c951df53c9
Thanks Ehsan!
Comment 33•10 years ago
|
||
No problem! Nice find on the race!
Comment 34•10 years ago
|
||
Comment on attachment 8577818 [details] [diff] [review]
Implement Cache IPC actor for streaming data from child to parent.
>+++ b/dom/cache/CacheParent.h
>@@ -27,16 +27,18 @@ class CacheParent MOZ_FINAL : public PCa
[...]
> private:
> // PCacheParent method
> virtual void ActorDestroy(ActorDestroyReason aReason) MOZ_OVERRIDE;
>+ virtual PCachePushStreamParent* AllocPCachePushStreamParent();
>+ virtual bool DeallocPCachePushStreamParent(PCachePushStreamParent* aActor);
> virtual bool RecvTeardown() MOZ_OVERRIDE;
These new methods needed MOZ_OVERRIDE annotations here (or rather, "override" annotations, post-bug 1145631), which broke my local warnings-as-errors clang 3.6 build.
I landed a followup using the blanket r+ that ehsan granted me for fixes of this sort over on bug 1126447 comment 2:
https://hg.mozilla.org/integration/mozilla-inbound/rev/052bf572cb94
Comment 35•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/498032321f32
https://hg.mozilla.org/mozilla-central/rev/61cb338ad147
https://hg.mozilla.org/mozilla-central/rev/11c951df53c9
https://hg.mozilla.org/mozilla-central/rev/052bf572cb94
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
status-firefox39:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•