1280110 - Crash in mozilla::layers::CompositingRenderTargetOGL::~CompositingRenderTargetOGL

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Nicholas Nethercote [inactive]]

This bug was filed from the Socorro interface and is 
report bp-42d04018-7f31-4b70-9f16-8a29d2160507.
=============================================================

This crash has occurred 349 times in the past week, in versions 47--50. It's only on Mac, and the crash address is always zero.

jgilbert, any ideas?

Comment 1

[Nicolas Silva [:nical]]

Attachment: Simple null check

Not a great solution, but i'd uplift this patch eyes closed.

Comment 2

[Nicolas Silva [:nical]]

Attachment: Make sure we never create a CompositingRenderTargetOGL without a gl context

Return a null render target rather than creating one without a GL context, make sure the callers of CreateRenderTarget either null-check or are never called if the compositor is destroyed (which would be the reason mGLContext is null when creating the render target).

Comment 3

[Sotaro Ikeda [:sotaro]]

From the crash, it seems that there could be case that CompositorOGL::Destroy() is called before CompositingRenderTargetOGL destruction. From the source, when CompositorBridgeParent::ActorDestroy() is called without calling CompositorBridgeParent::RecvWillClose(), ClearCachedResources() is not called during the destruction.

Isn't it better to add calling the ClearCachedResources() in CompositorBridgeParent::ActorDestroy() like the RecvWillClose(), if mLayerManager is still valid, is it? It could ensure intermediate surfaces' CompositingRenderTarget destructions.

 https://dxr.mozilla.org/mozilla-central/source/gfx/layers/ipc/CompositorBridgeParent.cpp#673

Comment 4

[Nicolas Silva [:nical]]

(In reply to Sotaro Ikeda [:sotaro] from comment #3)
> Isn't it better to add calling the ClearCachedResources() in
> CompositorBridgeParent::ActorDestroy() like the RecvWillClose(), if
> mLayerManager is still valid, is it? It could ensure intermediate surfaces'
> CompositingRenderTarget destructions.

We should do both. My patch explicitly handles failure cases of this kind, while calling ClearCachedResource in ActorDestroy will remove one of the possible ways (the most likely one) to be in this situations, but I wouldn't be surprised that other ways to get there exist or will exist eventually.

I have a patch that implements your suggestion which I am about to upload. landing both patches (well, the three of them) makes the code more robust without adding significant complexity.

Comment 5

[Nicolas Silva [:nical]]

Attachment: Do more thorough cleanup in ActorDestoy in case RecvWillStop hasn't run.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1db92044b8de
Null check mGL when destroying CompositingRenderTargetOGL. r=jnicol

Comment 7

[Nicolas Silva [:nical]]

Comment on attachment 8763523 [details] [diff] [review]
Simple null check

Approval Request Comment
[Feature/regressing bug #]:
[User impact if declined]: Some crashes (rather low volume).
[Describe test coverage new/current, TreeHerder]: None.
[Risks and why]: None. A trivial null-check before dereferencing the pointer.
[String/UUID change made/needed]: None.

Comment 8

[Nicolas Silva [:nical]]

try push with all patches applied: https://treeherder.mozilla.org/#/jobs?repo=try&revision=794b4cf752b2

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/042ea219692c
Ensure CompositingRenderTargetOGL is never created without a gl context. r=sotaro
https://hg.mozilla.org/integration/mozilla-inbound/rev/0da5b65544ce
Make sure compositor and layer resources are cleared when shutting down CompositorBridgeParent. r=sotaro

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1db92044b8de
https://hg.mozilla.org/mozilla-central/rev/042ea219692c
https://hg.mozilla.org/mozilla-central/rev/0da5b65544ce

Comment 11

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8763523 [details] [diff] [review]
Simple null check

Fix a crash, taking it.
Merci!

Should be in 48 beta 3

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/9f5af086dcbb

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/04c2506670cf

Comment 14

[Andrew McCreight (out of office until 8/21) [:mccr8]]

Maybe this is expected, but a crash with this signature is still happening. For instance:
  https://crash-stats.mozilla.com/report/index/e7b26517-8e56-4c96-a0c4-ca2822160701

Comment 15

[Sotaro Ikeda [:sotaro]]

Ah, we forgot to clean up ContainerLayerComposite::mPrepared in ContainerLayerComposite::CleanupResources().

Comment 16

[Nicolas Silva [:nical]]

Yes indeed, patch incoming!

Comment 17

[Nicolas Silva [:nical]]

Attachment: Cleanup ContainerLayerComposite's intermediate targets in ClearCachedResources.

Comment 18

[Sotaro Ikeda [:sotaro]]

Comment on attachment 8767595 [details] [diff] [review]
Cleanup ContainerLayerComposite's intermediate targets in ClearCachedResources.

Looks good!

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/093656bb1702
Cleanup ContainerLayerComposite's intermediate render targets during shutdown. r=sotaro

Comment 20

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/093656bb1702

Comment 21

[Nicolas Silva [:nical]]

Comment on attachment 8767595 [details] [diff] [review]
Cleanup ContainerLayerComposite's intermediate targets in ClearCachedResources.

Approval Request Comment
[Feature/regressing bug #]:
[User impact if declined]: Some crashes.
[Describe test coverage new/current, TreeHerder]: None.
[Risks and why]: Rather low. Too late for beta, maybe (unless the crash volume is significant), but simple enough to take in aurora without second thoughts. 
[String/UUID change made/needed]: None.

Comment 22

[Liz Henry (:lizzard) (relman/hg->git project)]

5 crashes or so per week on beta versions, and we are heading into beta 6 build tonight. So I think this is ok to keep to 49.

Comment 23

[Liz Henry (:lizzard) (relman/hg->git project)]

Marking this affected so that it will show up on sheriff's queries as needing to land on aurora.

Comment 24

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8767595 [details] [diff] [review]
Cleanup ContainerLayerComposite's intermediate targets in ClearCachedResources.

Crash fix cleanup, ok to uplift to aurora.

Comment 25

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/a1df90640235

Comment 26

[Gerry Chang [:gchang]]

Hi Nicolas,
Do you consider to uplift this patch in 48 beta 7 if this is not too risky?

Comment 27

[Nicolas Silva [:nical]]

(In reply to Gerry Chang [:gchang] from comment #26)
> Hi Nicolas,
> Do you consider to uplift this patch in 48 beta 7 if this is not too risky?

We could if the crash volume warrants it. But quoting liz:

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #22)
> 5 crashes or so per week on beta versions, and we are heading into beta 6
> build tonight. So I think this is ok to keep to 49.

The patch is not very risky but there isn't a lot of time left for beta to bake. At this point it is a release management question more than an engineering one.

Comment 28

[Gerry Chang [:gchang]]

Ok, let's ride the train on 49/50 and won't fix in 48.

Comment 30

[Ting-Yu Chou [:ting] (away)]

The crash is still happening, bp-1d5ab4c7-eced-4faf-ae5d-446e62160822.

Comment 31

[Nicolas Silva [:nical]]

(In reply to Ting-Yu Chou [:ting] from comment #30)
> The crash is still happening, bp-1d5ab4c7-eced-4faf-ae5d-446e62160822.

Let's reopen, then. Only 3 crashes since the patch landed, so it's fairly low priority for me right now.

Comment 32

[Mark Banner (:standard8)]

(In reply to Nicolas Silva [:nical] from comment #31)
> (In reply to Ting-Yu Chou [:ting] from comment #30)
> > The crash is still happening, bp-1d5ab4c7-eced-4faf-ae5d-446e62160822.
> 
> Let's reopen, then. Only 3 crashes since the patch landed, so it's fairly
> low priority for me right now.

I've just created bug 1297390 as a new one - I've been reproducing it running activity stream tests.

I would recommend leaving this one as fixed, as its already shipped on non-nightly versions, and it makes the tracking awkward if there's a second round of uplifts.