1203760 - XHR hangs when attempting to cache in service worker

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Brendan Dahl [:bdahl]]

STR:
1) Load https://people.mozilla.org/~bdahl/sandbox/service_worker_cache_bug/
2) Observe the message on the page "okay now do a regular reload"
3) Reload the page (not force/shift reload)

Result:
After reload the message becomes "waiting for xhr" and never does anything else.

Expected:
After reload the message becomes
"waiting for xhr
xhr progress
xhr load
success"

Notes from conversation with nsm:
- The test works if I comment out sw.js line "cache.put(event.request, responseToCache);"
- The test also works if I wait to return the response until after the cache.put e.g. |return caches.open('...').then(() => { return caches.put(...).then(() { return response; }|

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

It could be bug 1134372.  Can you try with a resource under 64kb in size?

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

It also works in e10s mode.  Interesting.

It could still be bug 1134372, but I'm not sure why the two different halves of the clone would be reading at different rates.

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

Hmm, its unlikely to be bug 1134372.  I opened this in a local build and looked at the files in the profile.  The full pdf was downloaded AFAICT.

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

And it does not work in e10s.  It just works if it was previously run, stopped, and then re-run since the Cache file is already there.

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

I think this is a pipe bug:

  [13388] ###!!! ASSERTION: already have an input event: '!mInputCallback', file c:/devel/mozilla-cent
ral/xpcom/io/nsPipe3.cpp, line 84

This happens when an nsPipeEvents RAII object tries to register more than one callback.  This previously was impossible until I changed it to support multiple input streams for cloning.  Now multiple streams can be woken up simultaneously for a single write to the pipe.

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Allow pipe to wake up multiple streams at the same time. r=froydnj

This fixes the problem for me locally.

Comment 7

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Add gtests for nsPipeInputStream AsyncRead(). r=froydnj

This test causes our current tree to hit the same assertion as comment 5.  With the P1 fix we pass the test.

And now we actually have a gtest for nsPipeInputStream::AsyncRead(). :-\

Comment 8

[Nathan Froyd [:froydnj]]

Comment on attachment 8659641 [details] [diff] [review]
P1 Allow pipe to wake up multiple streams at the same time. r=froydnj

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

r=me with the template cleverness removed.

::: xpcom/io/nsPipe3.cpp
@@ +93,5 @@
>    }
>  
>  private:
> +  template <typename S, typename C>
> +  struct Entry

Why are we getting all clever with templates here?  Is this because we're thinking about:

Entry<nsIAsyncOutputStream, nsIOutputStreamCallback>

?  I don't think you need to worry about that...

Comment 9

[Nathan Froyd [:froydnj]]

Comment on attachment 8659644 [details] [diff] [review]
P2 Add gtests for nsPipeInputStream AsyncRead(). r=froydnj

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

Hooray for tests.

Comment 10

[Ben Kelly [:bkelly, not reviewing]]

These patches were included in this try build:

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

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6d24b09bb144
https://hg.mozilla.org/integration/mozilla-inbound/rev/e2cdb157f476

Comment 12

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

https://hg.mozilla.org/mozilla-central/rev/6d24b09bb144
https://hg.mozilla.org/mozilla-central/rev/e2cdb157f476