Closed Bug 1424701 Opened 2 years ago Closed 2 years ago

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

Categories

(Core :: DOM: File, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox59 --- fixed

People

(Reporter: jaffathecake, Assigned: baku)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

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.
This works in non-e10s mode, but fails in e10s mode.
Summary: Posting a file fails if there's a service worker → Posting a file fails if there's a service worker in e10s mode
Clarify this is a pass-through FetchEvent handler.
Summary: Posting a file fails if there's a service worker in e10s mode → Posting a file fails with pass-through FetchEvent handler in e10s mode
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.
Flags: needinfo?(amarchesini)
Component: Untriaged → DOM: File
Product: Firefox → Core
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.
Assignee: nobody → amarchesini
Flags: needinfo?(amarchesini)
Apologies for all the cloning in the original test, I was trying to see at what point the browser loses track of the body.
(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!
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
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.
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Flags: needinfo?(bkelly)
Resolution: --- → DUPLICATE
Duplicate of bug: 1421094
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.
Status: RESOLVED → REOPENED
Depends on: 1421094
Flags: needinfo?(bkelly) → needinfo?(amarchesini)
Resolution: DUPLICATE → ---
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
Flags: needinfo?(amarchesini)
It would be nice to have a browser chrome test I guess, then.
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?
Attached patch test.patchSplinter Review
Attachment #8941413 - Flags: review?(bkelly)
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().
Attachment #8941413 - Flags: review?(bkelly) → review+
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/479b32d510f5
Test for uploading files via ServiceWorker, r=bkelly
https://hg.mozilla.org/mozilla-central/rev/479b32d510f5
Status: REOPENED → RESOLVED
Closed: 2 years ago2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
You need to log in before you can comment on or make changes to this bug.