1952787 - nsRefreshDriver continues to tick with VsyncRefreshDriverTimer when Firefox window is occluded

Status: NEW

(Core :: Layout, defect)

Description

[Sotaro Ikeda [:sotaro]]

Created from Bug 1924932 comment 67.

With the following STR, there were cases that nsRefreshDriver continued to tick with VsyncRefreshDriverTimer and Vsync keeps running. It seemed like a problem.

• [1] Start Firefox
• [2] Move to https://www.youtube.com/watch?v=LXb3EKWsInQ
• [3] Start video playback
• [4] Move the Firefox window to be occluded by other app window

Comment 1

[Sotaro Ikeda [:sotaro]]

Attachment: temporal patch Add logs around vsync handling

Comment 2

[Sotaro Ikeda [:sotaro]]

With Attachment 9470695 [details] [diff], when the problem happened, PresShell::ComputeActiveness() returned true because "browserChild->IsPreservingLayers()" returned false. Then nsRefreshDriver was set active and it kept ticking.

Comment 3

[Sotaro Ikeda [:sotaro]]

nsRefreshDriver::SetActivity() was changed by Bug 1847929. It might cause the regression.

Comment 4

[Sotaro Ikeda [:sotaro]]

:emilio, can you look into this bug?

Comment 5

[Sotaro Ikeda [:sotaro]]

(In reply to Sotaro Ikeda [:sotaro] from comment #3)

nsRefreshDriver::SetActivity() was changed by Bug 1847929. It might cause the regression.

Ah, it seems wrong from Bug 1924932 Comment 46, its regression happened around 112-114.

Comment 6

[Sotaro Ikeda [:sotaro]]

Attachment: temporal patch Add logs around vsync handling

Replace Attachment 9470695 [details] [diff] by this patch.

Comment 7

[Emilio Cobos Álvarez [:emilio]]

Do you still need me to look at this? If so please ni? me again.

When the widget becomes occluded you should end up here, which should end up here or here.

Maybe one of those codepaths doesn't deactivate the browser properly?

Comment 8

[Sotaro Ikeda [:sotaro]]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #7)

Do you still need me to look at this? If so please ni? me again.

When the widget becomes occluded you should end up here, which should end up here or here.

Maybe one of those codepaths doesn't deactivate the browser properly?

The above code patch was not used. The problem was caused by PreloadedBrowser.

PreloadedBrowser was created by NewTabPagePreloading.maybeCreatePreloadedBrowser(). And timer start was trigged by ActivityStreamMessageChannel.tabLoaded()

Comment 9

[Emilio Cobos Álvarez [:emilio]]

So, bug 1548683?

Comment 10

[Sotaro Ikeda [:sotaro]]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #9)

So, bug 1548683?

Yes.

Comment 11

[Sotaro Ikeda [:sotaro]]

function calls are like the following.

---

browser.renderLayers = true;
->BrowserHost::SetRenderLayers()
->BrowserParent::SetRenderLayers()
->BrowserParent::SetRenderLayersInternal()
->BrowserParent::SendRenderLayers()
->// IPC
->BrowserChild::RecvRenderLayers()
->BrowserChild::UpdateVisibility()
->BrowserChild::MakeVisible()
->BrowserChild::PresShellActivenessMaybeChanged()
-----> PresShell::ComputeActiveness()
-----> PresShell::SetIsActive()
--------> nsRefreshDriver::SetActivity()
--------> nsRefreshDriver::EnsureTimerStarted()

Comment 12

[Sotaro Ikeda [:sotaro]]

Then PreloadedBrowser always requests vsync enabled. It is not good.

Comment 13

[Sotaro Ikeda [:sotaro]]

Attachment: temporal patch Add logs around vsync handling

Replace Attachment 9470702 [details] [diff].

Comment 14

[Sotaro Ikeda [:sotaro]]

It might be better to add a workaround before fixing Bug 1548683.

Comment 15

[BugBot [:suhaib / :marco]]

This bug has been marked as a regression. Setting status flag for Nightly to affected.

Comment 16

[Daniel Holbert [:dholbert]]

(setting old version flags to wontfix, to make it clear that they were also affected, since this regressed before 115 per comment 5. Otherwise bugdash surfaces this as a potential new regression in current nightly, which it's not.)

Comment 17

[BugBot [:suhaib / :marco]]

The severity field is not set for this bug.
:TYLin, could you have a look please?

For more information, please visit BugBot documentation.

Comment 18

[Ting-Yu Lin [:TYLin] (PST, UTC-7)]

This is an old regression, so S3 feels right for now.