Closed Bug 1282026 Opened 3 years ago Closed 3 years ago

Add assertions in the DTOR of WorkerHolder

Categories

(Core :: DOM: Workers, defect)

49 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox50 --- fixed

People

(Reporter: baku, Assigned: baku)

References

Details

(Whiteboard: btpp-active)

Attachments

(3 files, 1 obsolete file)

No description provided.
Attached patch holder.patch (obsolete) — Splinter Review
We cannot add an assertion in the DTOR because WorkerHolder can be deleted in any thread if correctly unregistered. What we want here is that, if you don't do manual unregistration, then it _must_ be deleted in the right thread.
Attachment #8764867 - Flags: review?(bkelly)
Comment on attachment 8764867 [details] [diff] [review]
holder.patch

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

Personally I think we should back out the other patch and rethink.  If we can't rely on the destructor to clean up because it might run on the wrong thread, then what have we gained here?  At the same time, it has increased the risk of running code unsafely on the wrong thread in release.

Perhaps we should just leave WorkerHolder with old behavior, but add an AutoWorkerHolder base class with the destructor cleanup.  This auto class would strictly assert thread usage.

Kyle, what do you think?
Attachment #8764867 - Flags: review?(bkelly) → review-
See comment 2.
Flags: needinfo?(khuey)
I think we can make WorkerHolder so that the CTOR and the DTOR run on the same thread. But we have to check all the places where they are used and we must be sure that we don't move them cross threads. For instance, they cannot be nsRunnable + WorkerHolder.
Whiteboard: btpp-active
(In reply to Andrea Marchesini (:baku) from comment #4)
> I think we can make WorkerHolder so that the CTOR and the DTOR run on the
> same thread. But we have to check all the places where they are used and we
> must be sure that we don't move them cross threads. For instance, they
> cannot be nsRunnable + WorkerHolder.

Ok, but I don't understand how the current code in the tree is safe then.  If we don't know where the destructors are going to run then we might execute the worker code in the wrong place.  If we know we always do the worker code before the destructor, then what have we gained from bug 1269154?
Hold on. I disagree :)
The only problem about adding assertions in WorkerHolder is that we use it in runnables and runnables can be deleted everywhere because they are thread-safe. Elsewhere WorkerHolder is the "perfect" solution and it avoids many errors about keeping worker alive, or sending notifications to dead objects.

In bug 1282366 I removed WorkerHolder in all the runnables except 1 (ScriptLoaderRunnable requires a bit more work).

So, the situation, from my point of view, is in a good shape: I just need to change ScriptLoaderRunnable and then we can add assertions in the CTOR and in the DTOR of this object.

What do you think about this approach?
Flags: needinfo?(bkelly)
(In reply to Andrea Marchesini (:baku) from comment #7)
> So, the situation, from my point of view, is in a good shape: I just need to
> change ScriptLoaderRunnable and then we can add assertions in the CTOR and
> in the DTOR of this object.
> 
> What do you think about this approach?

Sounds good to me if the end result is strict thread assertions.  Thanks!
Flags: needinfo?(bkelly)
Attachment #8764867 - Attachment is obsolete: true
Attachment #8771305 - Flags: review?(bkelly)
Attachment #8771308 - Flags: review?(bkelly)
Comment on attachment 8771305 [details] [diff] [review]
part 1 - WorkerHolder of Script loading

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

Looks good, thanks!  r=me with comments addressed.

::: dom/workers/ScriptLoader.cpp
@@ +2080,5 @@
>                               aRv);
>  
>    NS_ASSERTION(aLoadInfos.IsEmpty(), "Should have swapped!");
>  
> +  UniquePtr<ScriptLoaderHolder> workerHolder(new ScriptLoaderHolder(loader));

Can you just make ScriptLoaderHolder a MOZ_STACK_CLASS and allocate it on the stack?  Why do we need it heap allocated here?
Attachment #8771305 - Flags: review?(bkelly) → review+
Comment on attachment 8771308 [details] [diff] [review]
part 2 - assertions

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

Looks good, but I think we should just use our existing thread assertion utilities.  r=me with comments addressed.

::: dom/workers/WorkerHolder.cpp
@@ +18,5 @@
>  }
>  
>  WorkerHolder::~WorkerHolder()
>  {
> +  AssertIsOwningThread();

Please use NS_ASSERT_OWNINGTHREAD(WorkerHolder) here.

::: dom/workers/WorkerHolder.h
@@ +88,5 @@
> +private:
> +  void AssertIsOwningThread() const;
> +
> +#ifdef DEBUG
> +  PRThread* mOwningThread;

Please use NS_DECL_OWNINGTHREAD here.
Attachment #8771308 - Flags: review?(bkelly) → review+
Another piece is needed.
Attachment #8771740 - Flags: review?(bkelly)
Comment on attachment 8771740 [details] [diff] [review]
part 3 - PromiseWorkerProxy

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

::: dom/promise/Promise.cpp
@@ +2947,5 @@
> +    }
> +
> +    return true;
> +  }
> +

nit: extraneous line here

@@ +2950,5 @@
> +  }
> +
> +};
> +
> +

nit: and an extra line here
Attachment #8771740 - Flags: review?(bkelly) → review+
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0619d5ea9d35
Add assertions in the DTOR of WorkerHolder - part 1 - WorkerHolder of Script loading, r=bkelly
https://hg.mozilla.org/integration/mozilla-inbound/rev/51d8d12d035c
Add assertions in the DTOR of WorkerHolder - part 2 - assertions, r=bkelly
https://hg.mozilla.org/integration/mozilla-inbound/rev/ff9608965b49
Add assertions in the DTOR of WorkerHolder - part 3 - PromiseWorkerProxy, r=bkelly
You need to log in before you can comment on or make changes to this bug.