Memory leaks during re-creating NaCl modules

This patch fixes one of three memory leaks found when page
reloads NaCl module(s).

In mentioned scenario HTMLPlugin element is added to LocalFrameView
in LoadPlugin method, however there is no way to remove plugin from
LocalFrameView.

Bug: 922925
Change-Id: I1bf01f417f24bcbd82d5289a2138e3a69c31109f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1680175
Commit-Queue: Nate Chapin <japhet@chromium.org>
Reviewed-by: Nate Chapin <japhet@chromium.org>
Cr-Commit-Position: refs/heads/master@{#690776}

third_party/blink/renderer/core/BUILD.gn

@@ -1259,6 +1259,7 @@ jumbo_source_set("unit_tests") {
     "html/html_link_element_test.cc",
     "html/html_meta_element_test.cc",
     "html/html_object_element_test.cc",
+    "html/html_plugin_element_test.cc",
     "html/html_slot_element_test.cc",
     "html/html_table_row_element_test.cc",
     "html/image_document_test.cc",

third_party/blink/renderer/core/frame/local_frame_view.cc

@@ -3455,6 +3455,11 @@ void LocalFrameView::AddPlugin(WebPluginContainerImpl* plugin) {
   plugins_.insert(plugin);
 }
 
+void LocalFrameView::RemovePlugin(WebPluginContainerImpl* plugin) {
+  DCHECK(plugins_.Contains(plugin));
+  plugins_.erase(plugin);
+}
+
 void LocalFrameView::RemoveScrollbar(Scrollbar* scrollbar) {
   DCHECK(scrollbars_.Contains(scrollbar));
   scrollbars_.erase(scrollbar);

third_party/blink/renderer/core/frame/local_frame_view.h

@@ -492,6 +492,7 @@ class CORE_EXPORT LocalFrameView final
   using PluginSet = HeapHashSet<Member<WebPluginContainerImpl>>;
   const PluginSet& Plugins() const { return plugins_; }
   void AddPlugin(WebPluginContainerImpl*);
+  void RemovePlugin(WebPluginContainerImpl*);
   // Custom scrollbars in PaintLayerScrollableArea need to be called with
   // StyleChanged whenever window focus is changed.
   void RemoveScrollbar(Scrollbar*);

third_party/blink/renderer/core/html/html_plugin_element.cc

@@ -312,6 +312,7 @@ void HTMLPlugInElement::DetachLayoutTree(bool performing_reattach) {
   if (!performing_reattach)
     SetDisposeView();
 
+  RemovePluginFromFrameView(plugin);
   ResetInstance();
 
   HTMLFrameOwnerElement::DetachLayoutTree(performing_reattach);
@@ -626,7 +627,9 @@ bool HTMLPlugInElement::LoadPlugin(const KURL& url,
   loaded_url_ = url;
 
   if (persisted_plugin_) {
+    auto* plugin = persisted_plugin_.Get();
     SetEmbeddedContentView(persisted_plugin_.Release());
+    layout_object->GetFrameView()->AddPlugin(plugin);
   } else {
     bool load_manually =
         GetDocument().IsPluginDocument() && !GetDocument().ContainsPlugins();
@@ -634,7 +637,7 @@ bool HTMLPlugInElement::LoadPlugin(const KURL& url,
         *this, url, plugin_params.Names(), plugin_params.Values(), mime_type,
         load_manually);
     if (!plugin) {
-      if (layout_object && !layout_object->ShowsUnavailablePluginIndicator()) {
+      if (!layout_object->ShowsUnavailablePluginIndicator()) {
         plugin_is_available_ = false;
         layout_object->SetPluginAvailability(
             LayoutEmbeddedObject::kPluginMissing);
@@ -642,12 +645,8 @@ bool HTMLPlugInElement::LoadPlugin(const KURL& url,
       return false;
     }
 
-    if (layout_object) {
-      SetEmbeddedContentView(plugin);
-      layout_object->GetFrameView()->AddPlugin(plugin);
-    } else {
-      SetPersistedPlugin(plugin);
-    }
+    SetEmbeddedContentView(plugin);
+    layout_object->GetFrameView()->AddPlugin(plugin);
   }
 
   GetDocument().SetContainsPlugins();
@@ -722,6 +721,25 @@ bool HTMLPlugInElement::AllowedToLoadPlugin(const KURL& url,
   return true;
 }
 
+void HTMLPlugInElement::RemovePluginFromFrameView(
+    WebPluginContainerImpl* plugin) {
+  if (!plugin)
+    return;
+
+  auto* layout_object = GetLayoutEmbeddedObject();
+  if (!layout_object)
+    return;
+
+  auto* frame_view = layout_object->GetFrameView();
+  if (!frame_view)
+    return;
+
+  if (!frame_view->Plugins().Contains(plugin))
+    return;
+
+  frame_view->RemovePlugin(plugin);
+}
+
 void HTMLPlugInElement::DidAddUserAgentShadowRoot(ShadowRoot&) {
   UserAgentShadowRoot()->AppendChild(
       HTMLSlotElement::CreateUserAgentDefaultSlot(GetDocument()));

third_party/blink/renderer/core/html/html_plugin_element.h

@@ -200,6 +200,7 @@ class CORE_EXPORT HTMLPlugInElement
   bool AllowedToLoadPlugin(const KURL&, const String& mime_type);
   // Perform checks based on the URL and MIME-type of the object to load.
   bool AllowedToLoadObject(const KURL&, const String& mime_type);
+  void RemovePluginFromFrameView(WebPluginContainerImpl* plugin);
 
   enum class ObjectContentType {
     kNone,

third_party/blink/renderer/core/html/html_plugin_element_test.cc

+// Copyright 2019 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 "third_party/blink/renderer/core/html/html_plugin_element.h"
+
+#include "testing/gtest/include/gtest/gtest.h"
+#include "third_party/blink/public/web/web_plugin_params.h"
+#include "third_party/blink/renderer/core/dom/document.h"
+#include "third_party/blink/renderer/core/exported/web_plugin_container_impl.h"
+#include "third_party/blink/renderer/core/frame/settings.h"
+#include "third_party/blink/renderer/core/loader/empty_clients.h"
+#include "third_party/blink/renderer/core/testing/fake_web_plugin.h"
+#include "third_party/blink/renderer/core/testing/page_test_base.h"
+#include "third_party/blink/renderer/platform/wtf/text/wtf_string.h"
+
+namespace blink {
+
+namespace {
+
+class TestPluginLocalFrameClient : public EmptyLocalFrameClient {
+ public:
+  TestPluginLocalFrameClient() = default;
+
+  int plugin_created_count() const { return plugin_created_count_; }
+
+ private:
+  std::unique_ptr<WebURLLoaderFactory> CreateURLLoaderFactory() override {
+    return Platform::Current()->CreateDefaultURLLoaderFactory();
+  }
+
+  WebPluginContainerImpl* CreatePlugin(HTMLPlugInElement& element,
+                                       const KURL& url,
+                                       const Vector<String>& param_names,
+                                       const Vector<String>& param_values,
+                                       const String& mime_type,
+                                       bool load_manually) override {
+    ++plugin_created_count_;
+
+    // Based on LocalFrameClientImpl::CreatePlugin
+    WebPluginParams params;
+    params.url = url;
+    params.mime_type = mime_type;
+    params.attribute_names = param_names;
+    params.attribute_values = param_values;
+    params.load_manually = load_manually;
+
+    WebPlugin* web_plugin = new FakeWebPlugin(params);
+    if (!web_plugin)
+      return nullptr;
+
+    // The container takes ownership of the WebPlugin.
+    auto* container =
+        MakeGarbageCollected<WebPluginContainerImpl>(element, web_plugin);
+
+    if (!web_plugin->Initialize(container))
+      return nullptr;
+
+    if (!element.GetLayoutObject())
+      return nullptr;
+
+    return container;
+  }
+
+  int plugin_created_count_ = 0;
+};
+
+}  // namespace
+
+class HTMLPlugInElementTest : public PageTestBase,
+                              public testing::WithParamInterface<const char*> {
+ protected:
+  void SetUp() final {
+    frame_client_ = MakeGarbageCollected<TestPluginLocalFrameClient>();
+    PageTestBase::SetupPageWithClients(nullptr, frame_client_, nullptr);
+    GetFrame().GetSettings()->SetPluginsEnabled(true);
+  }
+
+  void TearDown() final {
+    PageTestBase::TearDown();
+    frame_client_ = nullptr;
+  }
+
+  LocalFrameView& GetFrameView() const {
+    return GetDummyPageHolder().GetFrameView();
+  }
+
+  int plugin_created_count() const {
+    return frame_client_->plugin_created_count();
+  }
+
+ private:
+  Persistent<TestPluginLocalFrameClient> frame_client_;
+};
+
+INSTANTIATE_TEST_SUITE_P(,
+                         HTMLPlugInElementTest,
+                         testing::Values("embed", "object"));
+
+TEST_P(HTMLPlugInElementTest, RemovePlugin) {
+  constexpr char kDivWithPlugin[] = R"HTML(
+    <div>
+      <%s id='test_plugin'
+          type='application/x-test-plugin'
+          src='test_plugin'>
+      </%s>
+    </div>
+  )HTML";
+
+  const char* container_type = GetParam();
+  GetDocument().body()->SetInnerHTMLFromString(
+      String::Format(kDivWithPlugin, container_type, container_type));
+
+  auto* plugin =
+      ToHTMLPlugInElement(GetDocument().getElementById("test_plugin"));
+  ASSERT_TRUE(plugin);
+  EXPECT_EQ(container_type, plugin->tagName().LowerASCII());
+
+  UpdateAllLifecyclePhasesForTest();
+  plugin->UpdatePlugin();
+
+  EXPECT_EQ(1, plugin_created_count());
+
+  auto* owned_plugin = plugin->OwnedPlugin();
+  ASSERT_TRUE(owned_plugin);
+
+  EXPECT_EQ(1u, GetFrameView().Plugins().size());
+  ASSERT_TRUE(GetFrameView().Plugins().Contains(owned_plugin));
+
+  plugin->parentNode()->removeChild(plugin);
+  EXPECT_FALSE(GetDocument().HasElementWithId("test_plugin"));
+
+  UpdateAllLifecyclePhasesForTest();
+  EXPECT_EQ(0u, GetFrameView().Plugins().size());
+  EXPECT_FALSE(GetFrameView().Plugins().Contains(owned_plugin));
+}
+
+}  // namespace blink