933082 - (RefcntAllocator) Make it possible to hold on to an ISurfaceAllocator and check if its IPC connection is still open

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

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

In bug 914823, we have a crash in code like this in SharedSurface_Gralloc (see bug 914823 comment 71) in B2G 1.2:

    if (mAllocator) {
        mAllocator->DestroySharedSurface(&desc); // crash here
    }

Here, mAllocator is currently a WeakPtr<ISurfaceAllocator>.

The crash is a segfault at address 0x18, which happens to be the offset of ISurfaceAllocator::DestroySharedSurface in ISurfaceAllocator's vtable (it being the 7th virtual method by order of declaration, we have (7 - 1) * sizeof(void*) == 6*4 == 24 == 0x18).

So it seems that mAllocator is null at the crash line, even though this is inside a if (mAllocator) block.

Maybe we have a race condition here, which would be surprising as this is not supposed to be accessed from another thread, and the crash reports' raw dumps don't have any thread in places that might race with this. Or maybe we have a bug in WeakPtr...

Anyway. The reason why this was made a WeakPtr in the first place is that we used to have crashes (earlier in the same bug 914823) with the ISurfaceAllocator dying unexpectedly, and we couldn't hold a strong pointer to it because this would have closed a strong reference cycle. Indeed, the ISurfaceAllocator here is the base class of the ClientLayerManager, which has a chain of strong references to us (basically, ClientLayerManager->ClientCanvasLayer->GLContext->GLScreenBuffer->SurfaceStream->SharedSurface_Gralloc)

We can fix that by making ClientLayerManager not inherit from ISurfaceAllocator anymore, instead hold itself a strong reference to an ISurfaceAllocator. It then becomes possible for our SharedSurface_Gralloc to also hold a strong reference to that ISurfaceAllocator, without closing a reference cycle.

Once we have that, we can be confident that the ISurfaceAllocator doesn't go away under our feet, but we have a new problem to solve: it becomes possible for the IPC channel to be closed under our feet even though the ISurfaceAllocator is alive. The IPC channel here is a LayerTransactionChild. The way that we can become able to tell if it was closed, is to hold a strong reference to it, and have a |mIPCOpen| flag on it that is set to false when the IPDL Dealloc*() method is called on it. Currently, LayerTransactionChild is not refcounted, and Dealloc just destroys it.

So our first prerequisite is to make LayerTransactionChild refcounted and add that flag (patch 1)

Once we have that, we add a IPCOpen virtual method in ISurfaceAllocator whose default impl just returns true, but which is overridden in ShadowLayerForwarder to return the right answer from LayerTransactionChild. DestroySharedSurface uses this flag to avoid actually sending messages after the channel is gone. (patch 2)

Next, we must be careful: as LayerTransactionChild is now refcounted, we must have ShadowLayerForwarder hold a strong reference to it, otherwise it could go away under its feet. (patch 3).

Finally, we can remove ClientLayerManager's inheritance of ShadowLayerForwarder and instead have ClientLayerManager hold a strong reference to a ShadowLayerForwarder. This allows to make ISurfaceAllocator (the base class) refcounted, and in turn this allows SharedSurface_Gralloc to hold a strong reference to ISurfaceAllocator (patch 4)

Comment 1

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

Attachment: Part 1: make LayerTransactionChild reference-counted

Comment 2

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

Attachment: Part 2: propagate the mIPCOpen flag, avoid crashing in DestroySharedSurface on a dead channel

Comment 3

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

Attachment: Part 3: let ShadowLayerForwarder hold a strong ref to its LayerTransactionChild

Comment 4

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

Attachment: Part 4: let ClientLayerManager hold a ShadowLayerForwarder instead of inheriting from ShadowLayerForwarder. Make ISurfaceAllocator refcounted. Make SharedSurface_Gralloc keep its mAllocator alive.

Comment 5

[Jason Smith [:jsmith]]

Is this bug required to fix the blocking bug? If so, then this should be set to koi?

Comment 6

[Matt Woodrow (:mattwoodrow)]

If we're crashing at 0x18, then it means *mAllocator is null, not mAllocator.

Comment 7

[Matt Woodrow (:mattwoodrow)]

And if mAllocator is just a raw pointer (https://bugzilla.mozilla.org/show_bug.cgi?id=914823#c86), then it makes sense that this could be a valid pointer to deleted memory.

It's surprising to me that the value at that address is 0x0 though, rather than garbage or the allocators poison value.

Comment 8

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

(In reply to Jason Smith [:jsmith] from comment #5)
> Is this bug required to fix the blocking bug? If so, then this should be set
> to koi?

We're actually still unsure, I just marked it as blocking preemptively... but the below means I was wrong!

(In reply to Matt Woodrow (:mattwoodrow) from comment #6)
> If we're crashing at 0x18, then it means *mAllocator is null, not mAllocator.

Dang! Sorry for being so stupid! Indeed it means that mAllocator->vptr is null, which means that it was already destroyed and overwritten by something!

Unblocking 914823.

Comment 9

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

Getting back to this as it blocks bug 941389 and bug 941390.

Comment 10

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

Attachment: Part 1: make LayerTransactionChild reference-counted

Comment 11

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

Attachment: Part 2: propagate the mIPCOpen flag, avoid crashing in DestroySharedSurface on a dead channel

Comment 12

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

Attachment: Part 3: let ShadowLayerForwarder hold a strong ref to its LayerTransactionChild

Comment 13

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

Attachment: Part 4: let ClientLayerManager hold a ShadowLayerForwarder instead of inheriting from ShadowLayerForwarder. Make ISurfaceAllocator refcounted. Make SharedSurface_Gralloc keep its mAllocator alive.

Ready for review!

Comment 14

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

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

Comment 15

[Nicolas Silva [:nical]]

Comment on attachment 8338011 [details] [diff] [review]
Part 4: let ClientLayerManager hold a ShadowLayerForwarder instead of inheriting from ShadowLayerForwarder. Make ISurfaceAllocator refcounted. Make SharedSurface_Gralloc keep its mAllocator alive.

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

Nice!

Comment 16

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

https://tbpl.mozilla.org/?tree=Try&rev=6d5cff2d0899

Comment 17

[Benoit Girard (:BenWa)]

Attachment: mac fix

Comment 18

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

Many thanks for the mac fix!

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/9cc4a77561e8
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/a249bd79e71a
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/1509f8be5df3
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/7caa8e80b06a

Comment 19

[Ryan VanderMeulen [:RyanVM]]

Backed out for mass mochitest asserts.
https://hg.mozilla.org/integration/mozilla-inbound/rev/641a86dd8f8d

https://tbpl.mozilla.org/php/getParsedLog.php?id=31108505&tree=Mozilla-Inbound

Comment 20

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

Had forgotten to update a few {A,Dea}llocPLayerTransaction{Parent,Child}. I don't suppose that another cycle of reviews is needed for that.

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

Comment 21

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

Had forgotten to initialize LayerTransactionParent::mIPCOpen to false >_<

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

Comment 22

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

Had forgotten to remove the NS_INLINE_DECL_THREADSAFE_REFCOUNTING(ImageBridgeParent)

https://tbpl.mozilla.org/?tree=Try&rev=3c48adddf0f6

Comment 23

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/1cf4d557bf94
https://hg.mozilla.org/integration/mozilla-inbound/rev/849931c5c32e
https://hg.mozilla.org/integration/mozilla-inbound/rev/958fa3ca2b59
https://hg.mozilla.org/integration/mozilla-inbound/rev/b5efbd83b16b

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1cf4d557bf94
https://hg.mozilla.org/mozilla-central/rev/849931c5c32e
https://hg.mozilla.org/mozilla-central/rev/958fa3ca2b59
https://hg.mozilla.org/mozilla-central/rev/b5efbd83b16b

Comment 25

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

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