support streaming nsPipe data from child to parent in Service Worker Cache

RESOLVED FIXED in Firefox 39

Status

()

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: bkelly, Assigned: bkelly)

Tracking

(Blocks: 1 bug)

unspecified
mozilla39
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox39 fixed)

Details

Attachments

(3 attachments, 7 obsolete attachments)

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.
Created attachment 8559411 [details] [diff] [review]
P1 Make NS_AsyncCopy() use nsICancelableRunnables so it can operate on Workers.  (v0)
Assignee: nobody → bkelly
Status: NEW → ASSIGNED
Created attachment 8559412 [details] [diff] [review]
P2 Use CrossProcessPipe in Cache for non-serializable async streams. (v0)
Attachment #8537400 - Attachment is obsolete: true
Created attachment 8560634 [details] [diff] [review]
P3 Evil work around for non-e10s.  CrossProcessPipe currently asserts there.

Work around current CrossProcessPipe problems on non-e10s.
Created attachment 8560635 [details] [diff] [review]
P3 Evil work around for non-e10s. CrossProcessPipe currently asserts there.

Now with actual source files.
Attachment #8560634 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Blocks: 1140872
(Assignee)

Updated

4 years ago
Blocks: 931249
(Assignee)

Updated

4 years ago
Summary: integrate CrossProcessPipe into Service Worker Cache → support streaming nsPipe data from child to parent in Service Worker Cache
Created attachment 8577688 [details] [diff] [review]
P1 Implement Cache actor for streaming data from child to parent.

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
Created attachment 8577777 [details] [diff] [review]
Implement Cache IPC actor for streaming data from child to parent. r=khuey

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)
Created attachment 8577818 [details] [diff] [review]
Implement Cache IPC actor for streaming data from child to parent.

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

4 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.
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)
Try build including a patch to re-enable unified builds in dom/cache:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d6a5ebaf2661
(Assignee)

Updated

4 years ago
Blocks: 1110485
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.
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)
(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.
Created attachment 8578680 [details] [diff] [review]
interdiff 001 Re-enable unified builds, fix try failures, address review feedback

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)
(Assignee)

Updated

4 years ago
Blocks: 1144175
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+
(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.
I'm going to work bug 1143223 first as it could explain some bad behavior on opt.

Comment 21

4 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

4 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

4 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.
No longer depends on: 1093357
At this point I think I just need to add a ton of instrumentation and push to try.
Flags: needinfo?(bkelly)
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
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.
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.
Created attachment 8581159 [details] [diff] [review]
P0 Fully initialize Response before resolve Fetch promise. r=nsm

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)
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+
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

4 years ago
No problem!  Nice find on the race!
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
You need to log in before you can comment on or make changes to this bug.