Closed Bug 1057994 Opened 8 years ago Closed 7 years ago

DataStore crashes if used in worker.onclose()

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla37

People

(Reporter: baku, Assigned: baku)

Details

Attachments

(1 file, 4 obsolete files)

We have a MOZ_ASSERT when AddFeature() fails in PromiseWorkerProxy constructor.
Attached patch crash.patch (obsolete) — Splinter Review
Attachment #8478219 - Flags: review?(ehsan)
Attached patch crash.patch (obsolete) — Splinter Review
I touched WorkerRunnable, I would like to have a review from bent.
Just to share the conversation with nsm, what we want to have when AddFeature fails is:

1. no runnable dispatched
2. no rejected promises

In order to do this, the patch overwrites Dispatch() in order to check if the creation of the PrmiseProxyWorker failed.
Attachment #8478219 - Attachment is obsolete: true
Attachment #8478219 - Flags: review?(ehsan)
Attachment #8478387 - Flags: review?(bent.mozilla)
Comment on attachment 8478387 [details] [diff] [review]
crash.patch

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

::: dom/workers/WorkerRunnable.h
@@ +358,5 @@
>  
>    virtual bool MainThreadRun() = 0;
>  
>  public:
> +  virtual bool Dispatch(JSContext* aCx);

I don't really think this should be virtual since we never call this from a base class. Just don't call Dispatch if you don't need to.
Attachment #8478387 - Flags: review?(bent.mozilla) → review-
> I don't really think this should be virtual since we never call this from a
> base class. Just don't call Dispatch if you don't need to.

This means that any subclass must have a different method. For instance in dom/workers/DataStore.cpp we have 4 or 5 classes inheriting WorkerMainThreadRunnable (actually inheriting DataStoreProxyRunnable and DataStoreRunnable).

For them, I cannot call Dispatch() because I cannot overwrite it and I have to call something like CheckAndDispatch(). This can introduce errors in the long run.
Plus, I don't think it's a good oo programming approach :)
Attachment #8478387 - Flags: review- → review?(bent.mozilla)
> Plus, I don't think it's a good oo programming approach :)

So, actually you are right, I don't use the class as a WorkerMainThreadRunnable, so I can just implement my own Dispatch() method. New patch's coming.
Attached patch crash.patch (obsolete) — Splinter Review
Attachment #8478387 - Attachment is obsolete: true
Attachment #8478387 - Flags: review?(bent.mozilla)
Attachment #8517414 - Flags: review?(bent.mozilla)
Comment on attachment 8517414 [details] [diff] [review]
crash.patch

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

::: dom/workers/DataStore.cpp
@@ +151,5 @@
> +    mPromiseWorkerProxy =
> +      PromiseWorkerProxy::Create(aWorkerPrivate, aWorkerPromise);
> +  }
> +
> +  virtual bool Dispatch(JSContext* aCx) MOZ_OVERRIDE

I can't see that this overrides anything... This won't compile on windows.

::: dom/workers/DataStoreCursor.cpp
@@ +84,5 @@
>      mPromiseWorkerProxy =
> +      PromiseWorkerProxy::Create(aWorkerPrivate, aWorkerPromise);
> +  }
> +
> +  virtual bool Dispatch(JSContext* aCx) MOZ_OVERRIDE

What does this override?

::: dom/workers/Navigator.cpp
@@ +223,5 @@
>      MOZ_ASSERT(aWorkerPrivate);
>      aWorkerPrivate->AssertIsOnWorkerThread();
>  
>      mPromiseWorkerProxy =
> +      PromiseWorkerProxy::Create(aWorkerPrivate,

Nit: Comment that this might return null if the worker has started the close handler
Attachment #8517414 - Flags: review?(bent.mozilla) → review+
Attached patch id.patch (obsolete) — Splinter Review
It turned out I have to cleanup the promise Proxy when if dispatching events fails.
Attachment #8534367 - Flags: review?(bent.mozilla)
Comment on attachment 8534367 [details] [diff] [review]
id.patch

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

::: dom/promise/Promise.cpp
@@ +1379,5 @@
> +  PromiseWorkerProxyControlRunnable(WorkerPrivate* aWorkerPrivate,
> +                                    PromiseWorkerProxy* aProxy)
> +    : WorkerControlRunnable(aWorkerPrivate, WorkerThreadUnchangedBusyCount)
> +    , mProxy(aProxy)
> +  {}

MOZ_ASSERT(mProxy) here.
Attachment #8534367 - Flags: review?(bent.mozilla) → review+
Attached patch crash.patchSplinter Review
https://tbpl.mozilla.org/?tree=Try&rev=7137ee901dcc
Attachment #8517414 - Attachment is obsolete: true
Attachment #8534367 - Attachment is obsolete: true
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/aa82fdbf3362
Status: NEW → RESOLVED
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.