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

VERIFIED FIXED in Firefox 46

Status

()

defect
VERIFIED FIXED
3 years ago
3 years ago

People

(Reporter: klaus_trainer, Assigned: bkelly)

Tracking

(Blocks 1 bug, {testcase})

46 Branch
mozilla48
Points:
---

Firefox Tracking Flags

(firefox45 wontfix, firefox46 fixed, firefox47 fixed, firefox48 fixed)

Details

Attachments

(2 attachments, 5 obsolete attachments)

(Reporter)

Description

3 years ago
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.
(Reporter)

Comment 1

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

Updated

3 years ago
Component: Untriaged → DOM: Service Workers
Product: Firefox → Core

Updated

3 years ago
Keywords: testcase
(Reporter)

Comment 2

3 years ago
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.
I'll see if I can figure this out in time to uplift for FF46 release in April.  Thanks for the report!
Assignee: nobody → bkelly
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
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.
Attachment #8728023 - Flags: review?(josh)
This fails on current mozilla-central, but passes with the P1 patch applies.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ad6f741cec1a
Attachment #8728077 - Flags: review?(josh)
We should get this uplifted to beta 46.
This time with actual test files included.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9fd25886f478
Attachment #8728077 - Attachment is obsolete: true
Attachment #8728077 - Flags: review?(josh)
Attachment #8728087 - Flags: review?(josh)
Attachment #8728087 - Attachment is obsolete: true
Attachment #8728087 - Flags: review?(josh)
Attachment #8728092 - Flags: review?(josh)
(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.
(Reporter)

Comment 10

3 years ago
(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 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.
Attachment #8728023 - Flags: review?(josh)
(Assignee)

Updated

3 years ago
Attachment #8728092 - Flags: review?(josh)
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.
Attachment #8728023 - Attachment is obsolete: true
Attachment #8728483 - Flags: review?(josh)
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
Attachment #8728092 - Flags: review?(josh)
Try looks pretty green.
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/.
Attachment #8728483 - Flags: review?(josh) → review+
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.
Attachment #8728092 - Flags: review?(josh) → review+
(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 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.
Attachment #8728615 - Flags: approval-mozilla-beta?
Attachment #8728615 - Flags: approval-mozilla-aurora?
Comment on attachment 8728616 [details] [diff] [review]
P2 Test passing a file-backed blob to FetchEvent.respondWith(). r=jdm

See comment 21.
Attachment #8728616 - Flags: approval-mozilla-beta?
Attachment #8728616 - Flags: approval-mozilla-aurora?

Comment 23

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/afa617af6717
https://hg.mozilla.org/mozilla-central/rev/7c166ec445b3
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
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!
Flags: needinfo?(klaus_trainer)
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.
Attachment #8728615 - Flags: approval-mozilla-beta?
Attachment #8728615 - Flags: approval-mozilla-beta+
Attachment #8728615 - Flags: approval-mozilla-aurora?
Attachment #8728615 - Flags: approval-mozilla-aurora+
Attachment #8728616 - Flags: approval-mozilla-beta?
Attachment #8728616 - Flags: approval-mozilla-beta+
Attachment #8728616 - Flags: approval-mozilla-aurora?
Attachment #8728616 - Flags: approval-mozilla-aurora+
(Reporter)

Comment 26

3 years ago
Thanks :) I'll test it this weekend and report!
(Reporter)

Comment 27

3 years ago
Just verified that this is fixed as expected. Thanks everybody :)
Flags: needinfo?(klaus_trainer)
(In reply to klaus_trainer from comment #27)
> Just verified that this is fixed as expected. Thanks everybody :)

Great!
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.