2011-06-24 Gavin Peters <gavinp@chromium.org>
authorgavinp@chromium.org <gavinp@chromium.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 25 Jun 2011 00:39:33 +0000 (00:39 +0000)
committergavinp@chromium.org <gavinp@chromium.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 25 Jun 2011 00:39:33 +0000 (00:39 +0000)
        Reviewed by Darin Adler.

        fix possible race in LinkLoader
        https://bugs.webkit.org/show_bug.cgi?id=63360

        In chromium bug 80729
        http://code.google.com/p/chromium/issues/detail?id=80729 I am
        seeing some kind of double triggering of the timer; I am concerned
        that it is possible that a Link element errors out or succeeds,
        sets a timer, and shortly before the timer is triggered it is
        editted, launches another request.  After that, the first timer
        triggers, zeroing out m_cachedResource.  Then, the second load
        finishes, and *crash*.  If this is the case, this fix should stop
        it.

        No new tests; I haven't reproduced this.  I hope chrome's crash
        telemetry will give good feedback; this crash is occuring many times a
        day so the difference should be obvious.

        * loader/LinkLoader.cpp:
        (WebCore::LinkLoader::LinkLoader):
        (WebCore::LinkLoader::linkLoadTimerFired):
        (WebCore::LinkLoader::linkLoadingErrorTimerFired):
        (WebCore::LinkLoader::notifyFinished):
        * loader/LinkLoader.h:

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

Source/WebCore/ChangeLog
Source/WebCore/loader/LinkLoader.cpp
Source/WebCore/loader/LinkLoader.h

index 2736c12..8ef2abb 100644 (file)
@@ -1,3 +1,31 @@
+2011-06-24  Gavin Peters  <gavinp@chromium.org>
+
+        Reviewed by Darin Adler.
+
+        fix possible race in LinkLoader
+        https://bugs.webkit.org/show_bug.cgi?id=63360
+
+        In chromium bug 80729
+        http://code.google.com/p/chromium/issues/detail?id=80729 I am
+        seeing some kind of double triggering of the timer; I am concerned
+        that it is possible that a Link element errors out or succeeds,
+        sets a timer, and shortly before the timer is triggered it is
+        editted, launches another request.  After that, the first timer
+        triggers, zeroing out m_cachedResource.  Then, the second load
+        finishes, and *crash*.  If this is the case, this fix should stop
+        it.
+
+        No new tests; I haven't reproduced this.  I hope chrome's crash
+        telemetry will give good feedback; this crash is occuring many times a
+        day so the difference should be obvious.
+
+        * loader/LinkLoader.cpp:
+        (WebCore::LinkLoader::LinkLoader):
+        (WebCore::LinkLoader::linkLoadTimerFired):
+        (WebCore::LinkLoader::linkLoadingErrorTimerFired):
+        (WebCore::LinkLoader::notifyFinished):
+        * loader/LinkLoader.h:
+
 2011-06-24  Jer Noble  <jer.noble@apple.com>
 
         Reviewed by Eric Carlson.
index 63c9e91..b924088 100644 (file)
@@ -48,7 +48,8 @@ namespace WebCore {
 
 LinkLoader::LinkLoader(LinkLoaderClient* client)
     : m_client(client)
-    , m_linkLoadedTimer(this, &LinkLoader::linkLoadedTimerFired)
+    , m_linkLoadTimer(this, &LinkLoader::linkLoadTimerFired)
+    , m_linkLoadingErrorTimer(this, &LinkLoader::linkLoadingErrorTimerFired)
 {
 }
 
@@ -58,21 +59,29 @@ LinkLoader::~LinkLoader()
         m_cachedLinkResource->removeClient(this);
 }
 
-void LinkLoader::linkLoadedTimerFired(Timer<LinkLoader>* timer)
+void LinkLoader::linkLoadTimerFired(Timer<LinkLoader>* timer)
 {
-    ASSERT_UNUSED(timer, timer == &m_linkLoadedTimer);
-    if (m_cachedLinkResource->errorOccurred())
-        m_client->linkLoadingErrored();
-    else if (!m_cachedLinkResource->wasCanceled())
-        m_client->linkLoaded();
-    m_cachedLinkResource->removeClient(this);
-    m_cachedLinkResource = 0;
+    ASSERT_UNUSED(timer, timer == &m_linkLoadTimer);
+    m_client->linkLoaded();
+}
+
+void LinkLoader::linkLoadingErrorTimerFired(Timer<LinkLoader>* timer)
+{
+    ASSERT_UNUSED(timer, timer == &m_linkLoadingErrorTimer);
+    m_client->linkLoadingErrored();
 }
 
 void LinkLoader::notifyFinished(CachedResource* resource)
 {
     ASSERT_UNUSED(resource, m_cachedLinkResource.get() == resource);
-    m_linkLoadedTimer.startOneShot(0);
+
+    if (m_cachedLinkResource->errorOccurred())
+        m_linkLoadingErrorTimer.startOneShot(0);
+    else 
+        m_linkLoadTimer.startOneShot(0);
+
+    m_cachedLinkResource->removeClient(this);
+    m_cachedLinkResource = 0;
 }
 
 bool LinkLoader::loadLink(const LinkRelAttribute& relAttribute, const String& type,
index ee65e99..6c448d8 100644 (file)
@@ -53,12 +53,14 @@ public:
     bool loadLink(const LinkRelAttribute&, const String& type, const KURL&, Document*);
 
 private:
-    void linkLoadedTimerFired(Timer<LinkLoader>*);
+    void linkLoadTimerFired(Timer<LinkLoader>*);
+    void linkLoadingErrorTimerFired(Timer<LinkLoader>*);
 
     LinkLoaderClient* m_client;
 
     CachedResourceHandle<CachedResource> m_cachedLinkResource;
-    Timer<LinkLoader> m_linkLoadedTimer;
+    Timer<LinkLoader> m_linkLoadTimer;
+    Timer<LinkLoader> m_linkLoadingErrorTimer;
 };
     
 }