932537 - Make GrallocBufferActor reference-counted and offer variants of SurfaceDescriptors that do refcounting

Status: RESOLVED INVALID

(Core :: Graphics, defect)

Description

[Benoit Jacob [:bjacob] (mostly away)]

There's been a lot of discussion about what is the real cause of all the GrallocBufferActor crashes. Currently GrallocBufferActor is not reference-counted, so its lifetime is entirely tied to the IPC system.

That used to be arguably reasonable: our entire layers system is tied to the IPC system, and one could argue that gralloc buffers are only useful as long as we have a layers system around. Under that line of reasoning, GrallocBufferActor-related crashes were arguably only the consequences of bugs around our layers system, e.g. around the shutdown sequence of layers. And indeed, several of the early GrallocBufferActor-related crashes that we fixed were arguably in that category. Example: bug 862324.

But more recently, we have had GrallocBufferActor-related crashes of a different nature. The problem was not anymore with the Gralloc buffers used by the layers system, but with the ones used by the swap chain used for efficient compositing of canvases, whose gralloc back-end landed in bug 843599. These crashes are tracked mostly in bug 914823. The basic problem here is that the swap chain used for fast canvas compositing uses its own gralloc buffers, outside of the layers system. There is good reason for that: one may do canvas rendering without having a layer for that canvas (just don't insert the canvas into the DOM), and one still has a good reason to render into gralloc and create a full-blown swap chain for that as the canvas could be added to the DOM at any time later. As canvas rendering contexts are exposed to JavaScript, their lifetime is tied to GC so they can outlive the layers system during shutdown. So the crash scenario that we have in bug 914823 is that a canvas, and its swap chain, outlive the layers system, so by the time we try to destroy the swap chain, its pointers to GrallocBufferActors are dangling. In short, the fast canvas compositing paths added in bug 843599 break a basic assumption of our codebase: that gralloc buffers don't need to be referenced by things that outlive the layers system.

So during the graphics work week in Paris (last week), with :sotaro and :bent, we discussed what would be our best short-term solution to this problem. We hesitated between two options:

 1) Make a reference-counted wrapper for GrallocBufferActor, that would be the only thing referencing it. This is equivalent to using weak pointers to reference GrallocBufferActor's.

 2) Reference-count GrallocBufferActor and use strong pointers to reference them.

Initially I didn't want to do 2) because I thought that keeping actors alive was futile as the IPC system would go down anyway, so we'd be only shifting the problem. But in fact there is a common idiom already in use for many other IPC actors, to handle that: just have a bool mIPCOpen flag that is set to false by the IPDL Dealloc function, so that we can easily know if IPC is already down and avoid doing IPC then.

So 2) is what we're going to do here.

The other problem is with SurfaceDescriptor. Currently a SurfaceDescriptorGralloc is basically just a pair of raw pointers to PGrallocBuffer{Parent,Child}. We want RefPtr's instead. We can't have RefPtr's in IPDL-defined classes. Moreover, a lot of user code uses SurfaceDescriptor, which is a IPDL union of all the surfacedescriptor types, which would be tedious and bug-prone to recreate entirely in C++.

The solution proposed here is to introduce a templated RefcountingSurfaceDescriptorWrapper<DescriptorType> that does refcounting, and can be used to wrap either a generic SurfaceDescriptor or a specialized [New]SurfaceDescriptorGralloc.

Comment 1

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: Make GrallocBufferRefcounted

Comment 2

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: Offer a RefcountingSurfaceDescriptorWrapper that wraps a SurfaceDescriptor and behaves as a strong ref to any GrallocBufferActor that it may wrap

Comment 3

[Matt Woodrow (:mattwoodrow)]

I thought the plan was to just stop using SurfaceDescriptor except as a serialization wrapper. In that case, we should never hang on to them, and we don't need refcounts.

Comment 4

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

(In reply to Matt Woodrow (:mattwoodrow) from comment #3)
> I thought the plan was to just stop using SurfaceDescriptor except as a
> serialization wrapper.

I believe that's true, long-term. This is the short-term fix. Right bjacob?

Comment 5

[Benoit Jacob [:bjacob] (mostly away)]

That is indeed the longer-term plan, but we also need a short-term solution.

The present situation is that
 1. We have a lot of code hanging on to SurfaceDescriptor's right now;
 2. We do not have a replacement for SurfaceDescriptor ready right now;
 3. This is a cause of crashes that are flagged koi+ so we need a solution right now.

The long-term solution was also discussed in the gfx/paris work week last week (as you know since you participated in these sessions! But I'm saying that for the benefit of other people reading this bug) has to involve first addressing the above point 2. i.e. developing a replacement for SurfaceDescriptor, i.e. an abstraction for a surface that may be passed over IPC and is generic wrt the actual type of surface that that is. That's what we discussed at length in the 'surfaces' sessions last week, and we sort of arrived at the conclusion that once the TextureClient/TextureHost pair becomes unified into a single class (that we referred to hypothetically as 'MozSurface') and becomes independent of any layers-specific concepts (as should happen with bug 897452 removing the dependency on Compositable's), it should then be a suitable replacement for SurfaceDescriptor.

But we wanted to have a short-term fix so that we could then take our time to get the long-term solution right.

Comment 6

[u480271]

Do we have a bug to remove the proliferation of code holding onto SurfaceDescriptors?
Do we have a blocking bug that crashes on GrAllocBufferActor that this is fixing?

Comment 7

[Benoit Jacob [:bjacob] (mostly away)]

So yes, this is the short-term fix that I'm using right now to try to fix at least bug 914823. See how I'm using this in attachment 824338 [details] [diff] [review] to try to fix this bug.

Comment 8

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

Comment on attachment 824322 [details] [diff] [review]
Make GrallocBufferRefcounted

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

This looks good to me!

::: gfx/layers/ipc/ImageBridgeParent.cpp
@@ +145,2 @@
>  #else
>    NS_RUNTIMEABORT("No gralloc buffers for you");

While you're here can you change this to a NS_WARNING? We shouldn't abort the parent process if a hacked child sends us this message. Returning null already causes the child to be killed. Same goes for Dealloc below, and same in LayerTransactionParent. (Just the parent classes, not the child classes).

::: gfx/layers/ipc/ShadowLayerUtilsGralloc.cpp
@@ +334,5 @@
>      PGrallocBufferChild* gbc =
>        aSurface->get_SurfaceDescriptorGralloc().bufferChild();
> +    if (static_cast<GrallocBufferActor*>(gbc)->IPCOpen()) {
> +      unused << PGrallocBufferChild::Send__delete__(gbc);
> +    }

If you're trying to track down places where the actors were previously used after being destroyed you could add warnings/asserts to all these spots if IPCOpen() is false?

::: gfx/layers/ipc/ShadowLayerUtilsGralloc.h
@@ +69,5 @@
>    friend class ImageBridgeChild;
>    typedef android::GraphicBuffer GraphicBuffer;
>  
>  public:
>    virtual ~GrallocBufferActor();

Now that this is refcounted it should only be deleted through the Release method. You can make this private to prevent anything from compiling that tries to manually delete one of these.

@@ +80,1 @@
>    Create();

I think this is fine but the old code had the benefit of self-documenting which Create() was supposed to be called in each process. Now I guess a comment to that effect would be useful.

@@ +94,5 @@
>    android::GraphicBuffer* GetGraphicBuffer();
>  
>    void InitFromHandle(const MagicGrallocBufferHandle& aHandle);
>  
> +  void AddIPDLReference() {

In debug builds it might be worth asserting |!mIPCOpen| here before setting it to true (and asserting the opposite in ReleaseIPDLReference).

You could also make the various manager classes (ImageBridge{Child|Parent}/LayerTransaction{Child|Parent}) friends and then hide these methods in a private section. That's slightly paranoid I guess but it might prevent future misuse.

Actually, now that I look closer, I think you could remove AddIPDLReference entirely and just call AddRef() as the last step of your Create() functions. That way IPDL could only ever call the ReleaseIPDLReference function. Dunno if you think that is cleaner or messier ;)

Comment 9

[u480271]

(In reply to Dan Glastonbury :djg :kamidphish from comment #6)
> Do we have a bug to remove the proliferation of code holding onto
> SurfaceDescriptors?
> Do we have a blocking bug that crashes on GrAllocBufferActor that this is
> fixing?

I see :bjacob answered my question as I was typing it.

Comment 10

[Benoit Jacob [:bjacob] (mostly away)]

(In reply to Dan Glastonbury :djg :kamidphish from comment #6)
> Do we have a bug to remove the proliferation of code holding onto
> SurfaceDescriptors?

Not yet. IMO it would only make sense to have such a thing once we have a replacement for SurfaceDescriptor ready. What we could have right now is a tracking bug for at least using the RefcountingSurfaceDescriptorWrapper thing introduced in the present patch, everywhere we are currently holding on to SurfaceDescriptor's.

> Do we have a blocking bug that crashes on GrAllocBufferActor that this is
> fixing?

Yes, this is bug 914823, and I'm getting B2G release management alert emails very frequently about it!

Comment 11

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

Comment on attachment 824324 [details] [diff] [review]
Offer a RefcountingSurfaceDescriptorWrapper that wraps a SurfaceDescriptor and behaves as a strong ref to any GrallocBufferActor that it may wrap

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

This looks good also!

::: gfx/layers/ipc/RefcountingSurfaceDescriptor.h
@@ +20,5 @@
> +  return desc.bufferParent() ? static_cast<GrallocBufferActor*>(desc.bufferParent())
> +                             : static_cast<GrallocBufferActor*>(desc.bufferChild());
> +}
> +
> +inline void AddRefSurfaceDescriptorActor(...)

I tend to think this sort of thing is cleaner with templates but that's just a personal opinion I guess.

@@ +84,5 @@
> +    case SurfaceDescriptor::TSurfaceDescriptorGralloc:
> +      return ReleaseSurfaceDescriptorActor(desc.get_SurfaceDescriptorGralloc());
> +    case SurfaceDescriptor::TNewSurfaceDescriptorGralloc:
> +      return ReleaseSurfaceDescriptorActor(desc.get_NewSurfaceDescriptorGralloc());
> +    default: return; // do nothing for unhandled descriptor types

In general I try to handle all the types and have an assertion that fires if a type that I don't expect crops up. Otherwise future people who add a new SurfaceDescriptor type would just have to know to update the code here.

@@ +89,5 @@
> +  }
> +}
> +
> +template<typename DescriptorType>
> +class RefcountingSurfaceDescriptorWrapper

I don't think this is going to be a problem but the way this is currently written I believe RefcountingSurfaceDescriptorWrapper<int> would compile, wouldn't it?

@@ +129,5 @@
> +  RefcountingSurfaceDescriptorWrapper& operator=(const DescriptorType& desc)
> +  {
> +    const DescriptorType& oldDesc = mDesc;
> +    mDesc = desc;
> +    if (&mDesc != &oldDesc) {

Hm, this isn't quite right. |oldDesc| is just an alias to |mDesc|. Replacing |mDesc| with |oldDesc| doesn't change anything about the address of |mDesc| or |oldDesc| so this branch will never be taken.

You can probably just always do this:

  AddRefSurfaceDescriptorActor(desc);
  ReleaseSurfaceDescriptorActor(mDesc);
  desc = mDesc;

Right?

Comment 12

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: Make GrallocBufferActor refcounted

This version addresses Ben's feedback:

(In reply to ben turner [:bent] (needinfo? encouraged) from comment #8)
> ::: gfx/layers/ipc/ImageBridgeParent.cpp
> @@ +145,2 @@
> >  #else
> >    NS_RUNTIMEABORT("No gralloc buffers for you");
> 
> While you're here can you change this to a NS_WARNING? We shouldn't abort
> the parent process if a hacked child sends us this message. Returning null
> already causes the child to be killed. Same goes for Dealloc below, and same
> in LayerTransactionParent. (Just the parent classes, not the child classes).

Great idea, but I'm deferring this to another patch to avoid making this one bigger than necessary.

> 
> ::: gfx/layers/ipc/ShadowLayerUtilsGralloc.cpp
> @@ +334,5 @@
> >      PGrallocBufferChild* gbc =
> >        aSurface->get_SurfaceDescriptorGralloc().bufferChild();
> > +    if (static_cast<GrallocBufferActor*>(gbc)->IPCOpen()) {
> > +      unused << PGrallocBufferChild::Send__delete__(gbc);
> > +    }
> 
> If you're trying to track down places where the actors were previously used
> after being destroyed you could add warnings/asserts to all these spots if
> IPCOpen() is false?

I don't know that I'm trying to track that down: the reason for the present changes is precisely that it seems inevitable that there will be things hanging on to GrallocBufferActors after IPC is already gone i.e. with mIPCOpen=false, so it seems that having these things silently avoid trying to do IPC is actually what I want. IOW, if I added a warning there, we already know that that warning would be generated e.g. in the situation of bug 914823 and we wouldn't do anything about that warning.

> 
> ::: gfx/layers/ipc/ShadowLayerUtilsGralloc.h
> @@ +69,5 @@
> >    friend class ImageBridgeChild;
> >    typedef android::GraphicBuffer GraphicBuffer;
> >  
> >  public:
> >    virtual ~GrallocBufferActor();
> 
> Now that this is refcounted it should only be deleted through the Release
> method. You can make this private to prevent anything from compiling that
> tries to manually delete one of these.

Great idea, done. It's a little bit unfortunate that this required befriending internals of mozilla::Refcounted from MFBT's "detail" namespace, but that's still a useful safety measure given all the trouble we've had with that.

> 
> @@ +80,1 @@
> >    Create();
> 
> I think this is fine but the old code had the benefit of self-documenting
> which Create() was supposed to be called in each process. Now I guess a
> comment to that effect would be useful.

Added comments; do they say what they should?

> 
> @@ +94,5 @@
> >    android::GraphicBuffer* GetGraphicBuffer();
> >  
> >    void InitFromHandle(const MagicGrallocBufferHandle& aHandle);
> >  
> > +  void AddIPDLReference() {
> 
> In debug builds it might be worth asserting |!mIPCOpen| here before setting
> it to true (and asserting the opposite in ReleaseIPDLReference).

Done (and agree it's a good idea).

> 
> You could also make the various manager classes
> (ImageBridge{Child|Parent}/LayerTransaction{Child|Parent}) friends and then
> hide these methods in a private section. That's slightly paranoid I guess
> but it might prevent future misuse.

Done (and agree it's a good idea).

> 
> Actually, now that I look closer, I think you could remove AddIPDLReference
> entirely and just call AddRef() as the last step of your Create() functions.
> That way IPDL could only ever call the ReleaseIPDLReference function. Dunno
> if you think that is cleaner or messier ;)

It would irk me a bit to break the symmetry between AddIPDLReference and ReleaseIPDLReference. It would seem a little bit "actor lifetime out of balance", as in http://www.youtube.com/watch?v=StzE0VjjNp4

Comment 13

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: Don't let a child process crash the parent process

This is the discussed change that removes RUNTIMEABORT from parent-side code in a few places.

Comment 14

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: Offer a RefcountingSurfaceDescriptorWrapper that wraps a SurfaceDescriptor and behaves as a strong ref to any GrallocBufferActor that it may wrap

(In reply to ben turner [:bent] (needinfo? encouraged) from comment #11)
> I tend to think this sort of thing is cleaner with templates but that's just
> a personal opinion I guess.

Done.

> 
> @@ +84,5 @@
> > +    case SurfaceDescriptor::TSurfaceDescriptorGralloc:
> > +      return ReleaseSurfaceDescriptorActor(desc.get_SurfaceDescriptorGralloc());
> > +    case SurfaceDescriptor::TNewSurfaceDescriptorGralloc:
> > +      return ReleaseSurfaceDescriptorActor(desc.get_NewSurfaceDescriptorGralloc());
> > +    default: return; // do nothing for unhandled descriptor types
> 
> In general I try to handle all the types and have an assertion that fires if
> a type that I don't expect crops up. Otherwise future people who add a new
> SurfaceDescriptor type would just have to know to update the code here.

Here, it is intentional to do nothing in the generic case, because the vast majority of SurfaceDescriptor types do not have refcounted actors. I changed the comment to reflect that. Is that OK?

> 
> @@ +89,5 @@
> > +  }
> > +}
> > +
> > +template<typename DescriptorType>
> > +class RefcountingSurfaceDescriptorWrapper
> 
> I don't think this is going to be a problem but the way this is currently
> written I believe RefcountingSurfaceDescriptorWrapper<int> would compile,
> wouldn't it?

Right, the new version makes sure that this only compiles for types that are convertible to SurfaceDescriptor.

> 
> @@ +129,5 @@
> > +  RefcountingSurfaceDescriptorWrapper& operator=(const DescriptorType& desc)
> > +  {
> > +    const DescriptorType& oldDesc = mDesc;
> > +    mDesc = desc;
> > +    if (&mDesc != &oldDesc) {
> 
> Hm, this isn't quite right. |oldDesc| is just an alias to |mDesc|. Replacing
> |mDesc| with |oldDesc| doesn't change anything about the address of |mDesc|
> or |oldDesc| so this branch will never be taken.
> 
> You can probably just always do this:
> 
>   AddRefSurfaceDescriptorActor(desc);
>   ReleaseSurfaceDescriptorActor(mDesc);
>   desc = mDesc;
> 
> Right?

Ooops! Obviously, C++ is too hard for me. Thanks a lot for spotting this! Applied your suggestion. I was trying to avoid doing useless work (atomic ops are a little bit expensive on mobile), but that is futile: since the wrapper stores its descriptor by value, it's never going to be the case that one assigns to it its own descriptor.

Comment 15

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: Offer a RefcountingSurfaceDescriptorWrapper that wraps a SurfaceDescriptor and behaves as a strong ref to any GrallocBufferActor that it may wrap

Comment 16

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

Comment on attachment 824628 [details] [diff] [review]
Don't let a child process crash the parent process

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

::: gfx/layers/ipc/ImageBridgeParent.cpp
@@ +88,5 @@
>  ImageBridgeParent::RecvUpdateNoSwap(const EditArray& aEdits)
>  {
>    InfallibleTArray<EditReply> noReplies;
>    bool success = RecvUpdate(aEdits, &noReplies);
> +  NS_ASSERTION(noReplies.Length() == 0, "RecvUpdateNoSwap requires a sync Update to carry Edits");

NS_ABORT_IF_FALSE and NS_ASSERTION are the same, they both disappear on non-DEBUG builds. You want to just NS_WARN and return false here to kill the child.

::: gfx/layers/ipc/LayerTransactionParent.cpp
@@ +348,5 @@
>          imageLayer->SetScaleToSize(attrs.scaleToSize(), attrs.scaleMode());
>          break;
>        }
>        default:
> +        NS_WARNING("unhandled SpecificLayerAttributes type in LayerTransactionParent::RecvUpdate");

This should also return false to kill the child. (Also, if you don't return here, you'll just continue to loop even though you know the child is misbehaving).

@@ +425,5 @@
>        compositableParent->SetCompositorID(mLayerManager->GetCompositor()->GetCompositorID());
>        break;
>      }
>      default:
> +      NS_WARNING("unhandled Edit type in LayerTransactionParent::RecvUpdate");

Same here.

Comment 17

[Sotaro Ikeda [:sotaro]]

Comment on attachment 824626 [details] [diff] [review]
Make GrallocBufferActor refcounted

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

I have some questions.

How is IPC error situation handled by GrallocBufferActor. When it happens, GrallocBufferActor should not send IPC. Isn't mIPCOpen handle also IPC disconnect because of IPC error?

::: gfx/layers/ipc/ImageBridgeChild.cpp
@@ +666,5 @@
>                                             MaybeMagicGrallocBufferHandle*)
>  {
>  #ifdef MOZ_HAVE_SURFACEDESCRIPTORGRALLOC
> +  GrallocBufferActor* actor = GrallocBufferActor::Create();
> +  actor->AddIPDLReference();

Isn't it necessary to check if actor is not nullptr before using it?

::: gfx/layers/ipc/ImageBridgeParent.cpp
@@ +139,5 @@
>                                               MaybeMagicGrallocBufferHandle* aOutHandle)
>  {
>  #ifdef MOZ_HAVE_SURFACEDESCRIPTORGRALLOC
> +  GrallocBufferActor* actor = GrallocBufferActor::Create(aSize, aFormat, aUsage, aOutHandle);
> +  actor->AddIPDLReference();

Isn't it necessary to check if actor is not nullptr before using it?

::: gfx/layers/ipc/LayerTransactionChild.cpp
@@ +35,5 @@
>                                             MaybeMagicGrallocBufferHandle*)
>  {
>  #ifdef MOZ_HAVE_SURFACEDESCRIPTORGRALLOC
> +  GrallocBufferActor* actor = GrallocBufferActor::Create();
> +  actor->AddIPDLReference();

Isn't it necessary to check if actor is not nullptr before using it?

::: gfx/layers/ipc/LayerTransactionParent.cpp
@@ +548,5 @@
>                                              MaybeMagicGrallocBufferHandle* aOutHandle)
>  {
>  #ifdef MOZ_HAVE_SURFACEDESCRIPTORGRALLOC
> +  GrallocBufferActor* actor = GrallocBufferActor::Create(aSize, aFormat, aUsage, aOutHandle);
> +  actor->AddIPDLReference();

Isn't it necessary to check if actor is not nullptr before using it?

Comment 18

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

(In reply to Sotaro Ikeda [:sotaro] from comment #17)
> How is IPC error situation handled by GrallocBufferActor. When it happens,
> GrallocBufferActor should not send IPC. Isn't mIPCOpen handle also IPC
> disconnect because of IPC error?

Any error will trigger the DeallocPGrallocBuffer call in the manager actors so the flag will get set regardless.

Comment 19

[Sotaro Ikeda [:sotaro]]

(In reply to ben turner [:bent] (needinfo? encouraged) from comment #18)
> 
> Any error will trigger the DeallocPGrallocBuffer call in the manager actors
> so the flag will get set regardless.

Thanks!

Comment 20

[Sotaro Ikeda [:sotaro]]

(In reply to Sotaro Ikeda [:sotaro] from comment #17)
> Comment on attachment 824626 [details] [diff] [review]
> Make GrallocBufferActor refcounted
> 
> Review of attachment 824626 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I have some questions.
> 
> How is IPC error situation handled by GrallocBufferActor. When it happens,
> GrallocBufferActor should not send IPC. Isn't mIPCOpen handle also IPC
> disconnect because of IPC error?
> 
> ::: gfx/layers/ipc/ImageBridgeChild.cpp
> @@ +666,5 @@
> >                                             MaybeMagicGrallocBufferHandle*)
> >  {
> >  #ifdef MOZ_HAVE_SURFACEDESCRIPTORGRALLOC
> > +  GrallocBufferActor* actor = GrallocBufferActor::Create();
> > +  actor->AddIPDLReference();
> 
> Isn't it necessary to check if actor is not nullptr before using it?
> 
> ::: gfx/layers/ipc/ImageBridgeParent.cpp
> @@ +139,5 @@
> >                                               MaybeMagicGrallocBufferHandle* aOutHandle)
> >  {
> >  #ifdef MOZ_HAVE_SURFACEDESCRIPTORGRALLOC
> > +  GrallocBufferActor* actor = GrallocBufferActor::Create(aSize, aFormat, aUsage, aOutHandle);
> > +  actor->AddIPDLReference();
> 
> Isn't it necessary to check if actor is not nullptr before using it?
> 
> ::: gfx/layers/ipc/LayerTransactionChild.cpp
> @@ +35,5 @@
> >                                             MaybeMagicGrallocBufferHandle*)
> >  {
> >  #ifdef MOZ_HAVE_SURFACEDESCRIPTORGRALLOC
> > +  GrallocBufferActor* actor = GrallocBufferActor::Create();
> > +  actor->AddIPDLReference();
> 
> Isn't it necessary to check if actor is not nullptr before using it?
> 
> ::: gfx/layers/ipc/LayerTransactionParent.cpp
> @@ +548,5 @@
> >                                              MaybeMagicGrallocBufferHandle* aOutHandle)
> >  {
> >  #ifdef MOZ_HAVE_SURFACEDESCRIPTORGRALLOC
> > +  GrallocBufferActor* actor = GrallocBufferActor::Create(aSize, aFormat, aUsage, aOutHandle);
> > +  actor->AddIPDLReference();
> 
> Isn't it necessary to check if actor is not nullptr before using it?

OK, I understand the function never return nullptr.

Comment 21

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 824634 [details] [diff] [review]
Offer a RefcountingSurfaceDescriptorWrapper that wraps a SurfaceDescriptor and behaves as a strong ref to any GrallocBufferActor that it may wrap

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

I believe this should be ref counting the other kinds of things in the surface descriptor for consistency.

::: gfx/layers/ipc/RefcountingSurfaceDescriptor.h
@@ +5,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#ifndef RefcountingSurfaceDescriptor_h_
> +#define RefcountingSurfaceDescriptor_h_

Add a description of why this exists and what you hope for it in the future.

Comment 22

[Nicolas Silva [:nical]]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #21)
> Comment on attachment 824634 [details] [diff] [review]
> Offer a RefcountingSurfaceDescriptorWrapper that wraps a SurfaceDescriptor
> and behaves as a strong ref to any GrallocBufferActor that it may wrap
> 
> Review of attachment 824634 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I believe this should be ref counting the other kinds of things in the
> surface descriptor for consistency.

I disagree. Other  kinds of things are value type thingies that are copied around so ref counting them doesn't make much sense.

Comment 23

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 824626 [details] [diff] [review]
Make GrallocBufferActor refcounted

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

I believe we decided not to take this.

Comment 24

[Gregory Pappas [:gregp]]

Closing old B2G bugs