1049552 - BackgroundChild::GetForCurrentThread() hits MOZ_ASSERT() if actor already created

Status: RESOLVED FIXED

(Core :: DOM: Workers, defect)

Description

[Ben Kelly [:bkelly, not reviewing]]

Over in bug 966439 Andrea found that if you try to use BackgroundChild::GetOrCreateForCurrentThread() in workers then you hit a MOZ_ASSERT about non-threadsafe ref counters.

This is due to this code in GetOrCreateForCurrentThread():

  if (threadLocalInfo->mActor) {
    nsRefPtr<ChildImpl> actor = threadLocalInfo->mActor;

    nsCOMPtr<nsIRunnable> runnable = new CreateCallbackRunnable(actor.forget());
    MOZ_ALWAYS_TRUE(NS_SUCCEEDED(NS_DispatchToCurrentThread(runnable)));

    return true;
  }

Using the nsRefPtr<> there is illegal since the actor was created on the main thread.

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

Can we drop the NS_DispatchToCurrentThread() and then dispense with the nsRefPtr<>?

I'm not sure how to hold the ref here if we need to put a runnable on the event queue.  I guess we could assume that the event queue will be drained before the actor is destroyed, but that seems racy.  Or we could hold references to all the callback runnables and somehow cancel/process them in ActorDestroy.

Ben, what do you think?

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

And the tests should be expanded to catch this case.  Right now this path is not exercised.

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

Actually, I think the best thing to do here would be to just schedule a runnable which calls GetForCurrentThread() instead of storing a ref.  If that returns null because the actor has been destroyed in the intervening time, then the runnable can call GetOrCreateForCurrentThread() again.

I think that should cleanly avoid any races.

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Avoid PBackground addref off main thread in GetOrCreateForCurrentThread(). (v0)

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

https://tbpl.mozilla.org/?tree=Try&rev=2f9a78f430b2

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Avoid PBackground addref off main thread in GetOrCreateForCurrentThread(). (v1)

Lets try again; this time with more destructors.

  https://tbpl.mozilla.org/?tree=Try&rev=2c5c239ac3d4

Comment 7

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

Comment on attachment 8468926 [details] [diff] [review]
Avoid PBackground addref off main thread in GetOrCreateForCurrentThread(). (v1)

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

::: dom/workers/RuntimeService.cpp
@@ +1088,5 @@
>    }
>  #endif
>  
>  #ifdef ENABLE_TESTS
> +  class TestPBackgroundCreateCallback :

Nit: MOZ_FINAL

@@ +1103,5 @@
> +      MOZ_CRASH("TestPBackground() should not fail GetOrCreateForCurrentThread()");
> +    }
> +
> +  private:
> +    virtual ~TestPBackgroundCreateCallback()

Nit: Non-virtual with MOZ_FINAL.

::: ipc/glue/BackgroundImpl.cpp
@@ +1650,5 @@
>  
>    if (threadLocalInfo->mActor) {
> +    // Runnable will use GetForCurrentThread() to retrieve actor again.  This
> +    // allows us to avoid addref'ing on the wrong thread.
> +    nsCOMPtr<nsIRunnable> runnable = new CreateCallbackRunnable();

It looks to me like this was the only caller that passed an actor to the constructor? In that case you should remove that constructor entirely, and maybe the actor member too?

@@ +1779,5 @@
> +  // means we are in the process of cleaning up a worker thread and do not want
> +  // a new actor created.  Unfortunately we cannot report back to the callback
> +  // because the thread local is gone at this point.  Instead simply do nothing
> +  // and return.
> +  if (!actor) {

Let's add an NS_WARN_IF here.

@@ -1773,5 @@
>    while (callback) {
> -    if (actor) {
> -      callback->ActorCreated(actor);
> -    } else {
> -      callback->ActorFailed();

Wait, does this mean that we never call ActorFailed?

@@ +1796,5 @@
> +NS_IMETHODIMP
> +ChildImpl::CreateCallbackRunnable::Cancel()
> +{
> +  // These are IPC infrastructure objects and need to run unconditionally.
> +  return Run();

Let's just call Run() and then always return NS_OK.

Comment 8

[Ben Kelly [:bkelly, not reviewing]]

(In reply to ben turner [:bent] (use the needinfo? flag!) from comment #7)
> ::: ipc/glue/BackgroundImpl.cpp
> @@ +1650,5 @@
> >  
> >    if (threadLocalInfo->mActor) {
> > +    // Runnable will use GetForCurrentThread() to retrieve actor again.  This
> > +    // allows us to avoid addref'ing on the wrong thread.
> > +    nsCOMPtr<nsIRunnable> runnable = new CreateCallbackRunnable();
> 
> It looks to me like this was the only caller that passed an actor to the
> constructor? In that case you should remove that constructor entirely, and
> maybe the actor member too?

No, the child classes OpenChildProcessActorRunnable and OpenMainProcessActorRunnable pass an actor to this constructor.  They then use mActor in overridden Run() methods.

I could remove mActor from the base class and just move it into these child classes.

Or alternatively I could just make all these runnables inherit from nsRunnable directly as they are not really getting anything out of the inheritence right now.  That seems cleanest, so its probably what I will do unless you have a preference.

> @@ -1773,5 @@
> >    while (callback) {
> > -    if (actor) {
> > -      callback->ActorCreated(actor);
> > -    } else {
> > -      callback->ActorFailed();
> 
> Wait, does this mean that we never call ActorFailed?

ActorFailed() gets called correctly from the child classes OpenChildProcessActorRunnable and OpenMainProcessActorRunnable.

Comment 9

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

Ok, sounds good.

Comment 10

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Avoid PBackground addref off main thread in GetOrCreateForCurrentThread(). (v2)

Ok, I broke apart the runnables into separate, non-derived classes.  I also added a FailedCreateCallbackRunnable class to be used from DispatchFailureCallback() which was not being handled properly in my last patch.

Note, I left destructors on the new runnables I created virtual because they use NS_IMPL_ISUPPORTS_INHERITED which requires destruction through a virtual pointer on the base class.  Given that the style guide indicates overrides should be annotated with virtual as well, I thought leaving the keyword on was the most clear.

I removed the virtual keyword from the TestPBackgroundCreateCallback as requested since it defines its own Release() method.

https://tbpl.mozilla.org/?tree=Try&rev=16fcc7486b5c

Comment 11

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

Comment on attachment 8470257 [details] [diff] [review]
Avoid PBackground addref off main thread in GetOrCreateForCurrentThread(). (v2)

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

Thanks!

Comment 12

[Ben Kelly [:bkelly, not reviewing]]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/1aa229f110ad

Comment 13

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/1aa229f110ad