Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2703283005:
Destroy web_app::ShortcutInfo on UI thread (Closed)
|
Created:
3 years, 10 months ago by tzik Modified:
3 years, 9 months ago CC:
chromium-reviews, qsr+mojo_chromium.org, vmpstr+watch_chromium.org, viettrungluu+watch_chromium.org, jam, gavinp+memory_chromium.org, yzshen+watch_chromium.org, abarth-chromium, Aaron Boodman, darin-cc_chromium.org, oshima+watch_chromium.org, mac-reviews_chromium.org, piman+watch_chromium.org, cc-bugs_chromium.org, darin (slow to review), davemoore+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionDestroy web_app::ShortcutInfo on UI thread
web_app::ShortcutInfo used in cr/b/web_applications needs to be destroyed
on UI thread, because it has a reference to a ImageStorage via
gfx::ImageFamily. Since gfx::internal::ImageStorage has a non-thread-safe
ref count, created on UI thread, and shared by others on UI thread, it
needs on UI thread too on destruction.
BUG=688072
Review-Url: https://codereview.chromium.org/2703283005
Cr-Commit-Position: refs/heads/master@{#454183}
Committed: https://chromium.googlesource.com/chromium/src/+/5604ddf6347cb5023c40d7ecf4edfc2411fd6805
Patch Set 1 #Patch Set 2 : fix #Patch Set 3 : fix #Patch Set 4 : +create_exact_fix #Patch Set 5 : fix #Patch Set 6 : merge pending fixes #Patch Set 7 : rebase #Patch Set 8 : debugging #Patch Set 9 : rebase #Patch Set 10 : . #
Total comments: 4
Patch Set 11 : s/auto/web_app::ShortcutInfo/. +Unretained. #
Total comments: 11
Patch Set 12 : pointer -> const ref #Patch Set 13 : +DCHECK_CURRENTLY_ON #Patch Set 14 : fix #Patch Set 15 : +TestBrowserThreadBundle #Messages
Total messages: 71 (58 generated)
Description was changed from ========== Fixing //chrome/browser/web_applications/ BUG= ========== to ========== Fixing //chrome/browser/web_applications/ BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ==========
Description was changed from ========== Fixing //chrome/browser/web_applications/ BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== Fixing //chrome/browser/web_applications/ BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ==========
The CQ bit was checked by tzik@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by tzik@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by tzik@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by tzik@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by tzik@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by tzik@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by tzik@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by tzik@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by tzik@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
The CQ bit was checked by tzik@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Fixing //chrome/browser/web_applications/ BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== Destroy web_app::ShortcutInfo on UI thread web_app::ShortcutInfo used in cr/b/web_applications needs to be destroyed on UI thread, because it has a reference to a ImageStorage via gfx::ImageFamily. Since gfx::internal::ImageStorage has a non-thread-safe ref count, created on UI thread, and shared by others on UI thread, it needs on UI thread too on destruction. BUG= ==========
Description was changed from ========== Destroy web_app::ShortcutInfo on UI thread web_app::ShortcutInfo used in cr/b/web_applications needs to be destroyed on UI thread, because it has a reference to a ImageStorage via gfx::ImageFamily. Since gfx::internal::ImageStorage has a non-thread-safe ref count, created on UI thread, and shared by others on UI thread, it needs on UI thread too on destruction. BUG= ========== to ========== Destroy web_app::ShortcutInfo on UI thread web_app::ShortcutInfo used in cr/b/web_applications needs to be destroyed on UI thread, because it has a reference to a ImageStorage via gfx::ImageFamily. Since gfx::internal::ImageStorage has a non-thread-safe ref count, created on UI thread, and shared by others on UI thread, it needs on UI thread too on destruction. BUG=688072 ==========
tzik@chromium.org changed reviewers: + tapted@chromium.org
PTAL
tapted@chromium.org changed reviewers: + mgiuca@chromium.org
+Matt - he's been poking a lot around the thread safety side of gfx::Image and friends (and probably wrote a lot of this code :). If we go this route, we should update the comment on ShortCutInfo (see below). But i'm also thinking this would all be a lot neater, and easier to reason about, if we just made web_app::ShortcutInfo inherit from base::RefCountedThreadSafe<ShortcutInfo, content::BrowserThread::DeleteOnUIThread> private: // ShortcutInfo must not be copied; generally it is passed around via // scoped_ptrs. This is to allow passing ShortcutInfos between threads, // despite ImageFamily having a non-thread-safe reference count. DISALLOW_COPY_AND_ASSIGN(ShortcutInfo); https://codereview.chromium.org/2703283005/diff/180001/chrome/browser/web_app... File chrome/browser/web_applications/web_app.cc (right): https://codereview.chromium.org/2703283005/diff/180001/chrome/browser/web_app... chrome/browser/web_applications/web_app.cc:93: auto* shortcut_info_ptr = shortcut_info.get(); perhaps spell out what the auto type is to make this easier to reason about https://codereview.chromium.org/2703283005/diff/180001/chrome/browser/web_app... chrome/browser/web_applications/web_app.cc:97: shortcut_data_dir, old_app_title, shortcut_info_ptr), the raw pointer here feels bad.. and shouldn't it need base::Unretained? (same comments below)
The CQ bit was checked by tzik@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2703283005/diff/180001/chrome/browser/web_app... File chrome/browser/web_applications/web_app.cc (right): https://codereview.chromium.org/2703283005/diff/180001/chrome/browser/web_app... chrome/browser/web_applications/web_app.cc:93: auto* shortcut_info_ptr = shortcut_info.get(); On 2017/02/27 06:57:37, tapted wrote: > perhaps spell out what the auto type is to make this easier to reason about Done. https://codereview.chromium.org/2703283005/diff/180001/chrome/browser/web_app... chrome/browser/web_applications/web_app.cc:97: shortcut_data_dir, old_app_title, shortcut_info_ptr), On 2017/02/27 06:57:37, tapted wrote: > the raw pointer here feels bad.. and shouldn't it need base::Unretained? > > (same comments below) Added base::Unretained(). base::Bind() does enforce Unretained only when it's a receiver of a method and a raw pointer to a non-refcounted types. It does not yet to parameters.
On 2017/02/27 06:57:37, tapted wrote: > +Matt - he's been poking a lot around the thread safety side of gfx::Image and > friends (and probably wrote a lot of this code :). > > If we go this route, we should update the comment on ShortCutInfo (see below). > > But i'm also thinking this would all be a lot neater, and easier to reason > about, if we just made web_app::ShortcutInfo inherit from > base::RefCountedThreadSafe<ShortcutInfo, > content::BrowserThread::DeleteOnUIThread> > Made another CL to go that way: https://codereview.chromium.org/2721553002/ > > > > private: > // ShortcutInfo must not be copied; generally it is passed around via > // scoped_ptrs. This is to allow passing ShortcutInfos between threads, > // despite ImageFamily having a non-thread-safe reference count. > DISALLOW_COPY_AND_ASSIGN(ShortcutInfo);
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/02/27 07:58:05, tzik wrote: > On 2017/02/27 06:57:37, tapted wrote: > > +Matt - he's been poking a lot around the thread safety side of gfx::Image and > > friends (and probably wrote a lot of this code :). > > > > If we go this route, we should update the comment on ShortCutInfo (see below). > > > > But i'm also thinking this would all be a lot neater, and easier to reason > > about, if we just made web_app::ShortcutInfo inherit from > > base::RefCountedThreadSafe<ShortcutInfo, > > content::BrowserThread::DeleteOnUIThread> > > > > Made another CL to go that way: https://codereview.chromium.org/2721553002/ > > > > > > > > > private: > > // ShortcutInfo must not be copied; generally it is passed around via > > // scoped_ptrs. This is to allow passing ShortcutInfos between threads, > > // despite ImageFamily having a non-thread-safe reference count. > > DISALLOW_COPY_AND_ASSIGN(ShortcutInfo); So we're trying to decide between this and https://crrev.com/2721553002? My eyes are too tired to think critically about this now. Thanks for doing all this work. Will look tomorrow.
I very much prefer this one over the refcounted one. I think refcounting should be a last resort (if it's actually not possible to statically reason about the lifetime of an object). Here, we can. I really like what you've done here -- pass arguments by reference instead of passing ownership, then delete them on the UI thread. Much neater than impossible-to-reason-about refcounting. In fact I have a long-outstanding CL (https://crbug.com/600237) to try and convert the Image class to not be internally refcounted. It simplifies life a lot. lgtm https://codereview.chromium.org/2703283005/diff/200001/chrome/browser/web_app... File chrome/browser/web_applications/web_app.h (right): https://codereview.chromium.org/2703283005/diff/200001/chrome/browser/web_app... chrome/browser/web_applications/web_app.h:234: ShortcutInfo* shortcut_info, This should take a const ShortcutInfo&, not a pointer. (It looks like all four implementations don't need a pointer.) If there's a reason I'm not seeing, feel free to ignore. https://codereview.chromium.org/2703283005/diff/200001/chrome/browser/web_app... chrome/browser/web_applications/web_app.h:242: ShortcutInfo* shortcut_info); Same. https://codereview.chromium.org/2703283005/diff/200001/chrome/browser/web_app... chrome/browser/web_applications/web_app.h:249: ShortcutInfo* shortcut_info); Same.
lgtm with a dcheck in ~ShortcutInfo (I don't think anyone will read the comment above DISALLOW_COPY..) I obviously prefer the other CL, but I don't hate this one entirely :) https://codereview.chromium.org/2703283005/diff/200001/chrome/browser/web_app... File chrome/browser/web_applications/web_app.cc (right): https://codereview.chromium.org/2703283005/diff/200001/chrome/browser/web_app... chrome/browser/web_applications/web_app.cc:93: web_app::ShortcutInfo* shortcut_info_ptr = shortcut_info.get(); (I see this as taking an extra reference to a ShortcutInfo, but worse than refcounted - it's not counted at all :/). https://codereview.chromium.org/2703283005/diff/200001/chrome/browser/web_app... chrome/browser/web_applications/web_app.cc:179: ShortcutInfo::~ShortcutInfo() {} DCHECK_CURRENTLY_ON(BrowserThread::UI); https://codereview.chromium.org/2703283005/diff/200001/chrome/browser/web_app... File chrome/browser/web_applications/web_app.h (right): https://codereview.chromium.org/2703283005/diff/200001/chrome/browser/web_app... chrome/browser/web_applications/web_app.h:234: ShortcutInfo* shortcut_info, On 2017/03/01 08:49:55, Matt Giuca wrote: > This should take a const ShortcutInfo&, not a pointer. > > (It looks like all four implementations don't need a pointer.) > > If there's a reason I'm not seeing, feel free to ignore. Yah - I think base::Bind requires types passed to const& to be movable (or copyable) - they just get stored in a std::tuple. (but I might be wrong - if const& works then \o/)
The CQ bit was checked by tzik@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by tzik@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2703283005/diff/200001/chrome/browser/web_app... File chrome/browser/web_applications/web_app.cc (right): https://codereview.chromium.org/2703283005/diff/200001/chrome/browser/web_app... chrome/browser/web_applications/web_app.cc:179: ShortcutInfo::~ShortcutInfo() {} On 2017/03/01 10:38:57, tapted wrote: > DCHECK_CURRENTLY_ON(BrowserThread::UI); Done. https://codereview.chromium.org/2703283005/diff/200001/chrome/browser/web_app... File chrome/browser/web_applications/web_app.h (right): https://codereview.chromium.org/2703283005/diff/200001/chrome/browser/web_app... chrome/browser/web_applications/web_app.h:234: ShortcutInfo* shortcut_info, On 2017/03/01 10:38:57, tapted wrote: > On 2017/03/01 08:49:55, Matt Giuca wrote: > > This should take a const ShortcutInfo&, not a pointer. > > > > (It looks like all four implementations don't need a pointer.) > > > > If there's a reason I'm not seeing, feel free to ignore. > > Yah - I think base::Bind requires types passed to const& to be movable (or > copyable) - they just get stored in a std::tuple. (but I might be wrong - if > const& works then \o/) It's not common, but we have base::ConstRef() wrapper to do that. Updated. https://codereview.chromium.org/2703283005/diff/200001/chrome/browser/web_app... chrome/browser/web_applications/web_app.h:242: ShortcutInfo* shortcut_info); On 2017/03/01 08:49:55, Matt Giuca wrote: > Same. Done. https://codereview.chromium.org/2703283005/diff/200001/chrome/browser/web_app... chrome/browser/web_applications/web_app.h:249: ShortcutInfo* shortcut_info); On 2017/03/01 08:49:55, Matt Giuca wrote: > Same. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by tzik@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tapted@chromium.org, mgiuca@chromium.org Link to the patchset: https://codereview.chromium.org/2703283005/#ps260001 (title: "fix")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
This lgtm with the const ShortcutInfo& version and the DCHECK. It looks *very* nice to have those methods taking a simple ref instead of taking ownership of the ShortcutInfo just because they happen to be the last thing to use it. https://codereview.chromium.org/2703283005/diff/200001/chrome/browser/web_app... File chrome/browser/web_applications/web_app.cc (right): https://codereview.chromium.org/2703283005/diff/200001/chrome/browser/web_app... chrome/browser/web_applications/web_app.cc:93: web_app::ShortcutInfo* shortcut_info_ptr = shortcut_info.get(); On 2017/03/01 10:38:57, tapted wrote: > (I see this as taking an extra reference to a ShortcutInfo, but worse than > refcounted - it's not counted at all :/). The difference is that this code (you can verify by reading this and nothing else) guarantees that |shortcut_info| outlives the pointer passed to UpdatePlatformShortcuts. Really this is no different to having a unique_ptr<X>, and passing it to a function that expects a const X&; sure there's temporarily an extra reference but there are static guarantees about the lifetime. Whereas if we used a refcount then who knows whether UpdatePlatformShortcuts would hold onto the reference or not. (With the const X* version, there's a chance of miscommunication where the receiver holds onto the pointer for longer than they're supposed to, but the const X& version is very clear that you aren't supposed to do that.)
The CQ bit was checked by tzik@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by tzik@chromium.org
The CQ bit was checked by tzik@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tapted@chromium.org, mgiuca@chromium.org Link to the patchset: https://codereview.chromium.org/2703283005/#ps280001 (title: "+TestBrowserThreadBundle")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 280001, "attempt_start_ts": 1488428133725190,
"parent_rev": "b4b64c9259ebd0deceb1ddee451b93e5f7f698a0", "commit_rev":
"5604ddf6347cb5023c40d7ecf4edfc2411fd6805"}
Message was sent while issue was closed.
Description was changed from ========== Destroy web_app::ShortcutInfo on UI thread web_app::ShortcutInfo used in cr/b/web_applications needs to be destroyed on UI thread, because it has a reference to a ImageStorage via gfx::ImageFamily. Since gfx::internal::ImageStorage has a non-thread-safe ref count, created on UI thread, and shared by others on UI thread, it needs on UI thread too on destruction. BUG=688072 ========== to ========== Destroy web_app::ShortcutInfo on UI thread web_app::ShortcutInfo used in cr/b/web_applications needs to be destroyed on UI thread, because it has a reference to a ImageStorage via gfx::ImageFamily. Since gfx::internal::ImageStorage has a non-thread-safe ref count, created on UI thread, and shared by others on UI thread, it needs on UI thread too on destruction. BUG=688072 Review-Url: https://codereview.chromium.org/2703283005 Cr-Commit-Position: refs/heads/master@{#454183} Committed: https://chromium.googlesource.com/chromium/src/+/5604ddf6347cb5023c40d7ecf4ed... ==========
Message was sent while issue was closed.
Committed patchset #15 (id:280001) as https://chromium.googlesource.com/chromium/src/+/5604ddf6347cb5023c40d7ecf4ed... |
