1167818 - [e10s] background tabs use CPU as though they're in foreground, following "Restore All Crashed Tabs"

Status: RESOLVED FIXED

(Firefox :: Session Restore, defect)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I'm using a Linux debug build, self-built, mozilla-central.

I just hit a child process crash, and hit the "Restore All Crashed Tabs".  After everything was recovered, I noticed Firefox was using way more CPU than I'd expect given the set of tabs.  I poked at the child process in gdb; it was spending time refresh driver ticks for various background tabs, including painting-related stuff.  (Some of these tabs had various visual animations in them.)

When I used Ctrl+PgDn to cycle through all the tabs, then the high CPU usage stopped.

I'm guessing that following a "Restore All Crashed Tabs", we're not correctly restoring state that knows whether a tab is in the background, and we're doing too much work as a result.  I'm not sure if that state is the state that flows through nsDocShell::SetIsActive (which should get to the refresh driver via SetIsActive itself or PresShell::QueryIsActive, to PresShell::SetIsActive, to nsRefreshDriver::SetThrottled, to nsRefreshDriver::ChooseTimer) or whether it's something else.

Comment 1

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Tim, any chance you could look into this?

Comment 2

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0001-Bug-1167818-Update-docShellIsActive-flag-after-a-bro.patch

We set browser.docShellIsActive=false in addTab() because we know a new docShell/frameLoader just got created. When switching remoteness when navigating (or after a tab crashed to load about:tabcrashed) we don't. This patch does exactly that and I can't reproduce the issue anymore.

Comment 3

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8612409 [details] [diff] [review]
0001-Bug-1167818-Update-docShellIsActive-flag-after-a-bro.patch

Review of attachment 8612409 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/tabbrowser.xml
@@ +1510,5 @@
>  
> +            // Switching a browser's remoteness will create a new frameLoader.
> +            // As frameLoaders start out with an active docShell we have to
> +            // deactivate it if this is not the selected tab's browser or the
> +            // browser windows minimized.

Changed that to "browser window is minimized."

Comment 4

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8612409 [details] [diff] [review]
0001-Bug-1167818-Update-docShellIsActive-flag-after-a-bro.patch

Review of attachment 8612409 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!

::: browser/base/content/tabbrowser.xml
@@ +1511,5 @@
> +            // Switching a browser's remoteness will create a new frameLoader.
> +            // As frameLoaders start out with an active docShell we have to
> +            // deactivate it if this is not the selected tab's browser or the
> +            // browser windows minimized.
> +            aBrowser.docShellIsActive = (aBrowser == this.selectedBrowser &&

It seems safer to me to copy the docShellIsActive property from the old browser. But this should work too.

Comment 5

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Bill McCloskey (:billm) from comment #4)
> > +            aBrowser.docShellIsActive = (aBrowser == this.selectedBrowser &&
> 
> It seems safer to me to copy the docShellIsActive property from the old
> browser. But this should work too.

Yeah indeed. We can do that once remote browser support the docShellIsActive getter :/

https://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/remote-browser.xml#217

Comment 6

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/182a3a33ac08

Comment 7

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/182a3a33ac08