New issue
Advanced search Search tips

Issue 780191 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Jan 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Android , Windows , Mac
Pri: 2
Type: Bug



Sign in to add a comment

Renderer Scheduler unaware of backgrounded state on initial load

Project Member Reported by fmea...@chromium.org, Oct 31 2017

Issue description

Based on my tests and reading the code, if a tab is initially loaded in the background (Ctrl+Click), the renderer scheduler assumes that the page is in the foreground.

To verify, the timer_queue_policy_ is never stopped for those tabs until it is foregrounded and backgrounded again.

This might NOT be the case on android, "Open in new tab" brings the new tab to the front quickly before it goes in the background so the relevant events might be fired.
 
Cc: oysteine@chromium.org
Hm, it feels that the browser should tell the renderer process the visibility status even in this case.

+oysteine@, I remember that you did hook this into GRC. Could we do it there?
Cc: -oysteine@chromium.org
Owner: oysteine@chromium.org
Discussed offline with oysteine@, he is working on a similar issue, and he will bundle it with this one.
Labels: OS-Android
I just confirmed that opening tabs in the background on android does NOT send a backgrounded signal to them.

The pages seemed cached but not rendered (painted) until they are brought to foreground, but I believe that a renderer process was created (confirmed by using devtools inspector).
Cc: oysteine@chromium.org
Owner: fmea...@chromium.org
It turns out that this might be a different issue, I have a solution in the works.
Project Member

Comment 5 by bugdroid1@chromium.org, Dec 21 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/99a51fcacaad639093357727542fb1ef7c376a25

commit 99a51fcacaad639093357727542fb1ef7c376a25
Author: Fadi Meawad <fmeawad@chromium.org>
Date: Thu Dec 21 02:10:15 2017

[PageLifecyle] Move the launch process state to Webkit/common

This CL introduces Webkit/common/page/launching_process_state.h that
includes the launch background and launch boosted state.

The reason this file is needed, is because the state is different per
OS but needs to be consistent between the RenderProcessHostImpl and
the RendererSchedulerImpl. To prevent future changes in one not
reflecting in the other, the code is moved to a common place and
shared.

This CL fixes an issue where the scheduler assumes that it is
foregrounded while it is actually backgrounded during the initial
phase on Android.

Bug:  chromium:780191 
Change-Id: Ib8dbfb6447f58c8b998a94ae2c69f8bc906d150f
Reviewed-on: https://chromium-review.googlesource.com/815678
Commit-Queue: Fadi Meawad <fmeawad@chromium.org>
Reviewed-by: Alexander Timin <altimin@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Reviewed-by: Bo <boliu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#525563}
[modify] https://crrev.com/99a51fcacaad639093357727542fb1ef7c376a25/content/browser/renderer_host/render_process_host_impl.cc
[modify] https://crrev.com/99a51fcacaad639093357727542fb1ef7c376a25/third_party/WebKit/Source/platform/scheduler/renderer/queueing_time_estimator.h
[modify] https://crrev.com/99a51fcacaad639093357727542fb1ef7c376a25/third_party/WebKit/Source/platform/scheduler/renderer/queueing_time_estimator_unittest.cc
[modify] https://crrev.com/99a51fcacaad639093357727542fb1ef7c376a25/third_party/WebKit/Source/platform/scheduler/renderer/renderer_metrics_helper_unittest.cc
[modify] https://crrev.com/99a51fcacaad639093357727542fb1ef7c376a25/third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc
[modify] https://crrev.com/99a51fcacaad639093357727542fb1ef7c376a25/third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl_unittest.cc
[modify] https://crrev.com/99a51fcacaad639093357727542fb1ef7c376a25/third_party/WebKit/common/BUILD.gn
[add] https://crrev.com/99a51fcacaad639093357727542fb1ef7c376a25/third_party/WebKit/common/page/launching_process_state.h

Project Member

Comment 6 by bugdroid1@chromium.org, Dec 21 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/db0b963d1d5fb33f56f2c410f036cbd93041b06e

commit db0b963d1d5fb33f56f2c410f036cbd93041b06e
Author: Fadi Meawad <fmeawad@chromium.org>
Date: Thu Dec 21 16:50:08 2017

Revert "[PageLifecyle] Move the launch process state to Webkit/common"

This reverts commit 99a51fcacaad639093357727542fb1ef7c376a25.

Reason for revert: Speculative revert for crashes in
https://bugs.chromium.org/p/chromium/issues/detail?id=796940

Original change's description:
> [PageLifecyle] Move the launch process state to Webkit/common
> 
> This CL introduces Webkit/common/page/launching_process_state.h that
> includes the launch background and launch boosted state.
> 
> The reason this file is needed, is because the state is different per
> OS but needs to be consistent between the RenderProcessHostImpl and
> the RendererSchedulerImpl. To prevent future changes in one not
> reflecting in the other, the code is moved to a common place and
> shared.
> 
> This CL fixes an issue where the scheduler assumes that it is
> foregrounded while it is actually backgrounded during the initial
> phase on Android.
> 
> Bug:  chromium:780191 
> Change-Id: Ib8dbfb6447f58c8b998a94ae2c69f8bc906d150f
> Reviewed-on: https://chromium-review.googlesource.com/815678
> Commit-Queue: Fadi Meawad <fmeawad@chromium.org>
> Reviewed-by: Alexander Timin <altimin@chromium.org>
> Reviewed-by: Kentaro Hara <haraken@chromium.org>
> Reviewed-by: Bo <boliu@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#525563}

TBR=boliu@chromium.org,fmeawad@chromium.org,haraken@chromium.org,oysteine@chromium.org,altimin@chromium.org

Change-Id: I1f386d502333abf2494208425e2b0979eaed042c
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug:  chromium:780191 
Reviewed-on: https://chromium-review.googlesource.com/840160
Reviewed-by: Fadi Meawad <fmeawad@chromium.org>
Commit-Queue: Fadi Meawad <fmeawad@chromium.org>
Cr-Commit-Position: refs/heads/master@{#525715}
[modify] https://crrev.com/db0b963d1d5fb33f56f2c410f036cbd93041b06e/content/browser/renderer_host/render_process_host_impl.cc
[modify] https://crrev.com/db0b963d1d5fb33f56f2c410f036cbd93041b06e/third_party/WebKit/Source/platform/scheduler/renderer/queueing_time_estimator.h
[modify] https://crrev.com/db0b963d1d5fb33f56f2c410f036cbd93041b06e/third_party/WebKit/Source/platform/scheduler/renderer/queueing_time_estimator_unittest.cc
[modify] https://crrev.com/db0b963d1d5fb33f56f2c410f036cbd93041b06e/third_party/WebKit/Source/platform/scheduler/renderer/renderer_metrics_helper_unittest.cc
[modify] https://crrev.com/db0b963d1d5fb33f56f2c410f036cbd93041b06e/third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc
[modify] https://crrev.com/db0b963d1d5fb33f56f2c410f036cbd93041b06e/third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl_unittest.cc
[modify] https://crrev.com/db0b963d1d5fb33f56f2c410f036cbd93041b06e/third_party/WebKit/common/BUILD.gn
[delete] https://crrev.com/a3e55aafc0dc0019ed20c1636311c30eb6290396/third_party/WebKit/common/page/launching_process_state.h

Project Member

Comment 7 by bugdroid1@chromium.org, Dec 21 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e442ea0666d7af6e564fc1eb2c31deb05ed54035

commit e442ea0666d7af6e564fc1eb2c31deb05ed54035
Author: Fadi Meawad <fmeawad@chromium.org>
Date: Thu Dec 21 22:54:43 2017

Reland "[PageLifecyle] Move the launch process state to Webkit/common"

This is a reland of 99a51fcacaad639093357727542fb1ef7c376a25

The failures described in  crbug.com/796940  was result of throttling
the scheduled tasks as a result of the new backgrounding state.

The fix foregrounds the RendererScheduler before running the tests
to avoid throttling.

Original change's description:
> [PageLifecyle] Move the launch process state to Webkit/common
>
> This CL introduces Webkit/common/page/launching_process_state.h that
> includes the launch background and launch boosted state.
>
> The reason this file is needed, is because the state is different per
> OS but needs to be consistent between the RenderProcessHostImpl and
> the RendererSchedulerImpl. To prevent future changes in one not
> reflecting in the other, the code is moved to a common place and
> shared.
>
> This CL fixes an issue where the scheduler assumes that it is
> foregrounded while it is actually backgrounded during the initial
> phase on Android.
>
> Bug:  chromium:780191 
> Change-Id: Ib8dbfb6447f58c8b998a94ae2c69f8bc906d150f
> Reviewed-on: https://chromium-review.googlesource.com/815678
> Commit-Queue: Fadi Meawad <fmeawad@chromium.org>
> Reviewed-by: Alexander Timin <altimin@chromium.org>
> Reviewed-by: Kentaro Hara <haraken@chromium.org>
> Reviewed-by: Bo <boliu@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#525563}

TBR=haraken@chromium.org,boliu@chromium.org

Bug:  chromium:780191 , chromium:796940 
Change-Id: I117afad3b910239656dbc7e9dc3a1447291263a9
Reviewed-on: https://chromium-review.googlesource.com/840340
Reviewed-by: Fadi Meawad <fmeawad@chromium.org>
Commit-Queue: Fadi Meawad <fmeawad@chromium.org>
Cr-Commit-Position: refs/heads/master@{#525841}
[modify] https://crrev.com/e442ea0666d7af6e564fc1eb2c31deb05ed54035/content/browser/renderer_host/render_process_host_impl.cc
[modify] https://crrev.com/e442ea0666d7af6e564fc1eb2c31deb05ed54035/third_party/WebKit/Source/core/exported/WebFrameTest.cpp
[modify] https://crrev.com/e442ea0666d7af6e564fc1eb2c31deb05ed54035/third_party/WebKit/Source/platform/scheduler/renderer/queueing_time_estimator.h
[modify] https://crrev.com/e442ea0666d7af6e564fc1eb2c31deb05ed54035/third_party/WebKit/Source/platform/scheduler/renderer/queueing_time_estimator_unittest.cc
[modify] https://crrev.com/e442ea0666d7af6e564fc1eb2c31deb05ed54035/third_party/WebKit/Source/platform/scheduler/renderer/renderer_metrics_helper_unittest.cc
[modify] https://crrev.com/e442ea0666d7af6e564fc1eb2c31deb05ed54035/third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc
[modify] https://crrev.com/e442ea0666d7af6e564fc1eb2c31deb05ed54035/third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl_unittest.cc
[modify] https://crrev.com/e442ea0666d7af6e564fc1eb2c31deb05ed54035/third_party/WebKit/common/BUILD.gn
[add] https://crrev.com/e442ea0666d7af6e564fc1eb2c31deb05ed54035/third_party/WebKit/common/page/launching_process_state.h

Project Member

Comment 8 by bugdroid1@chromium.org, Jan 3 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b85b48382bd9087a89afdc6fe0ac33dd71f1487e

commit b85b48382bd9087a89afdc6fe0ac33dd71f1487e
Author: Fadi Meawad <fmeawad@chromium.org>
Date: Wed Jan 03 02:18:33 2018

Revert "Reland "[PageLifecyle] Move the launch process state to Webkit/common""

This reverts commit e442ea0666d7af6e564fc1eb2c31deb05ed54035.

Reason for revert: 
This CL caused major regressions in android webview performance.
See  https://crbug.com/798310  

Original change's description:
> Reland "[PageLifecyle] Move the launch process state to Webkit/common"
> 
> This is a reland of 99a51fcacaad639093357727542fb1ef7c376a25
> 
> The failures described in  crbug.com/796940  was result of throttling
> the scheduled tasks as a result of the new backgrounding state.
> 
> The fix foregrounds the RendererScheduler before running the tests
> to avoid throttling.
> 
> Original change's description:
> > [PageLifecyle] Move the launch process state to Webkit/common
> >
> > This CL introduces Webkit/common/page/launching_process_state.h that
> > includes the launch background and launch boosted state.
> >
> > The reason this file is needed, is because the state is different per
> > OS but needs to be consistent between the RenderProcessHostImpl and
> > the RendererSchedulerImpl. To prevent future changes in one not
> > reflecting in the other, the code is moved to a common place and
> > shared.
> >
> > This CL fixes an issue where the scheduler assumes that it is
> > foregrounded while it is actually backgrounded during the initial
> > phase on Android.
> >
> > Bug:  chromium:780191 
> > Change-Id: Ib8dbfb6447f58c8b998a94ae2c69f8bc906d150f
> > Reviewed-on: https://chromium-review.googlesource.com/815678
> > Commit-Queue: Fadi Meawad <fmeawad@chromium.org>
> > Reviewed-by: Alexander Timin <altimin@chromium.org>
> > Reviewed-by: Kentaro Hara <haraken@chromium.org>
> > Reviewed-by: Bo <boliu@chromium.org>
> > Cr-Commit-Position: refs/heads/master@{#525563}
> 
> TBR=haraken@chromium.org,boliu@chromium.org
> 
> Bug:  chromium:780191 , chromium:796940 
> Change-Id: I117afad3b910239656dbc7e9dc3a1447291263a9
> Reviewed-on: https://chromium-review.googlesource.com/840340
> Reviewed-by: Fadi Meawad <fmeawad@chromium.org>
> Commit-Queue: Fadi Meawad <fmeawad@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#525841}

TBR=boliu@chromium.org,fmeawad@chromium.org,haraken@chromium.org,oysteine@chromium.org,altimin@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug:  chromium:780191 ,  chromium:796940 ,  chromium:798310 
Change-Id: I0c8b7e075c29761d2a6f0d02461551b651682748
Reviewed-on: https://chromium-review.googlesource.com/847653
Reviewed-by: Fadi Meawad <fmeawad@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Commit-Queue: Fadi Meawad <fmeawad@chromium.org>
Cr-Commit-Position: refs/heads/master@{#526586}
[modify] https://crrev.com/b85b48382bd9087a89afdc6fe0ac33dd71f1487e/content/browser/renderer_host/render_process_host_impl.cc
[modify] https://crrev.com/b85b48382bd9087a89afdc6fe0ac33dd71f1487e/third_party/WebKit/Source/core/exported/WebFrameTest.cpp
[modify] https://crrev.com/b85b48382bd9087a89afdc6fe0ac33dd71f1487e/third_party/WebKit/Source/platform/scheduler/renderer/queueing_time_estimator.h
[modify] https://crrev.com/b85b48382bd9087a89afdc6fe0ac33dd71f1487e/third_party/WebKit/Source/platform/scheduler/renderer/queueing_time_estimator_unittest.cc
[modify] https://crrev.com/b85b48382bd9087a89afdc6fe0ac33dd71f1487e/third_party/WebKit/Source/platform/scheduler/renderer/renderer_metrics_helper_unittest.cc
[modify] https://crrev.com/b85b48382bd9087a89afdc6fe0ac33dd71f1487e/third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc
[modify] https://crrev.com/b85b48382bd9087a89afdc6fe0ac33dd71f1487e/third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl_unittest.cc
[modify] https://crrev.com/b85b48382bd9087a89afdc6fe0ac33dd71f1487e/third_party/WebKit/common/BUILD.gn
[delete] https://crrev.com/8d5d19d557008d887d4ea133d9b80594908a0e3a/third_party/WebKit/common/page/launching_process_state.h

Project Member

Comment 9 by bugdroid1@chromium.org, Jan 16 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e3b2cdc494c40bb7caf974806aad32043c7b6b62

commit e3b2cdc494c40bb7caf974806aad32043c7b6b62
Author: Fadi Meawad <fmeawad@chromium.org>
Date: Tue Jan 16 21:44:42 2018

Fix Android Webview Rendrer Scheduler background state

When the renderer is run in process, the backgrounding state does not
propagate properly to the RendererScheduler. This CL enables updating
the RendererSchduler background state even if the renderer is run in
process.

This fix is required to reland:
https://chromium-review.googlesource.com/c/chromium/src/+/847653

Bug:  chromium:780191 ,  chromium:798310 
Change-Id: Ic4dc609de1884ecd9297ceb0b8774e4d4e313428
: 
Reviewed-on: https://chromium-review.googlesource.com/865769
Reviewed-by: Bo <boliu@chromium.org>
Commit-Queue: Fadi Meawad <fmeawad@chromium.org>
Cr-Commit-Position: refs/heads/master@{#529505}
[modify] https://crrev.com/e3b2cdc494c40bb7caf974806aad32043c7b6b62/content/browser/renderer_host/render_process_host_impl.cc

Project Member

Comment 10 by bugdroid1@chromium.org, Jan 18 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/bb30dbdbdf9a183ada3e129ffee817b8a6fd494a

commit bb30dbdbdf9a183ada3e129ffee817b8a6fd494a
Author: Fadi Meawad <fmeawad@chromium.org>
Date: Thu Jan 18 22:04:33 2018

Reland(2): [PageLifecyle] Move the launch process state to Webkit/common

The first reland caused a major regression in Android Webview bots
running with a single process, since Webview had a bug that did not
propagate the background state properly to the renderer scheduler.

This bug was fixed in:
https://chromium-review.googlesource.com/c/chromium/src/+/865769

> Original change's description:
> > Reland "[PageLifecyle] Move the launch process state to Webkit/common"
> >
> > This is a reland of 99a51fcacaad639093357727542fb1ef7c376a25
> >
> > The failures described in  crbug.com/796940  was result of throttling
> > the scheduled tasks as a result of the new backgrounding state.
> >
> > The fix foregrounds the RendererScheduler before running the tests
> > to avoid throttling.
> >
> > Original change's description:
> > > [PageLifecyle] Move the launch process state to Webkit/common
> > >
> > > This CL introduces Webkit/common/page/launching_process_state.h that
> > > includes the launch background and launch boosted state.
> > >
> > > The reason this file is needed, is because the state is different per
> > > OS but needs to be consistent between the RenderProcessHostImpl and
> > > the RendererSchedulerImpl. To prevent future changes in one not
> > > reflecting in the other, the code is moved to a common place and
> > > shared.
> > >
> > > This CL fixes an issue where the scheduler assumes that it is
> > > foregrounded while it is actually backgrounded during the initial
> > > phase on Android.
> > >
> > > Bug:  chromium:780191 
> > > Change-Id: Ib8dbfb6447f58c8b998a94ae2c69f8bc906d150f
> > > Reviewed-on: https://chromium-review.googlesource.com/815678
> > > Commit-Queue: Fadi Meawad <fmeawad@chromium.org>
> > > Reviewed-by: Alexander Timin <altimin@chromium.org>
> > > Reviewed-by: Kentaro Hara <haraken@chromium.org>
> > > Reviewed-by: Bo <boliu@chromium.org>
> > > Cr-Commit-Position: refs/heads/master@{#525563}
> >
> > TBR=haraken@chromium.org,boliu@chromium.org
> >
> > Bug:  chromium:780191 , chromium:796940 
> > Change-Id: I117afad3b910239656dbc7e9dc3a1447291263a9
> > Reviewed-on: https://chromium-review.googlesource.com/840340
> > Reviewed-by: Fadi Meawad <fmeawad@chromium.org>
> > Commit-Queue: Fadi Meawad <fmeawad@chromium.org>
> > Cr-Commit-Position: refs/heads/master@{#525841}
>
> TBR=boliu@chromium.org,fmeawad@chromium.org,haraken@chromium.org,oysteine@chromium.org,altimin@chromium.org
>
> # Not skipping CQ checks because original CL landed > 1 day ago.
>
> Bug:  chromium:780191 ,  chromium:796940 ,  chromium:798310 
> Change-Id: I0c8b7e075c29761d2a6f0d02461551b651682748
> Reviewed-on: https://chromium-review.googlesource.com/847653
> Reviewed-by: Fadi Meawad <fmeawad@chromium.org>
> Reviewed-by: Kentaro Hara <haraken@chromium.org>
> Commit-Queue: Fadi Meawad <fmeawad@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#526586}

TBR=boliu@chromium.org,altimin@chromium.org

Bug:  chromium:780191 ,  chromium:796940 ,  chromium:798310 
Change-Id: I213457c16ee09f272c5e188bc88e915e23ed40ea
Reviewed-on: https://chromium-review.googlesource.com/871370
Commit-Queue: Fadi Meawad <fmeawad@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Cr-Commit-Position: refs/heads/master@{#530289}
[modify] https://crrev.com/bb30dbdbdf9a183ada3e129ffee817b8a6fd494a/content/browser/renderer_host/render_process_host_impl.cc
[modify] https://crrev.com/bb30dbdbdf9a183ada3e129ffee817b8a6fd494a/third_party/WebKit/Source/core/exported/WebFrameTest.cpp
[modify] https://crrev.com/bb30dbdbdf9a183ada3e129ffee817b8a6fd494a/third_party/WebKit/Source/platform/scheduler/renderer/queueing_time_estimator.h
[modify] https://crrev.com/bb30dbdbdf9a183ada3e129ffee817b8a6fd494a/third_party/WebKit/Source/platform/scheduler/renderer/queueing_time_estimator_unittest.cc
[modify] https://crrev.com/bb30dbdbdf9a183ada3e129ffee817b8a6fd494a/third_party/WebKit/Source/platform/scheduler/renderer/renderer_metrics_helper_unittest.cc
[modify] https://crrev.com/bb30dbdbdf9a183ada3e129ffee817b8a6fd494a/third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc
[modify] https://crrev.com/bb30dbdbdf9a183ada3e129ffee817b8a6fd494a/third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl_unittest.cc
[modify] https://crrev.com/bb30dbdbdf9a183ada3e129ffee817b8a6fd494a/third_party/WebKit/common/BUILD.gn
[add] https://crrev.com/bb30dbdbdf9a183ada3e129ffee817b8a6fd494a/third_party/WebKit/common/page/launching_process_state.h

Status: Fixed (was: Untriaged)

Sign in to add a comment