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.

Work around current CrossProcessPipe problems on non-e10s.

Now with actual source files.

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.

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?

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.

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!

Try build including a patch to re-enable unified builds in dom/cache: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d6a5ebaf2661

New try build to fix static analysis failure: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2a9e28cdc80d

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.

(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.

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

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?

(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.

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.

(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!

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.

At this point I think I just need to add a ton of instrumentation and push to try.

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.

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

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.

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

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!

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

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