Closed Bug 1203680 Opened 4 years ago Closed 4 years ago

Fix file-backed blob uploads when service worker is controlling the document

Categories

(Core :: DOM: Service Workers, defect)

32 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla43
Tracking Status
firefox43 --- fixed

People

(Reporter: bkelly, Assigned: bkelly)

References

Details

Attachments

(8 files, 7 obsolete files)

9.78 KB, patch
bkelly
: review+
Details | Diff | Splinter Review
2.18 KB, patch
bkelly
: review+
Details | Diff | Splinter Review
9.56 KB, patch
mcmanus
: review+
Details | Diff | Splinter Review
1.25 KB, patch
nsm
: review+
Details | Diff | Splinter Review
1.09 KB, patch
jdm
: review+
Details | Diff | Splinter Review
5.10 KB, patch
bkelly
: review+
Details | Diff | Splinter Review
3.72 KB, patch
bkelly
: review+
Details | Diff | Splinter Review
1.35 KB, patch
ehsan
: review+
Details | Diff | Splinter Review
+++ This bug was initially created as a clone of Bug #1093357 +++

As described in bug 1093357 comment 75 we need a different solution from the IPC pipe to solve file-backed blob uploads with service workers.

Instead of using the NS_CloneInputStream() fallback to the nsPipe, we need to fully populate an nsStorageStream.  This type is IPC serializable, cloneable, and seekable which satisfies all of the necko requirements.

This will add some delay to the FetchEvent dispatch for file-backed uploads, but I don't see a way to avoid that right now.
Move P5 test patch from bug 1093357 over to here.  Address nsm's review comments and carry forward r+.
Attachment #8659467 - Flags: review+
Also move the NS_InputStreamIsCloneable() patch from bug 1093357 over to here.
Attachment #8659963 - Flags: review+
Patrick, this implements what we were talking about in #necko IRC the other day.  Instead of having ServiceWorkerManager clone the upload stream to a pipe, we provide a way for SWM to convert the upload stream to an nsStorageStream.  The storage stream is then fully cloneable, seekable, and has the correct final Available() value.
Attachment #8660120 - Flags: review?(mcmanus)
Fix a bug where an nsStorageInputStream is unusable if you create it before the storage stream is actually populated with data.  There is no reason this can't work.

The provided gtest fails before the code fix and succeeds after the fix.
Attachment #8660121 - Flags: review?(nfroyd)
Add an extra async step to launching the FetchEvent in order to ensure the channel's upload stream is directly cloneable.  In practical terms, this will only really trigger if the upload stream is a file stream, etc.  The added latency is regrettable, but is limited to file uploads and unavoidable at the moment.

This is in effect replaces the pipe for uploads with an nsStorageStream.  The storage stream is more appropriate because necko doesn't really understand truly variable length streams like pipe.  It really needs a stable Available() value and the ability to Seek() the stream back to the beginning.
Attachment #8660124 - Flags: review?(nsm.nikhil)
I'm also working on re-enabling some mochitests that were disabled in e10s due to the pipe serialization issue.
Comment on attachment 8660124 [details] [diff] [review]
P5 Make ServiceWorkerManager ensure channel upload stream is cloneable. r=nsm

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

::: dom/workers/ServiceWorkerManager.cpp
@@ +3914,5 @@
> +      nsAutoCString rvString;
> +      nsAutoCString statusString;
> +      GetErrorName(rv, rvString);
> +      GetErrorName(status, statusString);
> +      printf_stderr("### ### got %s, %s\n", rvString.get(), statusString.get());

Remove debugging.

@@ +3928,5 @@
> +      HandleError();
> +      return NS_OK;
> +    }
> +
> +    AutoJSAPI api;

Nit: while you are here, please call this jsapi.
Attachment #8660124 - Flags: review?(nsm.nikhil) → review+
Update the P3 patch to remove the old EnsureStreamBuffered() on the cloned stream. I believe this was added previously so the resulting update stream could Seek(), but was only a partial fix because you could only seek streams smaller than the buffer size.

The new storage stream solution will allow seeking for all sized upload streams.

Also, here is a try:

 https://treeherder.mozilla.org/#/jobs?repo=try&revision=262e0f25bea7
Attachment #8660120 - Attachment is obsolete: true
Attachment #8660120 - Flags: review?(mcmanus)
Attachment #8660331 - Flags: review?(mcmanus)
Address review feedback.
Attachment #8660124 - Attachment is obsolete: true
Attachment #8660332 - Flags: review+
Update storage stream fix patch to remove some debug.
Attachment #8660121 - Attachment is obsolete: true
Attachment #8660121 - Flags: review?(nfroyd)
Attachment #8660334 - Flags: review?(nfroyd)
Remove debug for real this time.
Attachment #8660334 - Attachment is obsolete: true
Attachment #8660334 - Flags: review?(nfroyd)
Attachment #8660338 - Flags: review?(nfroyd)
Re-enable e10s mochitest.  Note, this requires a further fix coming in P7.
Attachment #8660343 - Flags: review?(nsm.nikhil)
Fix handling of redirect status codes in e10s.  When an interception returns a redirect we want to follow it.  The old code, however, was trying to continue processing the channel for status codes like 401, 500, etc.
Attachment #8660344 - Flags: review?(josh)
Attachment #8660331 - Flags: review?(mcmanus) → review+
Comment on attachment 8660344 [details] [diff] [review]
P7 Fix e10s handling on interceptions resulting in redirect status codes. r=jdm

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

This should go through Honza; sorry.
Attachment #8660344 - Flags: review?(josh) → review+
Comment on attachment 8660338 [details] [diff] [review]
P4 Fix bug in nsStorageStream with reading streams created before data is populated. r=froydnj

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

Thank you for writing the test!

::: xpcom/io/nsStorageStream.cpp
@@ +463,5 @@
> +      // If mSegmentEnd is 0 and yet there is still data available in the
> +      // stream, that means this is the first read for stream that was
> +      // constructed before data was appended to the stream.  Skip moving
> +      // to the next segment so we can initialize our state and perform
> +      // the initial read.

The wording of this comment confuses me.  How about:

// We have data in the stream, but if mSegmentEnd is zero, then we
// were likely constructed prior to any data being written into
// the stream.  Therefore, if mSegmentEnd is non-zero, we should
// move into the next segment; otherwise, we should stay in this
// segment so our input state can be updated and we can properly
// perform the initial read.
Attachment #8660338 - Flags: review?(nfroyd) → review+
(In reply to Josh Matthews [:jdm] from comment #16)
> Comment on attachment 8660344 [details] [diff] [review]
> P7 Fix e10s handling on interceptions resulting in redirect status codes.
> r=jdm
> 
> Review of attachment 8660344 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This should go through Honza; sorry.

I looked closer at the changes, and I no longer believe it's necessary to get some network eyes on this. This is just aligning e10s and non-e10s behaviour.
Attachment #8660336 - Attachment is obsolete: true
That treeherder link in comment 22 did not work for me.  This shows the problem, though:

  https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=e4733b9eb53c

I have no idea how my changes could effect test_interfaces.html.  I can't reproduce it locally.  Lets see if try shows it:

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

I'm guessing this was really caused by a different patch in inbound.
Flags: needinfo?(bkelly)
Actually, this is a better example of the error:

  https://treeherder.mozilla.org/logviewer.html#?job_id=14090040&repo=mozilla-inbound

Unrelated to that test_interfaces.html spam.
Ah, bug 1198544 landed after this bug last night.  I think the error in comment 24 was essentially the problem in bug 1198544 and occurred more frequently in e10s.  Now that bug 1198544 is fixed I can't reproduce any more.

If the try build is green I will just reland.
Actually the failure persists.  Its bug 1174872.  I will leave the test disabled on e10s for now and re-enable it in bug 1174872 instead.
Actually, bug 1174872 is ostensibly about non-cors, so I will fix the CORS issue here.
Attachment #8661336 - Flags: review?(ehsan) → review+
You need to log in before you can comment on or make changes to this bug.