Closed Bug 1273070 Opened 8 years ago Closed 8 years ago

Failed to fetch in subworker


(Core :: DOM: Workers, defect)

Not set



Tracking Status
firefox49 --- fixed


(Reporter: tnguyen, Assigned: bkelly)



(Whiteboard: btpp-active)


(4 files)

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.
Attached file Log
Attachment #8752715 - Attachment mime type: text/x-log → text/plain

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

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:

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

You also need to make sure that the WorkerPrivate is guaranteed to still be alive at this point.
Flags: needinfo?(khuey)
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
Flags: needinfo?(khuey)
That is enough.
Flags: needinfo?(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)
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.
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)
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
This is the try build I did for this:

Note, jmaher tells me that the win7 failures are due to clipboard issues with aws test runners.
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Depends on: 1275715
You need to log in before you can comment on or make changes to this bug.