Make WebContentsImpl aware of all of its RWHs.

content/browser/web_contents/web_contents_impl.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "content/browser/web_contents/web_contents_impl.h"
 
 #include <utility>
 
 #include "base/command_line.h"
 #include "base/lazy_instance.h"
@@ skipping 410 matching lines @@
 
   // Delete all RFH pending shutdown, which will lead the corresponding RVH to
   // shutdown and be deleted as well.
   frame_tree_.ForEach(
       base::Bind(&RenderFrameHostManager::ClearRFHsPendingShutdown));
 
   ClearAllPowerSaveBlockers();
 
   for (std::set<RenderWidgetHostImpl*>::iterator iter =
            created_widgets_.begin(); iter != created_widgets_.end(); ++iter) {
     (*iter)->DetachDelegate();

[lazyboy, 2015/11/20 23:52:05]

not sure if this is an issue, but:
Technically the delegate can call back to WebContentsImpl and cause
badness if that code tries to access |created_widgets_|.
Should we consider clearing created_widgets_ first after copying it and
start calling DetachDelegate() on that copy of the set?

[Charlie Reis, 2015/11/21 01:19:19]

I don't follow your concern here.  DetachDelegate is a method on RWHI that just
sets the delegate_ field to null.  It doesn't reach back into WebContents, and
we haven't started deleting created_widgets_ yet anyway.

Are you referring to something else?

[lazyboy, 2015/11/21 02:22:59]

Sorry for not being precise, this is just a hypothetical scenario I was talking
about: *if* RWHI::DetachDelegate() would have called into WCI (assuming someone
later on adds code to do that) and tried to access |created_widgets_|, that
would
become a problem.
This is probably a non-issue here.

   }
   created_widgets_.clear();
 
   // Clear out any JavaScript state.
   if (dialog_manager_)
     dialog_manager_->ResetDialogState(this);
 
   if (color_chooser_info_.get())
     color_chooser_info_->chooser->End();
 
@@ skipping 844 matching lines @@
                     observers_,
                     DidCloneToNewWebContents(this, tc));
   return tc;
 }
 
 void WebContentsImpl::Observe(int type,
                               const NotificationSource& source,
                               const NotificationDetails& details) {
   switch (type) {
     case NOTIFICATION_RENDER_WIDGET_HOST_DESTROYED: {
-      RenderWidgetHost* host = Source<RenderWidgetHost>(source).ptr();
+      RenderWidgetHostImpl* host = Source<RenderWidgetHostImpl>(source).ptr();
+
+      std::set<RenderWidgetHostImpl*>::iterator iter =
+          created_widgets_.find(host);
+      if (iter != created_widgets_.end())
+        created_widgets_.erase(iter);

[Charlie Reis, 2015/11/20 18:41:41]

Why is this part necessary?  Won't we take care of it in RenderWidgetDeleted?

(If not, let's add a comment why it needs to be removed in both places.)

[Avi (use Gerrit), 2015/11/20 19:06:30]

Side comment: Why do you check to see if the widget is in the set before erasing
it? Just blindly call erase; it works fine and is simpler to reason about.

[lazyboy, 2015/11/20 23:52:05]

Thanks I've updated the relevant places.
(Side effect of copy-paste).

[lazyboy, 2015/11/20 23:52:05]

Yes it would have, however there's a WCI::is_being_destroyed_ bailout
at the very first line which misses exactly the case I'm trying to 
fix.
Thinking of this, I'm not sure if comment on line 1508 is true:
"// |created_widgets_| might have been destroyed."
The original CL that introduced it doesn't seem to have any reasoning
about this. See line 1173 on the ride side:
https://codereview.chromium.org/10968037/diff/10005/content/browser/web_conte...

I've gone ahead and merged this and removed the !is_being_destroyed_ requirement
while deleting RWHs from |created_widgets_|. Fingers crossed.

[Charlie Reis, 2015/11/21 01:19:19]

Hmm, that CL didn't add any tests to prevent regression, so it's hard to tell
from the try jobs if you've reintroduced a bug.  Did you try the repro steps
from the bug?

https://code.google.com/p/chromium/issues/detail?id=149821
1. pick any youtube video and play it
2. go fullscreen
3. press ctrl+w


At any rate, I don't see an immediate problem with your change.  If the
WebContents is being destroyed, all of its remaining widgets will have their
delegate_ cleared before created_widgets_ is destroyed, so they won't be able to
call back into RenderWidgetDeleted and access it.  So it seems ok to me.

[lazyboy, 2015/11/21 02:22:59]

Yes, I've tried with and without flash fullscreen. Seemed fine.

+
       RenderWidgetHostView* view = host->GetView();
       if (view == GetFullscreenRenderWidgetHostView()) {
         // We cannot just call view_->RestoreFocus() here.  On some platforms,
         // attempting to focus the currently-invisible WebContentsView will be
         // flat-out ignored.  Therefore, this boolean is used to track whether
         // we will request focus after the fullscreen widget has been
         // destroyed.
         fullscreen_widget_had_focus_at_shutdown_ = (view && view->HasFocus());
       } else {
         for (PendingWidgetViews::iterator i = pending_widget_views_.begin();
@@ skipping 174 matching lines @@
 }
 
 void WebContentsImpl::LostCapture(RenderWidgetHostImpl* render_widget_host) {
   if (!RenderViewHostImpl::From(render_widget_host))
     return;
 
   if (delegate_)
     delegate_->LostCapture();
 }
 
+void WebContentsImpl::RenderWidgetCreated(
+    RenderWidgetHostImpl* render_widget_host) {
+  std::set<RenderWidgetHostImpl*>::iterator iter =
+      created_widgets_.find(render_widget_host);

[Avi (use Gerrit), 2015/11/20 19:06:30]

You totally can use "auto" here.

[lazyboy, 2015/11/20 23:52:05]

Not required anymore.

+  if (iter == created_widgets_.end())
+    created_widgets_.insert(render_widget_host);

[Avi (use Gerrit), 2015/11/20 19:06:30]

Same thing; you should be able to blindly insert.

[lazyboy, 2015/11/20 23:52:05]

Done.

+}
+
 void WebContentsImpl::RenderWidgetDeleted(
     RenderWidgetHostImpl* render_widget_host) {
   if (is_being_destroyed_) {
     // |created_widgets_| might have been destroyed.
     return;
   }
 
   std::set<RenderWidgetHostImpl*>::iterator iter =
       created_widgets_.find(render_widget_host);
   if (iter != created_widgets_.end())
     created_widgets_.erase(iter);

[Avi (use Gerrit), 2015/11/20 19:06:30]

... same here, though it's not your code.

[lazyboy, 2015/11/20 23:52:05]

Done.

 
   if (render_widget_host &&
       render_widget_host->GetRoutingID() == fullscreen_widget_routing_id_) {
     if (delegate_ && delegate_->EmbedsFullscreenWidget())
       delegate_->ExitFullscreenModeForTab(this);
     FOR_EACH_OBSERVER(WebContentsObserver,
                       observers_,
                       DidDestroyFullscreenWidget(
                           fullscreen_widget_routing_id_));
     fullscreen_widget_routing_id_ = MSG_ROUTING_NONE;
@@ skipping 389 matching lines @@
     if (process_handle != base::kNullProcessHandle) {
       RecordAction(
           base::UserMetricsAction("Terminate_ProcessMismatch_CreateNewWidget"));
       rph->Shutdown(RESULT_CODE_KILLED, false);
     }
     return;
   }
 
   RenderWidgetHostImpl* widget_host =
       new RenderWidgetHostImpl(this, process, route_id, IsHidden());
-  created_widgets_.insert(widget_host);
 
   RenderWidgetHostViewBase* widget_view =
       static_cast<RenderWidgetHostViewBase*>(
           view_->CreateViewForPopupWidget(widget_host));
   if (!widget_view)
     return;
   if (!is_fullscreen) {
     // Popups should not get activated.
     widget_view->SetPopupType(popup_type);
   }
@@ skipping 2852 matching lines @@
   return NULL;
 }
 
 void WebContentsImpl::SetForceDisableOverscrollContent(bool force_disable) {
   force_disable_overscroll_content_ = force_disable;
   if (view_)
     view_->SetOverscrollControllerEnabled(CanOverscrollContent());
 }
 
 }  // namespace content