1561104 - Crash in [@ RetainedDisplayListBuilder::IncrementSubDocPresShellPaintCount]

Status: RESOLVED FIXED

(Core :: Web Painting, defect, P3)

Description

[Marcia Knous [:marcia]]

This bug is for crash report bp-3940c1d9-7423-4fd5-9906-c52f90190620.

Seen while looking at nightly crash stats: https://bit.ly/2XqMUle. Nightly crash started in 20190620094631. There are also a few crashes on 68. There seem to be crashes on release, but volume is higher on nightly.

Top 10 frames of crashing thread:

0 xul.dll void RetainedDisplayListBuilder::IncrementSubDocPresShellPaintCount layout/painting/RetainedDisplayListBuilder.cpp
1 xul.dll bool RetainedDisplayListBuilder::PreProcessDisplayList layout/painting/RetainedDisplayListBuilder.cpp:294
2 xul.dll bool RetainedDisplayListBuilder::PreProcessDisplayList layout/painting/RetainedDisplayListBuilder.cpp:259
3 xul.dll bool RetainedDisplayListBuilder::PreProcessDisplayList layout/painting/RetainedDisplayListBuilder.cpp:259
4 xul.dll RetainedDisplayListBuilder::AttemptPartialUpdate layout/painting/RetainedDisplayListBuilder.cpp:1449
5 xul.dll nsLayoutUtils::PaintFrame layout/base/nsLayoutUtils.cpp:3966
6 xul.dll mozilla::PresShell::Paint layout/base/PresShell.cpp:6149
7 xul.dll nsViewManager::ProcessPendingUpdatesPaint view/nsViewManager.cpp:461
8 xul.dll nsViewManager::ProcessPendingUpdatesForView view/nsViewManager.cpp:396
9 xul.dll nsViewManager::ProcessPendingUpdates view/nsViewManager.cpp:1019

Comment 1

[Pascal Chevrel:pascalc]

Only a handful of crashes on 67/68, so wontfix for those. The fact that we are recently crashing more on Nightly with this signature hints at a recent regression indeed.

Comment 2

[Vlad Lucaci, Desktop QA (:vlucaci)]

https://crash-stats.mozilla.org/report/index/053adce7-e0c4-4dfc-a484-c367f0190625#tab-details

We also experienced a tab crash with the same signature while browsing random videos on youtube and scrolling on Reddit on Windows 10x64 with the latest FF Nightly. (2019-06-27)

Crash for youtube occurred when watching a longer video all while fission was active.

Comment 3

[Matt Woodrow (:mattwoodrow)]

Timothy, do you have any time to look at this?

It might be related to fission.

I had a look at a crash dump and it appears that 'presShell' in IncrementSubDocPresShellPaintCount is nullptr, so I guess something changed in the subdoc frame such that we no longer had a subdoc pres shell and nothing invalidated.

We probably should at least not crash in this situation, but ideally we'd figure out the actual cause and make sure we invalidate correctly.

Comment 4

[Timothy Nikkel (:tnikkel)]

I think it's definitely possible to get a nsDisplaySubdocument for which it's mSubDoc frames returns null from GetSubdocumentPresShellForPainting (and hence get a null presshell in IncrementSubDocPresShellPaintCount). In nsSubDocumentFrame::BuildDisplayList we build a nsDisplaySubdocument that doesn't want to be flattened for all root content documents, even if the subdoc doesn't have a root frame (which happens briefly early in the creation of the layout structures for a document). GetSubdocumentPresShellForPainting could definitely return null if the subdoc doesn't have a root frame.

(We can get a presshell via a view now, which was not true when GetSubdocumentPresShellForPainting was written. So we could change GetSubdocumentPresShellForPainting to get a presshell in the case of no subdoc root frame, but that means changing the main painting path too, and I think I would like to avoid that: painting a subdoc without a root frame is useless anyways and the change might cause other problems.)

So I think just null checking this for now is the right move, unless/until we get more evidence that there is an actual bug here other than a missing null check at this site.

Comment 5

[Timothy Nikkel (:tnikkel)]

(In reply to Timothy Nikkel (:tnikkel) from comment #4)

GetSubdocumentPresShellForPainting could definitely return null if the
subdoc doesn't have a root frame.

Ah, yes, but we probably don't very often or at all because we fall back to asking the frame loader (for the docshell and then presshell) in GetSubdocumentPresShellForPainting. Back to the drawing board I guess.

Comment 6

[Timothy Nikkel (:tnikkel)]

Next theory: mIsRemoteFrame changes on the frame loader after creating an nsDisplaySubdocument. This causes GetSubdocumentPresShellForPainting to return null because no subdocu root frame and the frame loader doesn't have a docshell to return for the sub doc when it is a remote frame.

The only possible way I could see this happening is in nsGenericHTMLFrameElement::CreateRemoteFrameLoader

https://searchfox.org/mozilla-central/rev/40ef22080910c2e2c27d9e2120642376b1d8b8b2/dom/html/nsGenericHTMLFrameElement.cpp#157

We create a frame loader then call InitializeFromBrowserParent which sets mIsRemoteFrame to true. Then the comment there indicates that reflow could have happened already (not sure where) so painting could probably have happened to maybe. If reflow/painting happened before the InitializeFromBrowserParent call we could hit my theorhetical situation.

Comment 7

[Timothy Nikkel (:tnikkel)]

Bug 1565922 has reproducible steps for a crash with this signature.

Comment 8

[Timothy Nikkel (:tnikkel)]

Patch landed in bug 1565922, so hopefully this crash goes away.

Comment 9

[Liz Henry (:lizzard) (relman/hg->git project)]

Happy to take a patch for 70 or beyond.
Since we are getting close to the end of the 69 beta cycle and this is set to P3, I'm marking it fix-optional for 69 and 70 to remove it from weekly triage.

Comment 10

[Timothy Nikkel (:tnikkel)]

(In reply to Timothy Nikkel (:tnikkel) from comment #8)

Patch landed in bug 1565922, so hopefully this crash goes away.

Since then there has only been 1 crash in a build with bug 1565922 fixed. So volume was decreased to almost nothing.

Comment 11

[Matt Woodrow (:mattwoodrow)]

Looks this this is fixed now, due to bug 1565922.