1471671 - Crash in mozilla::layers::ClipManager::DefineScrollLayers

Status: VERIFIED FIXED

(Core :: Graphics: WebRender, defect, P2)

Description

[Darkspirit]

Seen on Socorro. Linux and Mac so far.

bp-9ec7baa6-214b-4eb2-89b2-271460180622 build 20180622100109 Linux
> MOZ_RELEASE_ASSERT(mIsSome)

Comment 1

[Darkspirit]

>  0 	libxul.so 	mozilla::layers::ClipManager::DefineScrollLayers(mozilla::ActiveScrolledRoot const*, nsDisplayItem*, mozilla::layers::StackingContextHelper const&) 	mfbt/Maybe.h:549

Is this something like bug 1467999?

Comment 2

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

Maybe. It's odd that there's only 7 crashes, and it looks like 6 of them are from a single user. From the crash stack it's not clear which Maybe<> object is being improperly dereferenced, but I think [1] is most likely. And that might be the result of the user having flipped some random prefs and triggering a weird (unsupported) codepath.

Let's wait and see if this keeps happening. I can upgrade the MOZ_ASSERT just above [1] to a MOZ_RELEASE_ASSERT to make it crash earlier and confirm my theory, if needed.

[1] https://searchfox.org/mozilla-central/rev/14cb8f1c238735ba1abe18ad44e39808983c2572/gfx/layers/wr/ClipManager.cpp#268

Comment 3

[Darkspirit]

30 crashes on 3 Windows installations.

Comment 4

[Maire Reavy [:mreavy]]

We can't release this to the field, but we can let this ride to beta.

Comment 5

[Andrew McCreight [:mccr8]]

#2 non-hang crash on the 9-19 Linux Nightlies, with 6 crashes.

Comment 6

[Darkspirit]

The target.apk of "WebRender on GeckoView on October 17"[1] is crashing on my Moto G5:
bp-d451985d-06f7-4965-814c-d38bd0181017

[1] https://treeherder.mozilla.org/#/jobs?repo=try&group_state=expanded&selectedJob=206132017&revision=53892fbb31ba1fff82fb70a62b0725dd6ee30ca8

Comment 7

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

Yeah, target.apk is the Firefox for Android build, which is still expected to crash (and probably won't ever be fixed). Try using the geckoview_example.apk instead.

Comment 8

[Andrew Osmond [:aosmond] (he/him)]

24 reports on beta already with the brief time it has been out, and the highest volume WR one (AFAIK). Probably should stamp this out sooner rather than later.

Comment 9

[Andrew Osmond [:aosmond] (he/him)]

I believe the actual line given by the crash report is misleading. The only place (that I have found thus far) where we deref a Maybe without checking first is:

https://searchfox.org/mozilla-central/rev/a7f4d3ba4fbfe3efbde832869f1d672fce7122f6/gfx/layers/wr/ClipManager.cpp#295

Which is calculated by:

https://searchfox.org/mozilla-central/rev/a7f4d3ba4fbfe3efbde832869f1d672fce7122f6/layout/generic/nsGfxScrollFrame.cpp#3959

Which can generate Nothing, if our assumptions are failing to hold.

Comment 10

[Andrew Osmond [:aosmond] (he/him)]

Whoops I should have read comment 2 :).

Comment 11

[Andrew Osmond [:aosmond] (he/him)]

Bug 1453367 comment 2 (through to 4 and beyond) seem relevant here.

Comment 12

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

I think the crash volume is too high to be a result of people flipping that containerful scrollframe pref. But it probably makes sense to eliminate as a possibility, which we can do by adding a MOZ_RELEASE_ASSERT(!gfxPrefs::LayoutUseContainersForRootFrames()) somewhere in the ClipManager constructor or similar.

Comment 13

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

Also if it were due to that the crashes would all be startup crashes, I think. I haven't checked if that's the case.

Comment 14

[Andrew Osmond [:aosmond] (he/him)]

The other path this could happen is if mWillBuildScrollableLayer is false in ComputeScrollMetadata. That seems to boil down to the result from GetDisplayPort(Impl).

https://searchfox.org/mozilla-central/rev/a7f4d3ba4fbfe3efbde832869f1d672fce7122f6/layout/base/nsLayoutUtils.cpp#1016

Could we just not have the display port data setup?

Comment 15

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

I don't know of a scenario in which that would happen, but I suppose it must be happening. The code that does all this is super hairy and complex and I haven't looked at it for over a year so I wouldn't be surprised if stuff is broken in some cases.

If we want to get rid of the crash and replace it with possibly-incorrect behaviour I think we should detect when ComputeScrollMetadata returns Nothing() and treat it the same as we do the !IsScrollable() case a few lines down - i.e. just return the ancestorScrollId (but also add a MOZ_ASSERT(false) there to catch the case in local debug builds). At least then we might get people filing bugs about scrolling not working properly in some cases and we might get reproducible STR.

Comment 16

[Andrew Osmond [:aosmond] (he/him)]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #15)
> I don't know of a scenario in which that would happen, but I suppose it must
> be happening. The code that does all this is super hairy and complex and I
> haven't looked at it for over a year so I wouldn't be surprised if stuff is
> broken in some cases.
> 
> If we want to get rid of the crash and replace it with possibly-incorrect
> behaviour I think we should detect when ComputeScrollMetadata returns
> Nothing() and treat it the same as we do the !IsScrollable() case a few
> lines down - i.e. just return the ancestorScrollId (but also add a
> MOZ_ASSERT(false) there to catch the case in local debug builds). At least
> then we might get people filing bugs about scrolling not working properly in
> some cases and we might get reproducible STR.

Sounds good to me. I'll put up a patch. I reviewed the URL data with the crash reports, and there doesn't seem to be any standouts, except maybe YouTube in general, but I couldn't reproduce the crashes on any of the top links for Windows or Linux.

Looking at the non-WR code, it looks like it will just skip handling the ASR without the metadata:

https://searchfox.org/mozilla-central/rev/72b1e834f384a2ffec6eb4ce405fbd4b5e881109/layout/painting/FrameLayerBuilder.cpp#6169-6171

So it is possible returning the ancestor is the right thing to do anyways? :)

Comment 17

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Bug 1471671 - Avoid crash with WebRender when the scroll metadata is unavailable.

We are seeing crash reports in the wild where there is no scroll
metadata available for an ASR for a display item and its clip. It
appears that in the non-WR path, it skips such items, so we should
probably do the same thing with WebRender. If the scrolling ends up
being wrong, hopefully a reproducible use case will make its way to use
to further debug, as the crash reports have not yielded anything to date.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6d897bc50f75
Avoid crash with WebRender when the scroll metadata is unavailable. r=kats

Comment 19

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/6d897bc50f75

Comment 20

[Ryan VanderMeulen [:RyanVM]]

No crashes on Nightly since this landed \m/. Please nominate this for Beta uplift when you get a chance.

Comment 21

[Andrew Osmond [:aosmond] (he/him)]

It morphed into bug 1502454, so I would like to request uplift of them together, once it has landed and been verified.

Comment 22

[Andrew Osmond [:aosmond] (he/him)]

Comment on attachment 9019813 [details]
Bug 1471671 - Avoid crash with WebRender when the scroll metadata is unavailable.

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: None

User impact if declined: May occasionally crash the content process if using WebRender.

Is this code covered by automated tests?: Yes

Has the fix been verified in Nightly?: Yes

Needs manual test from QE?: No

If yes, steps to reproduce: 

List of other uplifts needed: Bug 1471671

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): We were crashing before, now the theoretical worst case would be the scrolling on the page is flaky. Only affects WebRender.

String changes made/needed: N/A

Comment 23

[Julien Cristau [:jcristau]]

Comment on attachment 9019813 [details]
Bug 1471671 - Avoid crash with WebRender when the scroll metadata is unavailable.

webrender crash fix, approved for 64.0b8

Comment 24

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/releases/mozilla-beta/rev/d29760cf44dd

Comment 25

[Cornel Ionce [:noni] [Hubs QA]]

(In reply to Andrew Osmond [:aosmond] from comment #22)
> Is this code covered by automated tests?: Yes
> 
> Needs manual test from QE?: No

Marking as qe-verify- per comment 22.