1360478 - Fix crash when initializing APZ after compositor shutdown

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[David Anderson [:dvander] - inactive, e-mail if emergency]

See bug 1350634 comment #22 - TabChild::InitAPZState can be called after a window has shutdown its compositor, in which case LayerTreeState::mParent will be null. We should protect against this like we do elsewhere in CPCBP.

Comment 1

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: patch

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9cf2165db83fe44303c3a2bed721f7bbfa7f7206

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+1]]

David, would this also fix the crashes we see for various other tests like bug 1338422?

Comment 3

[Kartikaya Gupta (email:[e-mail])]

(In reply to Henrik Skupin (:whimboo) from comment #2)
> David, would this also fix the crashes we see for various other tests like
> bug 1338422?

I would expect it to, yes.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e51294711514
Fix an APZ crash when a tab loads after its widget has been destroyed. (bug 1360478, r=kats)

Comment 5

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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

Comment 6

[Ryan VanderMeulen [:RyanVM]]

Going of the bugs this is fixing, I believe this needs an uplift request for 54 as well.

Comment 7

[David Anderson [:dvander] - inactive, e-mail if emergency]

Comment on attachment 8862753 [details] [diff] [review]
patch

Approval Request Comment
[Feature/Bug causing the regression]: Changes to APZ for the GPU Process
[User impact if declined]: Rare crashes when closing a window while a new tab opens.
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]:
[Is the change risky?]: No
[Why is the change risky/not risky?]: Previously this code could null-deref and crash due to a rare timing issue. The null deref is no longer possible since we construct a temporary object. The tab and window are in the process of closing so not much interesting can really go wrong.
[String changes made/needed]:

Comment 8

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8862753 [details] [diff] [review]
patch

54 is on Beta now :)

Comment 9

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: fix leak

Comment 10

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8864001 [details] [diff] [review]
fix leak

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

::: gfx/layers/ipc/CrossProcessCompositorBridgeParent.cpp
@@ +123,5 @@
>    // to unmap our layers id, and we could get here without a parent compositor.
>    // In this case return an empty APZCTM.
>    if (!state.mParent) {
> +    // Note: we immediately call ClearTree since otherwise the APZCTM will
> +    // retained a reference to itself, through the checkerboard observer.

s/retained/retain/

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f0c85b49e722
Make sure to call ClearTree when creating a temporary APZCTreeManager. (bug 1360478 follow-up, r=kats)

Comment 12

[Carsten Book [:Tomcat]]

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

Comment 13

[Gerry Chang [:gchang]]

Comment on attachment 8862753 [details] [diff] [review]
patch

Fix a crash. Beta54+. Should be in 54 beta 6.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/ac2b6545bb36
https://hg.mozilla.org/releases/mozilla-beta/rev/6f633ed9547e

Comment 15

[Andrei Vaida [:avaida]]

(In reply to David Anderson [:dvander] from comment #7)
> [Is this code covered by automated tests?]: Yes
> [Has the fix been verified in Nightly?]: Yes
> [Needs manual test from QE? If yes, steps to reproduce]: No

Setting qe-verify- based on David's assesment on manual testing needs and the fact that this fix has automated coverage.