1577084 - fission iframes that are scrolled far out of view and not painted or visible are set to active

Status: RESOLVED FIXED

(Core :: Layout, defect, P3)

Description

[Hiroyuki Ikezoe (:hiro)]

Attachment: A mochitest to confirm the issue

When you run attaching mochitest with enabling fission (./mach mochitest --enable-fission), you can see something like this;

0:08.23 GECKO(2866) loadComplete on 83.46
0:08.23 GECKO(2866) frameUpdate on 83.46
0:09.25 GECKO(2866) frameUpdate on 1100.86

The mochitest opens an out-of-process iframe which is scrolled out initially and has an animated GIF. If you run the mochitest without "--enable-fission" option, you will just see a 'loadComplete' or a pair of 'frameUpdate' and 'loadComplete'.

So, the second frameUpdate around 1 sec later looks wrong to me.

Comment 1

[Timothy Nikkel (:tnikkel)]

Thanks for the testcase!

For how this is supposed to work see bug 1554832, comment 3 and bug 1554832, comment 4.

There are two bugs here.

1. The PresShell of child.html in the fission case has mIsActive == true. dom/ipc/BrowserChild.cpp is supposed to be managing this. In this mochitest I see that PresShell::SetIsActive(false) is called on the about:blank document that loads before child.html. But BrowserChild does not call PresShell::SetIsActive at all on the PresShell for child.html. PresShell's init their mIsActive variable by looking at the docshell in QueryIsActive. BrowserChild.cpp manages nsDocShell::SetIsActive separately from PresShell::SetIsActive (looks like separating these happened in https://hg.mozilla.org/integration/autoland/rev/b0c38584a789 ) . I'm not sure what the correct fix is here but if I naively call SetIsActive on the docshell instead of the preshell in BrowserChild.cpp then child.html gets an inactive presshell. But I think that is probably not the right fix.

2. When the reflow finishes on the image frame containing the gif we call UpdateVisibilitySynchronously. Without fission we can walk from child.html to test_animated_gif.html because they are in the same process, and we can observe that the iframe the gif is in is scrolled far out of view so we mark the image as not visible. With fission we can't walk above child.html, so we determine that it is visible (in that process at least). This calls nsImageFrame::OnVisibilityChange which decodes the image. So we'll need to stop this from happening if the image frame is in an inactive presshell. I can write a patch for this. There are probably other places that suffer from the same problem. This problem is actually an existing problem without fission (ie if we reflow an image frame in a background tab we will decode it and we shouldn't).

Comment 2

[Timothy Nikkel (:tnikkel)]

(In reply to Timothy Nikkel (:tnikkel) from comment #1)

There are two bugs here.

Bug 1578158 should have fixed 2). I will make this bug about 1).

Comment 3

[Timothy Nikkel (:tnikkel)]

I think bug 1582042 should have fixed 1). So we should check that and land this test.

Comment 4

[Chris Peterson [:cpeterson]]

Tentatively moving all bugs whose summaries mention "Fission" (or other Fission-related keywords) but are not assigned to a Fission Milestone to the "?" triage milestone.

This will generate a lot of bugmail, so you can filter your bugmail for the following UUID and delete them en masse:

0ee3c76a-bc79-4eb2-8d12-05dc0b68e732

Comment 5

[Neha Kochar [:neha]]

Hiro, can you check if this works now? And can we land the test then?

Comment 6

[Hiroyuki Ikezoe (:hiro)]

Timothy, I wonder what the right behavior on this test is.

non-fission I see;

loadComplete on 119.5

in fission I see;

frameUpdate on 69.38
loadComplete on 69.38

IIRC, I saw loadComplete (sometimes?) on non-fission case, but now I don't see it on non-fission. I suppose it's fixed by 2) in comment 1, so there seems to be another trigger of image decoding in fission?

Comment 7

[Timothy Nikkel (:tnikkel)]

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

Timothy, I wonder what the right behavior on this test is.

non-fission I see;

loadComplete on 119.5

in fission I see;

frameUpdate on 69.38
loadComplete on 69.38

IIRC, I saw loadComplete (sometimes?) on non-fission case, but now I don't see it on non-fission. I suppose it's fixed by 2) in comment 1, so there seems to be another trigger of image decoding in fission?

I can reproduc the same. With non-fission I can sometimes see the same frameUpdate. The image is definitely not getting decoded in either fission or non-fission (I checked). This is because there is a bit of a race condition.

SyncNotifyInternal sends frameUpdate if we have an image and a non empty dirty rect [1]. ProgressTracker::SyncNotify passes a non empty rect to SyncNotifyInternal if we have an image [2]. This happens on the main thread, but the return value of ProgressTracker::GetImage is mImage which can get set off main thread. It gets set in PrepareForNewPart called from OnDataAvailable [3]. OnDataAvailable is called off main thread from the network code for images in the most common cases. I'm pretty sure fission changes somethings with how network code works, so it's seems very plausible that enabling fission changes this race to usually get resolved a different way from when fission is disabled.

So frameUpdate's aren't really the thing we care too much about, since it looks like they can get sent even if the image is never decoded. We just want the image to not get decoded, we can listen for decodeComplete.

[1] https://searchfox.org/mozilla-central/rev/923eec8d2fb8078ebc7a33a9e1ce73eac01f7446/image/ProgressTracker.cpp#326
[2] https://searchfox.org/mozilla-central/rev/923eec8d2fb8078ebc7a33a9e1ce73eac01f7446/image/ProgressTracker.cpp#386
[3] https://searchfox.org/mozilla-central/rev/923eec8d2fb8078ebc7a33a9e1ce73eac01f7446/image/imgRequest.cpp#1012

Comment 8

[Hiroyuki Ikezoe (:hiro)]

Thank you Timothy! I will modify the test.

Comment 9

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1577084 - A mochitest to make sure images outside of display port is not decoded. r?tnikkel

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c400f7480696
A mochitest to make sure images outside of display port is not decoded. r=tnikkel

Comment 11

[Oana Pop-Rus]

https://hg.mozilla.org/mozilla-central/rev/c400f7480696