Closed Bug 1445587 Opened 2 years ago Closed 2 years ago

Port Fetch to WorkerRef

Categories

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

58 Branch
enhancement

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: baku, Assigned: baku)

References

Details

Attachments

(4 files, 5 obsolete files)

No description provided.
Attached patch part 1 - FetchConsumer (obsolete) — Splinter Review
Attachment #8958755 - Flags: review?(bugs)
Attached patch part 2 - FetchStream (obsolete) — Splinter Review
Attachment #8958756 - Flags: review?(bugs)
Attachment #8958757 - Flags: review?(bugs)
Attached patch part 4 - FetchSplinter Review
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.
Attachment #8958760 - Flags: review?(bugs)
Attachment #8958760 - Flags: review?(bugs) → review+
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)
Attachment #8958756 - Flags: review?(bugs) → review-
Attachment #8958757 - Flags: review?(bugs) → review+
Attachment #8958755 - Flags: review?(bugs) → review+
Attached patch part 2 - FetchStream (obsolete) — Splinter Review
Attachment #8958756 - Attachment is obsolete: true
Attachment #8958815 - Flags: review?(bugs)
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.
Attachment #8958815 - Flags: review?(bugs) → review-
Priority: -- → P2
Attachment #8958815 - Attachment is obsolete: true
Attachment #8959087 - Flags: review?(bugs)
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
Attachment #8959087 - Flags: review?(bugs) → review+
Attached patch part 1 - FetchConsumer (obsolete) — Splinter Review
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.
Attachment #8958755 - Attachment is obsolete: true
Attachment #8959352 - Flags: review?(bugs)
Attached patch part 1 - FetchConsumer (obsolete) — Splinter Review
Attachment #8959352 - Attachment is obsolete: true
Attachment #8959352 - Flags: review?(bugs)
Attachment #8959378 - Flags: review?(bugs)
Depends on: 1446204
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.
Attachment #8959378 - Flags: review?(bugs) → review-
Attachment #8959378 - Attachment is obsolete: true
Attachment #8959472 - Flags: review?(bugs)
Attachment #8959472 - Flags: review?(bugs) → review+
Pushed by amarchesini@mozilla.com:
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
You need to log in before you can comment on or make changes to this bug.