1727674 - Make nsIFrame::UpdateVisibilitySynchronously consistent with/without Fission

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Hiroyuki Ikezoe (:hiro)]

Attachment: Add a mochitest fails without Fission suceeds with Fission

nsIFrame::UpdateVisibilitySynchronously has an nsLayoutUtils::GetCrossDocParentFrame call, if it gets called in OOP iframes it will return a wrong value.

Attaching file is a changeset to add a mochitest that you can see different results with/without Fission.

When the test run --enable-fission, an image in the test which is scolled out a bit is not decoded, but it's decoded without Fission.

I am not yet confident that the inconsistency of UpdateVisibilitySynchronously is the cause of the difference, there may be other issues to cause the difference (I am suspecting this may be somewhat related to displayport size Timothy talked about in this week's meeting).

Comment 1

[Chris Peterson [:cpeterson]]

Tentatively tracking for Fission MVP.

Comment 2

[Hiroyuki Ikezoe (:hiro)]

Attachment: A mochitest

I've written a mochitest which fails with Fission and succeeds without Fission. That means, the test properly covers nsIFrame::UpdateVisibilitySynchronously. That said, the test doesn't pass with a fix for the UpdateVisibilitySynchronously inconsistency.

There's another problem with Fission in PresShell::RebuildApproximateFrameVisibility. In PresShell::RebuildApproximateFrameVisibility, we do get the presshell's visible rect and call MarkFramesInSubtreeApproximatelyVisible with the rect, but unfortunately the visible rect doesn't consider ancestor frames at all.

Comment 3

[Hiroyuki Ikezoe (:hiro)]

It turns out the mochitest in comment 2 is not appropriate for this issue, the mochtiest did just work as expected, I mean it failed due to a race condition in between "setting display:none to <img> in iframe" and "scrolling an ancestor scroller including the iframe". And in fact in the mochitest case, the pres shell for the iframe isn't active since it's invisible, so the image will never be decoded because of PresShell::IsActive checks here and here. I need to figure out a different way to test it.

Comment 4

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1727674 - Track frame visibility in OOP iframes. r?tnikkel

One caveat is that this change doesn't mean we handle frame visibility in
iframes in a same manner regardless whether the iframe is out-of-process or not.
In fact, we handle OOP iframe cases in a slightly different way since in OOP
iframes if the iframe is invisible because it's scrolled out or some such, then
we can't tell how far the iframe is from the visible scroll port. For example;

<div style="height: 100px; overflow: scroll;">
  <div id="spacer" style="width: 100%; height: 200px;"></div>
  <iframe style="height: 100px;"></iframe>
</div>

In this case, we consider all frames inside the iframe are invisible because
the iframe position is out of our frame visibility tracking margins. But if
the #spacer element's height is 100px, we consider all frames inside the iframe
viewport are visible if the iframe is in-process iframe, whereas we consider
all frames are invisible if the iframe is out-of-process iframe.

So in short, the frame visibilityn inside OOP iframes is depending on whether
the iframe gets painted in the parent document, i.e. depending on display port
and other factors controlling the paint stuff, the visibility inside in-process
iframes is depending on same factors for normal scrollable frames.

Comment 5

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1727674 - Replace nsLayoutUtils::GetCrossDocParentFrame with nsLayoutUtils::GetCrossDocParentFrameInProcess in nsIFrame::UpdateVisibilitySynchronously. r?tnikkel

Depends on D125125

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1cb07b002fea
Track frame visibility in OOP iframes. r=tnikkel
https://hg.mozilla.org/integration/autoland/rev/04d7074ef46a
Replace nsLayoutUtils::GetCrossDocParentFrame with nsLayoutUtils::GetCrossDocParentFrameInProcess in nsIFrame::UpdateVisibilitySynchronously. r=tnikkel

Comment 7

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/1cb07b002fea
https://hg.mozilla.org/mozilla-central/rev/04d7074ef46a

Comment 8

[Chris Peterson [:cpeterson]]

Setting status-firefox93=wontfix. I assume we don't need to uplift this fix to Beta 93 if it fixes a image visibility problem that only affects Fission.