894925 - ImageBridgeChild::DispatchImageClientUpdate doesn't attempt to keep ImageContainer alive

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Markus Stange [:mstange]]

There's this code in ImageBridgeChild.cpp:

> static void UpdateImageClientNow(ImageClient* aClient, nsRefPtr<ImageContainer> aContainer)
> {
>   MOZ_ASSERT(aClient);
>   MOZ_ASSERT(aContainer);
>   sImageBridgeChildSingleton->BeginTransaction();
>   aClient->UpdateImage(aContainer, Layer::CONTENT_OPAQUE);
>   sImageBridgeChildSingleton->EndTransaction();
> }
> 
> //static
> void ImageBridgeChild::DispatchImageClientUpdate(ImageClient* aClient,
>                                                  ImageContainer* aContainer)
> {
>   if (InImageBridgeChildThread()) {
>     UpdateImageClientNow(aClient, aContainer);
>     return;
>   }
>   nsRefPtr<ImageContainer> container = aContainer;
>   sImageBridgeChildSingleton->GetMessageLoop()->PostTask(
>     FROM_HERE,
>     NewRunnableFunction(&UpdateImageClientNow, aClient, container));
> }

In bug 546837 I ran across crashes where an ImageContainer was accessed from UpdateImageClientNow after it had already been destroyed, because the runnable function only takes a raw pointer to the container and doesn't attempt to keep it alive.
Nical, will bug 858914 take care of this in some way, or do we need to add a strong reference somewhere here? Or is there something wrong on my end?

Comment 1

[Markus Stange [:mstange]]

Oops, actually, that's the code after I added the nsRefPtr. In fact it looks like this at the moment:

> static void UpdateImageClientNow(ImageClient* aClient, ImageContainer* aContainer)
> {
>   MOZ_ASSERT(aClient);
>   MOZ_ASSERT(aContainer);
>   sImageBridgeChildSingleton->BeginTransaction();
>   aClient->UpdateImage(aContainer, Layer::CONTENT_OPAQUE);
>   sImageBridgeChildSingleton->EndTransaction();
> }
> 
> //static
> void ImageBridgeChild::DispatchImageClientUpdate(ImageClient* aClient,
>                                                  ImageContainer* aContainer)
> {
>   if (InImageBridgeChildThread()) {
>     UpdateImageClientNow(aClient, aContainer);
>     return;
>   }
>   sImageBridgeChildSingleton->GetMessageLoop()->PostTask(
>     FROM_HERE,
>     NewRunnableFunction(&UpdateImageClientNow, aClient, aContainer));
> }

Comment 2

[Nicolas Silva [:nical]]

Bug 858914 doesn't change this, so we indeed need to hold a strong ref here.
I suppose I would addref (in before dispatching or calling UpdateImageClientNiw) and release it (at the end of UpdateImageClientNow) manually.

Comment 3

[Markus Stange [:mstange]]

Attachment: with nsRefPtr, approach A

I'm not a big fan of manual reference counting. What do you think of this instead?

Comment 4

[Markus Stange [:mstange]]

Attachment: with nsRefPtr, approach B

Or maybe this instead?

Comment 5

[Markus Stange [:mstange]]

Also, do we need to hold a strong ref to the ImageClient, too?

Comment 6

[Nicolas Silva [:nical]]

Comment on attachment 777167 [details] [diff] [review]
with nsRefPtr, approach A

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

I am not exactly sure how this interacts with all the magic that happens in NewRunnableFunction and the MakeTuple stuff underneath. If this works I am all for it!

Comment 7

[Nicolas Silva [:nical]]

Comment on attachment 777168 [details] [diff] [review]
with nsRefPtr, approach B

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

I like this explicit approach (but again it's just me not feeling at ease with the template magic going on in NewRunnableFunction, so if you are certain that both approaches work, pick the one one you prefer), and also you are right, please add a strong ref to the ImageClient too.

Comment 8

[Markus Stange [:mstange]]

(In reply to Nicolas Silva [:nical] from comment #7)
> I like this explicit approach (but again it's just me not feeling at ease
> with the template magic going on in NewRunnableFunction, so if you are
> certain that both approaches work, pick the one one you prefer)

I prefer approach B, too.

, and also
> you are right, please add a strong ref to the ImageClient too.

OK. Should I do that in DispatchReleaseImageClient, too?

Comment 9

[Nicolas Silva [:nical]]

Yes, please and also the other dispatched functions (CreateImageClientNow, etc.)

Comment 10

[Markus Stange [:mstange]]

Actually it looks like nothing needs to be done for ImageClient. The only way for the ImageClient to go away is through the ->Release() call in ReleaseImageClientNow, and that's guaranteed not to happen as long as its owner ImageContainer is alive.

Comment 11

[Markus Stange [:mstange]]

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

Comment 12

[Ryan VanderMeulen [:RyanVM]]

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