1424701 - Posting a file fails with pass-through FetchEvent handler in e10s mode

Status: RESOLVED FIXED

(Core :: DOM: File, defect)

Description

[Jake Archibald]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_12_6) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/62.0.3202.94 Safari/537.36

Steps to reproduce:

https://post-file-serviceworker-test.glitch.me/

1. Click the file input and select a file.
1. JS will post the file to the server.
1. See the console.

Without the service worker (shift+reload) the server posts the file back. However, this fails when the service worker is controlling the page.

The service worker sees the body & can read it, but it appears to get lost when the actual fetch happens.

I added a "Send hello world blob" button which sends a JS-constructed blob. This appears to work fine, so the problem isn't universal to all blobs.

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

This works in non-e10s mode, but fails in e10s mode.

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

Clarify this is a pass-through FetchEvent handler.

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

Andrea, do you have any ideas here?

I made a slightly different test case that avoids the clone() to simplify what is going on:

https://post-file-serviceworker-test-wanderview.glitch.me

I verified with instrumentation that we are passing the upload body stream through the service worker to the FetchDriver's http channel correctly:

==> outer window fetch()
### ### [0x7f45a0b1d6b0] FetchDriver::HttpFetch upload stream
### ### [0x7f45a0b1d6b0] FetchDriver::HttpFetch upload body 0x7f459f91ba60

==> intercept nsIChannel and create FetchEvent.request upload stream
### ### [0x7f45a159f028] HttpBaseChannel::ExplicitSetUploadStream replace upload 0x7f459f91ba60 with PariallySeekableInputStream() 0x7f45a0afeb08
### ### [0x7f45c7859120] ServiceWorkerManager::DispatchFetchEvent calling EnsureUploadStreamIsCloneable
### ### [0x7f45a159f028] HttpBaseChannel::EnsureUploadStreamIsCloneable already cloneable
### ### [0x7f459f9fee70] ContinueDispatchFetchEventRunnable::Run
### ### [0x7f45a159f028] HttpBaseChannel::CloneUploadStream clone 0x7f45a0afeb08 to 0x7f45a0aff108
### ### [0x7f45a0ab5200] FetchEventRunnable::DispatchFetchEvent setting upload body 0x7f45a0aff108

==> service worker fetch(event.request)
### ### [0x7f45a0b1d7a0] FetchDriver::HttpFetch upload stream
### ### [0x7f45a0b1d7a0] FetchDriver::HttpFetch upload body 0x7f45a0aff108
### ### [0x7f45a0aff100] PartiallySeekableInputStream::Serialize

==> parent-side of the SW fetch()
### ### DeserializeInputStream blob input stream
### ### [0x7f382c3867a0] HttpChannelParent::DoAsyncOpen deserialized stream 0x7f38305b0e08
[Parent 19069, StreamTrans #20] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80470002: file /srv/mozilla-central/netwerk/base/nsFileStreams.cpp, line 74
JavaScript error: , line 0: TypeError: NetworkError when attempting to fetch resource.

It seems deserializing this kind of file blob is not working right.

Note, I'm not 100% sure if that last nsFileStreams.cpp error is due to this request or not.

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

Note, according to the network monitor we are setting the content length correctly on the service worker fetch(event.request) channel.  I guess that's explicitly set by FetchDriver after it extracts it from the blob in the child, though.

Comment 5

[Jake Archibald]

Apologies for all the cloning in the original test, I was trying to see at what point the browser loses track of the body.

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Jake Archibald from comment #5)
> Apologies for all the cloning in the original test, I was trying to see at
> what point the browser loses track of the body.

No problem.  Turns out it didn't matter much since we do a clone internally when performing a service worker interception.  Thanks for the reduced test case!

Comment 7

[Andrea Marchesini [:baku]]

bkelly, can you reproduce it with the latest nightly? I think I fixed the problem in bug 1421094 making nsIUploadChannel2::cloneUploadStream to pass the length of the stream (content-length).
I mark it as duplicate but in case I'm wrong, reopen it. Maybe we should uplift bug 1421094.

Comment 8

[Ben Kelly [:bkelly, not reviewing]]

Yes, its fixed for me now.  Thanks!

I think uplifting would be nice, but not worth it if the patches are risky.  Currently chrome fails this test case as well.

Can you please write a WPT test for this, though?  I'm going to reopen this bug to do that.

Comment 9

[Andrea Marchesini [:baku]]

Currently it's not possible to simulate a file selection for a <input type=file> in a WPT.
See https://github.com/w3c/web-platform-tests/issues/8114

Comment 10

[Ben Kelly [:bkelly, not reviewing]]

It would be nice to have a browser chrome test I guess, then.

Comment 11

[Jake Archibald]

Maybe there's a way to create this failure case without a file input? Maybe getting the blob from IndexedDB or the cache API would do it?

Comment 12

[Andrea Marchesini [:baku]]

Attachment: test.patch

Comment 13

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8941413 [details] [diff] [review]
test.patch

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

Thanks!

::: dom/workers/test/serviceworkers/sw_file_upload.js
@@ +1,4 @@
> +self.skipWaiting();
> +
> +addEventListener('activate', event => {
> +  event.waitUntil(self.clients.claim());

Move this to a message event handler per my comments on test_file_upload.html.

::: dom/workers/test/serviceworkers/test_file_upload.html
@@ +12,5 @@
> +<body>
> +<input id="input" type="file">
> +<script class="testbody" type="text/javascript">
> +
> +function onOpened(message) {

nit: This could be a bit easier to read if it was an async function.

@@ +18,5 @@
> +  SpecialPowers.wrap(input).mozSetFileArray([message.file]);
> +  script.destroy();
> +
> +  let reg;
> +  navigator.serviceWorker.register('sw_file_upload.js')

It would be nice to use a scope unique to this test.  Since you only need it to control this test file, can you try using window.location.href as the scope?

@@ +21,5 @@
> +  let reg;
> +  navigator.serviceWorker.register('sw_file_upload.js')
> +  .then(swr => {
> +    reg = swr;
> +    return fetch('server_file_upload.sjs', {

This is racy and will likely fail a --verify test run.

You need to move the clients.claim() to a message event handler in the service worker and then do something like this:

https://searchfox.org/mozilla-central/rev/03877052c151a8f062eea177f684a2743cd7b1d5/dom/workers/test/serviceworkers/test_fetch_integrity.html#55-58

See utils.js for waitForState() and waitForControlled().

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/479b32d510f5
Test for uploading files via ServiceWorker, r=bkelly

Comment 15

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/479b32d510f5