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

RESOLVED FIXED in mozilla34

Status

()

defect
RESOLVED FIXED
5 years ago
4 months ago

People

(Reporter: bkelly, Assigned: bkelly)

Tracking

unspecified
mozilla34
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

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.
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?
Assignee: nobody → bkelly
Status: NEW → ASSIGNED
Flags: needinfo?(bent.mozilla)
And the tests should be expanded to catch this case.  Right now this path is not exercised.
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.
Flags: needinfo?(bent.mozilla)
Lets try again; this time with more destructors.

  https://tbpl.mozilla.org/?tree=Try&rev=2c5c239ac3d4
Attachment #8468750 - Attachment is obsolete: true
Attachment #8468750 - Flags: review?(bent.mozilla)
Attachment #8468926 - Flags: review?(bent.mozilla)
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.
(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.
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
Attachment #8468926 - Attachment is obsolete: true
Attachment #8470257 - Flags: review?(bent.mozilla)
Comment on attachment 8470257 [details] [diff] [review]
Avoid PBackground addref off main thread in GetOrCreateForCurrentThread(). (v2)

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

Thanks!
Attachment #8470257 - Flags: review?(bent.mozilla) → review+
Blocks: 1013571
https://hg.mozilla.org/mozilla-central/rev/1aa229f110ad
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
You need to log in before you can comment on or make changes to this bug.