Closed Bug 1360478 Opened 2 years ago Closed 2 years ago

Fix crash when initializing APZ after compositor shutdown

Categories

(Core :: Graphics: Layers, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox-esr52 --- unaffected
firefox53 --- unaffected
firefox54 --- fixed
firefox55 --- fixed

People

(Reporter: dvander, Assigned: dvander)

References

Details

(Keywords: crash)

Attachments

(2 files)

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.
David, would this also fix the crashes we see for various other tests like bug 1338422?
(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.
Pushed by danderson@mozilla.com:
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)
https://hg.mozilla.org/mozilla-central/rev/e51294711514
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Going of the bugs this is fixing, I believe this needs an uplift request for 54 as well.
Flags: needinfo?(dvander)
Keywords: crash
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]:
Flags: needinfo?(dvander)
Attachment #8862753 - Flags: approval-mozilla-aurora?
Comment on attachment 8862753 [details] [diff] [review]
patch

54 is on Beta now :)
Attachment #8862753 - Flags: approval-mozilla-aurora? → approval-mozilla-beta?
Attached patch fix leakSplinter Review
Attachment #8864001 - Flags: review?(bugmail)
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/
Attachment #8864001 - Flags: review?(bugmail) → review+
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Pushed by danderson@mozilla.com:
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)
https://hg.mozilla.org/mozilla-central/rev/f0c85b49e722
Status: REOPENED → RESOLVED
Closed: 2 years ago2 years ago
Resolution: --- → FIXED
Comment on attachment 8862753 [details] [diff] [review]
patch

Fix a crash. Beta54+. Should be in 54 beta 6.
Attachment #8862753 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(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.
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.