1700245 - Use nsLayoutUtils::IsAncestorFrameCrossDocInProcess instead of nsLayoutUtils::IsAncestorFrameCrossDoc

Status: RESOLVED FIXED

(Core :: Layout, task)

Description

[Hiroyuki Ikezoe (:hiro)]

nsLayoutUtils::IsAncestorFrameCrossDoc is using GetCrossDocParentFrame which doesn't work across process boundaries. In bug 1523500 I am going to introduce a new similar function named IsAncestorFrameCrossDocInProcess which is using GetCrossDocParentFrameInProcess instead.

All call sites of IsAncestorFrameCrossDoc seems to have been audited in bug 1523439, so hopefully they can be simply replaced by the new IsAncestorFrameCrossDocInProcess function.

Comment 1

[Daniel Holbert [:dholbert]]

hiro, hope you don't mind if I assign this to you, to get it out of the unassigned-fission-bugs queue.

(It sounds like this is just a simple string-replacement and there's no more auditing/investigation that really needs to be done here -- is that right? If so, I'm hoping it doesn't add too much to have this on your queue.)

Comment 2

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1700245 - Replace IsAncestorFrameCrossDoc with IsAncestorFrameCrossDocInProcess in DOMIntersectionObserver::Update. r?emilio

Our IntersectionObserver has been working in OOP iframe since bug 1599795.

Comment 3

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1700245 - Replace IsAncestorFrameCrossDoc with IsAncestorFrameCrossDocInProcess for assertions in AGR related stuff. r?mattwoodrow

As we audited in bug 1523500, the root scrollable frame in the OOP iframe is
always active, so using IsAncestorFrameCrossDocInProcess would be sufficient
for the assertions.

Depends on D117500

Comment 4

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1700245 - Replace IsAncestorFrameCrossDoc with IsAncestorFrameCrossDocInProcess in TransformFrameRectToAncestor. r?emilio

And add a note to clarify IsAncestorFrameCrossDocInProcess doesn't work
across different process documents.

Depends on D117501

Comment 5

[Hiroyuki Ikezoe (:hiro)]

I've replaced some of the call sites of IsAncestorFrameCrossDocInProcess, but I haven't some of them. Reasonings are below;

• In DOMIntersectionObserver::Update handled in D117500

• In GetClosest, deferred to bug 1715932, it's mobile specific.

• In PresShell::ClearMouseCapture, this was audited in bug 1533499, but it's quite unclear to me why it's okay to replace with the in-process version of IsAncestorFrameCrossDoc (there is no reasoning description in the bug)

• In nsLayoutUtils::IsProperAncestorFrameCrossDoc, there are two call sites of this functionl

  • In GetClosest will be also handled in bug 1715932.
  • In PruneDisplayListForExtraPage which was introduced in bug 792314, it was for display-list-
    based-invalidation (DLBI) stuff on print preview, it looks okay to me since print preview basically isn't partially updated, it's fully refreshed when print settings are changed or some such, but I am not 100% sure

• In nsLayoutUtils::TransformFrameRectToAncestor for an assertion which was introduced in bug 1668156, it's supposed to work in the same process, handled in D117502

• Remaining call sites are for assertions in AGR related stuff(A search query of IsAncestorFrameCrossDoc calls in layout/paint as of 20210611) handled in D117501

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/36115f44f68e
Replace IsAncestorFrameCrossDoc with IsAncestorFrameCrossDocInProcess in DOMIntersectionObserver::Update. r=emilio
https://hg.mozilla.org/integration/autoland/rev/26a3a728fa3d
Replace IsAncestorFrameCrossDoc with IsAncestorFrameCrossDocInProcess for assertions in AGR related stuff. r=mattwoodrow
https://hg.mozilla.org/integration/autoland/rev/ccdda6174617
Replace IsAncestorFrameCrossDoc with IsAncestorFrameCrossDocInProcess in TransformFrameRectToAncestor. r=emilio

Comment 7

[Marian-Vasile Laza]

https://hg.mozilla.org/mozilla-central/rev/36115f44f68e
https://hg.mozilla.org/mozilla-central/rev/26a3a728fa3d
https://hg.mozilla.org/mozilla-central/rev/ccdda6174617

Comment 8

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1700245 - Split PresShell::ClearMouseCapture into two functions, one takes `nsIFrame`, the other takes no argument. r?tnikkel

Depends on D119067

Comment 9

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1700245 - Use IsAncestorFrameCrossDocInProcess in PresShell::ClearMouseCapture(nsIFrame*). r?tnikkel

Since bug 1717726 we've disallowed having remote <browser> or remote <iframe>
inside nsDeckFrame which means captureingFrame in ClearMouseCapture(nsIFrame*)
function will never be called in an out-of-process thus we can use
IsAncestorFrameCrossDocInProcess there.

Also add an assertions that the function gets called only with a child frame of
nsDeckFrame since this function is used only inside nsDeckFrame.

Depends on D119068

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/83ece80a31b1
Split PresShell::ClearMouseCapture into two functions, one takes `nsIFrame`, the other takes no argument. r=tnikkel
https://hg.mozilla.org/integration/autoland/rev/0fcde78b9932
Use IsAncestorFrameCrossDocInProcess in PresShell::ClearMouseCapture(nsIFrame*). r=tnikkel

Comment 11

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/83ece80a31b1
https://hg.mozilla.org/mozilla-central/rev/0fcde78b9932

Comment 12

[Hiroyuki Ikezoe (:hiro)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #5)

• In PruneDisplayListForExtraPage which was introduced in bug 792314, it was for display-list-
  based-invalidation (DLBI) stuff on print preview, it looks okay to me since print preview basically isn't partially updated, it's fully refreshed when print settings are changed or some such, but I am not 100% sure

This was wrong. PruneDisplayListForExtraPage was introduced in bug 411585.

Comment 13

[Hiroyuki Ikezoe (:hiro)]

The last remaining part of this bug is IsProperAncestorFrameCrossDoc call in PruneDisplayListForExtraPage. Now I am ~100% sure since our printing machinery for cross process iframes are totally different and it's been working (since bug 1662336 I presume).

PruneDisplayListForExtraPage is for cases where placeholders for out-of-flow frames are inside a page frame but the real frame we need to paint is in a different page such as;

<div style="height:800px;"></div>                                               
<div style="position:absolute; top:0; left:0;">This text should be visible in print preview</div>

This is actually a test case introduced along with PruneDisplayListForExtraPage.

The PruneDisplayListForExtraPage seems to try to eliminate unnecessary display items for the given page frame if there's any out-of-flow frames as a comment described. We don't need to be worried in OOP iframe cases since we will never have display items for OOP iframe's document elements. (Though I think PruneDisplayListForExtraPage should work for cases where there's no out-of-flow contents, such as a tall div element is fragmented into two pages or some such).

Anyway, I am going to add a couple of test cases similar to 411585-1.html but oop iframe's version in this bug. To make the iframe's test work correctly we need bug 1695806.

Comment 14

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1700245 - Add two wpt tests where the placeholder of abs-pos cross-origin iframe is in the second page but the real position is in the first page. r?mattwoodrow

These tests are variants of layout/reftests/bugs/411585-1.html but with
cross-origin iframes.

position-absolute-iframe-print-001.sub.html is a simple case like this;
<div style="height:200vh;"></div>
<iframe style="position:absolute; top: 0; left: 0;"></iframe>

position-absolute-iframe-print-002.sub.html is a nested version something like;
document A1 -> domain B -> domain A2, A1 and A2 is the same domain.

Comment 15

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1700245 - Drop |aExtraPage| argument from PruneDisplayListForExtraPage. r?mattwoodrow

It's not used at all.

Depends on D120426

Comment 16

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1700245 - Introduce IsProperAncestorFrameCrossDocInProcess and use it in PruneDisplayListForExtraPage. r?mattwoodrow

Depends on D120427

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bf8d248a505d
Add two wpt tests where the placeholder of abs-pos cross-origin iframe is in the second page but the real position is in the first page. r=mattwoodrow
https://hg.mozilla.org/integration/autoland/rev/dd0f8b4683e6
Drop |aExtraPage| argument from PruneDisplayListForExtraPage. r=mattwoodrow
https://hg.mozilla.org/integration/autoland/rev/089c3e31a467
Introduce IsProperAncestorFrameCrossDocInProcess and use it in PruneDisplayListForExtraPage. r=mattwoodrow

Comment 18

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/29800 for changes under testing/web-platform/tests

Comment 19

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/bf8d248a505d
https://hg.mozilla.org/mozilla-central/rev/dd0f8b4683e6
https://hg.mozilla.org/mozilla-central/rev/089c3e31a467

Comment 20

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged by moz-wptsync-bot