New issue
Advanced search Search tips

Issue 778451 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Oct 2017
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 2
Type: Bug

Blocking:
issue 776464



Sign in to add a comment

All wallpaper requests should go through PendingWallpaper for rate limiting

Project Member Reported by wzang@chromium.org, Oct 25 2017

Issue description

WallpaperManager::PendingWallpaper implements rate limiting, however currently all wallpaper setting requests except for the login screen bypasses it (technically they do go through PendingWallpaper, but the delay is always zero even if there's a previous request being processed). This may result in wallpaper being stuck, and there's code duplication because there are both 'now' and 'delayed' versions for 'user' and 'default' wallpapers.

 

Comment 1 by wzang@chromium.org, Oct 27 2017

Blocking: 776464
Project Member

Comment 2 by bugdroid1@chromium.org, Oct 28 2017

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

commit 8adb299d383823d8a514aed948ea13633bd8037d
Author: Wenzhao Zang <wzang@chromium.org>
Date: Sat Oct 28 02:01:31 2017

wallpaper refactor: Make delayed PendingWallpaper default

1) All the PendingWallpaper requests are now subject to a delay
calculated from average loading time. However, in most cases the delay
is still zero, and this change only affects corner cases that multiple
requests coming together, which may result in wallpaper being stuck.

2) Add the account_id update in PendingWallpaper. Did not modify
the functionality of PendingWallpaper, except for some renaming.

TBR=oshima@chromium.org

Bug:  778451 
Change-Id: I4c1e9dd0a894d80fe9798dd528183e7ea5ebd1f7
Reviewed-on: https://chromium-review.googlesource.com/734801
Reviewed-by: Wenzhao (Colin) Zang <wzang@chromium.org>
Reviewed-by: Alexander Alekseev <alemate@chromium.org>
Commit-Queue: Wenzhao (Colin) Zang <wzang@chromium.org>
Cr-Commit-Position: refs/heads/master@{#512371}
[modify] https://crrev.com/8adb299d383823d8a514aed948ea13633bd8037d/chrome/browser/chromeos/BUILD.gn
[modify] https://crrev.com/8adb299d383823d8a514aed948ea13633bd8037d/chrome/browser/chromeos/arc/wallpaper/arc_wallpaper_service.cc
[modify] https://crrev.com/8adb299d383823d8a514aed948ea13633bd8037d/chrome/browser/chromeos/arc/wallpaper/arc_wallpaper_service_unittest.cc
[modify] https://crrev.com/8adb299d383823d8a514aed948ea13633bd8037d/chrome/browser/chromeos/customization/customization_wallpaper_downloader_browsertest.cc
[modify] https://crrev.com/8adb299d383823d8a514aed948ea13633bd8037d/chrome/browser/chromeos/extensions/wallpaper_private_api.cc
[modify] https://crrev.com/8adb299d383823d8a514aed948ea13633bd8037d/chrome/browser/chromeos/login/lock/views_screen_locker.cc
[modify] https://crrev.com/8adb299d383823d8a514aed948ea13633bd8037d/chrome/browser/chromeos/login/ui/user_adding_screen.cc
[modify] https://crrev.com/8adb299d383823d8a514aed948ea13633bd8037d/chrome/browser/chromeos/login/ui/webui_login_display.cc
[modify] https://crrev.com/8adb299d383823d8a514aed948ea13633bd8037d/chrome/browser/chromeos/login/users/chrome_user_manager_impl.cc
[modify] https://crrev.com/8adb299d383823d8a514aed948ea13633bd8037d/chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager.cc
[modify] https://crrev.com/8adb299d383823d8a514aed948ea13633bd8037d/chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager.h
[modify] https://crrev.com/8adb299d383823d8a514aed948ea13633bd8037d/chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager_browsertest.cc
[modify] https://crrev.com/8adb299d383823d8a514aed948ea13633bd8037d/chrome/browser/ui/ash/lock_screen_client.cc
[modify] https://crrev.com/8adb299d383823d8a514aed948ea13633bd8037d/chrome/browser/ui/ash/multi_user/user_switch_animator_chromeos.cc
[modify] https://crrev.com/8adb299d383823d8a514aed948ea13633bd8037d/chrome/browser/ui/webui/chromeos/login/supervised_user_creation_screen_handler.cc

Comment 3 by wzang@chromium.org, Oct 28 2017

Status: Fixed (was: Started)
Project Member

Comment 4 by bugdroid1@chromium.org, Oct 30 2017

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

commit 4650358b35c8287857d1b8b1d39a9f1d7f353ef6
Author: Tim Schumann <tschumann@chromium.org>
Date: Mon Oct 30 15:40:35 2017

Revert "wallpaper refactor: Make delayed PendingWallpaper default"

This reverts commit 8adb299d383823d8a514aed948ea13633bd8037d.

Reason for revert: breaks unit tests (among other things ASAN).
Details: https://bugs.chromium.org/p/chromium/issues/detail?id=779504

Original change's description:
> wallpaper refactor: Make delayed PendingWallpaper default
> 
> 1) All the PendingWallpaper requests are now subject to a delay
> calculated from average loading time. However, in most cases the delay
> is still zero, and this change only affects corner cases that multiple
> requests coming together, which may result in wallpaper being stuck.
> 
> 2) Add the account_id update in PendingWallpaper. Did not modify
> the functionality of PendingWallpaper, except for some renaming.
> 
> TBR=oshima@chromium.org
> 
> Bug:  778451 
> Change-Id: I4c1e9dd0a894d80fe9798dd528183e7ea5ebd1f7
> Reviewed-on: https://chromium-review.googlesource.com/734801
> Reviewed-by: Wenzhao (Colin) Zang <wzang@chromium.org>
> Reviewed-by: Alexander Alekseev <alemate@chromium.org>
> Commit-Queue: Wenzhao (Colin) Zang <wzang@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#512371}

TBR=oshima@chromium.org,alemate@chromium.org,wzang@chromium.org

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

Bug:  778451 
Change-Id: Id1978a1bfbd87ba1f4d1c5a895e15cdff0d67690
Reviewed-on: https://chromium-review.googlesource.com/743961
Reviewed-by: Tim Schumann <tschumann@chromium.org>
Commit-Queue: Tim Schumann <tschumann@chromium.org>
Cr-Commit-Position: refs/heads/master@{#512502}
[modify] https://crrev.com/4650358b35c8287857d1b8b1d39a9f1d7f353ef6/chrome/browser/chromeos/BUILD.gn
[modify] https://crrev.com/4650358b35c8287857d1b8b1d39a9f1d7f353ef6/chrome/browser/chromeos/arc/wallpaper/arc_wallpaper_service.cc
[modify] https://crrev.com/4650358b35c8287857d1b8b1d39a9f1d7f353ef6/chrome/browser/chromeos/arc/wallpaper/arc_wallpaper_service_unittest.cc
[modify] https://crrev.com/4650358b35c8287857d1b8b1d39a9f1d7f353ef6/chrome/browser/chromeos/customization/customization_wallpaper_downloader_browsertest.cc
[modify] https://crrev.com/4650358b35c8287857d1b8b1d39a9f1d7f353ef6/chrome/browser/chromeos/extensions/wallpaper_private_api.cc
[modify] https://crrev.com/4650358b35c8287857d1b8b1d39a9f1d7f353ef6/chrome/browser/chromeos/login/lock/views_screen_locker.cc
[modify] https://crrev.com/4650358b35c8287857d1b8b1d39a9f1d7f353ef6/chrome/browser/chromeos/login/ui/user_adding_screen.cc
[modify] https://crrev.com/4650358b35c8287857d1b8b1d39a9f1d7f353ef6/chrome/browser/chromeos/login/ui/webui_login_display.cc
[modify] https://crrev.com/4650358b35c8287857d1b8b1d39a9f1d7f353ef6/chrome/browser/chromeos/login/users/chrome_user_manager_impl.cc
[modify] https://crrev.com/4650358b35c8287857d1b8b1d39a9f1d7f353ef6/chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager.cc
[modify] https://crrev.com/4650358b35c8287857d1b8b1d39a9f1d7f353ef6/chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager.h
[modify] https://crrev.com/4650358b35c8287857d1b8b1d39a9f1d7f353ef6/chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager_browsertest.cc
[modify] https://crrev.com/4650358b35c8287857d1b8b1d39a9f1d7f353ef6/chrome/browser/ui/ash/lock_screen_client.cc
[modify] https://crrev.com/4650358b35c8287857d1b8b1d39a9f1d7f353ef6/chrome/browser/ui/ash/multi_user/user_switch_animator_chromeos.cc
[modify] https://crrev.com/4650358b35c8287857d1b8b1d39a9f1d7f353ef6/chrome/browser/ui/webui/chromeos/login/supervised_user_creation_screen_handler.cc

Project Member

Comment 5 by bugdroid1@chromium.org, Oct 31 2017

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

commit fd502dfe65e7daa4f37b3dfe16fd013d0e1770f2
Author: Wenzhao Zang <wzang@chromium.org>
Date: Tue Oct 31 18:30:24 2017

Make PendingWallpaper pass weak pointers with a default delay

After applying a default delay to PendingWallpaper, some tests
failed ( crbug.com/779504 ) or have memory leak [1].

These tests indirectly called SetUserWallpaper, but they do not
care/know about this, so they do not actively wait until wallpaper
loading is done, but start the destruction directly.

This used to be okay when SetUserWallpaper had zero delay.

But when it has the default delay, the timer is still running when
the destruction starts, and the timer holds a reference to the shared
pointer of PendingWallpaper, which results in memory leak.

What's worse, if the WallpaperManager already destructs itself,
PendingWallpaper::ProcessRequest does not know it, and it crashes.

After changing the shared pointer to a weak pointer, WallpaperManager
will be able to completely manage the lifetime of PendingWallpaper.

[1] https://luci-milo.appspot.com/buildbot/chromium.memory/Linux%20Chromium%20OS%20ASan%20LSan%20Tests%20(1)/24405

Below are the description of the original CL:

1) All the PendingWallpaper requests are now subject to a delay
calculated from average loading time. However, in most cases the delay
is still zero, and this change only affects corner cases that multiple
requests coming together, which may result in wallpaper being stuck.

2) Add the account_id update in PendingWallpaper. Did not modify
the functionality of PendingWallpaper, except for some renaming.

TBR=oshima@chromium.org

Bug:  779504 ,  778451 
Change-Id: Iae943f88bbe046849c3afab75f35a253934fe75b
Reviewed-on: https://chromium-review.googlesource.com/745022
Reviewed-by: Wenzhao (Colin) Zang <wzang@chromium.org>
Reviewed-by: Alexander Alekseev <alemate@chromium.org>
Commit-Queue: Wenzhao (Colin) Zang <wzang@chromium.org>
Cr-Commit-Position: refs/heads/master@{#512883}
[modify] https://crrev.com/fd502dfe65e7daa4f37b3dfe16fd013d0e1770f2/chrome/browser/chromeos/BUILD.gn
[modify] https://crrev.com/fd502dfe65e7daa4f37b3dfe16fd013d0e1770f2/chrome/browser/chromeos/arc/wallpaper/arc_wallpaper_service.cc
[modify] https://crrev.com/fd502dfe65e7daa4f37b3dfe16fd013d0e1770f2/chrome/browser/chromeos/arc/wallpaper/arc_wallpaper_service_unittest.cc
[modify] https://crrev.com/fd502dfe65e7daa4f37b3dfe16fd013d0e1770f2/chrome/browser/chromeos/customization/customization_wallpaper_downloader_browsertest.cc
[modify] https://crrev.com/fd502dfe65e7daa4f37b3dfe16fd013d0e1770f2/chrome/browser/chromeos/extensions/wallpaper_private_api.cc
[modify] https://crrev.com/fd502dfe65e7daa4f37b3dfe16fd013d0e1770f2/chrome/browser/chromeos/login/lock/views_screen_locker.cc
[modify] https://crrev.com/fd502dfe65e7daa4f37b3dfe16fd013d0e1770f2/chrome/browser/chromeos/login/ui/user_adding_screen.cc
[modify] https://crrev.com/fd502dfe65e7daa4f37b3dfe16fd013d0e1770f2/chrome/browser/chromeos/login/ui/webui_login_display.cc
[modify] https://crrev.com/fd502dfe65e7daa4f37b3dfe16fd013d0e1770f2/chrome/browser/chromeos/login/users/chrome_user_manager_impl.cc
[modify] https://crrev.com/fd502dfe65e7daa4f37b3dfe16fd013d0e1770f2/chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager.cc
[modify] https://crrev.com/fd502dfe65e7daa4f37b3dfe16fd013d0e1770f2/chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager.h
[modify] https://crrev.com/fd502dfe65e7daa4f37b3dfe16fd013d0e1770f2/chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager_browsertest.cc
[modify] https://crrev.com/fd502dfe65e7daa4f37b3dfe16fd013d0e1770f2/chrome/browser/ui/ash/lock_screen_client.cc
[modify] https://crrev.com/fd502dfe65e7daa4f37b3dfe16fd013d0e1770f2/chrome/browser/ui/ash/multi_user/user_switch_animator_chromeos.cc
[modify] https://crrev.com/fd502dfe65e7daa4f37b3dfe16fd013d0e1770f2/chrome/browser/ui/webui/chromeos/login/supervised_user_creation_screen_handler.cc

Project Member

Comment 6 by bugdroid1@chromium.org, Nov 1 2017

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

commit 9618fdc418cd5576e0c04e121bd19a84459c1642
Author: Makoto Shimazu <shimazu@chromium.org>
Date: Wed Nov 01 01:37:22 2017

Revert "Make PendingWallpaper pass weak pointers with a default delay"

This reverts commit fd502dfe65e7daa4f37b3dfe16fd013d0e1770f2.

Reason for revert: This patch seems causing interactive_ui_tests failures on Linux Chromium MSan/LSan Tests bots.
https://build.chromium.org/p/chromium.memory/builders/Linux%20ChromiumOS%20MSan%20Tests/builds/3892

Original change's description:
> Make PendingWallpaper pass weak pointers with a default delay
> 
> After applying a default delay to PendingWallpaper, some tests
> failed ( crbug.com/779504 ) or have memory leak [1].
> 
> These tests indirectly called SetUserWallpaper, but they do not
> care/know about this, so they do not actively wait until wallpaper
> loading is done, but start the destruction directly.
> 
> This used to be okay when SetUserWallpaper had zero delay.
> 
> But when it has the default delay, the timer is still running when
> the destruction starts, and the timer holds a reference to the shared
> pointer of PendingWallpaper, which results in memory leak.
> 
> What's worse, if the WallpaperManager already destructs itself,
> PendingWallpaper::ProcessRequest does not know it, and it crashes.
> 
> After changing the shared pointer to a weak pointer, WallpaperManager
> will be able to completely manage the lifetime of PendingWallpaper.
> 
> [1] https://luci-milo.appspot.com/buildbot/chromium.memory/Linux%20Chromium%20OS%20ASan%20LSan%20Tests%20(1)/24405
> 
> Below are the description of the original CL:
> 
> 1) All the PendingWallpaper requests are now subject to a delay
> calculated from average loading time. However, in most cases the delay
> is still zero, and this change only affects corner cases that multiple
> requests coming together, which may result in wallpaper being stuck.
> 
> 2) Add the account_id update in PendingWallpaper. Did not modify
> the functionality of PendingWallpaper, except for some renaming.
> 
> TBR=oshima@chromium.org
> 
> Bug:  779504 ,  778451 
> Change-Id: Iae943f88bbe046849c3afab75f35a253934fe75b
> Reviewed-on: https://chromium-review.googlesource.com/745022
> Reviewed-by: Wenzhao (Colin) Zang <wzang@chromium.org>
> Reviewed-by: Alexander Alekseev <alemate@chromium.org>
> Commit-Queue: Wenzhao (Colin) Zang <wzang@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#512883}

TBR=oshima@chromium.org,alemate@chromium.org,wzang@chromium.org

Change-Id: I62ae0d91bc1cbe953de45a77402f38754de858cf
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug:  779504 ,  778451 
Reviewed-on: https://chromium-review.googlesource.com/748381
Reviewed-by: Makoto Shimazu <shimazu@chromium.org>
Commit-Queue: Makoto Shimazu <shimazu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#513039}
[modify] https://crrev.com/9618fdc418cd5576e0c04e121bd19a84459c1642/chrome/browser/chromeos/BUILD.gn
[modify] https://crrev.com/9618fdc418cd5576e0c04e121bd19a84459c1642/chrome/browser/chromeos/arc/wallpaper/arc_wallpaper_service.cc
[modify] https://crrev.com/9618fdc418cd5576e0c04e121bd19a84459c1642/chrome/browser/chromeos/arc/wallpaper/arc_wallpaper_service_unittest.cc
[modify] https://crrev.com/9618fdc418cd5576e0c04e121bd19a84459c1642/chrome/browser/chromeos/customization/customization_wallpaper_downloader_browsertest.cc
[modify] https://crrev.com/9618fdc418cd5576e0c04e121bd19a84459c1642/chrome/browser/chromeos/extensions/wallpaper_private_api.cc
[modify] https://crrev.com/9618fdc418cd5576e0c04e121bd19a84459c1642/chrome/browser/chromeos/login/lock/views_screen_locker.cc
[modify] https://crrev.com/9618fdc418cd5576e0c04e121bd19a84459c1642/chrome/browser/chromeos/login/ui/user_adding_screen.cc
[modify] https://crrev.com/9618fdc418cd5576e0c04e121bd19a84459c1642/chrome/browser/chromeos/login/ui/webui_login_display.cc
[modify] https://crrev.com/9618fdc418cd5576e0c04e121bd19a84459c1642/chrome/browser/chromeos/login/users/chrome_user_manager_impl.cc
[modify] https://crrev.com/9618fdc418cd5576e0c04e121bd19a84459c1642/chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager.cc
[modify] https://crrev.com/9618fdc418cd5576e0c04e121bd19a84459c1642/chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager.h
[modify] https://crrev.com/9618fdc418cd5576e0c04e121bd19a84459c1642/chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager_browsertest.cc
[modify] https://crrev.com/9618fdc418cd5576e0c04e121bd19a84459c1642/chrome/browser/ui/ash/lock_screen_client.cc
[modify] https://crrev.com/9618fdc418cd5576e0c04e121bd19a84459c1642/chrome/browser/ui/ash/multi_user/user_switch_animator_chromeos.cc
[modify] https://crrev.com/9618fdc418cd5576e0c04e121bd19a84459c1642/chrome/browser/ui/webui/chromeos/login/supervised_user_creation_screen_handler.cc

Project Member

Comment 7 by bugdroid1@chromium.org, Nov 2 2017

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

commit d4eb6d78cc6d84cffa7e9475029cd45716272b81
Author: Wenzhao Zang <wzang@chromium.org>
Date: Thu Nov 02 01:19:50 2017

Reland "Make PendingWallpaper pass weak pointers with a default delay"

The differences with the original CL [1]:
1) Changed the scope of on_finish in case ProcessRequest is still being
executed after PendingWallpaper object is destroyed.
2) Changed the order of destruction within RemovePendingWallpaperFromList
(a bug),

[1] Original CL: https://chromium-review.googlesource.com/c/chromium/src/+/745022

TBR=oshima@chromium.org

Bug:  778451 
Change-Id: Ib5483b5427fe568963831fc132a81263f66cbf43
Reviewed-on: https://chromium-review.googlesource.com/749684
Reviewed-by: Wenzhao (Colin) Zang <wzang@chromium.org>
Reviewed-by: Alexander Alekseev <alemate@chromium.org>
Commit-Queue: Wenzhao (Colin) Zang <wzang@chromium.org>
Cr-Commit-Position: refs/heads/master@{#513358}
[modify] https://crrev.com/d4eb6d78cc6d84cffa7e9475029cd45716272b81/chrome/browser/chromeos/BUILD.gn
[modify] https://crrev.com/d4eb6d78cc6d84cffa7e9475029cd45716272b81/chrome/browser/chromeos/arc/wallpaper/arc_wallpaper_service.cc
[modify] https://crrev.com/d4eb6d78cc6d84cffa7e9475029cd45716272b81/chrome/browser/chromeos/arc/wallpaper/arc_wallpaper_service_unittest.cc
[modify] https://crrev.com/d4eb6d78cc6d84cffa7e9475029cd45716272b81/chrome/browser/chromeos/customization/customization_wallpaper_downloader_browsertest.cc
[modify] https://crrev.com/d4eb6d78cc6d84cffa7e9475029cd45716272b81/chrome/browser/chromeos/extensions/wallpaper_private_api.cc
[modify] https://crrev.com/d4eb6d78cc6d84cffa7e9475029cd45716272b81/chrome/browser/chromeos/login/lock/views_screen_locker.cc
[modify] https://crrev.com/d4eb6d78cc6d84cffa7e9475029cd45716272b81/chrome/browser/chromeos/login/ui/user_adding_screen.cc
[modify] https://crrev.com/d4eb6d78cc6d84cffa7e9475029cd45716272b81/chrome/browser/chromeos/login/ui/webui_login_display.cc
[modify] https://crrev.com/d4eb6d78cc6d84cffa7e9475029cd45716272b81/chrome/browser/chromeos/login/users/chrome_user_manager_impl.cc
[modify] https://crrev.com/d4eb6d78cc6d84cffa7e9475029cd45716272b81/chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager.cc
[modify] https://crrev.com/d4eb6d78cc6d84cffa7e9475029cd45716272b81/chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager.h
[modify] https://crrev.com/d4eb6d78cc6d84cffa7e9475029cd45716272b81/chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager_browsertest.cc
[modify] https://crrev.com/d4eb6d78cc6d84cffa7e9475029cd45716272b81/chrome/browser/ui/ash/lock_screen_client.cc
[modify] https://crrev.com/d4eb6d78cc6d84cffa7e9475029cd45716272b81/chrome/browser/ui/ash/multi_user/user_switch_animator_chromeos.cc
[modify] https://crrev.com/d4eb6d78cc6d84cffa7e9475029cd45716272b81/chrome/browser/ui/webui/chromeos/login/supervised_user_creation_screen_handler.cc

Comment 8 by dchan@chromium.org, Jan 22 2018

Status: Archived (was: Fixed)

Comment 9 by dchan@chromium.org, Jan 23 2018

Status: Fixed (was: Archived)

Sign in to add a comment