1701027 - Using Eyedropper crash in [@ RetainedDisplayListBuilder::MergeDisplayLists]

Status: RESOLVED FIXED

(Core :: Web Painting, defect, P2)

Description

[Alice0775 White]

Crash report: https://crash-stats.mozilla.org/report/index/f5b2f9e7-1312-4c24-817c-3c0720210325

MOZ_CRASH Reason: Item found was in the wrong list! type 585 (outer type was 53 at depth 2, now is 2)

Top 10 frames of crashing thread:

0 xul.dll RetainedDisplayListBuilder::MergeDisplayLists layout/painting/RetainedDisplayListBuilder.cpp:801
1 xul.dll RetainedDisplayListBuilder::MergeDisplayLists layout/painting/RetainedDisplayListBuilder.cpp:812
2 xul.dll RetainedDisplayListBuilder::MergeDisplayLists layout/painting/RetainedDisplayListBuilder.cpp:812
3 xul.dll RetainedDisplayListBuilder::MergeDisplayLists layout/painting/RetainedDisplayListBuilder.cpp:812
4 xul.dll RetainedDisplayListBuilder::AttemptPartialUpdate layout/painting/RetainedDisplayListBuilder.cpp:1462
5 xul.dll static nsLayoutUtils::PaintFrame layout/base/nsLayoutUtils.cpp:3366
6 xul.dll mozilla::PresShell::Paint layout/base/PresShell.cpp:6391
7 xul.dll nsViewManager::ProcessPendingUpdatesPaint view/nsViewManager.cpp:459
8 xul.dll nsViewManager::ProcessPendingUpdatesForView view/nsViewManager.cpp:394
9 xul.dll nsViewManager::ProcessPendingUpdates view/nsViewManager.cpp:972

Steps to reproduce:

1. Open about:processes or about:performance
2. Web Developer > Eyedropper
3. Left mouse click on the list

Actual Results:
Nightly crash

Comment 1

[Alice0775 White]

Regression window;
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=a518405abe4c93e6d8df622f59d5f01c4766e1d5&tochange=a09e4ce9ebba8fa4a29a4833560c8ee807018c03

Comment 2

[Timothy Nikkel (:tnikkel)]

regression window with apz.allow_zooming turned on
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=7c2f0754a3118ee806ae019bce830012c53c7a47&tochange=3346f60d3fa72741bdde67feb985bdfb211134ad
-> bug 1644180

Comment 3

[Timothy Nikkel (:tnikkel)]

Probably mWillBuildScrollableLayer is changing in a way that nothing invalidates.

Comment 4

[Timothy Nikkel (:tnikkel)]

No, that's actually not the problem. We get a displayport and that invalidates. It's on the paint after we handle that invalidation that we hit the assert.

The eyedropper is implemented using top layer items. We wrap the top layer items in a display wrap list that uses the viewport frame as it's mFrame here

https://searchfox.org/mozilla-central/rev/bb37e6fe8bbe383a57a4ad21a201e26416613ca1/layout/generic/ViewportFrame.cpp#236

So when we invalidate the root scroll frame for getting a new display port we do a partial update. The wrap list item for the top layer items has a new display item parent, a display async zoom item (the creation of the display port changed this), so the a new display wrap list item is created. All of it's children are matched with the old children of the old display wrap list so those go away, but the old display wrap list item itself sticks around: it's mFrame is the viewport frame which was not marked modified. So the next paint we have these two identical display wrap list items (one empty), and we hit the assert.

So it seems like we need to mark the viewport frame modified if the root scroll frame is marked modified if we have top layer items (since those top layers items end up being inside the root scroll frames display list). This in turn would disable a partial update for the next paint because ShouldBuildPartial would return false because a viewport frame is modified

https://searchfox.org/mozilla-central/rev/bb37e6fe8bbe383a57a4ad21a201e26416613ca1/layout/painting/RetainedDisplayListBuilder.cpp#1309

I'm not sure this is even a bad thing, since we also disable partial updates if the canvas frame (child of the root scroll frame) is marked modified. Perhaps we would want to disable partial updates if the root scroll frame is marked modified, but we can't detect root scroll frames purely from their layout frame type.

Comment 5

[Timothy Nikkel (:tnikkel)]

Attachment: Bug 1701027. Disable partial display list updates when the root scroll frame is modified. r?mattwoodrow

This is basically equivalent to marking the viewport frame modified when the root scroll frame is marked modified.

We wrap the top layer items in a display wrap list that uses the viewport frame as it's mFrame here

https://searchfox.org/mozilla-central/rev/bb37e6fe8bbe383a57a4ad21a201e26416613ca1/layout/generic/ViewportFrame.cpp#236

So when we invalidate the root scroll frame for getting a new display port we do a partial update. The wrap list item for the top layer items has a new display item parent, a display async zoom item (the creation of the display port changed this), so the a new display wrap list item is created. All of it's children are matched with the old children of the old display wrap list so those go away, but the old display wrap list item itself sticks around: it's mFrame is the viewport frame which was not marked modified. So the next paint we have these two identical display wrap list items (one empty), and we hit the assert.

So it seems like we need to mark the viewport frame modified if the root scroll frame is marked modified if we have top layer items (since those top layers items end up being inside the root scroll frames display list). This in turn would disable a partial update for the next paint because ShouldBuildPartial would return false because a viewport frame is modified

https://searchfox.org/mozilla-central/rev/bb37e6fe8bbe383a57a4ad21a201e26416613ca1/layout/painting/RetainedDisplayListBuilder.cpp#1309

I'm not sure this is even a bad thing, since we also disable partial updates if the canvas frame (child of the root scroll frame) is marked modified. Perhaps we would want to disable partial updates if the root scroll frame is marked modified, but we can't detect root scroll frames purely from their layout frame type.

Comment 6

[BugBot [:suhaib / :marco/ :calixte]]

There's a r+ patch which didn't land and no activity in this bug for 2 weeks.
:tnikkel, could you have a look please?
For more information, please visit auto_nag documentation.

Comment 7

[Timothy Nikkel (:tnikkel)]

Need to write a test but I'm doing mr1 work.

Comment 8

[Julien Cristau [:jcristau]]

If we don't have a test yet can we file a follow-up for that and land the patch so we at least stop crashing?

Comment 10

[Timothy Nikkel (:tnikkel)]

Attachment: Bug 1701027. Add test. r?hiro

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8bc7d0c76297
Disable partial display list updates when the root scroll frame is modified.  r=mattwoodrow
https://hg.mozilla.org/integration/autoland/rev/9b2eab0bda3d
Add test. r=hiro

Comment 12

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/8bc7d0c76297
https://hg.mozilla.org/mozilla-central/rev/9b2eab0bda3d

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Please nominate this for ESR91 approval when you get a chance.

Comment 14

[Timothy Nikkel (:tnikkel)]

Comment on attachment 9211685 [details]
Bug 1701027. Disable partial display list updates when the root scroll frame is modified. r?mattwoodrow

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: easily reproduced crash with simple fix
• User impact if declined: crash in certain cases
• Fix Landed on Version: 92
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): adds a simple case to skip trying to do a partial display list update
• String or UUID changes made by this patch:

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9211685 [details]
Bug 1701027. Disable partial display list updates when the root scroll frame is modified. r?mattwoodrow

Approved for 91.1esr.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr91/rev/8a3d46751544
https://hg.mozilla.org/releases/mozilla-esr91/rev/dcf4447a3313