Title: [187620] trunk
Revision
187620
Author
ander...@apple.com
Date
2015-07-30 16:38:23 -0700 (Thu, 30 Jul 2015)

Log Message

Assertion failure when a plug-in loads a resource that redirects somewhere
https://bugs.webkit.org/show_bug.cgi?id=147469

Reviewed by Alexey Proskuryakov.

Source/WebCore:

Test: http/tests/plugins/get-url-redirect.html

r186597 moved the call to addPlugInStreamLoader to willSendRequest. This is wrong since
willSendRequest can be invoked more than once.

Fix this by making the initialization phase of NetscapePlugInStreamLoader be more like
SubresourceLoader where we only call addPlugInStreamLoader once we've successfully initialized
the loader, and only call removePlugInStreamLoader if we've called addPlugInStreamLoader.

Also change addPlugInStreamLoader and removePlugInStreamLoader to take references.

* loader/DocumentLoader.cpp:
(WebCore::DocumentLoader::addPlugInStreamLoader):
(WebCore::DocumentLoader::removePlugInStreamLoader):
* loader/DocumentLoader.h:
* loader/NetscapePlugInStreamLoader.cpp:
(WebCore::NetscapePlugInStreamLoader::create):
(WebCore::NetscapePlugInStreamLoader::init):
(WebCore::NetscapePlugInStreamLoader::didFinishLoading):
(WebCore::NetscapePlugInStreamLoader::didFail):
(WebCore::NetscapePlugInStreamLoader::didCancel):
(WebCore::NetscapePlugInStreamLoader::notifyDone):
* loader/NetscapePlugInStreamLoader.h:
* loader/ResourceLoader.cpp:
(WebCore::ResourceLoader::willSendRequest): Deleted.
* loader/ResourceLoader.h:
(WebCore::ResourceLoader::isPlugInStreamLoader): Deleted.
* loader/SubframeLoader.cpp:
(WebCore::SubframeLoader::requestObject):

LayoutTests:

Add a test.

* http/tests/plugins/get-url-redirect-expected.txt: Added.
* http/tests/plugins/get-url-redirect.html: Added.
* http/tests/plugins/resources/redirection-response.php: Added.

Modified Paths

Added Paths

Diff

Modified: trunk/LayoutTests/ChangeLog (187619 => 187620)


--- trunk/LayoutTests/ChangeLog	2015-07-30 23:25:43 UTC (rev 187619)
+++ trunk/LayoutTests/ChangeLog	2015-07-30 23:38:23 UTC (rev 187620)
@@ -1,3 +1,16 @@
+2015-07-30  Anders Carlsson  <ander...@apple.com>
+
+        Assertion failure when a plug-in loads a resource that redirects somewhere
+        https://bugs.webkit.org/show_bug.cgi?id=147469
+
+        Reviewed by Alexey Proskuryakov.
+
+        Add a test.
+
+        * http/tests/plugins/get-url-redirect-expected.txt: Added.
+        * http/tests/plugins/get-url-redirect.html: Added.
+        * http/tests/plugins/resources/redirection-response.php: Added.
+
 2015-07-29  Matt Rajca  <mra...@apple.com>
 
         Media Session: test Play/Pause media control events delivered to non-Content media sessions

Added: trunk/LayoutTests/http/tests/plugins/get-url-redirect-expected.txt (0 => 187620)


--- trunk/LayoutTests/http/tests/plugins/get-url-redirect-expected.txt	                        (rev 0)
+++ trunk/LayoutTests/http/tests/plugins/get-url-redirect-expected.txt	2015-07-30 23:38:23 UTC (rev 187620)
@@ -0,0 +1 @@
+This tests that NPN_GetURLNotify does not ASSERT in debug builds. 

Added: trunk/LayoutTests/http/tests/plugins/get-url-redirect.html (0 => 187620)


--- trunk/LayoutTests/http/tests/plugins/get-url-redirect.html	                        (rev 0)
+++ trunk/LayoutTests/http/tests/plugins/get-url-redirect.html	2015-07-30 23:38:23 UTC (rev 187620)
@@ -0,0 +1,19 @@
+<html>
+<body>
+This tests that NPN_GetURLNotify does not ASSERT in debug builds.
+<embed name="plg" type="application/x-webkit-test-netscape"></embed>
+<script>
+    function notify()
+    {
+        if (window.testRunner)
+            testRunner.notifyDone();
+    }
+    if (window.testRunner) {
+        testRunner.dumpAsText();
+        testRunner.waitUntilDone();
+    }
+
+    plg.getURLNotify("resources/redirection-response.php?status=301&target=load-me-1.txt", null, "notify");
+</script>
+</body>
+</html>

Added: trunk/LayoutTests/http/tests/plugins/resources/redirection-response.php (0 => 187620)


--- trunk/LayoutTests/http/tests/plugins/resources/redirection-response.php	                        (rev 0)
+++ trunk/LayoutTests/http/tests/plugins/resources/redirection-response.php	2015-07-30 23:38:23 UTC (rev 187620)
@@ -0,0 +1,31 @@
+<?php
+$status_code = $_GET['status'];
+
+$uri = rtrim(dirname($_SERVER['PHP_SELF']), '/\\') . "/" . $_GET['target'];
+
+$host = $_SERVER['HTTP_HOST'];
+if (isset($_GET['host']))
+    $host = $_GET['host'];
+
+switch ($status_code) {
+    case 301:
+        header("HTTP/1.1 301 Moved Permanently");
+        header("Location: http://" . $host . $uri);
+        break;
+    case 302:
+        header("HTTP/1.1 302 Found");
+        header("Location: http://" . $host . $uri);
+        break;
+    case 303:
+        header("HTTP/1.1 303 See Other");
+        header("Location: http://" . $host . $uri);
+        break;
+    case 307:
+        header("HTTP/1.1 307 Temporary Redirect");
+        header("Location: http://" . $host . $uri);
+        break;
+    default:
+        header("HTTP/1.1 500 Internal Server Error");
+        echo "Unexpected status code ($status_code) received.";
+}
+?>

Modified: trunk/Source/WebCore/ChangeLog (187619 => 187620)


--- trunk/Source/WebCore/ChangeLog	2015-07-30 23:25:43 UTC (rev 187619)
+++ trunk/Source/WebCore/ChangeLog	2015-07-30 23:38:23 UTC (rev 187620)
@@ -1,3 +1,40 @@
+2015-07-30  Anders Carlsson  <ander...@apple.com>
+
+        Assertion failure when a plug-in loads a resource that redirects somewhere
+        https://bugs.webkit.org/show_bug.cgi?id=147469
+
+        Reviewed by Alexey Proskuryakov.
+
+        Test: http/tests/plugins/get-url-redirect.html
+
+        r186597 moved the call to addPlugInStreamLoader to willSendRequest. This is wrong since
+        willSendRequest can be invoked more than once.
+        
+        Fix this by making the initialization phase of NetscapePlugInStreamLoader be more like
+        SubresourceLoader where we only call addPlugInStreamLoader once we've successfully initialized
+        the loader, and only call removePlugInStreamLoader if we've called addPlugInStreamLoader.
+
+        Also change addPlugInStreamLoader and removePlugInStreamLoader to take references.
+
+        * loader/DocumentLoader.cpp:
+        (WebCore::DocumentLoader::addPlugInStreamLoader):
+        (WebCore::DocumentLoader::removePlugInStreamLoader):
+        * loader/DocumentLoader.h:
+        * loader/NetscapePlugInStreamLoader.cpp:
+        (WebCore::NetscapePlugInStreamLoader::create):
+        (WebCore::NetscapePlugInStreamLoader::init):
+        (WebCore::NetscapePlugInStreamLoader::didFinishLoading):
+        (WebCore::NetscapePlugInStreamLoader::didFail):
+        (WebCore::NetscapePlugInStreamLoader::didCancel):
+        (WebCore::NetscapePlugInStreamLoader::notifyDone):
+        * loader/NetscapePlugInStreamLoader.h:
+        * loader/ResourceLoader.cpp:
+        (WebCore::ResourceLoader::willSendRequest): Deleted.
+        * loader/ResourceLoader.h:
+        (WebCore::ResourceLoader::isPlugInStreamLoader): Deleted.
+        * loader/SubframeLoader.cpp:
+        (WebCore::SubframeLoader::requestObject):
+
 2015-07-30  Jer Noble  <jer.no...@apple.com>
 
         [iOS] Pressing 'done' in fullscreen video sometimes does nothing; stuck in fullscreen

Modified: trunk/Source/WebCore/loader/DocumentLoader.cpp (187619 => 187620)


--- trunk/Source/WebCore/loader/DocumentLoader.cpp	2015-07-30 23:25:43 UTC (rev 187619)
+++ trunk/Source/WebCore/loader/DocumentLoader.cpp	2015-07-30 23:38:23 UTC (rev 187620)
@@ -1352,20 +1352,20 @@
         frame->loader().checkLoadComplete();
 }
 
-void DocumentLoader::addPlugInStreamLoader(ResourceLoader* loader)
+void DocumentLoader::addPlugInStreamLoader(ResourceLoader& loader)
 {
-    ASSERT(loader->identifier());
-    ASSERT(!m_plugInStreamLoaders.contains(loader->identifier()));
+    ASSERT(loader.identifier());
+    ASSERT(!m_plugInStreamLoaders.contains(loader.identifier()));
 
-    m_plugInStreamLoaders.add(loader->identifier(), loader);
+    m_plugInStreamLoaders.add(loader.identifier(), &loader);
 }
 
-void DocumentLoader::removePlugInStreamLoader(ResourceLoader* loader)
+void DocumentLoader::removePlugInStreamLoader(ResourceLoader& loader)
 {
-    ASSERT(loader->identifier());
-    ASSERT(loader == m_plugInStreamLoaders.get(loader->identifier()));
+    ASSERT(loader.identifier());
+    ASSERT(&loader == m_plugInStreamLoaders.get(loader.identifier()));
 
-    m_plugInStreamLoaders.remove(loader->identifier());
+    m_plugInStreamLoaders.remove(loader.identifier());
     checkLoadComplete();
 }
 

Modified: trunk/Source/WebCore/loader/DocumentLoader.h (187619 => 187620)


--- trunk/Source/WebCore/loader/DocumentLoader.h	2015-07-30 23:25:43 UTC (rev 187619)
+++ trunk/Source/WebCore/loader/DocumentLoader.h	2015-07-30 23:38:23 UTC (rev 187620)
@@ -228,8 +228,8 @@
 
         void addSubresourceLoader(ResourceLoader*);
         void removeSubresourceLoader(ResourceLoader*);
-        WEBCORE_EXPORT void addPlugInStreamLoader(ResourceLoader*);
-        WEBCORE_EXPORT void removePlugInStreamLoader(ResourceLoader*);
+        void addPlugInStreamLoader(ResourceLoader&);
+        void removePlugInStreamLoader(ResourceLoader&);
 
         void subresourceLoaderFinishedLoadingOnePart(ResourceLoader*);
 

Modified: trunk/Source/WebCore/loader/NetscapePlugInStreamLoader.cpp (187619 => 187620)


--- trunk/Source/WebCore/loader/NetscapePlugInStreamLoader.cpp	2015-07-30 23:25:43 UTC (rev 187619)
+++ trunk/Source/WebCore/loader/NetscapePlugInStreamLoader.cpp	2015-07-30 23:38:23 UTC (rev 187620)
@@ -60,6 +60,7 @@
     RefPtr<NetscapePlugInStreamLoader> loader(adoptRef(new NetscapePlugInStreamLoader(frame, client)));
     if (!loader->init(request))
         return nullptr;
+
     return loader.release();
 }
 
@@ -74,6 +75,19 @@
     ResourceLoader::releaseResources();
 }
 
+bool NetscapePlugInStreamLoader::init(const ResourceRequest& request)
+{
+    if (!ResourceLoader::init(request))
+        return false;
+
+    ASSERT(!reachedTerminalState());
+
+    m_documentLoader->addPlugInStreamLoader(*this);
+    m_isInitialized = true;
+
+    return true;
+}
+
 void NetscapePlugInStreamLoader::didReceiveResponse(const ResourceResponse& response)
 {
     Ref<NetscapePlugInStreamLoader> protect(*this);
@@ -124,7 +138,8 @@
 {
     Ref<NetscapePlugInStreamLoader> protect(*this);
 
-    m_documentLoader->removePlugInStreamLoader(this);
+    notifyDone();
+
     m_client->didFinishLoading(this);
     ResourceLoader::didFinishLoading(finishTime);
 }
@@ -133,7 +148,8 @@
 {
     Ref<NetscapePlugInStreamLoader> protect(*this);
 
-    m_documentLoader->removePlugInStreamLoader(this);
+    notifyDone();
+
     m_client->didFail(this, error);
     ResourceLoader::didFail(error);
 }
@@ -145,10 +161,16 @@
 
 void NetscapePlugInStreamLoader::didCancel(const ResourceError&)
 {
-    // We need to remove the stream loader after the call to didFail, since didFail can 
-    // spawn a new run loop and if the loader has been removed it won't be deferred when
-    // the document loader is asked to defer loading.
-    m_documentLoader->removePlugInStreamLoader(this);
+    notifyDone();
 }
 
+void NetscapePlugInStreamLoader::notifyDone()
+{
+    if (!m_isInitialized)
+        return;
+
+    m_documentLoader->removePlugInStreamLoader(*this);
 }
+
+
+}

Modified: trunk/Source/WebCore/loader/NetscapePlugInStreamLoader.h (187619 => 187620)


--- trunk/Source/WebCore/loader/NetscapePlugInStreamLoader.h	2015-07-30 23:25:43 UTC (rev 187619)
+++ trunk/Source/WebCore/loader/NetscapePlugInStreamLoader.h	2015-07-30 23:38:23 UTC (rev 187620)
@@ -56,6 +56,8 @@
     WEBCORE_EXPORT bool isDone() const;
 
 private:
+    virtual bool init(const ResourceRequest&) override;
+
     virtual void didReceiveResponse(const ResourceResponse&) override;
     virtual void didReceiveData(const char*, unsigned, long long encodedDataLength, DataPayloadType) override;
     virtual void didReceiveBuffer(PassRefPtr<SharedBuffer>, long long encodedDataLength, DataPayloadType) override;
@@ -63,7 +65,6 @@
     virtual void didFail(const ResourceError&) override;
 
     virtual void releaseResources() override;
-    virtual bool isPlugInStreamLoader() override { return true; }
 
     NetscapePlugInStreamLoader(Frame*, NetscapePlugInStreamLoaderClient*);
 
@@ -72,7 +73,10 @@
 
     void didReceiveDataOrBuffer(const char*, int, PassRefPtr<SharedBuffer>, long long encodedDataLength, DataPayloadType);
 
+    void notifyDone();
+
     NetscapePlugInStreamLoaderClient* m_client;
+    bool m_isInitialized { false };
 };
 
 }

Modified: trunk/Source/WebCore/loader/ResourceLoader.cpp (187619 => 187620)


--- trunk/Source/WebCore/loader/ResourceLoader.cpp	2015-07-30 23:25:43 UTC (rev 187619)
+++ trunk/Source/WebCore/loader/ResourceLoader.cpp	2015-07-30 23:38:23 UTC (rev 187620)
@@ -314,9 +314,6 @@
         }
     }
 #endif
-    
-    if (isPlugInStreamLoader())
-        documentLoader()->addPlugInStreamLoader(this);
 
     if (request.isNull()) {
         didFail(cannotShowURLError());

Modified: trunk/Source/WebCore/loader/ResourceLoader.h (187619 => 187620)


--- trunk/Source/WebCore/loader/ResourceLoader.h	2015-07-30 23:25:43 UTC (rev 187619)
+++ trunk/Source/WebCore/loader/ResourceLoader.h	2015-07-30 23:38:23 UTC (rev 187620)
@@ -180,7 +180,6 @@
     virtual void didCancel(const ResourceError&) = 0;
 
     void addDataOrBuffer(const char*, unsigned, SharedBuffer*, DataPayloadType);
-    virtual bool isPlugInStreamLoader() { return false; }
 
     // ResourceHandleClient
     virtual void willSendRequest(ResourceHandle*, ResourceRequest&, const ResourceResponse& redirectResponse) override;

Modified: trunk/Source/WebCore/loader/SubframeLoader.cpp (187619 => 187620)


--- trunk/Source/WebCore/loader/SubframeLoader.cpp	2015-07-30 23:25:43 UTC (rev 187619)
+++ trunk/Source/WebCore/loader/SubframeLoader.cpp	2015-07-30 23:38:23 UTC (rev 187620)
@@ -212,6 +212,7 @@
 
 bool SubframeLoader::requestObject(HTMLPlugInImageElement& ownerElement, const String& url, const AtomicString& frameName, const String& mimeType, const Vector<String>& paramNames, const Vector<String>& paramValues)
 {
+    printf("request oject url %s mime type %s\n", url.ascii().data(), mimeType.ascii().data());
     if (url.isEmpty() && mimeType.isEmpty())
         return false;
 
_______________________________________________
webkit-changes mailing list
webkit-changes@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-changes

Reply via email to