Closed Bug 1565922 Opened 5 years ago Closed 5 years ago

Crash in [@ RetainedDisplayListBuilder::IncrementSubDocPresShellPaintCount]

Categories

(Core :: Print Preview, defect)

69 Branch
Desktop
Windows 10
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla70
Tracking Status
firefox-esr60 --- unaffected
firefox-esr68 --- unaffected
firefox68 --- unaffected
firefox69 --- verified
firefox70 --- verified

People

(Reporter: alice0775, Assigned: tnikkel)

References

(Regression)

Details

(Keywords: crash, regression, reproducible)

Crash Data

Attachments

(3 files)

This bug is for crash report bp-9602e09c-1602-48c4-8b06-b60e40190714.

Top 10 frames of crashing thread:

0 xul.dll void RetainedDisplayListBuilder::IncrementSubDocPresShellPaintCount layout/painting/RetainedDisplayListBuilder.cpp:321
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 RetainedDisplayListBuilder::AttemptPartialUpdate layout/painting/RetainedDisplayListBuilder.cpp:1450
4 xul.dll nsLayoutUtils::PaintFrame layout/base/nsLayoutUtils.cpp:3984
5 xul.dll mozilla::PresShell::Paint layout/base/PresShell.cpp:6155
6 xul.dll void nsViewManager::ProcessPendingUpdatesPaint view/nsViewManager.cpp:461
7 xul.dll nsViewManager::ProcessPendingUpdatesForView view/nsViewManager.cpp:396
8 xul.dll nsViewManager::ProcessPendingUpdates view/nsViewManager.cpp:1019
9 xul.dll void nsRefreshDriver::Tick layout/base/nsRefreshDriver.cpp:2104

Reproducible: 100%

Steps To Reproduce:

  1. Menu > Options... > Search
  2. Menu > Print... if not crash then repeat step2

Actual results:
Browser crashes

Has STR: --- → yes

:alice0775, could you try to find a regression range in using for example mozregression?

Regression window:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=0ca9a03071b76cb0aca81b83ed451dedd2b7c1f5&tochange=87409f291fa6a003f00ae782528bcd4734eeaee0

Regressed by: 87409f291fa6a003f00ae782528bcd4734eeaee0 Miko Mynttinen — Bug 1413546 - Enable retained display lists for parent process r=mattwoodrow

Regressed by: 1413546
Attached file about:support (Nightly69.0a1) (deleted) —

Clearing the keyword section based on comment 2.

Adding a ni on :miko based on the regression range.

Flags: needinfo?(mikokm)

I do intend to look at this soon.

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

I do intend to look at this soon.

Thank you Timothy, let me know if you do not have time for it.

Flags: needinfo?(mikokm)

We paint and create an nsDisplaySubDocument, the subdoc doesn't have a root frame at this time, so the nsDisplaySubDocument only has a reference to the subdocument frame (in the parent document), not anything in the child document. The subdoc is about:blank at this point. The document loads, gets put in print preview mode. We try to clone the document to print it, this fails because xul documents can't be cloned. This bubbles up to https://searchfox.org/mozilla-central/rev/da855d65d1fbdd714190cab2c46130f7422f3699/layout/printing/nsPrintJob.cpp#712 where we turn off print preview because of the failure. This calls nsDocumentViewer::SetIsPrintPreview(false) and we clear the mPresContext pointer on the nsDocumentViewer here https://searchfox.org/mozilla-central/rev/da855d65d1fbdd714190cab2c46130f7422f3699/layout/base/nsDocumentViewer.cpp#4036 putting the document viewer into a kind of dead state. None of this would effect the nsDisplaySubDocument from the perspective of retained display lists because it is for a frame in the parent document I guess.

So either we can insert some invalidation/marking modified somewhere in that sequence of events or just allow nsDisplaySubDocument to not have a subdocument presshell.

These nsDisplaySubDocument's that don't have a reference to the root frame of the subdocument seem like trouble. Not relevant to this bug, but what would mark them modified when the subdoc gets a root frame? I guess this works today because the root frame creation would cause the rebuild region to intersect the nsSubdocumentFrame area and so we rebuild it. Is it possible we'd end up reusing the old item in that case? It seems like it? If that is the case we should mark the subdoc frame modified when we create a root frame.

Flags: needinfo?(matt.woodrow)

If the subdocument display item gets created when the subdocument doesn't have a root frame it'll be based on the subdocument frame in the parent document. This means that the prescontext pointer on the document viewer of the subdocument can go away without the subdocument display item getting any kind of invalidation. So we make sure the subdocument display item gets rebuilt if that happens. The reason the prescontext pointer is important is that is what nsSubDocumentFrame::GetSubdocumentPresShellForPainting uses (via nsDocShell::GetPresShell) to get the subdocument presshell. So it will fail to get a presshell and RetainedDisplayListBuilder::IncrementSubDocPresShellPaintCount will crash.

Pushed by tnikkel@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/bd07269f2969 When the document viewer loses it prescontext pointer we need to make sure that any potential subdocument display items get rebuilt. r=mattwoodrow
Flags: needinfo?(matt.woodrow)
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla70
Assignee: nobody → tnikkel

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

Flags: needinfo?(tnikkel)

Comment on attachment 9080563 [details]
Bug 1565922. When the document viewer loses it prescontext pointer we need to make sure that any potential subdocument display items get rebuilt. r?mattwoodrow

Beta/Release Uplift Approval Request

  • User impact if declined: crashes
  • Is this code covered by automated tests?: No
  • 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): Just invalidates a display item
  • String changes made/needed:
Flags: needinfo?(tnikkel)
Attachment #9080563 - Flags: approval-mozilla-beta?

Comment on attachment 9080563 [details]
Bug 1565922. When the document viewer loses it prescontext pointer we need to make sure that any potential subdocument display items get rebuilt. r?mattwoodrow

Fixes a crash. Approved for 69.0b10.

Attachment #9080563 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attached patch patch for beta (deleted) — Splinter Review
Flags: needinfo?(tnikkel) → needinfo?(aryx.bugmail)
Flags: qe-verify+
QA Whiteboard: [qa-triaged]

I have managed to reproduce this issue using Firefox 69.0b4 on Windows 10 64bit.

This issue is verified fixed using Firefox 70.0a1 (BuildId:20190801214227) and Firefox 69.0b10 (BuildId:20190731232009) on Windows 10 64bit and Ubuntu 18.04 64bit.

Status: RESOLVED → VERIFIED
Flags: qe-verify+
Has Regression Range: --- → yes
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: