ImageManager::pending_cache_requests_ is never cleared |
|||||
Issue descriptionNoticed that pending_cache_requests_ is never cleared which results in Chrome browser keeping thousands of RepeatingCallback objects alive. Not to mention the GURL objects are also kept around. Should we be using OnceCallback here instead of RepeatingCallback? Also in this instance I had kept chrome alive for 2 days and looked at memory. Noticed that it keeps queuing requests. This means that the database_ready_ was never set to true. So, there could be error that the database was never loaded? Do we need to have a timeout to clear these callbacks? This came up because favicon service is somehow passing callbacks bound with favicon images. Keeping around these images is expensive.
,
Jan 29 2018
Looks like we're missing a "pending_cache_requests_.clear()" in ImageManager::ServePendingCacheRequests. Plus maybe a "shrink_to_fit()" to make sure the memory gets released. Otherwise, if some DB operation fails, database_ gets reset to null, and we should stop queueing new requests. If we are adding requests indefinitely, then it looks like OnDatabaseInit or OnDatabaseLoad never got called at all? If that's the case, then that'd be a bug in the underlying ProtoDatabase, and we should fix that rather than adding a timeout as a workaround. Yes, the callbacks should probably be OnceCallback. I think the only reason they're Repeating is that OnceCallback didn't exist when this was written. mathp: This looks like a good starter bug for the new NTP team :) so back to you for triage. If it's urgent and nobody else is available yet, then I can also take it.
,
Jan 30 2018
,
Feb 1 2018
,
Feb 12 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/68c5d492e3601bd4cf4450973c1d089ae9bcd1c3 commit 68c5d492e3601bd4cf4450973c1d089ae9bcd1c3 Author: Siddhartha <ssid@chromium.org> Date: Mon Feb 12 21:00:10 2018 Clear ImageManager request queue after serving Clear the request queue after serviing requests. Also do not queue too many requests. Sometimes opening database could take a lot of time due to disk access being slow on some devices. In these cases it is faster to serve from network. So stop queuing after 100 requests. BUG= 806397 Change-Id: Id2e8662c3ed4e731cec32e368109f5b4567820e3 Reviewed-on: https://chromium-review.googlesource.com/896585 Reviewed-by: Mathieu Perreault <mathp@chromium.org> Commit-Queue: Siddhartha S <ssid@chromium.org> Cr-Commit-Position: refs/heads/master@{#536185} [modify] https://crrev.com/68c5d492e3601bd4cf4450973c1d089ae9bcd1c3/components/suggestions/image_manager.cc [modify] https://crrev.com/68c5d492e3601bd4cf4450973c1d089ae9bcd1c3/components/suggestions/image_manager.h [modify] https://crrev.com/68c5d492e3601bd4cf4450973c1d089ae9bcd1c3/components/suggestions/image_manager_unittest.cc
,
Feb 14 2018
|
|||||
►
Sign in to add a comment |
|||||
Comment 1 by ma...@chromium.org
, Jan 27 2018