1517275 - Crash in mozilla::layers::APZCTreeManager::FlushApzRepaints

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

[David Bolter [:davidb] (NeedInfo me for attention)]

This bug was filed from the Socorro interface and is
report bp-e5cecca8-81ad-4181-bd8f-d4bd00190102.
=============================================================

Top 10 frames of crashing thread:

0 libxul.so mozilla::layers::APZCTreeManager::FlushApzRepaints gfx/layers/apz/src/APZCTreeManager.cpp:1150
1 libxul.so mozilla::detail::RunnableFunction<mozilla::layers::CompositorBridgeParent::FlushApzRepaints gfx/layers/ipc/CompositorBridgeParent.cpp:1500
2 libxul.so mozilla::RunAndroidUiTasks widget/android/AndroidUiThread.cpp:360
3 libxul.so long long mozilla::jni::NativeStub<mozilla::java::GeckoThread::RunUiThreadCallback_t, GeckoThreadSupport, mozilla::jni::Args<> >::Wrap<&GeckoThreadSupport::RunUiThreadCallback> widget/android/nsAppShell.cpp:243
4 base.odex base.odex@0x82e3a6 
5 dalvik-LinearAlloc (deleted) dalvik-LinearAlloc @0xc657 
6 dalvik-main space (deleted) dalvik-main space @0x7ae8f 
7 dalvik-main space (deleted) dalvik-main space @0x97b7 
8 dalvik-main space (deleted) dalvik-main space @0x1cdf 
9 dalvik-main space (deleted) dalvik-main space @0x33d97f 

=============================================================

Comment 1

[David Bolter [:davidb] (NeedInfo me for attention)]

Speculating mozilla::layers::CompositorBridgeParent::FlushApzRepaints signature is similar cause.

Comment 2

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

Presumably the problem here is that by the time the runnable at [1] is actually run, the mApzcTreeManager field on CompositorBridgeParent has been nulled out.

[1] https://searchfox.org/mozilla-central/rev/0ee0b63732d35d16ba22d5a1120622e2e8d58c29/gfx/layers/ipc/CompositorBridgeParent.cpp#1362

Comment 3

[Botond Ballo [:botond]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #2)
> Presumably the problem here is that by the time the runnable at [1] is
> actually run, the mApzcTreeManager field on CompositorBridgeParent has been
> nulled out.

APZCTreeManager::FlushApzRepaints() doesn't appear to access the 'this' pointer at all. (It and GetContentController() could both be made static and things would compile.)

As an alternative theory, the controller could be null here [2].

[2] https://searchfox.org/mozilla-central/rev/0ee0b63732d35d16ba22d5a1120622e2e8d58c29/gfx/layers/apz/src/APZCTreeManager.cpp#1134

Comment 4

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

Ah, you're right, it's more likely to be the controller that's null, given the stack. Making those functions static seems like a good idea.

Comment 5

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

https://treeherder.mozilla.org/#/jobs?repo=try&group_state=expanded&revision=909d0579f1206cb38248a8f97da3a734ae19f821

Comment 6

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

Attachment: Bug 1517275 - Add a null check before trying to use a controller. r?botond

In test code (which is where this codepath is mostly exercised), the
controller should never be null here. However this codepath is sadly
also used in production code on Android, and there we might experience a
page navigation or a similarly destructive action while the flush is
inflight. That can result in a null pointer dereference.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e38c1fa062ad
Add a null check before trying to use a controller. r=botond

Comment 8

[Daniel Varga [:dvarga]]

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

Comment 9

[Ryan VanderMeulen [:RyanVM]]

Worth backporting to Beta?

Comment 10

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

Yeah it should apply easily enough. I'll fill out the form.

Comment 11

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

Comment on attachment 9034029 [details]
Bug 1517275 - Add a null check before trying to use a controller. r?botond

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: unknown

User impact if declined: Occasional crashes on Android (GeckoView). No clear STR

Is this code covered by automated tests?: Yes

Has the fix been verified in Nightly?: No

Needs manual test from QE?: No

If yes, steps to reproduce: 

List of other uplifts needed: None

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): Basically a null check

String changes made/needed: None

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9034029 [details]
Bug 1517275 - Add a null check before trying to use a controller. r?botond

[Triage Comment]
Adds a null check to fix a GeckoView crash. Approved for 65.0b9.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/662cf99f3994