Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1453803002:
Separate RenderViewHost from RenderWidgetHost, part 10: shutdown. (Closed)
|
Created:
5 years, 1 month ago by Avi (use Gerrit) Modified:
5 years ago Reviewers:
ncarter (slow) CC:
chromium-reviews, creis+watch_chromium.org, yusukes+watch_chromium.org, shuchen+watch_chromium.org, nasko+codewatch_chromium.org, jam, sievers+watch_chromium.org, jbauman+watch_chromium.org, nona+watch_chromium.org, darin-cc_chromium.org, kalyank, piman+watch_chromium.org, danakj+watch_chromium.org, James Su, site-isolation-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSeparate RenderViewHost from RenderWidgetHost, part 10: shutdown.
BUG=542477, 404828
TEST=all green
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_chromium_asan_rel_ng
Committed: https://crrev.com/85a4cef354c4ae0f4277af6bf6be82b3326f0c55
Cr-Commit-Position: refs/heads/master@{#366156}
Patch Set 1 #
Total comments: 8
Patch Set 2 : more #Patch Set 3 : compiles #Patch Set 4 : all in destroy #Patch Set 5 : all in destroy, plus mac #
Total comments: 2
Patch Set 6 : rebase #Patch Set 7 : DCHECK #Patch Set 8 : destroy fix #Patch Set 9 : no dcheck for now #
Total comments: 4
Patch Set 10 : nick's nits #Messages
Total messages: 94 (46 generated)
The CQ bit was checked by avi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1453803002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1453803002/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
nick@chromium.org changed reviewers: + nick@chromium.org
https://codereview.chromium.org/1453803002/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/render_widget_host_impl.cc (right): https://codereview.chromium.org/1453803002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/render_widget_host_impl.cc:251: NOTIFICATION_RENDER_WIDGET_HOST_DESTROYED, Source<RenderWidgetHost>(this), We gotta worry about stuff like this: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/fil... That won't be safe after this transformation, I think, because ~RVH has already run. It might be safe (but gross) after we move to composition, since at that point it'll become possible to run the ~RWH dtor while RVH::widget_ is still a valid pointer.
https://codereview.chromium.org/1453803002/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/render_widget_host_impl.h (right): https://codereview.chromium.org/1453803002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/render_widget_host_impl.h:525: base::WeakPtr<RenderWidgetHostViewBase> view_; My understanding was that the RWH owns the RWHV, so the fact that there's a weak_ptr here is kind of suspect to me (maybe I had things wrong, or maybe there's weird platformy differences here that make ownership ambiguous). Generally speaking I don't think you should keep a weak_ptr to something you own, since it shouldn't disappear without your say-so. https://codereview.chromium.org/1453803002/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/render_widget_host_view_mac.mm (right): https://codereview.chromium.org/1453803002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/render_widget_host_view_mac.mm:61: #include "content/public/browser/notification_types.h" off topic, but can we remove these #includes? I can't find any NotificationService usage in this file. https://codereview.chromium.org/1453803002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/render_widget_host_view_mac.mm:1614: render_widget_host_->ShutdownWidget(true); So what's happening here: 1. RWHV tells the widget to delete itself (and thus the RWHV) 2. widget requests that its owner delegate delete widget 3. owner delegate (RVH) deletes the widget 4. widget dtor is supposed to deletes the attached RWHV Step 4 used to work because it kept a nonweak ptr. But the weak_factory_, above, was already invalidated, so it's not clear to me how the RWH would delete the RWHV? So, it kind of looks like we're leaking the RWHV here.
https://codereview.chromium.org/1453803002/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/render_widget_host_impl.cc (right): https://codereview.chromium.org/1453803002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/render_widget_host_impl.cc:251: NOTIFICATION_RENDER_WIDGET_HOST_DESTROYED, Source<RenderWidgetHost>(this), On 2015/11/17 20:59:57, ncarter wrote: > We gotta worry about stuff like this: > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/fil... > > That won't be safe after this transformation, I think, because ~RVH has already > run. It might be safe (but gross) after we move to composition, since at that > point it'll become possible to run the ~RWH dtor while RVH::widget_ is still a > valid pointer. Sigh. Awesomeness all around. But that should call through a null pointer, which should be easy to catch. https://codereview.chromium.org/1453803002/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/render_widget_host_impl.h (right): https://codereview.chromium.org/1453803002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/render_widget_host_impl.h:525: base::WeakPtr<RenderWidgetHostViewBase> view_; On 2015/11/17 22:22:55, ncarter wrote: > My understanding was that the RWH owns the RWHV The RWHV owns the RWHI. Note that they (being RenderWidgetHostViewAura/Mac) call Shutdown on it. This was in the days before we started using scoped_ptr to indicate ownership, but yeah, the ownership model here is very very very very wack. https://codereview.chromium.org/1453803002/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/render_widget_host_view_mac.mm (right): https://codereview.chromium.org/1453803002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/render_widget_host_view_mac.mm:61: #include "content/public/browser/notification_types.h" On 2015/11/17 22:22:55, ncarter wrote: > off topic, but can we remove these #includes? I can't find any > NotificationService usage in this file. Done. https://codereview.chromium.org/1453803002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/render_widget_host_view_mac.mm:1614: render_widget_host_->ShutdownWidget(true); On 2015/11/17 22:22:55, ncarter wrote: > So what's happening here: > > 1. RWHV tells the widget to delete itself (and thus the RWHV) > 2. widget requests that its owner delegate delete widget > 3. owner delegate (RVH) deletes the widget > 4. widget dtor is supposed to deletes the attached RWHV > > Step 4 used to work because it kept a nonweak ptr. But the weak_factory_, above, > was already invalidated, so it's not clear to me how the RWH would delete the > RWHV? > > So, it kind of looks like we're leaking the RWHV here. Lemme work on wrapping my brain around this.
The CQ bit was checked by avi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1453803002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1453803002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by avi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1453803002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1453803002/40001
Description was changed from ========== Separate RenderViewHost from RenderWidgetHost, part 10: shutdown. BUG=542477 TEST=all green ========== to ========== Separate RenderViewHost from RenderWidgetHost, part 10: shutdown. BUG=542477, 404828 TEST=all green ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)
The CQ bit was checked by avi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1453803002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1453803002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by avi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1453803002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1453803002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Nick, I'm still looking at the failures, but I'm curious if this looks better to you. https://codereview.chromium.org/1453803002/diff/80001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_impl.cc (right): https://codereview.chromium.org/1453803002/diff/80001/content/browser/rendere... content/browser/renderer_host/render_widget_host_impl.cc:252: Destroy(false); Perhaps instead DCHECK that Destroy() was called?
The CQ bit was checked by avi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1453803002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1453803002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)
I'm okay with this. https://codereview.chromium.org/1453803002/diff/80001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_impl.cc (right): https://codereview.chromium.org/1453803002/diff/80001/content/browser/rendere... content/browser/renderer_host/render_widget_host_impl.cc:252: Destroy(false); On 2015/11/20 20:10:40, Avi wrote: > Perhaps instead DCHECK that Destroy() was called? You can do the DCHECK, but you'll have to fix up at least the following unittests: https://code.google.com/p/chromium/codesearch#search/&q=scoped_ptr.*%5C%3C(Re... I think the DCHECK makes sense, because otherwise it seems we open a way for the callers to skip the Send() of the Shutdown IPC messages, and as far as I can tell we don't need to expose a "Destroy without Shutdown" method to outside this class (unless RVH will need it)?
The CQ bit was checked by avi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1453803002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1453803002/100001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by avi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1453803002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1453803002/120001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by avi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1453803002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1453803002/140001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by avi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1453803002/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1453803002/160001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)
The CQ bit was checked by avi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1453803002/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1453803002/160001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)
Description was changed from ========== Separate RenderViewHost from RenderWidgetHost, part 10: shutdown. BUG=542477, 404828 TEST=all green ========== to ========== Separate RenderViewHost from RenderWidgetHost, part 10: shutdown. BUG=542477, 404828 TEST=all green CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_chromium_asan_rel_ng,linux_chromium_msan_rel_ng ==========
The CQ bit was checked by avi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1453803002/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1453803002/160001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)
Nick, here you go. MSAN bots are crunching away, but the ASAN bots have always been happy.
lgtm! w/ the usual nits https://codereview.chromium.org/1453803002/diff/160001/content/browser/render... File content/browser/renderer_host/render_view_host_impl.h (right): https://codereview.chromium.org/1453803002/diff/160001/content/browser/render... content/browser/renderer_host/render_view_host_impl.h:108: void ShutdownAndDestroy(); This belongs after the ctor/dtor. https://codereview.chromium.org/1453803002/diff/160001/content/browser/render... File content/browser/renderer_host/render_widget_host_impl.h (right): https://codereview.chromium.org/1453803002/diff/160001/content/browser/render... content/browser/renderer_host/render_widget_host_impl.h:199: // Tells the renderer to die and optionally delete it. s/it/|this|/ or something. Otherwise it reads like we're deleting the renderer.
The CQ bit was checked by avi@chromium.org to run a CQ dry run
https://codereview.chromium.org/1453803002/diff/160001/content/browser/render... File content/browser/renderer_host/render_view_host_impl.h (right): https://codereview.chromium.org/1453803002/diff/160001/content/browser/render... content/browser/renderer_host/render_view_host_impl.h:108: void ShutdownAndDestroy(); On 2015/12/11 22:13:29, ncarter wrote: > This belongs after the ctor/dtor. Done. https://codereview.chromium.org/1453803002/diff/160001/content/browser/render... File content/browser/renderer_host/render_widget_host_impl.h (right): https://codereview.chromium.org/1453803002/diff/160001/content/browser/render... content/browser/renderer_host/render_widget_host_impl.h:199: // Tells the renderer to die and optionally delete it. On 2015/12/11 22:13:29, ncarter wrote: > s/it/|this|/ > > or something. Otherwise it reads like we're deleting the renderer. Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1453803002/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1453803002/180001
The CQ bit was unchecked by avi@chromium.org
The CQ bit was checked by avi@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from nick@chromium.org Link to the patchset: https://codereview.chromium.org/1453803002/#ps180001 (title: "nick's nits")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1453803002/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1453803002/180001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_msan_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by avi@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1453803002/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1453803002/180001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)
Description was changed from ========== Separate RenderViewHost from RenderWidgetHost, part 10: shutdown. BUG=542477, 404828 TEST=all green CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_chromium_asan_rel_ng,linux_chromium_msan_rel_ng ========== to ========== Separate RenderViewHost from RenderWidgetHost, part 10: shutdown. BUG=542477, 404828 TEST=all green CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_chromium_asan_rel_ng ==========
The CQ bit was checked by avi@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1453803002/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1453803002/180001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)
The CQ bit was checked by avi@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1453803002/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1453803002/180001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)
The CQ bit was checked by avi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1453803002/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1453803002/180001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by avi@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1453803002/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1453803002/180001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by avi@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1453803002/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1453803002/180001
Message was sent while issue was closed.
Description was changed from ========== Separate RenderViewHost from RenderWidgetHost, part 10: shutdown. BUG=542477, 404828 TEST=all green CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_chromium_asan_rel_ng ========== to ========== Separate RenderViewHost from RenderWidgetHost, part 10: shutdown. BUG=542477, 404828 TEST=all green CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_chromium_asan_rel_ng ==========
Message was sent while issue was closed.
Committed patchset #10 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== Separate RenderViewHost from RenderWidgetHost, part 10: shutdown. BUG=542477, 404828 TEST=all green CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_chromium_asan_rel_ng ========== to ========== Separate RenderViewHost from RenderWidgetHost, part 10: shutdown. BUG=542477, 404828 TEST=all green CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_chromium_asan_rel_ng Committed: https://crrev.com/85a4cef354c4ae0f4277af6bf6be82b3326f0c55 Cr-Commit-Position: refs/heads/master@{#366156} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/85a4cef354c4ae0f4277af6bf6be82b3326f0c55 Cr-Commit-Position: refs/heads/master@{#366156} |
