Make WebContentsImpl aware of all of its RWHs.

content/browser/web_contents/web_contents_impl.cc

Index: content/browser/web_contents/web_contents_impl.cc
diff --git a/content/browser/web_contents/web_contents_impl.cc b/content/browser/web_contents/web_contents_impl.cc
index 878fdb7be889bc4a7f3dfcf32129d4eadbe1b142..6448c0c617b0124273c27b29a6f9e020e6397cff 100644
--- a/content/browser/web_contents/web_contents_impl.cc
+++ b/content/browser/web_contents/web_contents_impl.cc
@@ -1293,7 +1293,13 @@ void WebContentsImpl::Observe(int type,
                               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,
@@ -1488,6 +1494,14 @@ void WebContentsImpl::LostCapture(RenderWidgetHostImpl* render_widget_host) {
     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_) {
@@ -1908,7 +1922,6 @@ void WebContentsImpl::CreateNewWidget(int32 render_process_id,
 
   RenderWidgetHostImpl* widget_host =
       new RenderWidgetHostImpl(this, process, route_id, IsHidden());
-  created_widgets_.insert(widget_host);
 
   RenderWidgetHostViewBase* widget_view =
       static_cast<RenderWidgetHostViewBase*>(