<rdar://problem/5453494> Better lifetime management of WebDataSource and...

        The problem was that WebDataSource does not hold a strong reference to the WebDocumentLoader.  If
        a WebDataSource is still around after the loader has been destroyed, it'll just point to
        a stale WebDocumentLoader.
        To fix this without a circular reference, WebDataSource now holds a strong reference to the
        WebDocumentLoader.  The WebDocumentLoader holds a strong reference to the WebDataSource
        until it's detached from the WebFrame.  When the WebDataSource is destroyed, it'll notify
        its WebDocumentLoader so the loader will clear any references to it.

        Reviewed by Darin.

        * WebDataSource.cpp:
        (WebDataSource::~WebDataSource): call WebDocumentLoader::detachDataSource() so the loader
        will clear any references to this data source
        (WebDataSource::documentLoader): m_loader is now a RefPtr so we need to call get().
        * WebDataSource.h:
        * WebDocumentLoader.cpp:
        (WebDocumentLoader::WebDocumentLoader): initialize m_dataSource since it's no longer a COMPtr.
        (WebDocumentLoader::~WebDocumentLoader): release m_dataSource if necessary
        (WebDocumentLoader::setDataSource): add a reference to m_dataSource
        (WebDocumentLoader::dataSource):
        (WebDocumentLoader::detachDataSource): clear m_detachedDataSource.
        (WebDocumentLoader::attachToFrame): call setDataSource() so it'll add the reference to the data source if necessary.
        (WebDocumentLoader::detachFromFrame): release the reference to the data source
        * WebDocumentLoader.h:

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

diff --git a/WebKit/win/ChangeLog b/WebKit/win/ChangeLog
index 4892e537773a50af871c68ed77b2c3876525b9d8..63e3955b4bd4ad1e4522a532818408560f7f5074 100644 (file)
--- a/WebKit/win/ChangeLog
+++ b/WebKit/win/ChangeLog
@@ -1,3 +1,31 @@
+2007-09-07  Ada Chan  <adachan@apple.com>
+
+        <rdar://problem/5453494> Better lifetime management of WebDataSource and WebDocumentLoader
+        The problem was that WebDataSource does not hold a strong reference to the WebDocumentLoader.  If
+        a WebDataSource is still around after the loader has been destroyed, it'll just point to
+        a stale WebDocumentLoader.
+        To fix this without a circular reference, WebDataSource now holds a strong reference to the
+        WebDocumentLoader.  The WebDocumentLoader holds a strong reference to the WebDataSource
+        until it's detached from the WebFrame.  When the WebDataSource is destroyed, it'll notify
+        its WebDocumentLoader so the loader will clear any references to it.
+
+        Reviewed by Darin.
+
+        * WebDataSource.cpp:
+        (WebDataSource::~WebDataSource): call WebDocumentLoader::detachDataSource() so the loader
+        will clear any references to this data source
+        (WebDataSource::documentLoader): m_loader is now a RefPtr so we need to call get().
+        * WebDataSource.h:
+        * WebDocumentLoader.cpp:
+        (WebDocumentLoader::WebDocumentLoader): initialize m_dataSource since it's no longer a COMPtr.
+        (WebDocumentLoader::~WebDocumentLoader): release m_dataSource if necessary
+        (WebDocumentLoader::setDataSource): add a reference to m_dataSource
+        (WebDocumentLoader::dataSource): 
+        (WebDocumentLoader::detachDataSource): clear m_detachedDataSource.
+        (WebDocumentLoader::attachToFrame): call setDataSource() so it'll add the reference to the data source if necessary.
+        (WebDocumentLoader::detachFromFrame): release the reference to the data source
+        * WebDocumentLoader.h:
+
 2007-09-05  Dave Hyatt <hyatt@apple.com>
 
         Make sure ALT+other keys is properly sent into the DOM so that Web pages (and editing fields) can

diff --git a/WebKit/win/WebDataSource.cpp b/WebKit/win/WebDataSource.cpp
index b2b719bce53b48693642b72a248dde4fddb74f6c..2c33ca2480d309f87aac4abdc9c99e80793c2647 100644 (file)
--- a/WebKit/win/WebDataSource.cpp
+++ b/WebKit/win/WebDataSource.cpp
@@ -65,6 +65,8 @@ WebDataSource::WebDataSource(WebDocumentLoader* loader)
 
 WebDataSource::~WebDataSource()
 {
+    if (m_loader)
+        m_loader->detachDataSource();
     WebDataSourceCount--;
     gClassCount--;
 }
@@ -78,7 +80,7 @@ WebDataSource* WebDataSource::createInstance(WebDocumentLoader* loader)
 
 WebDocumentLoader* WebDataSource::documentLoader() const
 {
-    return m_loader;
+    return m_loader.get();
 }
 
 // IWebDataSourcePrivate ------------------------------------------------------

diff --git a/WebKit/win/WebDataSource.h b/WebKit/win/WebDataSource.h
index fc0c91a3205f7001f23830f3efcdfe3b2c1432a7..50eee9b0d31a20c1b1aec02d5e5db78087aa6f43 100644 (file)
--- a/WebKit/win/WebDataSource.h
+++ b/WebKit/win/WebDataSource.h
@@ -28,6 +28,7 @@
 
 #include "IWebDataSource.h"
 #include "COMPtr.h"
+#include <WTF/RefPtr.h>
 
 class WebDocumentLoader;
 class WebMutableURLRequest;
@@ -113,7 +114,7 @@ public:
     WebDocumentLoader* documentLoader() const;
 protected:
     ULONG m_refCount;
-    WebDocumentLoader* m_loader;
+    RefPtr<WebDocumentLoader> m_loader;
     COMPtr<IWebDocumentRepresentation> m_representation;
 };
 

diff --git a/WebKit/win/WebDocumentLoader.cpp b/WebKit/win/WebDocumentLoader.cpp
index caba735fdf315e15d1e1139686c7c2baa763bdcf..f333c67fb3cd29a1e6d652e7f56c68ae4a071aad 100644 (file)
--- a/WebKit/win/WebDocumentLoader.cpp
+++ b/WebKit/win/WebDocumentLoader.cpp
@@ -31,18 +31,38 @@ using namespace WebCore;
 
 WebDocumentLoader::WebDocumentLoader(const ResourceRequest& request, const SubstituteData& substituteData)
     : DocumentLoader(request, substituteData)
+    , m_dataSource(0)
     , m_detachedDataSource(0)
 {
 }
 
+WebDocumentLoader::~WebDocumentLoader()
+{
+    if (m_dataSource) {
+        ASSERT(!m_detachedDataSource);
+        m_dataSource->Release();
+    }
+}
+
 void WebDocumentLoader::setDataSource(WebDataSource *dataSource)
 {
+    ASSERT(!m_dataSource);
     m_dataSource = dataSource;
+    if (m_dataSource)
+        m_dataSource->AddRef();
 }
 
 WebDataSource* WebDocumentLoader::dataSource() const
 {
-    return m_dataSource.get();
+    return m_dataSource;
+}
+
+void WebDocumentLoader::detachDataSource()
+{
+    // we only call detachDataSource when the WebDataSource is freed - and we won't get there if m_dataSource is not
+    // null (because that would mean the loader still has a reference to the data source)
+    ASSERT(!m_dataSource);
+    m_detachedDataSource = 0;
 }
 
 void WebDocumentLoader::attachToFrame()
@@ -50,7 +70,7 @@ void WebDocumentLoader::attachToFrame()
     DocumentLoader::attachToFrame();
     if (m_detachedDataSource) {
         ASSERT(!m_dataSource);
-        m_dataSource = m_detachedDataSource;
+        setDataSource(m_detachedDataSource);
         m_detachedDataSource = 0;
     }
 }
@@ -58,6 +78,13 @@ void WebDocumentLoader::attachToFrame()
 void WebDocumentLoader::detachFromFrame()
 {
     DocumentLoader::detachFromFrame();
-    m_detachedDataSource = m_dataSource.get();
-    m_dataSource = 0;
+    m_detachedDataSource = m_dataSource;
+    if (m_dataSource) {
+        WebDataSource* dataSourceToBeReleased = m_dataSource;
+        // It's important to null out m_dataSource before calling release on the data source.  That release can cause the data
+        // source to be deleted - which ends up calling loader->detachDataSource() which makes the assumption that the loader no 
+        // longer holds a reference to the data source.
+        m_dataSource = 0;
+        dataSourceToBeReleased->Release();
+    }
 }

diff --git a/WebKit/win/WebDocumentLoader.h b/WebKit/win/WebDocumentLoader.h
index ac9c3054e19f2d3984c059bf40d423bf965030b9..e6e628af6bf5d96d78085175c9ce9b11c7a66f65 100644 (file)
--- a/WebKit/win/WebDocumentLoader.h
+++ b/WebKit/win/WebDocumentLoader.h
@@ -25,8 +25,6 @@
 
 #include "WebDataSource.h"
 
-#include "COMPtr.h"
-
 #pragma warning(push, 0)
 #include <WebCore/DocumentLoader.h>
 #pragma warning(pop)
@@ -35,14 +33,16 @@ class WebDocumentLoader : public WebCore::DocumentLoader
 {
 public:
     WebDocumentLoader(const WebCore::ResourceRequest&, const WebCore::SubstituteData&);
+    ~WebDocumentLoader();
 
     void setDataSource(WebDataSource*);
     WebDataSource* dataSource() const;
+    void detachDataSource();
 
     virtual void attachToFrame();
     virtual void detachFromFrame();
 
 private:
-    COMPtr<WebDataSource> m_dataSource;
+    WebDataSource* m_dataSource;
     WebDataSource* m_detachedDataSource; // not retained
 };