Failed to fetch in subworker

RESOLVED FIXED in Firefox 49

Status

()

Core
DOM: Workers
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: tnguyen, Assigned: bkelly)

Tracking

unspecified
mozilla49
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox49 fixed)

Details

(Whiteboard: btpp-active)

Attachments

(4 attachments)

(Reporter)

Description

2 years ago
When attempting to add some test cases, I found that fetch API in subworker causes a crash in mochitest.
I attached log and my test related to that.
(Reporter)

Comment 1

2 years ago
Created attachment 8752714 [details] [diff] [review]
Call fetch in subworker
Attachment #8752715 - Attachment mime type: text/x-log → text/plain
Flags: needinfo?(bkelly)
(Assignee)

Comment 3

2 years ago
Kyle,

WorkerFetchResolve::OnRespondAvailable() gets calls on the main thread:

https://dxr.mozilla.org/mozilla-central/source/dom/fetch/Fetch.cpp#370

It attempts to dispatch a WorkerRunnable back to the original worker that performed the fetch().

It seems, however, that we don't allow main thread to dispatch to a nested worker thread?  We end up triggering this assert:

https://dxr.mozilla.org/mozilla-central/source/dom/workers/WorkerPrivate.cpp#3658

Without a parent we assert main thread, but otherwise we try to assert the parent worker thread.

What is the correct way to structure this?  Do we really have to bounce a runnable through every worker parent?  That seems... complicated.
Flags: needinfo?(bkelly) → needinfo?(khuey)
You need to override the Pre/PostDispatch assertions like so http://mxr.mozilla.org/mozilla-central/source/dom/base/WebSocket.cpp?force=1#2576

You also need to make sure that the WorkerPrivate is guaranteed to still be alive at this point.
Flags: needinfo?(khuey)
(Assignee)

Comment 5

2 years ago
We hold a WorkerFeature for the nested worker.  Is that enough?  Or do we need to hold the parent alive explicitly as well?
Assignee: nobody → bkelly
Status: NEW → ASSIGNED
Flags: needinfo?(khuey)
That is enough.
Flags: needinfo?(khuey)
(Assignee)

Comment 7

2 years ago
Created attachment 8752894 [details] [diff] [review]
P2 Execute fetch() mochitests in nested workers. r=khuey

This patch extends the test framework in dom/tests/mochitest/fetch to execute test cases in nested workers.  This easily triggered the reported assertion.  It also ensures we get a lot of test coverage in nested workers.
Attachment #8752894 - Flags: review?(khuey)
(Assignee)

Comment 8

2 years ago
Created attachment 8752901 [details] [diff] [review]
P1 Fix fetch() WorkerRunnable classes to allow it to work with nested Workers. r=khuey

This patch allows us to pass the test, but is not 100% correct yet.  We currently hold a WorkerFeature during body consumption, but not during the actual network request itself.
(Assignee)

Comment 9

2 years ago
Comment on attachment 8752901 [details] [diff] [review]
P1 Fix fetch() WorkerRunnable classes to allow it to work with nested Workers. r=khuey

Actually, we are holding a WorkerFeature during the network operation via the PromiseWorkerProxy.  So I believe this patch is complete.

I had to fix a number of worker runnables here.  Also, I had to drop the modification of the busy count since that performed the same parent thread assertion.  I think this is ok since we are holding the feature.
Attachment #8752901 - Flags: review?(khuey)
(Assignee)

Comment 10

2 years ago
Comment on attachment 8752894 [details] [diff] [review]
P2 Execute fetch() mochitests in nested workers. r=khuey

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

::: dom/tests/mochitest/fetch/nested_worker_wrapper.js
@@ +13,5 @@
> +  });
> +
> +  worker.addEventListener('error', function(evt) {
> +    self.postMessage({
> +      context: 'NextedWorker',

I fixed this typo locally.
Whiteboard: btpp-active
(Assignee)

Comment 11

2 years ago
This is the try build I did for this:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=721b03d6e55f&selectedJob=20894206

Note, jmaher tells me that the win7 failures are due to clipboard issues with aws test runners.

Comment 13

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/5733b66fdedf
https://hg.mozilla.org/mozilla-central/rev/00218374a90c
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox49: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
(Assignee)

Updated

2 years ago
Depends on: 1275715
You need to log in before you can comment on or make changes to this bug.