Avoid duplicate display items when swapping doc shells

RESOLVED FIXED in Firefox 62

Status

()

P2
normal
RESOLVED FIXED
9 months ago
8 months ago

People

(Reporter: mattwoodrow, Assigned: mattwoodrow)

Tracking

(Blocks: 1 bug)

Trunk
mozilla62
Points:
---

Firefox Tracking Flags

(firefox62 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

9 months ago
This shows up when enabling RDL for the parent process. I am unsure if it can show up in the content, maybe an extension can trigger docshell swapping?

nsDisplaySubDocument's primary frame is the root frame for the subdocument, not the nsSubDocumentFrame.

If we swap the inner document into a different outer document, and then paint, we'll build a new display item for the inner document while the retained item for it in the other outer document still exists.

This hits our duplicate item assertions, not entirely sure if it causes real-world issues.

We can't easily delete the item itself from the list it's in, but we can delete all the inner items, and disown the item from the frame.
Comment hidden (mozreview-request)
(In reply to Matt Woodrow (:mattwoodrow) from comment #0)
> We can't easily delete the item itself from the list it's in, but we can
> delete all the inner items, and disown the item from the frame.

Instead of deleting the items ourselves, could we use nsDisplayItem::CanBeReused() functionality for this? Or simply mark the frame modified after docshell swap if we are expecting to rebuild it anyway?
Priority: -- → P2
(Assignee)

Comment 3

9 months ago
(In reply to Miko Mynttinen [:miko] from comment #2)
> (In reply to Matt Woodrow (:mattwoodrow) from comment #0)
> > We can't easily delete the item itself from the list it's in, but we can
> > delete all the inner items, and disown the item from the frame.
> 
> Instead of deleting the items ourselves, could we use
> nsDisplayItem::CanBeReused() functionality for this? Or simply mark the
> frame modified after docshell swap if we are expecting to rebuild it anyway?

We are already marking it as modified, and this patch to clear the frame pointers makes double sure that merging won't reuse it.

This patch mainly fixes a false-positive with our debug assertions. The display item still exists in the display list for the old document, and we can build a new display list for the new document and build a duplicate item that one. We end up a with a temporary state where there's two identical nsDisplaySubDocuments, one in each display list. That gets cleaned up when we paint the old document again, but the temporary state triggers our assertions and makes lots of tests fail.
(In reply to Matt Woodrow (:mattwoodrow) from comment #3)
> (In reply to Miko Mynttinen [:miko] from comment #2)
> > (In reply to Matt Woodrow (:mattwoodrow) from comment #0)
> > > We can't easily delete the item itself from the list it's in, but we can
> > > delete all the inner items, and disown the item from the frame.
> > 
> > Instead of deleting the items ourselves, could we use
> > nsDisplayItem::CanBeReused() functionality for this? Or simply mark the
> > frame modified after docshell swap if we are expecting to rebuild it anyway?
> 
> We are already marking it as modified, and this patch to clear the frame
> pointers makes double sure that merging won't reuse it.
> 
> This patch mainly fixes a false-positive with our debug assertions. The
> display item still exists in the display list for the old document, and we
> can build a new display list for the new document and build a duplicate item
> that one. We end up a with a temporary state where there's two identical
> nsDisplaySubDocuments, one in each display list. That gets cleaned up when
> we paint the old document again, but the temporary state triggers our
> assertions and makes lots of tests fail.

This makes sense, thank you for the more detailed explanation.

Comment 5

9 months ago
mozreview-review
Comment on attachment 8982404 [details]
Bug 1466024 - Cleanup nsDisplaySubDocument display items when we swap the subdoc into a different outer document.

https://reviewboard.mozilla.org/r/248352/#review255794

LGTM.
Attachment #8982404 - Flags: review?(mikokm) → review+
Is this ready to land?
Flags: needinfo?(matt.woodrow)

Comment 7

8 months ago
Pushed by mwoodrow@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4432fb67de2d
Cleanup nsDisplaySubDocument display items when we swap the subdoc into a different outer document. r=miko
(Assignee)

Updated

8 months ago
Flags: needinfo?(matt.woodrow)

Comment 8

8 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/4432fb67de2d
Status: NEW → RESOLVED
Last Resolved: 8 months ago
status-firefox62: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
You need to log in before you can comment on or make changes to this bug.