Ben Kelly [:bkelly, not reviewing] Assignee
	Description • 9 years ago

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

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 1 • 9 years ago

Move P5 test patch from bug 1093357 over to here.  Address nsm's review comments and carry forward r+.

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 2 • 9 years ago

Also move the NS_InputStreamIsCloneable() patch from bug 1093357 over to here.

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 3 • 9 years ago

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.

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 4 • 9 years ago

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.

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 5 • 9 years ago

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.

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 6 • 9 years ago

I'm also working on re-enabling some mochitests that were disabled in e10s due to the pipe serialization issue.

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)
	Comment 7 • 9 years ago

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.

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 8 • 9 years ago

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

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 9 • 9 years ago

Address review feedback.

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 10 • 9 years ago

Update storage stream fix patch to remove some debug.

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 11 • 9 years ago

Another update to the P5 patch to unbreak opt builds.

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

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 12 • 9 years ago

Remove debug for real this time.

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 13 • 9 years ago

Re-enable e10s mochitest.  Note, this requires a further fix coming in P7.

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 14 • 9 years ago

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.

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 15 • 9 years ago

Fix NS_IMETHOD override signature to fix windows build.

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

	Josh Matthews [:jdm]
	Comment 16 • 9 years ago

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.

	Nathan Froyd [:froydnj]
	Comment 17 • 9 years ago

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.

	Josh Matthews [:jdm]
	Comment 18 • 9 years ago

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

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 19 • 9 years ago

Address review feedback.

	Pulsebot
	Comment 20 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/2369d63ef14a
https://hg.mozilla.org/integration/mozilla-inbound/rev/6fedc85dc0d9
https://hg.mozilla.org/integration/mozilla-inbound/rev/311f9810e0b3
https://hg.mozilla.org/integration/mozilla-inbound/rev/1026da4b02fb
https://hg.mozilla.org/integration/mozilla-inbound/rev/86642d84e604
https://hg.mozilla.org/integration/mozilla-inbound/rev/eb42e21bbb96

	Pulsebot
	Comment 21 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/e4733b9eb53c

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 22 • 9 years ago

All backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/21b8c95e507a for mochitest-4 bustage: https://treeherder.mozilla.org/logviewer.html#?job_id=14087313&repo=mozilla-inbound

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 23 • 9 years ago

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.

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 24 • 9 years ago

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.

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 25 • 9 years ago

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.

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 26 • 9 years ago

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.

	Ben Kelly [:bkelly, not reviewing] Assignee
	Comment 27 • 9 years ago

Actually, bug 1174872 is ostensibly about non-cors, so I will fix the CORS issue here.

	Pulsebot
	Comment 29 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/1231d6e5af3e
https://hg.mozilla.org/integration/mozilla-inbound/rev/4d35af8a3b5d
https://hg.mozilla.org/integration/mozilla-inbound/rev/278ddd4793c3
https://hg.mozilla.org/integration/mozilla-inbound/rev/0c0bfeb70612
https://hg.mozilla.org/integration/mozilla-inbound/rev/52093388544a
https://hg.mozilla.org/integration/mozilla-inbound/rev/46dc7055da66
https://hg.mozilla.org/integration/mozilla-inbound/rev/2ceadfc720f2
https://hg.mozilla.org/integration/mozilla-inbound/rev/e38a5ece0995

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 30 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/1231d6e5af3e
https://hg.mozilla.org/mozilla-central/rev/4d35af8a3b5d
https://hg.mozilla.org/mozilla-central/rev/278ddd4793c3
https://hg.mozilla.org/mozilla-central/rev/0c0bfeb70612
https://hg.mozilla.org/mozilla-central/rev/52093388544a
https://hg.mozilla.org/mozilla-central/rev/46dc7055da66
https://hg.mozilla.org/mozilla-central/rev/2ceadfc720f2
https://hg.mozilla.org/mozilla-central/rev/e38a5ece0995