1253777 - Service Worker throws an error when the Response contains a Blob from IndexedDB

Status: VERIFIED FIXED

(Core :: DOM: Service Workers, defect)

Description

[klaus_trainer]

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:44.0) Gecko/20100101 Firefox/44.0
Build ID: 20160209234642

Steps to reproduce:

When resolving `event.respondWith` in a Service Worker fetch event handler with a Response containing a Blob read from IndexedDB, the Service Workers throws an error.

See the following demo for reproducing that issue: https://klaustrainer.github.io/service-worker-indexed-db-demo/
Source code for the demo: https://github.com/KlausTrainer/service-worker-indexed-db-demo


Actual results:

The resource isn't loaded, and the console displays the following error message: 'A ServiceWorker intercepted the request and encountered an unexpected error.'


Expected results:

I expect the resource to be loaded without an error.

Comment 1

[klaus_trainer]

I've found this issue in Firefox version 44.0.2. When checking with version 46.0a2 I get an identical result.

Comment 2

[klaus_trainer]

Another interesting detail:

When resolving `event.respondWith` with a cloned response (e.g. `new Response(blob).clone()`), the described error doesn't occur anymore. However, with that, the response won't terminate sometimes (seems to depend on the blob size). That is, the resource doesn't get loaded either then.

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

I'll see if I can figure this out in time to uplift for FF46 release in April.  Thanks for the report!

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Buffer Response.body stream if necessary in FetchEvent.respondWith(). r=jdm

This fixes the problem for me.  The nsFileInputStream throws NS_ERROR_NOT_IMPLEMENTED for ::ReadSegments().  It seems the output stream provided by the nsIInterceptedChannel does not always provide ::WriteSegments(), so we can't just flip the constant to NS_AsyncCopy().

Instead, we must wrap in the input stream in a buffered stream in order for the NS_AsyncCopy() to succeed.

I did consider doing this wrapping immediately when the Response is constructed, but it seems more standard to do it only where necessary.  The other main consumer is the Response.text() method and friends.  These use the STS->CreateInputTransport() mechanism to read the data which I'm told handles unbuffered streams well.

I will write a test next.

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Test passing a file-backed blob to FetchEvent.respondWith(). r=jdm

This fails on current mozilla-central, but passes with the P1 patch applies.

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

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

We should get this uplifted to beta 46.

Comment 7

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Test passing a file-backed blob to FetchEvent.respondWith(). r=jdm

This time with actual test files included.

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

Comment 8

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Test passing a file-backed blob to FetchEvent.respondWith(). r=jdm

Comment 9

[Ben Kelly [:bkelly, not reviewing]]

(In reply to klaus_trainer from comment #2)
> Another interesting detail:
> 
> When resolving `event.respondWith` with a cloned response (e.g. `new
> Response(blob).clone()`), the described error doesn't occur anymore.
> However, with that, the response won't terminate sometimes (seems to depend
> on the blob size). That is, the resource doesn't get loaded either then.

This worked around the issue because .clone() copies the file stream into an nsPipe internally.

I'm guessing the broken states were if the blob exceeded 64kb in size?  That would be consistent with bug 1134372.

Comment 10

[klaus_trainer]

(In reply to Ben Kelly [:bkelly] from comment #9)
> (In reply to klaus_trainer from comment #2)
> > Another interesting detail:
> > 
> > When resolving `event.respondWith` with a cloned response (e.g. `new
> > Response(blob).clone()`), the described error doesn't occur anymore.
> > However, with that, the response won't terminate sometimes (seems to depend
> > on the blob size). That is, the resource doesn't get loaded either then.
> 
> This worked around the issue because .clone() copies the file stream into an
> nsPipe internally.
> 
> I'm guessing the broken states were if the blob exceeded 64kb in size?  That
> would be consistent with bug 1134372.

Yeah, that seems to be the case. I just checked with my demo app where there are lots of images of different sizes, and only the larger ones don't get loaded.

Comment 11

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8728023 [details] [diff] [review]
P1 Buffer Response.body stream if necessary in FetchEvent.respondWith(). r=jdm

There are some unexpected failures on try.  Dropping review while I investigate.

Comment 12

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Ensure buffered copying when reading body in service worker respondWith(). r=jdm

Apparently some of the input stream types we see do not play well with NS_InputStreamIsBuffered() and NS_NewBufferedInputStream().  I will file a follow-up bug on that.

For this case, though, we can simply wrap the output stream side if necessary.  This should work consistently since we should see only one of two different stream types depending on e10s configuration.

Comment 13

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8728092 [details] [diff] [review]
P2 Test passing a file-backed blob to FetchEvent.respondWith(). r=jdm

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6e8f8b67f243&selectedJob=17829381

Comment 14

[Ben Kelly [:bkelly, not reviewing]]

Try looks pretty green.

Comment 15

[Josh Matthews [:jdm]]

Comment on attachment 8728483 [details] [diff] [review]
P1 Ensure buffered copying when reading body in service worker respondWith(). r=jdm

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

::: dom/workers/ServiceWorkerEvents.cpp
@@ +635,5 @@
> +    // get a non-buffered input stream.  In addition, in some configurations the
> +    // destination channel's output stream can be unbuffered.  We wrap the output
> +    // stream side here so that NS_AsyncCopy() works.  Wrapping the output side
> +    // provides the most consistent operation since there are fewer stream types
> +    // we are writing too.  The input stream can be a wide variety of concrete

s/too/to/.

Comment 16

[Josh Matthews [:jdm]]

Comment on attachment 8728092 [details] [diff] [review]
P2 Test passing a file-backed blob to FetchEvent.respondWith(). r=jdm

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

I'm assuming that the File we get back from IndexedDB has different properties than `new File([JSON.stringify(obj)], "test", { type: 'application/json' })` that end up tickling the code from the previous patch.

::: dom/workers/test/serviceworkers/test_file_blob_response.html
@@ +23,5 @@
> +        registration = swr;
> +        return new Promise(function(resolve) {
> +          registration.installing.onstatechange = function(evt) {
> +            if (evt.target.state === 'activated') {
> +              registration.onstate = null;

This doesn't look like it's doing anything.

@@ +50,5 @@
> +      content.appendChild(frame);
> +
> +      SimpleTest.requestFlakyTimeout('There is no way to receive an event when ' +
> +        'an iframe fails to load.  We must use a timeout.');
> +      var timeout = setTimeout(function() {

The test will time out on its own if the load listener isn't called. Let's not add redundant flaky timeouts too.

Comment 17

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Josh Matthews [:jdm] from comment #16)
> I'm assuming that the File we get back from IndexedDB has different
> properties than `new File([JSON.stringify(obj)], "test", { type:
> 'application/json' })` that end up tickling the code from the previous patch.

I'm not sure, but I'd like to test the IDB path since that is what people are hitting in the wild.

Comment 18

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Ensure buffered copying when reading body in service worker respondWith(). r=jdm

Comment 19

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Test passing a file-backed blob to FetchEvent.respondWith(). r=jdm

Comment 20

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/afa617af6717
https://hg.mozilla.org/integration/mozilla-inbound/rev/7c166ec445b3

Comment 21

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8728615 [details] [diff] [review]
P1 Ensure buffered copying when reading body in service worker respondWith(). r=jdm

Approval Request Comment
[Feature/regressing bug #]: Service workers
[User impact if declined]: This is a web compat problem preventing people from using IndexedDB in service workers.  We should try to fix this as soon as possible to avoid further problems in the wild.
[Describe test coverage new/current, TreeHerder]: Test included in P2 patch.
[Risks and why]: Minimal.  We have extensive test coverage of this code for other cases and the new test covers this particular issue.  Any regression would be limited to service workers.
[String/UUID change made/needed]: None.

Comment 22

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8728616 [details] [diff] [review]
P2 Test passing a file-backed blob to FetchEvent.respondWith(). r=jdm

See comment 21.

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/afa617af6717
https://hg.mozilla.org/mozilla-central/rev/7c166ec445b3

Comment 24

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

klaus_trainer, could you please verify this issue is fixed as expected on 03-11 Nightly build (should be available in next 12 hours)? Thanks!

Comment 25

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8728615 [details] [diff] [review]
P1 Ensure buffered copying when reading body in service worker respondWith(). r=jdm

Has an automated test, taking it.

Comment 26

[klaus_trainer]

Thanks :) I'll test it this weekend and report!

Comment 27

[klaus_trainer]

Just verified that this is fixed as expected. Thanks everybody :)

Comment 28

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/668216085fce
https://hg.mozilla.org/releases/mozilla-aurora/rev/4ce28157620f

Comment 29

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-beta/rev/c6ad5532850f
https://hg.mozilla.org/releases/mozilla-beta/rev/9c123a44dc6b

Comment 30

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

(In reply to klaus_trainer from comment #27)
> Just verified that this is fixed as expected. Thanks everybody :)

Great!