1445587 - Port Fetch to WorkerRef

Status: RESOLVED FIXED

(Core :: DOM: Workers, enhancement, P2)

Comment 1

[Andrea Marchesini [:baku]]

Attachment: part 1 - FetchConsumer

Comment 2

[Andrea Marchesini [:baku]]

Attachment: part 2 - FetchStream

Comment 3

[Andrea Marchesini [:baku]]

Attachment: part 3 - FetchStreamReader

Comment 4

[Andrea Marchesini [:baku]]

Attachment: part 4 - Fetch

Note that here, we were using 'Canceling' state when registering the WorkerHolder. This patch uses (internally) 'Closing', but this is fine, because if you see, just a couple of line before there is PromiseWorkerProxy::Create, that uses a WorkerRef again.

So, if the Worker is shutting down, PromiseWorkerProxy would fail. No needs to use a 'Canceling' state here.

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8958756 [details] [diff] [review]
part 2 - FetchStream

FetchStreamWorkerHolder uses AllowIdleShutdownStart, but StrongWorkerRef::Create uses PreventIdleShutdownStart.
(actually, need to go through other cases too that we don't change the behavior)

Comment 6

[Andrea Marchesini [:baku]]

Attachment: part 2 - FetchStream

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8958815 [details] [diff] [review]
part 2 - FetchStream

>+  if (mWorkerRef) {
>+    // This method is called when the worker notifies the workerRef.
>+    WorkerPrivate* workerPrivate = mWorkerRef->GetPrivate();
>+    MOZ_ASSERT(workerPrivate);
>+
>+    RefPtr<FetchStreamWorkerRefShutdown> r =
>+      new FetchStreamWorkerRefShutdown(workerPrivate, Move(mWorkerRef),
>+                                       Move(mGlobal), Move(mStreamHolder));
I don't understand this. If we're shutting down already, the WorkerRef becomes dummy after the Notify call, so how is that
then passed to FetchStreamWorkerRefShutdown.

Comment 8

[Andrea Marchesini [:baku]]

Attachment: part 2 - FetchStream

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8959087 [details] [diff] [review]
part 2 - FetchStream



>+void
>+FetchStream::ReleaseObjects(const MutexAutoLock& aProofOfLock)
>+{
>+  // This method can be called on 2 possible threads: the owning one and a JS
>+  // thread used to release resources. If we are on the JS thread, we need to
>+  // dispatch a runnable to go back to the owning thread in order to release
>+  // resources correctly.
>+
>   if (mState == eClosed) {
>+    // Already gone. Nothing to do.
>     return;
>   }
> 
>   mState = eClosed;
> 
>-  if (mWorkerHolder) {
>-    RefPtr<FetchStreamWorkerHolderShutdown> r =
>-      new FetchStreamWorkerHolderShutdown(
>-        static_cast<FetchStreamWorkerHolder*>(mWorkerHolder.get())->GetWorkerPrivate(),
>-        Move(mWorkerHolder), Move(mGlobal), Move(mStreamHolder));
>-    r->Dispatch();
>-  } else {
>+  if (!NS_IsMainThread() && !IsCurrentThreadRunningWorker()) {
>+    // Worker thread.
this comment is confusing. We are explicitly not in worker thread, but dispatch something to
worker thread.

>+    // Main thread.
same here, but for main thread.
Please improve comments

>-  // Touched only on the target thread.
>+  // Protected by mutex.
>   State mState;


>+  // We need a mutex because JS engine can release FetchStream on a non-owning
>+  // thread. We must be sure that the releasing of resources doesn't trigger
>+  // race conditions.
>+  Mutex mMutex;
It isn't clear what all variables are protected by the mutex.
Perhaps move mMutex next to mState

Comment 10

[Andrea Marchesini [:baku]]

Attachment: part 1 - FetchConsumer

I need you to review this patch again. I found a much interesting approach.
FetchConsumer should not hold the worker alive itself. Instead, each runnable or listener involved in the fetch should have a reference to the ThreadSafeWorkerRef object. In this way we are 100% sure that each runnable doesn't depend on other components for keeping the worker alive.

When the last runnable/listener completes its task, the workerRef is released.

Comment 11

[Andrea Marchesini [:baku]]

Attachment: part 1 - FetchConsumer

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8959378 [details] [diff] [review]
part 1 - FetchConsumer

> class ContinueConsumeBlobBodyControlRunnable final
>   : public MainThreadWorkerControlRunnable
> {
>   RefPtr<FetchBodyConsumer<Derived>> mFetchBodyConsumer;
> 
> public:
>-  explicit ContinueConsumeBlobBodyControlRunnable(FetchBodyConsumer<Derived>* aFetchBodyConsumer)
>-    : MainThreadWorkerControlRunnable(aFetchBodyConsumer->GetWorkerPrivate())
>+  ContinueConsumeBlobBodyControlRunnable(FetchBodyConsumer<Derived>* aFetchBodyConsumer,
>+                                         ThreadSafeWorkerRef* aWorkerRef)
>+    : MainThreadWorkerControlRunnable(aWorkerRef->Private())
I don't like this setup where creating an object, ContinueConsumeBlobBodyControlRunnable in this case, 
takes ThreadSafeWorkerRef as a param, hinting that worker is being kept alive by the object, but it actually isn't.
These should just take WorkerPrivate* as argument.

ContinueConsumeBlobBodyRunnable and ContinueConsumeBodyControlRunnable at least have this pattern.

Comment 13

[Andrea Marchesini [:baku]]

Attachment: part 1 - FetchConsumer

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/143c179449cc
Port Fetch to WorkerRef - part 1 - FetchConsumer, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/a5ef5edfe67a
Port Fetch to WorkerRef - part 2 - FetchStream, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/5c7105dd8159
Port Fetch to WorkerRef - part 3 - FetchStreamReader, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/2e2d815db8e4
Port Fetch to WorkerRef - part 4 - Fetch Request, r=smaug

Comment 15

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/143c179449cc
https://hg.mozilla.org/mozilla-central/rev/a5ef5edfe67a
https://hg.mozilla.org/mozilla-central/rev/5c7105dd8159
https://hg.mozilla.org/mozilla-central/rev/2e2d815db8e4