Merge r228100 - Disallow evaluating JavaScript from NPP_Destroy() in …

…WebKit

https://bugs.webkit.org/show_bug.cgi?id=181889
<rdar://problem/36674701>

Reviewed by Brent Fulgham.

Source/WebKit:

Make the behavior of WebKit match the behavior of WebKitLegacy on Mac.

* Shared/Plugins/NPObjectMessageReceiver.cpp:
(WebKit::NPObjectMessageReceiver::hasMethod):
(WebKit::NPObjectMessageReceiver::invoke):
(WebKit::NPObjectMessageReceiver::invokeDefault):
(WebKit::NPObjectMessageReceiver::hasProperty):
(WebKit::NPObjectMessageReceiver::getProperty):
(WebKit::NPObjectMessageReceiver::setProperty):
(WebKit::NPObjectMessageReceiver::removeProperty):
(WebKit::NPObjectMessageReceiver::enumerate):
(WebKit::NPObjectMessageReceiver::construct):
Bail out if the plugin is executing NPP_Destroy().

* WebProcess/Plugins/Plugin.cpp:
(WebKit::Plugin::destroyPlugin):
* WebProcess/Plugins/Plugin.h:
(WebKit::Plugin::isBeingDestroyed const):
Move bookkeeping of whether the plugin is being destroyed from PluginView
to here. This makes it straightforward for NPObjectMessageReceiver to query
this information.

* WebProcess/Plugins/PluginView.cpp:
(WebKit::PluginView::~PluginView):
(WebKit::PluginView::destroyPluginAndReset):
(WebKit::PluginView::recreateAndInitialize):
(WebKit::PluginView::protectPluginFromDestruction):
(WebKit::PluginView::unprotectPluginFromDestruction):
Move bookkeeping of whether the plugin is being destroyed from here
to Plugin.

* WebProcess/Plugins/PluginView.h:
(WebKit::PluginView::isBeingDestroyed const): Turn around and ask the plugin if it
is being destroyed, if we have one.

LayoutTests:

Consolidate all the plugin tests that evaluate JavaScript from NPP_Destroy()
and mark them as Wont Fix. In a subsequent change we will look to replace
these tests with tests that ensure that we do not evaluate JavaScript from
NPP_Destroy().

* platform/mac/TestExpectations:
* platform/wk2/TestExpectations:

LayoutTests/ChangeLog

@@ -1,3 +1,19 @@
+2018-02-05  Daniel Bates  <dabates@apple.com>
+
+        Disallow evaluating JavaScript from NPP_Destroy() in WebKit
+        https://bugs.webkit.org/show_bug.cgi?id=181889
+        <rdar://problem/36674701>
+
+        Reviewed by Brent Fulgham.
+
+        Consolidate all the plugin tests that evaluate JavaScript from NPP_Destroy()
+        and mark them as Wont Fix. In a subsequent change we will look to replace
+        these tests with tests that ensure that we do not evaluate JavaScript from
+        NPP_Destroy().
+
+        * platform/mac/TestExpectations:
+        * platform/wk2/TestExpectations:
+
 2018-02-05  Antti Koivisto  <antti@apple.com>
 
         Crash on sfgate.com because mismatching link preload types

LayoutTests/platform/mac/TestExpectations

@@ -189,8 +189,6 @@ http/tests/images/webp-partial-load.html
 http/tests/images/webp-progressive-load.html
 fast/images/animated-webp-expected.html
 
-# Times out because plugins aren't allowed to execute JS after NPP_Destroy has been called in WebKit1's OOP plugins implementation
-webkit.org/b/48929 plugins/evaluate-js-after-removing-plugin-element.html
 
 # DRT does not support toggling caret browsing on / off
 editing/selection/caret-mode-paragraph-keys-navigation.html
@@ -435,12 +433,16 @@ http/tests/misc/willCacheResponse-delegate-callback.html [ Failure ]
 http/tests/xmlhttprequest/basic-auth-nopassword.html [ Failure ]
 
 # --- Plugins ---
-# WebKit1 OOP plug-ins: Can't evaluate JavaScript from NPP_Destroy.
-plugins/document-open.html
-plugins/geturlnotify-during-document-teardown.html
-plugins/nested-plugin-objects.html
-plugins/netscape-destroy-plugin-script-objects.html
-plugins/open-and-close-window-with-plugin.html
+# Out-of-process plug-ins are disallowed from evaluating JavaScript from NPP_Destroy().
+plugins/attach-during-destroy.html [ WontFix ]
+plugins/destroy-reentry.html [ WontFix ]
+plugins/document-open.html [ WontFix ]
+webkit.org/b/48929 plugins/evaluate-js-after-removing-plugin-element.html [ WontFix ]
+plugins/geturlnotify-during-document-teardown.html [ WontFix ]
+plugins/js-from-destroy.html [ WontFix ]
+plugins/nested-plugin-objects.html [ WontFix ]
+plugins/netscape-destroy-plugin-script-objects.html [ WontFix ]
+plugins/open-and-close-window-with-plugin.html [ WontFix ]
 
 # WebKit1 OOP plug-ins: No support for getting the form value.
 plugins/form-value.html

LayoutTests/platform/wk2/TestExpectations

@@ -125,11 +125,6 @@ http/tests/globalhistory/history-delegate-basic-refresh-redirect.html
 # <https://bugs.webkit.org/show_bug.cgi?id=56531>
 transitions/default-timing-function.html
 
-# WebKitTestRunner needs testRunner.setCallCloseOnWebViews
-# http://webkit.org/b/46714
-plugins/geturlnotify-during-document-teardown.html
-plugins/open-and-close-window-with-plugin.html
-
 # Sometimes fails
 # http://webkit.org/b/58990
 editing/undo/undo-iframe-location-change.html
@@ -513,6 +508,17 @@ fast/preloader/document-write-2.html [ Pass Failure ]
 ########################################
 ### START OF (4) Features that are not supported in WebKit2 and likely never will be
 
+# Plug-ins are disallowed from evaluating JavaScript from NPP_Destroy().
+plugins/attach-during-destroy.html [ WontFix ]
+plugins/destroy-reentry.html [ WontFix ]
+plugins/document-open.html [ WontFix ]
+webkit.org/b/48929 plugins/evaluate-js-after-removing-plugin-element.html [ WontFix ]
+plugins/geturlnotify-during-document-teardown.html [ WontFix ]
+plugins/js-from-destroy.html [ WontFix ]
+plugins/nested-plugin-objects.html [ WontFix ]
+plugins/netscape-destroy-plugin-script-objects.html [ WontFix ]
+plugins/open-and-close-window-with-plugin.html [ WontFix ]
+
 # Internals.registerDefaultPortForProtocol() does not affect NetworkProcess. We should
 # look to remove it and write these test to make use of an HTTP server running on port 80.
 http/tests/security/contentSecurityPolicy/script-src-parsing-implicit-and-explicit-port-number.html

Source/WebKit/ChangeLog

@@ -1,3 +1,46 @@
+2018-02-05  Daniel Bates  <dabates@apple.com>
+
+        Disallow evaluating JavaScript from NPP_Destroy() in WebKit
+        https://bugs.webkit.org/show_bug.cgi?id=181889
+        <rdar://problem/36674701>
+
+        Reviewed by Brent Fulgham.
+
+        Make the behavior of WebKit match the behavior of WebKitLegacy on Mac.
+
+        * Shared/Plugins/NPObjectMessageReceiver.cpp:
+        (WebKit::NPObjectMessageReceiver::hasMethod):
+        (WebKit::NPObjectMessageReceiver::invoke):
+        (WebKit::NPObjectMessageReceiver::invokeDefault):
+        (WebKit::NPObjectMessageReceiver::hasProperty):
+        (WebKit::NPObjectMessageReceiver::getProperty):
+        (WebKit::NPObjectMessageReceiver::setProperty):
+        (WebKit::NPObjectMessageReceiver::removeProperty):
+        (WebKit::NPObjectMessageReceiver::enumerate):
+        (WebKit::NPObjectMessageReceiver::construct):
+        Bail out if the plugin is executing NPP_Destroy().
+
+        * WebProcess/Plugins/Plugin.cpp:
+        (WebKit::Plugin::destroyPlugin):
+        * WebProcess/Plugins/Plugin.h:
+        (WebKit::Plugin::isBeingDestroyed const):
+        Move bookkeeping of whether the plugin is being destroyed from PluginView
+        to here. This makes it straightforward for NPObjectMessageReceiver to query
+        this information.
+
+        * WebProcess/Plugins/PluginView.cpp:
+        (WebKit::PluginView::~PluginView):
+        (WebKit::PluginView::destroyPluginAndReset):
+        (WebKit::PluginView::recreateAndInitialize):
+        (WebKit::PluginView::protectPluginFromDestruction):
+        (WebKit::PluginView::unprotectPluginFromDestruction):
+        Move bookkeeping of whether the plugin is being destroyed from here
+        to Plugin.
+
+        * WebProcess/Plugins/PluginView.h:
+        (WebKit::PluginView::isBeingDestroyed const): Turn around and ask the plugin if it
+        is being destroyed, if we have one.
+
 2018-02-05  Carlos Garcia Campos  <cgarcia@igalia.com>
 
         Unreviewed. Update OptionsGTK.cmake and NEWS for 2.19.90 release.

Source/WebKit/Shared/Plugins/NPObjectMessageReceiver.cpp

@@ -60,7 +60,7 @@ void NPObjectMessageReceiver::deallocate()
 
 void NPObjectMessageReceiver::hasMethod(const NPIdentifierData& methodNameData, bool& returnValue)
 {
-    if (!m_npObject->_class->hasMethod) {
+    if (m_plugin->isBeingDestroyed() || !m_npObject->_class->hasMethod) {
         returnValue = false;
         return;
     }
@@ -70,7 +70,7 @@ void NPObjectMessageReceiver::hasMethod(const NPIdentifierData& methodNameData,
 
 void NPObjectMessageReceiver::invoke(const NPIdentifierData& methodNameData, const Vector<NPVariantData>& argumentsData, bool& returnValue, NPVariantData& resultData)
 {
-    if (!m_npObject->_class->invoke) {
+    if (m_plugin->isBeingDestroyed() || !m_npObject->_class->invoke) {
         returnValue = false;
         return;
     }
@@ -100,7 +100,7 @@ void NPObjectMessageReceiver::invoke(const NPIdentifierData& methodNameData, con
 
 void NPObjectMessageReceiver::invokeDefault(const Vector<NPVariantData>& argumentsData, bool& returnValue, NPVariantData& resultData)
 {
-    if (!m_npObject->_class->invokeDefault) {
+    if (m_plugin->isBeingDestroyed() || !m_npObject->_class->invokeDefault) {
         returnValue = false;
         return;
     }
@@ -130,7 +130,7 @@ void NPObjectMessageReceiver::invokeDefault(const Vector<NPVariantData>& argumen
 
 void NPObjectMessageReceiver::hasProperty(const NPIdentifierData& propertyNameData, bool& returnValue)
 {
-    if (!m_npObject->_class->hasProperty) {
+    if (m_plugin->isBeingDestroyed() || !m_npObject->_class->hasProperty) {
         returnValue = false;
         return;
     }
@@ -140,7 +140,7 @@ void NPObjectMessageReceiver::hasProperty(const NPIdentifierData& propertyNameDa
 
 void NPObjectMessageReceiver::getProperty(const NPIdentifierData& propertyNameData, bool& returnValue, NPVariantData& resultData)
 {
-    if (!m_npObject->_class->getProperty) {
+    if (m_plugin->isBeingDestroyed() || !m_npObject->_class->getProperty) {
         returnValue = false;
         return;
     }
@@ -162,7 +162,7 @@ void NPObjectMessageReceiver::getProperty(const NPIdentifierData& propertyNameDa
 
 void NPObjectMessageReceiver::setProperty(const NPIdentifierData& propertyNameData, const NPVariantData& propertyValueData, bool& returnValue)
 {
-    if (!m_npObject->_class->setProperty) {
+    if (m_plugin->isBeingDestroyed() || !m_npObject->_class->setProperty) {
         returnValue = false;
         return;
     }
@@ -178,7 +178,7 @@ void NPObjectMessageReceiver::setProperty(const NPIdentifierData& propertyNameDa
 
 void NPObjectMessageReceiver::removeProperty(const NPIdentifierData& propertyNameData, bool& returnValue)
 {
-    if (!m_npObject->_class->removeProperty) {
+    if (m_plugin->isBeingDestroyed() || !m_npObject->_class->removeProperty) {
         returnValue = false;
         return;
     }
@@ -188,7 +188,7 @@ void NPObjectMessageReceiver::removeProperty(const NPIdentifierData& propertyNam
 
 void NPObjectMessageReceiver::enumerate(bool& returnValue, Vector<NPIdentifierData>& identifiersData)
 {
-    if (!NP_CLASS_STRUCT_VERSION_HAS_ENUM(m_npObject->_class) || !m_npObject->_class->enumerate) {
+    if (m_plugin->isBeingDestroyed() || !NP_CLASS_STRUCT_VERSION_HAS_ENUM(m_npObject->_class) || !m_npObject->_class->enumerate) {
         returnValue = false;
         return;
     }
@@ -208,7 +208,7 @@ void NPObjectMessageReceiver::enumerate(bool& returnValue, Vector<NPIdentifierDa
 
 void NPObjectMessageReceiver::construct(const Vector<NPVariantData>& argumentsData, bool& returnValue, NPVariantData& resultData)
 {
-    if (!NP_CLASS_STRUCT_VERSION_HAS_CTOR(m_npObject->_class) || !m_npObject->_class->construct) {
+    if (m_plugin->isBeingDestroyed() || !NP_CLASS_STRUCT_VERSION_HAS_CTOR(m_npObject->_class) || !m_npObject->_class->construct) {
         returnValue = false;
         return;
     }

Source/WebKit/WebProcess/Plugins/Plugin.cpp

@@ -28,6 +28,7 @@
 
 #include "WebCoreArgumentCoders.h"
 #include <WebCore/IntPoint.h>
+#include <wtf/SetForScope.h>
 
 using namespace WebCore;
 
@@ -98,9 +99,12 @@ bool Plugin::initialize(PluginController* pluginController, const Parameters& pa
 
 void Plugin::destroyPlugin()
 {
+    ASSERT(!m_isBeingDestroyed);
+    SetForScope<bool> scope { m_isBeingDestroyed, true };
+
     destroy();
 
-    m_pluginController = 0;
+    m_pluginController = nullptr;
 }
 
 void Plugin::updateControlTints(GraphicsContext&)

Source/WebKit/WebProcess/Plugins/Plugin.h

@@ -104,6 +104,8 @@ class Plugin : public ThreadSafeRefCounted<Plugin> {
     // Destroys the plug-in.
     void destroyPlugin();
 
+    bool isBeingDestroyed() const { return m_isBeingDestroyed; }
+
     // Returns the plug-in controller for this plug-in.
     PluginController* controller() { return m_pluginController; }
     const PluginController* controller() const { return m_pluginController; }
@@ -309,6 +311,8 @@ class Plugin : public ThreadSafeRefCounted<Plugin> {
 
     PluginType m_type;
 
+    bool m_isBeingDestroyed { false };
+
 private:
     PluginController* m_pluginController;
 };

Source/WebKit/WebProcess/Plugins/PluginView.cpp

@@ -319,7 +319,7 @@ PluginView::~PluginView()
     if (m_webPage)
         m_webPage->removePluginView(this);
 
-    ASSERT(!m_isBeingDestroyed);
+    ASSERT(!m_plugin || !m_plugin->isBeingDestroyed());
 
     if (m_isWaitingUntilMediaCanStart)
         m_pluginElement->document().removeMediaCanStartListener(this);
@@ -340,9 +340,7 @@ void PluginView::destroyPluginAndReset()
         it->key->setLoadListener(0);
 
     if (m_plugin) {
-        m_isBeingDestroyed = true;
         m_plugin->destroyPlugin();
-        m_isBeingDestroyed = false;
 
         m_pendingURLRequests.clear();
         m_pendingURLRequestsTimer.stop();
@@ -373,7 +371,6 @@ void PluginView::recreateAndInitialize(Ref<Plugin>&& plugin)
     m_isInitialized = false;
     m_isWaitingForSynchronousInitialization = false;
     m_isWaitingUntilMediaCanStart = false;
-    m_isBeingDestroyed = false;
     m_manualStreamState = ManualStreamState::Initial;
     m_transientPaintingSnapshot = nullptr;
 
@@ -1641,13 +1638,13 @@ bool PluginView::artificialPluginInitializationDelayEnabled() const
 
 void PluginView::protectPluginFromDestruction()
 {
-    if (!m_isBeingDestroyed)
+    if (m_plugin && !m_plugin->isBeingDestroyed())
         ref();
 }
 
 void PluginView::unprotectPluginFromDestruction()
 {
-    if (m_isBeingDestroyed)
+    if (!m_plugin || m_plugin->isBeingDestroyed())
         return;
 
     // A plug-in may ask us to evaluate JavaScript that removes the plug-in from the

Source/WebKit/WebProcess/Plugins/PluginView.h

@@ -70,7 +70,7 @@ class PluginView : public WebCore::PluginViewBase, public PluginController, priv
 
     WebCore::Frame* frame() const;
 
-    bool isBeingDestroyed() const { return m_isBeingDestroyed; }
+    bool isBeingDestroyed() const { return !m_plugin || m_plugin->isBeingDestroyed(); }
 
     void manualLoadDidReceiveResponse(const WebCore::ResourceResponse&);
     void manualLoadDidReceiveData(const char* bytes, int length);
@@ -248,7 +248,6 @@ class PluginView : public WebCore::PluginViewBase, public PluginController, priv
     bool m_isInitialized { false };
     bool m_isWaitingForSynchronousInitialization { false };
     bool m_isWaitingUntilMediaCanStart { false };
-    bool m_isBeingDestroyed { false };
     bool m_pluginProcessHasCrashed { false };
 
 #if ENABLE(PRIMARY_SNAPSHOTTED_PLUGIN_HEURISTIC)