<rdar://5983747> Safari crashes trying to load the SilverLight plugin
authorjhoneycutt@apple.com <jhoneycutt@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 3 Jul 2008 20:52:56 +0000 (20:52 +0000)
committerjhoneycutt@apple.com <jhoneycutt@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 3 Jul 2008 20:52:56 +0000 (20:52 +0000)
If a plug-in returned an error code from NPP_NewStream, we would call
NPP_DestroyStream while cleaning up the request. We now only call
NPP_DestroyStream if NPP_NewStream was successful, matching Firefox.

Reviewed by Anders.

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

LayoutTests/ChangeLog
LayoutTests/plugins/return-error-from-new-stream-doesnt-invoke-destroy-stream-expected.txt [new file with mode: 0644]
LayoutTests/plugins/return-error-from-new-stream-doesnt-invoke-destroy-stream.html [new file with mode: 0644]
WebCore/ChangeLog
WebCore/plugins/PluginStream.cpp
WebKitTools/ChangeLog
WebKitTools/DumpRenderTree/TestNetscapePlugIn.subproj/PluginObject.cpp
WebKitTools/DumpRenderTree/TestNetscapePlugIn.subproj/PluginObject.h
WebKitTools/DumpRenderTree/win/TestNetscapePlugin/main.cpp

index 829706a..675a838 100644 (file)
@@ -1,3 +1,12 @@
+2008-07-02  Jon Honeycutt  <jhoneycutt@apple.com>
+
+        Test for <rdar://5983747> Safari crashes trying to load the SilverLight plugin
+
+        Reviewed by Anders.
+
+        * plugins/return-error-from-new-stream-doesnt-invoke-destroy-stream-expected.txt: Added.
+        * plugins/return-error-from-new-stream-doesnt-invoke-destroy-stream.html: Added.
+
 2008-07-03  Alexey Proskuryakov  <ap@webkit.org>
 
         Reviewed by Darin.
diff --git a/LayoutTests/plugins/return-error-from-new-stream-doesnt-invoke-destroy-stream-expected.txt b/LayoutTests/plugins/return-error-from-new-stream-doesnt-invoke-destroy-stream-expected.txt
new file mode 100644 (file)
index 0000000..80d6628
--- /dev/null
@@ -0,0 +1,10 @@
+This tests that NPP_DestroyStream is not called if a plug-in returns an error from its NPP_NewStream callback.
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+PASS plugin.returnErrorFromNewStream is true
+PASS destroyStreamCalled is false
+
+TEST COMPLETE
+
diff --git a/LayoutTests/plugins/return-error-from-new-stream-doesnt-invoke-destroy-stream.html b/LayoutTests/plugins/return-error-from-new-stream-doesnt-invoke-destroy-stream.html
new file mode 100644 (file)
index 0000000..018040b
--- /dev/null
@@ -0,0 +1,56 @@
+<html>
+    <head>
+        <link rel="stylesheet" href="../fast/js/resources/js-test-style.css">
+        <script src="../fast/js/resources/js-test-pre.js"></script>
+    </head>
+    <body>
+        <p id="description"></p>
+        <div id="console"></div>
+
+        <script>
+            var destroyStreamCalled = false;
+
+            function main() {
+                if (!window.layoutTestController) {
+                    debug("This test can only run from within DumpRenderTree, because it requires TestNetscapePlugin.\n");
+                    return;
+                }
+                layoutTestController.waitUntilDone();
+            }
+
+            main();
+
+            function streamStarted() {
+                testFailed("TestNetscapePlugin should have returned an error from NewStream.");
+            }
+
+            function streamDestroyed() { destroyStreamCalled = true; }
+
+            function requestCompleted() {
+                shouldBeFalse("destroyStreamCalled");
+
+                debug('<br><span class="pass">TEST COMPLETE</span>');
+
+                layoutTestController.notifyDone();
+            }
+        </script>
+
+        <embed
+            type="application/x-webkit-test-netscape"
+            onStreamLoad="streamStarted()"
+            onStreamDestroy="streamDestroyed()"
+            onURLNotify="requestCompleted()"
+            id="plugin">
+    </body>
+    <script>
+        description("This tests that NPP_DestroyStream is not called if a plug-in returns an error from its NPP_NewStream callback.");
+
+        var plugin = document.getElementById("plugin");
+
+        plugin.returnErrorFromNewStream = true;
+        shouldBeTrue("plugin.returnErrorFromNewStream");
+
+        plugin.getURLNotify("data:,", null, "callback");
+    </script>
+</html>
+
index 42d345f..3ca9ebb 100644 (file)
@@ -1,3 +1,20 @@
+2008-07-02  Jon Honeycutt  <jhoneycutt@apple.com>
+
+        <rdar://5983747> Safari crashes trying to load the SilverLight plugin
+
+        If a plug-in returned an error code from NPP_NewStream, we would call
+        NPP_DestroyStream while cleaning up the request. We now only call
+        NPP_DestroyStream if NPP_NewStream was successful, matching Firefox.
+
+        Reviewed by Anders.
+
+        * plugins/PluginStream.cpp:
+        (WebCore::PluginStream::startStream): If NPP_NewStream returns an error,
+        don't set m_streamState to StreamStarted, and return after calling
+        cancelAndDestroyStream.
+        (WebCore::PluginStream::destroyStream): Don't call NPP_DestroyStream if
+        the stream didn't start successfully.
+
 2008-07-03  David Hyatt  <hyatt@apple.com>
 
         Revise Dan's fix for an assert on Windows, since layoutIfNeededRecursive doesn't exist on the
index 26a97a4..b8c75ab 100644 (file)
@@ -187,10 +187,12 @@ void PluginStream::startStream()
     if (m_reason != WebReasonNone)
         return;
         
-    m_streamState = StreamStarted;
-
-    if (npErr != NPERR_NO_ERROR)
+    if (npErr != NPERR_NO_ERROR) {
         cancelAndDestroyStream(npErr);
+        return;
+    }
+
+    m_streamState = StreamStarted;
 
     if (m_transferMode == NP_NORMAL)
         return;
@@ -252,12 +254,17 @@ void PluginStream::destroyStream()
                 m_loader->setDefersLoading(false);
         }
 
-        if (m_loader)
-            m_loader->setDefersLoading(true);
-        NPError npErr = m_pluginFuncs->destroystream(m_instance, &m_stream, m_reason);
-        if (m_loader)
-            m_loader->setDefersLoading(false);
-        LOG_NPERROR(npErr);
+        if (m_streamState != StreamBeforeStarted) {
+            if (m_loader)
+                m_loader->setDefersLoading(true);
+
+            NPError npErr = m_pluginFuncs->destroystream(m_instance, &m_stream, m_reason);
+
+            if (m_loader)
+                m_loader->setDefersLoading(false);
+
+            LOG_NPERROR(npErr);
+        }
 
         m_stream.ndata = 0;
     }
index 1f3f450..88a679a 100644 (file)
@@ -1,3 +1,32 @@
+2008-07-02  Jon Honeycutt  <jhoneycutt@apple.com>
+
+        Allow tests to define JavaScript to execute when NPP_DestroyStream or
+        NPP_URLNotify is called.
+
+        Reviewed by Anders.
+
+        * DumpRenderTree/TestNetscapePlugIn.subproj/PluginObject.cpp: Add a new
+        property, "returnErrorFromNewStream." This is to support the test for
+        <rdar://5983747> Safari crashes trying to load the SilverLight plugin,
+        caused by WebKit calling NPP_DestroyStream after a plug-in returns an
+        error from NPP_NewStream.
+        (pluginGetProperty): 
+        (pluginSetProperty):
+        (pluginAllocate):
+        * DumpRenderTree/TestNetscapePlugIn.subproj/PluginObject.h: Added new
+        members, onStreamDestroy and onURLNotify.
+        * DumpRenderTree/win/TestNetscapePlugin/main.cpp:
+        (NPP_New): Remove initialization of onStreamLoad; this was moved to
+        pluginAllocate. Look for new arguments onStreamDestroy and
+        onURLNotify, and store their values.
+        (NPP_Destroy): Free new members.
+        (executeScript): Code moved from onStreamLoad
+        (NPP_NewStream): If returnErrorFromNewStream has been set to true,
+        return a generic error code. If onStreamLoad is set, execute it as
+        JavaScript.
+        (NPP_DestroyStream): If onStreamDestroy is set, execute it as JS.
+        (NPP_URLNotify): Same, for onURLNotify.
+
 2008-07-02  Brady Eidson  <beidson@apple.com>
 
         Reviewed by Mitz Pettel and John Sullivan
index b5c9e85..e8dcb95 100644 (file)
@@ -61,12 +61,13 @@ NPClass *getPluginClass(void)
 
 static bool identifiersInitialized = false;
 
-#define ID_PROPERTY_PROPERTY        0
-#define ID_PROPERTY_EVENT_LOGGING   1
-#define ID_PROPERTY_HAS_STREAM      2
-#define ID_PROPERTY_TEST_OBJECT     3
-#define ID_PROPERTY_LOG_DESTROY     4
-#define NUM_PROPERTY_IDENTIFIERS    5
+#define ID_PROPERTY_PROPERTY                    0
+#define ID_PROPERTY_EVENT_LOGGING               1
+#define ID_PROPERTY_HAS_STREAM                  2
+#define ID_PROPERTY_TEST_OBJECT                 3
+#define ID_PROPERTY_LOG_DESTROY                 4
+#define ID_PROPERTY_RETURN_ERROR_FROM_NEWSTREAM 5
+#define NUM_PROPERTY_IDENTIFIERS                6
 
 static NPIdentifier pluginPropertyIdentifiers[NUM_PROPERTY_IDENTIFIERS];
 static const NPUTF8 *pluginPropertyIdentifierNames[NUM_PROPERTY_IDENTIFIERS] = {
@@ -75,6 +76,7 @@ static const NPUTF8 *pluginPropertyIdentifierNames[NUM_PROPERTY_IDENTIFIERS] = {
     "hasStream",
     "testObject",
     "logDestroy",
+    "returnErrorFromNewStream",
 };
 
 #define ID_TEST_CALLBACK_METHOD     0
@@ -164,6 +166,9 @@ static bool pluginGetProperty(NPObject* obj, NPIdentifier name, NPVariant* resul
         browser->retainobject(testObject);
         OBJECT_TO_NPVARIANT(testObject, *result);
         return true;
+    } else if (name == pluginPropertyIdentifiers[ID_PROPERTY_RETURN_ERROR_FROM_NEWSTREAM]) {
+        BOOLEAN_TO_NPVARIANT(plugin->returnErrorFromNewStream, *result);
+        return true;
     }
     return false;
 }
@@ -177,6 +182,9 @@ static bool pluginSetProperty(NPObject* obj, NPIdentifier name, const NPVariant*
     } else if (name == pluginPropertyIdentifiers[ID_PROPERTY_LOG_DESTROY]) {
         plugin->logDestroy = NPVARIANT_TO_BOOLEAN(*variant);
         return true;
+    } else if (name == pluginPropertyIdentifiers[ID_PROPERTY_RETURN_ERROR_FROM_NEWSTREAM]) {
+        plugin->returnErrorFromNewStream = NPVARIANT_TO_BOOLEAN(*variant);
+        return true;
     }
 
     return false;
@@ -574,6 +582,9 @@ static NPObject *pluginAllocate(NPP npp, NPClass *theClass)
     newInstance->npp = npp;
     newInstance->testObject = browser->createobject(npp, getTestClass());
     newInstance->eventLogging = FALSE;
+    newInstance->onStreamLoad = 0;
+    newInstance->onStreamDestroy = 0;
+    newInstance->onURLNotify = 0;
     newInstance->logDestroy = FALSE;
     newInstance->logSetWindow = FALSE;
     newInstance->returnErrorFromNewStream = FALSE;
index 31dbb96..a5441d8 100644 (file)
@@ -37,6 +37,8 @@ typedef struct {
     NPObject* testObject;
     NPStream* stream;
     char* onStreamLoad;
+    char* onStreamDestroy;
+    char* onURLNotify;
     char* firstUrl;
     char* firstHeaders;
     char* lastUrl;
index 8c542ed..ab54872 100644 (file)
@@ -76,12 +76,14 @@ NPError NPP_New(NPMIMEType pluginType, NPP instance, uint16 mode, int16 argc, ch
 {
     if (browser->version >= 14) {
         PluginObject* obj = (PluginObject*)browser->createobject(instance, getPluginClass());
-    
-        obj->onStreamLoad = NULL;
         
         for (int16 i = 0; i < argc; i++) {
             if (_stricmp(argn[i], "onstreamload") == 0 && !obj->onStreamLoad)
                 obj->onStreamLoad = _strdup(argv[i]);
+            else if (_stricmp(argn[i], "onStreamDestroy") == 0 && !obj->onStreamDestroy)
+                obj->onStreamDestroy = _strdup(argv[i]);
+            else if (_stricmp(argn[i], "onURLNotify") == 0 && !obj->onURLNotify)
+                obj->onURLNotify = _strdup(argv[i]);
         }
         
         instance->pdata = obj;
@@ -96,7 +98,13 @@ NPError NPP_Destroy(NPP instance, NPSavedData **save)
     if (obj) {
         if (obj->onStreamLoad)
             free(obj->onStreamLoad);
-        
+
+        if (obj->onURLNotify)
+            free(obj->onURLNotify);
+
+        if (obj->onStreamDestroy)
+            free(obj->onStreamDestroy);
+
         if (obj->logDestroy)
             printf("PLUGIN: NPP_Destroy\n");
 
@@ -110,30 +118,43 @@ NPError NPP_SetWindow(NPP instance, NPWindow *window)
     return NPERR_NO_ERROR;
 }
 
+static void executeScript(const PluginObject* obj, const char* script)
+{
+    NPObject *windowScriptObject;
+    browser->getvalue(obj->npp, NPNVWindowNPObject, &windowScriptObject);
+
+    NPString npScript;
+    npScript.UTF8Characters = script;
+    npScript.UTF8Length = strlen(script);
+
+    NPVariant browserResult;
+    browser->evaluate(obj->npp, windowScriptObject, &npScript, &browserResult);
+    browser->releasevariantvalue(&browserResult);
+}
+
 NPError NPP_NewStream(NPP instance, NPMIMEType type, NPStream *stream, NPBool seekable, uint16 *stype)
 {
     PluginObject* obj = (PluginObject*)instance->pdata;
+
+    if (obj->returnErrorFromNewStream)
+        return NPERR_GENERIC_ERROR;
+
     obj->stream = stream;
     *stype = NP_ASFILEONLY;
 
-    if (obj->onStreamLoad) {
-        NPObject *windowScriptObject;
-        browser->getvalue(obj->npp, NPNVWindowNPObject, &windowScriptObject);
-                
-        NPString script;
-        script.UTF8Characters = obj->onStreamLoad;
-        script.UTF8Length = strlen(obj->onStreamLoad);
-        
-        NPVariant browserResult;
-        browser->evaluate(obj->npp, windowScriptObject, &script, &browserResult);
-        browser->releasevariantvalue(&browserResult);
-    }
+    if (obj->onStreamLoad)
+        executeScript(obj, obj->onStreamLoad);
     
     return NPERR_NO_ERROR;
 }
 
 NPError NPP_DestroyStream(NPP instance, NPStream *stream, NPReason reason)
 {
+    PluginObject* obj = (PluginObject*)instance->pdata;
+
+    if (obj->onStreamDestroy)
+        executeScript(obj, obj->onStreamDestroy);
+
     return NPERR_NO_ERROR;
 }
 
@@ -168,6 +189,9 @@ int16 NPP_HandleEvent(NPP instance, void *event)
 void NPP_URLNotify(NPP instance, const char *url, NPReason reason, void *notifyData)
 {
     PluginObject *obj = (PluginObject*)instance->pdata;
+
+    if (obj->onURLNotify)
+        executeScript(obj, obj->onURLNotify);
         
     handleCallback(obj, url, reason, notifyData);
 }