1548483 - Crash in [@ mozilla::ScrollFrameHelper::DecideScrollableLayer]

Status: RESOLVED FIXED

(Core :: Web Painting, defect, P2)

Description

[Calixte Denizet (:calixte)]

This bug is for crash report bp-7f58db65-6ffe-471a-a862-e3eb00190502.

Top 10 frames of crashing thread:

0 xul.dll mozilla::ScrollFrameHelper::DecideScrollableLayer layout/generic/nsGfxScrollFrame.cpp:3934
1 xul.dll mozilla::ScrollFrameHelper::BuildDisplayList layout/generic/nsGfxScrollFrame.cpp:3307
2 xul.dll nsIFrame::BuildDisplayListForChild layout/generic/nsFrame.cpp:3903
3 xul.dll static void DisplayLine layout/generic/nsBlockFrame.cpp:6420
4 xul.dll nsBlockFrame::BuildDisplayList layout/generic/nsBlockFrame.cpp:6531
5 xul.dll nsIFrame::BuildDisplayListForChild layout/generic/nsFrame.cpp:3931
6 xul.dll static void DisplayLine layout/generic/nsBlockFrame.cpp:6420
7 xul.dll nsBlockFrame::BuildDisplayList layout/generic/nsBlockFrame.cpp:6531
8 xul.dll nsIFrame::BuildDisplayListForChild layout/generic/nsFrame.cpp:3931
9 xul.dll static void DisplayLine layout/generic/nsBlockFrame.cpp:6420

There are 3 crashes (from 3 installations) in nightly 68 with buildid 20190501215533. In analyzing the backtrace, the regression may have been introduced by patch [1] to fix bug 1535945.

[1] https://hg.mozilla.org/mozilla-central/rev?node=0e259884f052

Comment 1

[Julian Seward [:jseward]]

This crash appears 41 times, in perhaps 15 ish different installations, in the
Windows nightlies 20190502220333 and 20190502095227.

Comment 2

[Matt Woodrow (:mattwoodrow)]

It looks like we get a displayport added to a sub-scrollframe via MaybeCreateDisplayPortInFirstScrollFrameEncountered, but we don't paint that frame because it's offscreen.

Then on a latter paint, a descendant of the scrollframe becomes visible, making us do display list building for the scrollframe (it itself is still offscreen and unmodified, but its overflow area is visible).

On that paint, we detect that the displayport state has changed, but the frame isn't marked as invalid (it was on the paint where it first changed though), and we assert.

This should be fine, since the assert is trying to catch the case where we retain items even though we've changed. This case there are no items, and when it becomes visible we create new ones as needed.

The assert is getting overly complex though, so I'm just going to drop it.

Comment 3

[Matt Woodrow (:mattwoodrow)]

Attachment: Bug 1548483 - Don't assert that we have an invalidated frame when we encouter a new displayport, since it can have changed on an earlier paint if we didn't have display items. r?tnikkel

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ea3a73c7788e
Don't assert that we have an invalidated frame when we encouter a new displayport, since it can have changed on an earlier paint if we didn't have display items. r=tnikkel

Comment 5

[Stefan Hindli [:stefan_hindli]]

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