Relanding(r111754): HTMLPluginElement is not destroyed on reload or navigation if...
authorcommit-queue@webkit.org <commit-queue@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 23 Mar 2012 20:00:10 +0000 (20:00 +0000)
committercommit-queue@webkit.org <commit-queue@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 23 Mar 2012 20:00:10 +0000 (20:00 +0000)
https://bugs.webkit.org/show_bug.cgi?id=80428

.:

Patch by Dave Michael <dmichael@chromium.org> on 2012-03-23

Source/WebCore:

Patch by Dave Michael <dmichael@chromium.org> on 2012-03-23
Reviewed by Eric Seidel and Ryosuke Niwa.

Make HTMLPluginElement release its m_NPObject in detach() to break a
reference-counting cycle that happens on reload or navigation. With this
change, HTMLPlugInElement::removedFromDocument is unnecessary, so it
was removed. Note that Releasing m_NPObject does not result in a call to
the plugin; it simply releases a reference count on the wrapper object
for this HTMLPlugInElement. (The plugin's NPP_Deallocate is invoked
when the render tree is destroyed, when PluginView calls
PluginPackage::unload.) Thus, it is safe to release m_NPObject in
detach, because it can not result in layout or style changes.

Also added numberOfLiveNodes() and numberOfLiveDocuments() to
window.internals to enable testing.

Test: plugins/netscape-dom-access-and-reload.html

* WebCore.exp.in:
* html/HTMLPlugInElement.cpp:
(WebCore::HTMLPlugInElement::detach):
* html/HTMLPlugInElement.h:
(HTMLPlugInElement):
* testing/Internals.cpp:
(WebCore):
(WebCore::Internals::numberOfLiveNodes):
(WebCore::Internals::numberOfLiveDocuments):
* testing/Internals.h:
(Internals):
* testing/Internals.idl:

Source/WebKit2:

Patch by Dave Michael <dmichael@chromium.org> on 2012-03-23
Reviewed by Eric Seidel and Ryosuke Niwa.

* win/WebKit2.def: Export a symbol for InspectorCounters::counterValue
* win/WebKit2CFLite.def: Export a symbol for InspectorCounters::counterValue

LayoutTests:

Patch by Dave Michael <dmichael@chromium.org> on 2012-03-23
Reviewed by Eric Seidel and Ryosuke Niwa.

Due to unfortunate copy/paste laziness, the new test was using the same
window.sessionStorage as plugins/reloadplugins-and-pages.html, so that
if the tests were run in the same session, reloadplugins-and-pages.html
would *not* reload as it was supposed to, causing a text mismatch. This
patch uses a more appropriate and unique name so that these two tests
won't affect each other.

* plugins/netscape-dom-access-and-reload-expected.txt: Added.
* plugins/netscape-dom-access-and-reload.html: Added.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@111890 268f45cc-cd09-0410-ab3c-d52691b4dbfc

15 files changed:
ChangeLog
LayoutTests/ChangeLog
LayoutTests/plugins/netscape-dom-access-and-reload-expected.txt [new file with mode: 0644]
LayoutTests/plugins/netscape-dom-access-and-reload.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/WebCore.exp.in
Source/WebCore/html/HTMLPlugInElement.cpp
Source/WebCore/html/HTMLPlugInElement.h
Source/WebCore/testing/Internals.cpp
Source/WebCore/testing/Internals.h
Source/WebCore/testing/Internals.idl
Source/WebKit2/ChangeLog
Source/WebKit2/win/WebKit2.def
Source/WebKit2/win/WebKit2CFLite.def
Source/autotools/symbols.filter

index 0b945d4..b83c230 100644 (file)
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,14 @@
+2012-03-23  Dave Michael  <dmichael@chromium.org>
+
+        Relanding(r111754): HTMLPluginElement is not destroyed on reload or navigation if getNPObject is called
+        https://bugs.webkit.org/show_bug.cgi?id=80428
+
+Reviewed by Eric Seidel and Ryosuke Niwa.
+
+        Test: plugins/netscape-dom-access-and-reload.html
+
+        * Source/autotools/symbols.filter: Export a symbol for InspectorCounters::counterValue.
+
 2012-03-23  Ryosuke Niwa  <rniwa@webkit.org>
 
         REGRESSION(r111754): plugins/reloadplugins-and-pages.html fails on all platforms
index 7b3cca6..53eeeef 100644 (file)
@@ -1,3 +1,20 @@
+2012-03-23  Dave Michael  <dmichael@chromium.org>
+
+        Relanding(r111754): HTMLPluginElement is not destroyed on reload or navigation if getNPObject is called
+        https://bugs.webkit.org/show_bug.cgi?id=80428
+
+        Reviewed by Eric Seidel and Ryosuke Niwa.
+
+        Due to unfortunate copy/paste laziness, the new test was using the same
+        window.sessionStorage as plugins/reloadplugins-and-pages.html, so that
+        if the tests were run in the same session, reloadplugins-and-pages.html
+        would *not* reload as it was supposed to, causing a text mismatch. This
+        patch uses a more appropriate and unique name so that these two tests
+        won't affect each other.
+
+        * plugins/netscape-dom-access-and-reload-expected.txt: Added.
+        * plugins/netscape-dom-access-and-reload.html: Added.
+
 2012-03-23  Ryosuke Niwa  <rniwa@webkit.org>
 
         CSSParser doesn't set border-*-width/style/color to initial by border shorthand property
diff --git a/LayoutTests/plugins/netscape-dom-access-and-reload-expected.txt b/LayoutTests/plugins/netscape-dom-access-and-reload-expected.txt
new file mode 100644 (file)
index 0000000..55e7598
--- /dev/null
@@ -0,0 +1,3 @@
+This page tests reloading a Netscape plug-in that accesses its own DOM element. See https://bugs.webkit.org/show_bug.cgi?id=80428, "HTMLPluginElement is not destroyed on reload or navigation if getNPObject is called". If it succeeds, you should see SUCCESS below. 
+
+SUCCESS
diff --git a/LayoutTests/plugins/netscape-dom-access-and-reload.html b/LayoutTests/plugins/netscape-dom-access-and-reload.html
new file mode 100644 (file)
index 0000000..db95654
--- /dev/null
@@ -0,0 +1,55 @@
+<!DOCTYPE html>
+<html>
+<head>
+<script>
+function runTest()
+{
+    // Make the plugin retrieve a DOM element to itself. This exercises
+    // https://bugs.webkit.org/show_bug.cgi?id=80428
+    document.getElementById("testPlugin").testDOMAccess();
+
+    var callReload = true;
+    if (window.sessionStorage) {
+        if (window.sessionStorage.netscapeDomAccessAndReloadHasReloaded)
+            callReload = false;
+        else
+            window.sessionStorage.netscapeDomAccessAndReloadHasReloaded = 1;
+    }
+
+    if (window.layoutTestController)
+        layoutTestController.dumpAsText();
+    
+    if (callReload) {
+        if (window.layoutTestController)
+            layoutTestController.waitUntilDone();
+        location.reload();
+    } else {
+        window.GCController.collect();
+        // Note we could also collect the number of live nodes, but that seems
+        // more likely to give spurious failures. When an HTMLPluginElement is
+        // improperly retained, its owning Document also survives, so we'll
+        // detect an excess Document.
+        if (internals && internals.numberOfLiveDocuments) {
+            var numberOfLiveDocuments = internals.numberOfLiveDocuments();
+            if (numberOfLiveDocuments == 1) {
+                document.getElementById("result").innerHTML = "SUCCESS";
+            }
+        } else {
+              document.getElementById("result").innerHTML = "FAILED; This test is only valid in DumpRenderTree, and only when the Inspector is enabled.";
+        }
+        if (window.layoutTestController) {
+            layoutTestController.dumpAsText();
+            layoutTestController.notifyDone();
+        }
+    }
+}
+</script>
+</head>
+<body onload="runTest();">
+<p>This page tests reloading a Netscape plug-in that accesses its own DOM element. See https://bugs.webkit.org/show_bug.cgi?id=80428, "HTMLPluginElement is not destroyed on reload or navigation if getNPObject is called".
+
+If it succeeds, you should see SUCCESS below.
+<embed id="testPlugin" type="application/x-webkit-test-netscape" width="200" height="200"></embed>
+<div id="result">FAILURE</div>
+</body>
+</html>
index 27e7b5e..cb5327b 100644 (file)
@@ -1,3 +1,38 @@
+2012-03-23  Dave Michael  <dmichael@chromium.org>
+
+        Relanding(r111754): HTMLPluginElement is not destroyed on reload or navigation if getNPObject is called
+        https://bugs.webkit.org/show_bug.cgi?id=80428
+
+        Reviewed by Eric Seidel and Ryosuke Niwa.
+
+        Make HTMLPluginElement release its m_NPObject in detach() to break a
+        reference-counting cycle that happens on reload or navigation. With this
+        change, HTMLPlugInElement::removedFromDocument is unnecessary, so it
+        was removed. Note that Releasing m_NPObject does not result in a call to
+        the plugin; it simply releases a reference count on the wrapper object
+        for this HTMLPlugInElement. (The plugin's NPP_Deallocate is invoked
+        when the render tree is destroyed, when PluginView calls
+        PluginPackage::unload.) Thus, it is safe to release m_NPObject in
+        detach, because it can not result in layout or style changes.
+
+        Also added numberOfLiveNodes() and numberOfLiveDocuments() to
+        window.internals to enable testing.
+
+        Test: plugins/netscape-dom-access-and-reload.html
+
+        * WebCore.exp.in:
+        * html/HTMLPlugInElement.cpp:
+        (WebCore::HTMLPlugInElement::detach):
+        * html/HTMLPlugInElement.h:
+        (HTMLPlugInElement):
+        * testing/Internals.cpp:
+        (WebCore):
+        (WebCore::Internals::numberOfLiveNodes):
+        (WebCore::Internals::numberOfLiveDocuments):
+        * testing/Internals.h:
+        (Internals):
+        * testing/Internals.idl:
+
 2012-03-23  Ryosuke Niwa  <rniwa@webkit.org>
 
         CSSParser doesn't set border-*-width/style/color to initial by border shorthand property
index 92f453a..721ac4c 100644 (file)
@@ -1748,6 +1748,7 @@ __ZNK7WebCore12IconDatabase9isEnabledEv
 
 #if ENABLE(INSPECTOR)
 __ZN7WebCore15InspectorClient31doDispatchMessageOnFrontendPageEPNS_4PageERKN3WTF6StringE
+__ZN7WebCore17InspectorCounters12counterValueENS0_11CounterTypeE
 __ZN7WebCore19InspectorController14enableProfilerEv
 __ZN7WebCore19InspectorController15disableProfilerEv
 __ZN7WebCore19InspectorController15profilerEnabledEv
index 299e47b..458d157 100644 (file)
@@ -78,11 +78,6 @@ void HTMLPlugInElement::detach()
         m_isCapturingMouseEvents = false;
     }
 
-    HTMLFrameOwnerElement::detach();
-}
-
-void HTMLPlugInElement::removedFromDocument()
-{
 #if ENABLE(NETSCAPE_PLUGIN_API)
     if (m_NPObject) {
         _NPN_ReleaseObject(m_NPObject);
@@ -90,7 +85,7 @@ void HTMLPlugInElement::removedFromDocument()
     }
 #endif
 
-    HTMLFrameOwnerElement::removedFromDocument();
+    HTMLFrameOwnerElement::detach();
 }
 
 PassScriptInstance HTMLPlugInElement::getInstance()
index 22b14c4..65f869b 100644 (file)
@@ -57,7 +57,6 @@ protected:
     HTMLPlugInElement(const QualifiedName& tagName, Document*);
 
     virtual void detach();
-    virtual void removedFromDocument();
     virtual bool isPresentationAttribute(const QualifiedName&) const OVERRIDE;
     virtual void collectStyleForAttribute(Attribute*, StylePropertySet*) OVERRIDE;
 
index e409e56..6f2ee33 100644 (file)
@@ -42,6 +42,7 @@
 #include "HTMLNames.h"
 #include "HTMLTextAreaElement.h"
 #include "InspectorController.h"
+#include "InspectorCounters.h"
 #include "InspectorInstrumentation.h"
 #include "InternalSettings.h"
 #include "IntRect.h"
@@ -770,6 +771,18 @@ bool Internals::hasSpellingMarker(Document* document, int from, int length, Exce
     return document->frame()->editor()->selectionStartHasMarkerFor(DocumentMarker::Spelling, from, length);
 }
 
+#if ENABLE(INSPECTOR)
+unsigned Internals::numberOfLiveNodes() const
+{
+    return InspectorCounters::counterValue(InspectorCounters::NodeCounter);
+}
+
+unsigned Internals::numberOfLiveDocuments() const
+{
+    return InspectorCounters::counterValue(InspectorCounters::DocumentCounter);
+}
+#endif // ENABLE(INSPECTOR)
+
 bool Internals::hasGrammarMarker(Document* document, int from, int length, ExceptionCode&)
 {
     if (!document || !document->frame())
index f432b86..75ed7e2 100644 (file)
@@ -146,6 +146,11 @@ public:
 
     void setBatteryStatus(Document*, const String& eventType, bool charging, double chargingTime, double dischargingTime, double level, ExceptionCode&);
 
+#if ENABLE(INSPECTOR)
+    unsigned numberOfLiveNodes() const;
+    unsigned numberOfLiveDocuments() const;
+#endif
+
 private:
     explicit Internals(Document*);
     DocumentMarker* markerAt(Node*, const String& markerType, unsigned index, ExceptionCode&);
index 7b67abe..f2200ff 100644 (file)
@@ -120,7 +120,10 @@ module window {
 
 #if defined(ENABLE_BATTERY_STATUS) && ENABLE_BATTERY_STATUS
         void setBatteryStatus(in Document document, in DOMString eventType, in boolean charging, in double chargingTime, in double dischargingTime, in double level) raises (DOMException);
-#endif
+#endif 
+
+        [Conditional=INSPECTOR] unsigned long numberOfLiveNodes();
+        [Conditional=INSPECTOR] unsigned long numberOfLiveDocuments();
     };
 }
 
index 4af42c0..84ecd5b 100644 (file)
@@ -1,3 +1,13 @@
+2012-03-23  Dave Michael  <dmichael@chromium.org>
+
+        Relanding(r111754): HTMLPluginElement is not destroyed on reload or navigation if getNPObject is called
+        https://bugs.webkit.org/show_bug.cgi?id=80428
+
+        Reviewed by Eric Seidel and Ryosuke Niwa.
+
+        * win/WebKit2.def: Export a symbol for InspectorCounters::counterValue
+        * win/WebKit2CFLite.def: Export a symbol for InspectorCounters::counterValue
+
 2012-03-23  Sheriff Bot  <webkit.review.bot@gmail.com>
 
         Unreviewed, rolling out r111855.
index 82388ea..e2c8f6d 100644 (file)
@@ -115,6 +115,7 @@ EXPORTS
         ??1ThreadCondition@WTF@@QAE@XZ
         ?broadcast@ThreadCondition@WTF@@QAEXXZ
         ?callOnMainThread@WTF@@YAXP6AXPAX@Z0@Z
+        ?counterValue@InspectorCounters@WebCore@@SAHW4CounterType@12@@Z
         ?createThread@WTF@@YAIP6APAXPAX@Z0PBD@Z
         ?createThread@WTF@@YAIP6AXPAX@Z0PBD@Z
         ?currentThread@WTF@@YAIXZ
index e054d55..6769615 100644 (file)
@@ -108,6 +108,7 @@ EXPORTS
         ??1ThreadCondition@WTF@@QAE@XZ
         ?broadcast@ThreadCondition@WTF@@QAEXXZ
         ?callOnMainThread@WTF@@YAXP6AXPAX@Z0@Z
+        ?counterValue@InspectorCounters@WebCore@@SAHW4CounterType@12@@Z
         ?createThread@WTF@@YAIP6APAXPAX@Z0PBD@Z
         ?createThread@WTF@@YAIP6AXPAX@Z0PBD@Z
         ?currentThread@WTF@@YAIXZ
index 652f182..ac7b00c 100644 (file)
@@ -59,6 +59,7 @@ _ZN7WebCore16HTMLInputElement17setSuggestedValueERKN3WTF6StringE;
 _ZN7WebCore16jsStringSlowCaseEPN3JSC9ExecStateERN3WTF7HashMapIPNS3_10StringImplENS0_4WeakINS0_8JSStringEEENS3_10StringHashENS3_10HashTraitsIS6_EENSB_IS9_EEEES6_;
 _ZN7WebCore16scriptNameToCodeERKN3WTF6StringE;
 _ZN7WebCore17cacheDOMStructureEPNS_17JSDOMGlobalObjectEPN3JSC9StructureEPKNS2_9ClassInfoE;
+_ZN7WebCore17InspectorCounters12counterValueENS0_11CounterTypeE;
 _ZN7WebCore18HTMLContentElement6createEPNS_8DocumentE;
 _ZN7WebCore19InspectorController39setResourcesDataSizeLimitsFromInternalsEii;
 _ZN7WebCore20NodeRenderingContextC1EPNS_4NodeE;