1057994 - DataStore crashes if used in worker.onclose()

Status: RESOLVED FIXED

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

Description

[Andrea Marchesini [:baku]]

We have a MOZ_ASSERT when AddFeature() fails in PromiseWorkerProxy constructor.

Comment 1

[Andrea Marchesini [:baku]]

Attachment: crash.patch

Comment 2

[Andrea Marchesini [:baku]]

Attachment: crash.patch

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.

Comment 3

[Ben Turner (not reading bugmail, use the needinfo flag!)]

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.

Comment 4

[Andrea Marchesini [:baku]]

> 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 :)

Comment 5

[Andrea Marchesini [:baku]]

> 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.

Comment 6

[Andrea Marchesini [:baku]]

Attachment: crash.patch

Comment 7

[Ben Turner (not reading bugmail, use the needinfo flag!)]

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

Comment 8

[Andrea Marchesini [:baku]]

Attachment: id.patch

It turned out I have to cleanup the promise Proxy when if dispatching events fails.

Comment 9

[Ben Turner (not reading bugmail, use the needinfo flag!)]

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.

Comment 10

[Andrea Marchesini [:baku]]

Attachment: crash.patch

https://tbpl.mozilla.org/?tree=Try&rev=7137ee901dcc

Comment 11

[Andrea Marchesini [:baku]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/aa82fdbf3362

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/aa82fdbf3362