1466024 - Avoid duplicate display items when swapping doc shells

Status: RESOLVED FIXED

(Core :: Web Painting, enhancement, P2)

Description

[Matt Woodrow (:mattwoodrow)]

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 1

[Matt Woodrow (:mattwoodrow)]

Attachment: Bug 1466024 - Cleanup nsDisplaySubDocument display items when we swap the subdoc into a different outer document.

Review commit: https://reviewboard.mozilla.org/r/248352/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/248352/

Comment 2

[Miko Mynttinen]

(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?

Comment 3

[Matt Woodrow (:mattwoodrow)]

(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.

Comment 4

[Miko Mynttinen]

(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

[Miko Mynttinen]

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.

Comment 6

[Ryan VanderMeulen [:RyanVM]]

Is this ready to land?

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4432fb67de2d
Cleanup nsDisplaySubDocument display items when we swap the subdoc into a different outer document. r=miko

Comment 8

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/4432fb67de2d