1716436 - Consider not paint (i.e. paint with empty rect) OOP iframe if we haven't received EffectsInfo from the parent content

Status: RESOLVED FIXED

(Core :: Web Painting, enhancement)

Description

[Hiroyuki Ikezoe (:hiro)]

In nsLayoutUtils::PaintFrame() we apply BrowserChild's visible rect to clip the given frame's ink-overflowed rect. [Timothy raised a concern] that if we haven't received EffectsInfo and if the frame is quite large then we try to paint the whole frame rect.

Note that helper_fission_animation_styling_in_oopif.html is a test case you can see the case we haven't received EffectInfo.

Comment 1

[Timothy Nikkel (:tnikkel)]

Note that if we have display port margins then we should be okay. There are two cases to consider, either we have a display port base rect set or we don't. If one is not set we assume an empty base rect

https://searchfox.org/mozilla-central/rev/fbc78630d6d435b9b9647a4ac8a5def888f85f04/layout/base/DisplayPortUtils.cpp#260

which is okay. If the base rect is set then it should be set to only include what is visible. Although I guess we don't change our visible rect to the displayport until we enter the root scroll frame, so that is something to look into.

To fix this there are two ideas so far: 1) use an empty visible rect if the effects info visible rect hasn't been set yet, or 2) use the screen size to bound what is drawn.

Possible downsides for 1):
-potential for not painting an iframe when it is user visible
-possible that we decrease perf because the first time we ask the iframe to paint the visible rect is empty but its actually visible so now we have to wait longer for it to be painted

Possible downsides for 2):
-have to make sure the screen size is available in oop if child processes
-can we get the screen size in a low overhead way?

Comment 2

[Timothy Nikkel (:tnikkel)]

For reference the changeset that prompted this line of thought, and something that we need to consider to make sure we are setting the base rect properly is https://phabricator.services.mozilla.com/D116577

Comment 3

[Chris Peterson [:cpeterson]]

Timothy, is this bug strictly a performance optimization or does it affect what users see? Do you think this bug is important to fix before we ship Fission?

Tentatively tracking for Fission MVP milestone.

Comment 4

[Timothy Nikkel (:tnikkel)]

The importance of this bug is that pages can have arbitrarily large iframes, if we haven't received a visible rect for the oop if, and there aren't displayport margins set yet then we could try to allocate arbitrarily sized surfaces to try to paint it. We definitely do not want to allow web content to do that.

Comment 5

[Chris Peterson [:cpeterson]]

Timothy, do you think we should fix this bug for Fission MVP? If so, can someone on the Web Painting team take this bug? Otherwise, I can move it from blocking Fission MVP to our Future backlog.

Comment 6

[Timothy Nikkel (:tnikkel)]

I think probably MVP. I can take this.

Comment 7

[Timothy Nikkel (:tnikkel)]

Attachment: Bug 1716436. If a visible rect hasn't been set yet for an oop if, use an empty rect so as to bound it's content. r?hiro

Comment 8

[Timothy Nikkel (:tnikkel)]

Attachment: screensizelimit

Here's a patch that does a screen size limit just for reference if we ever need to revisit this in the future.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8a38c9a1d317
If a visible rect hasn't been set yet for an oop if, use an empty rect so as to bound it's content. r=hiro

Comment 10

[Iulian Moraru]

https://hg.mozilla.org/mozilla-central/rev/8a38c9a1d317