1351231 - Implement PFetch protocol and backing FetchService to allow Workers to request fetches without involving the main thread of their process

Status: RESOLVED FIXED

(Core :: DOM: Networking, task, P3)

Description

[Ben Kelly [:bkelly, not reviewing]]

Currently fetch() using nsIChannel on the main thread in the current process.  This means that a worker thread (like service worker) can hit main thread jank when it tries to perform a network request.  This is particularly bad for service workers since delaying the fetch() may in turn delay the network result for the main page load.

The network team is working on making nsIChannel support PBackground so it can be used OMT.  That will probably take a long time to fully happen, however.

In addition, our service worker e10s refactor will move the service worker instances to a new process.  That will also take some time to happen.  Its also possible the memory overhead of this new process will be too large.

An alternative we could do in the short term would be to implement a separate PBackground actor just for fetch().  We already support serializing Request and Response objects across IPC in the Cache API.  In addition, Andrea recently landed parent-to-child AutoIPCStream support.  Between these it might be easy to use PBackground to go to parent, proxy to main thread there, and then create the nsIChannel.

If we did this, though, we would not be able to fully remove the current implementation.  A PBackground fetch() would bypass the child-side service worker intercept today.  We would only be able to use it in fetch() calls from within the service worker global itself.  Eventually we could remove the old fetch() implementation after SWM is moved to the parent.

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

We may want to do this even with the necko changes.  Right now I think they are only making OnDataAvailable() work OMT which means we still need to touch main thread for On(Start|Stop)Request().

Another factor to consider, though, is we would need to do something special for devtools.  It currently requires nsIChannel in order to show up in the network monitor.

Comment 2

[Andrew Sutherland [:asuth] (he/him)]

Note: From a conversation with Dragana on Feb 14, 2019, I understand the plan is to make AsyncOpen, OnStart, and OnStop work off the main thread, so this bug probably wants to be duped.

Comment 3

[Andrew Sutherland [:asuth] (he/him)]

See https://bugzilla.mozilla.org/show_bug.cgi?id=1613912#c4 for the plan.

Comment 4

[Anne (:annevk)]

Does this block parsing worklets as module scripts, i.e., bug 1572644?

Comment 5

[Andrew Sutherland [:asuth] (he/him)]

(In reply to Anne (:annevk) from comment #4)

Does this block parsing worklets as module scripts, i.e., bug 1572644?

No. One could implement bug 1311726 while still performing the current "let's do all the network stuff on the main thread" behavior. Noting that right now the worklet script loader just assumes the one single invocation of WorkletFetchHandler that feeds into the module compilation.

The network-on-the-main-thread uses could then subsequently be replaced with PFetch later on.

Comment 6

[Eden Chuang[:edenchuang]]

Attachment: Bug 1351231 - Preference for PFetch. r=#dom-worker-reviewers.

Comment 7

[Eden Chuang[:edenchuang]]

Attachment: Bug 1351231 - Support conversation between InternalRequest and IPCInternalRequest. r=#dom-worker-reviewers

Depends on D138812

Comment 8

[Eden Chuang[:edenchuang]]

Attachment: Bug 1351231 - Preference for PFetch. r=#dom-worker-reviewers.

Comment 9

[Eden Chuang[:edenchuang]]

Attachment: Bug 1351231 - PFetch protocol declaration and implementation. r=#dom-worker-reviewers

Depends on D138813

Comment 10

[Eden Chuang[:edenchuang]]

Attachment: Bug 1351231 - FetchService integration for PFetch. r=#dom-worker-reviewers

Depends on D142436

Comment 11

[Eden Chuang[:edenchuang]]

Attachment: Bug 1351231 - Integrate FetchChild into Fetch.cpp r=#dom-worker-reviewers

Depends on D142437

Comment 12

[Andrew Sutherland [:asuth] (he/him)]

Various Investigation Notes

Re: the hand-off try push https://treeherder.mozilla.org/jobs?repo=try&selectedTaskRun=YUCMBF2yRbmQ5HHlTS4EIQ.0&revision=e1d06a199d576b745b6fa496312dcaa59b13f470

• A bunch of the last bunch of the try failures turn out to have been bug 1777181, but this isn't to say the tests Eden identified as failing won't actually still be failing on try with those gone.
• browser_download_canceled.js has a reproducible failure for me locally. It also has some (low occurrence) intermittent test bugs: single tracker bug 1776002 and Got "timeout", expected "canceled" bug 1771953.
  • I reproduced locally and here's a pernosco trace: https://pernos.co/debug/mKbEio8ZspzHWJfbhGV2TQ/index.html
  • I need to do more investigation here, but it looks a lot like at least what I reproduced may be a bug in the test. Specifically, nsExternalAppHandler::SaveDestinationAvailable is being invoked with a non-null target of my downloads folder which is what happens if you don't cancel the download.
    • This raises questions about how the test ever passes in automation. It does seem that we have explicitly disabled the test under "verify" so a theory I plan to disprove next is that browser_download_canceled only works in automation because a prior download-related test properly sets some configuration option that carries over to browser_download_canceled and if you run it on its own it's missing that inherited flag, and so it fails. (I believe verify only ever runs the test itself which would have identical semantics.)
    • In this case it's possible there will still be a PFetch bug after addressing any test issues; the background file saver intentionally invokes NS_AsyncCopy with aCloseSource=false because of the pivoting mechanism it uses (first it downloads to a temporary file, then when the user picks a final destination, the download is pivoted to the new target file), and there could be some edge-cases related to this. A notable thing here is that there is really an incredible level of useful discussion in the original bug at https://bugzilla.mozilla.org/show_bug.cgi?id=789932#c8 around this (and I think the code is also unusually well commented as well).
    • I need to call it a day but I'll dig more into that tomorrow.

Comment 13

[Andrew Sutherland [:asuth] (he/him)]

Also, I stood up a searchfox index on the "dev" channel for review related aspects of landing this, so that's https://dev.searchfox.org/

• https://dev.searchfox.org/mozilla-central/source/dom/fetch/PFetch.ipdl is a potentially good starting point
• The "query" endpoint diagram stuff works for this channel too, example query of calls to FetchParent::RecvFetchOp, but it's the same thing that's in production currently. (I may re-run with an experimental branch if I find some hobby time to finish updating cmd_traverse so the new binding slot mechanism can allow us to continue properly traversing the Send/Recv edges.)

Comment 14

[BugBot [:suhaib / :marco/ :calixte]]

There are some r+ patches which didn't land and no activity in this bug for 2 weeks.
:edenchuang, could you have a look please?
If you still have some work to do, you can add an action "Plan Changes" in Phabricator.
For more information, please visit auto_nag documentation.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d8fa8a006948
Preference for PFetch. r=dom-worker-reviewers,jesup.
https://hg.mozilla.org/integration/autoland/rev/5c3b5a384f2f
Support conversation between InternalRequest and IPCInternalRequest. r=dom-worker-reviewers,jesup
https://hg.mozilla.org/integration/autoland/rev/84162d09eef8
PFetch protocol declaration and implementation. r=dom-worker-reviewers,jesup
https://hg.mozilla.org/integration/autoland/rev/c0cb3c17f246
FetchService integration for PFetch. r=dom-worker-reviewers,jesup
https://hg.mozilla.org/integration/autoland/rev/cc26eeeaf3dd
Integrate FetchChild into Fetch.cpp r=dom-worker-reviewers,jesup

Comment 16

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ec41ebc45aec
Backed out 5 changesets for Fetch related failures. CLOSED TREE

Comment 17

[Serban Stanca [:SerbanS]]

Backed out for Fetch related failures.

• Backout link
• Push with failures
• Failure Log
• Failure line: PROCESS-CRASH | /fetch/api/request/request-bad-port.any.sharedworker.html | application crashed [@ (anonymous namespace)::ParentImpl::AssertIsOnBackgroundThread()]

*It also failed on some mochitests jobs. Failure log
*It also failed like this. Failure log

Comment 18

[Serban Stanca [:SerbanS]]

And also failed on this mochitests job. Failure log

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fd2a843599d1
Preference for PFetch. r=dom-worker-reviewers,jesup.
https://hg.mozilla.org/integration/autoland/rev/a1fcf22a2a0e
Support conversation between InternalRequest and IPCInternalRequest. r=dom-worker-reviewers,jesup
https://hg.mozilla.org/integration/autoland/rev/3eb8fdd0ba6d
PFetch protocol declaration and implementation. r=dom-worker-reviewers,jesup
https://hg.mozilla.org/integration/autoland/rev/66b279e5a513
FetchService integration for PFetch. r=dom-worker-reviewers,jesup
https://hg.mozilla.org/integration/autoland/rev/da5c4a821428
Integrate FetchChild into Fetch.cpp r=dom-worker-reviewers,jesup

Comment 20

[Atila Butkovits]

Backed out for causing fetch related failures.

Backout link: https://hg.mozilla.org/integration/autoland/rev/54f998c4a93fa059fc310b33dc41ca2f3578ccc5

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel&revision=da5c4a8214280ad3e50eb955a50feb47ccc4b65c&selectedTaskRun=Uc7Au2jJQYqMLhTzS7k5MA.0

Failure log: https://treeherder.mozilla.org/logviewer?job_id=402212374&repo=autoland&lineNumber=1812

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8c21c9468949
Preference for PFetch. r=dom-worker-reviewers,jesup.
https://hg.mozilla.org/integration/autoland/rev/ea27cd66fefd
Support conversation between InternalRequest and IPCInternalRequest. r=dom-worker-reviewers,jesup
https://hg.mozilla.org/integration/autoland/rev/dbdca4661a35
PFetch protocol declaration and implementation. r=dom-worker-reviewers,jesup
https://hg.mozilla.org/integration/autoland/rev/6cbb442f4772
FetchService integration for PFetch. r=dom-worker-reviewers,jesup
https://hg.mozilla.org/integration/autoland/rev/a41252141399
Integrate FetchChild into Fetch.cpp r=dom-worker-reviewers,jesup

Comment 22

[Marian-Vasile Laza]

Backed out 5 changesets (Bug 1351231) for causing multiple wpt failures.
Backout link
Push with failures <--> wpt7
Failure Log
Also wpt7 Failure Log

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/19b78046ffef
Preference for PFetch. r=dom-worker-reviewers,jesup.
https://hg.mozilla.org/integration/autoland/rev/9da00c1364a5
Support conversation between InternalRequest and IPCInternalRequest. r=dom-worker-reviewers,jesup
https://hg.mozilla.org/integration/autoland/rev/7a4e3f5f674a
PFetch protocol declaration and implementation. r=dom-worker-reviewers,jesup
https://hg.mozilla.org/integration/autoland/rev/4fd92351d64b
FetchService integration for PFetch. r=dom-worker-reviewers,jesup
https://hg.mozilla.org/integration/autoland/rev/257553919696
Integrate FetchChild into Fetch.cpp r=dom-worker-reviewers,jesup

Comment 24

[Noemi Erli[:noemi_erli]]

Backed out 5 changesets (Bug 1351231) for causing multiple wpt failures CLOSED TREE

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&group_state=expanded&collapsedPushes=842382&selectedTaskRun=WtOtjQToTweF_gc4JpnGQg.0&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel&searchStr=wpt&revision=257553919696613b9c3bceec992517f4684e4f90

Logs: https://treeherder.mozilla.org/logviewer?job_id=402767191&repo=autoland
https://treeherder.mozilla.org/logviewer?job_id=402783475&repo=autoland
https://treeherder.mozilla.org/logviewer?job_id=402782474&repo=autoland

Backout: https://hg.mozilla.org/integration/autoland/rev/c9a5e43b87883fc4a470e2ef5f1ccfe05557a4ba

Comment 25

[Iulian Moraru]

Also, this assertion became very frequent Assertion failure: aResponseTarget, at /builds/worker/workspace/obj-build/dist/include/mozilla/MozPromise.h:504 there is a bug for this, Bug 1756635.
Please take a look at this too before you re-land this bug.
LATER EDIT:
also please take a look at:

• Bug 1810967 which was caused by this bug. Bug 1810967 it is now closed as INVALID because this bug was backed out.
• Bug 1810821 which was caused by this bug. Bug 1810821 it is now closed as INVALID because this bug was backed out.
• Bug 1811064 which was caused by this bug. Bug 1811064 it is now closed as INVALID because this bug was backed out.

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/dcfa6d7e82c0
Preference for PFetch. r=dom-worker-reviewers,jesup.
https://hg.mozilla.org/integration/autoland/rev/d77b78a71a7e
Support conversation between InternalRequest and IPCInternalRequest. r=dom-worker-reviewers,jesup
https://hg.mozilla.org/integration/autoland/rev/989cbb08b87e
PFetch protocol declaration and implementation. r=dom-worker-reviewers,jesup
https://hg.mozilla.org/integration/autoland/rev/b9d73f961292
FetchService integration for PFetch. r=dom-worker-reviewers,jesup
https://hg.mozilla.org/integration/autoland/rev/162fe96c2c33
Integrate FetchChild into Fetch.cpp r=dom-worker-reviewers,jesup
https://hg.mozilla.org/integration/autoland/rev/0c260a33541a
1351231, 1351231, 1351231, 1351231: apply code formatting via Lando

Comment 27

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/dcfa6d7e82c0
https://hg.mozilla.org/mozilla-central/rev/d77b78a71a7e
https://hg.mozilla.org/mozilla-central/rev/989cbb08b87e
https://hg.mozilla.org/mozilla-central/rev/b9d73f961292
https://hg.mozilla.org/mozilla-central/rev/162fe96c2c33
https://hg.mozilla.org/mozilla-central/rev/0c260a33541a

Comment 28

[Cosmin Sabou [:CosminS]]

There are some bugs that became frequent with the landings in this bug, for eg:

• https://treeherder.mozilla.org/intermittent-failures/bugdetails?startday=2023-01-13&endday=2023-01-20&tree=trunk&bug=1810828
• https://treeherder.mozilla.org/intermittent-failures/bugdetails?startday=2023-01-13&endday=2023-01-20&tree=trunk&bug=1810816

In both of them can be observed the same behavior:
Bug 1351231 -> landed failures went up,
Bug 1351231 was backed out -> no more failures on tree
Bug 1351231 was relanded - failures are again frequent.

Eden, please have a look over these frequent failures. Thank you.