Dispose a persisted plugin widget if the content frame is detached.

third_party/WebKit/Source/core/html/HTMLPlugInElement.cpp

Index: third_party/WebKit/Source/core/html/HTMLPlugInElement.cpp
diff --git a/third_party/WebKit/Source/core/html/HTMLPlugInElement.cpp b/third_party/WebKit/Source/core/html/HTMLPlugInElement.cpp
index bb47c4e162da67daeeaabbc67f02d996527885c7..60960f26f49e1622bcd11ceda71c90c85bbf567e 100644
--- a/third_party/WebKit/Source/core/html/HTMLPlugInElement.cpp
+++ b/third_party/WebKit/Source/core/html/HTMLPlugInElement.cpp
@@ -209,6 +209,8 @@ void HTMLPlugInElement::removedFrom(ContainerNode* insertionPoint) {
   // If we've persisted the plugin and we're removed from the tree then
   // make sure we cleanup the persistance pointer.
   if (m_persistedPluginWidget) {
+    // TODO(dcheng): This UpdateSuspendScope doesn't seem to provide much;

[sof, 2016/12/17 15:01:53]

Given that the persisted-widget detach happens slightly earlier now, is this
override still needed?

[dcheng, 2016/12/17 20:20:02]

For now, yes. If I understand this correctly, PluginViews won't be detached by
disconnectContentFrame(). My long-term plan is to move  widget disposal into
ChildFrameDisconnector, which will allow this to be removed. I also plan on
making plugin loads respect SubframeLoadingDisabler.

The eventual goal is to remove deferred widget updates altogether, and change
UpdateSuspendScope to AssertNoWidgetUpdatesScope. However, I'm still trying to
understand FrameView::updateWidgets, which (usually) run in a timer but can
occasionally run synchronously when post layout tasks are flushed. I'm hoping
that in all cases where we run updateWidgets, we don't need to suspend widget
updates... but I'm not actually sure how this will work out in practice...

[sof, 2016/12/17 21:47:28]

WebPluginContainerImpl won't separately call removedFrom(), as far as i know.

[dcheng, 2016/12/18 06:40:38]

WebPluginContainerImpl won't, but things like ContainerNode::removeChild will
detach the layout tree (which may persist a widget), and then notify the Node
that it was removed (which is when we clear any persisted widget). This path is
not covered by frame disconnection.

+    // investigate removing it.
     HTMLFrameOwnerElement::UpdateSuspendScope suspendWidgetHierarchyUpdates;
     setPersistedPluginWidget(nullptr);
   }
@@ -422,6 +424,11 @@ bool HTMLPlugInElement::isPluginElement() const {
   return true;
 }
 
+void HTMLPlugInElement::disconnectContentFrame() {
+  HTMLFrameOwnerElement::disconnectContentFrame();
+  setPersistedPluginWidget(nullptr);

[sof, 2016/12/17 15:01:53]

If there is a reason for this (unexpected) ordering (to avoid an
UpdateSuspendScope..?), could you document it?

[dcheng, 2016/12/17 20:20:02]

I /think/ that removedFrom() isn't allowed to run script, while
disconnectContentFrame() is. Given that, it seems safer to clear the persisted
widget afterwards. I'll update this comment when I move widget disposal into
ChildFrameDisconnector.

+}
+
 bool HTMLPlugInElement::layoutObjectIsFocusable() const {
   if (HTMLFrameOwnerElement::supportsFocus() &&
       HTMLFrameOwnerElement::layoutObjectIsFocusable())