Reviewed by Maciej.
authordarin <darin@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 3 Jul 2007 22:02:27 +0000 (22:02 +0000)
committerdarin <darin@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 3 Jul 2007 22:02:27 +0000 (22:02 +0000)
        - fix <rdar://problem/5310848> WebDataSource lifetime problem -- may be cause of the leaks seen on the buildbot

        * WebView/WebDataSource.mm:
        (-[WebDataSourcePrivate dealloc]): Added a call to the new detachDataSource function.
        (-[WebDataSourcePrivate finalize]): Ditto.

        * WebView/WebDocumentLoaderMac.h: Added detachDataSource function to be used when the
        WebDataSource is deallocated. Added retain/releaseDataSource helper functions to be
        used to retain and release the data source object. Replaced the m_hasEverBeenDetached
        boolean with a more primitive and hence easier to understand m_isDataSourceRetained boolean.

        * WebView/WebDocumentLoaderMac.mm:
        (WebDocumentLoaderMac::WebDocumentLoaderMac): Initialize m_isDataSourceRetained to false.
        (WebDocumentLoaderMac::setDataSource): Call retainDataSource instead of calling HardRetain
        on the dataSource parameter. Also updated a comment.
        (WebDocumentLoaderMac::attachToFrame): Call retainDataSource unconditionally rather than
        trying to use m_hasEverBeenDetached to decide if a retain is needed. Also got rid of an
        assertion that m_loadingResources is empty -- not important any more.
        (WebDocumentLoaderMac::detachFromFrame): Call releaseDataSource instead of using
        HardRelease, but only if m_loadingResources is empty. If it's non-empty, then we'll
        do the releaseDataSource later in decreaseLoadCount.
        (WebDocumentLoaderMac::increaseLoadCount): Call retainDataSource unconditionally
        rather than calling HardRetain only if the old set of resources was empty.
        (WebDocumentLoaderMac::decreaseLoadCount): Call releaseDataSource if m_loadingResources
        is empty and we're not attached to a frame. If we are attached to a frame, then
        we'll do the releaseDataSource later in detachFromFrame.
        (WebDocumentLoaderMac::retainDataSource): Added. Calls CFRetain, but only if the data
        source is not already retained (according to the boolean).
        (WebDocumentLoaderMac::releaseDataSource): Added. Calls CFRelease, but only if the data
        source is currently retained (according to the boolean).
        (WebDocumentLoaderMac::detachDataSource): Added. Sets m_dataSource to nil. Since this
        is only called from WebDataSource's dealloc and finalize methods, it won't ever be called
        when the m_isDataSourceRetained boolean is true.

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

WebKit/ChangeLog
WebKit/WebView/WebDataSource.mm
WebKit/WebView/WebDocumentLoaderMac.h
WebKit/WebView/WebDocumentLoaderMac.mm

index eb07e64a73e3eed10d4578a4eef5eba9cc8f9658..308b3159aee381e8f23ca74ff01dcb52202dc1f6 100644 (file)
@@ -1,3 +1,41 @@
+2007-07-03  Darin Adler  <darin@apple.com>
+
+        Reviewed by Maciej.
+
+        - fix <rdar://problem/5310848> WebDataSource lifetime problem -- may be cause of the leaks seen on the buildbot
+
+        * WebView/WebDataSource.mm:
+        (-[WebDataSourcePrivate dealloc]): Added a call to the new detachDataSource function.
+        (-[WebDataSourcePrivate finalize]): Ditto.
+
+        * WebView/WebDocumentLoaderMac.h: Added detachDataSource function to be used when the
+        WebDataSource is deallocated. Added retain/releaseDataSource helper functions to be
+        used to retain and release the data source object. Replaced the m_hasEverBeenDetached
+        boolean with a more primitive and hence easier to understand m_isDataSourceRetained boolean.
+
+        * WebView/WebDocumentLoaderMac.mm:
+        (WebDocumentLoaderMac::WebDocumentLoaderMac): Initialize m_isDataSourceRetained to false.
+        (WebDocumentLoaderMac::setDataSource): Call retainDataSource instead of calling HardRetain
+        on the dataSource parameter. Also updated a comment.
+        (WebDocumentLoaderMac::attachToFrame): Call retainDataSource unconditionally rather than
+        trying to use m_hasEverBeenDetached to decide if a retain is needed. Also got rid of an
+        assertion that m_loadingResources is empty -- not important any more.
+        (WebDocumentLoaderMac::detachFromFrame): Call releaseDataSource instead of using
+        HardRelease, but only if m_loadingResources is empty. If it's non-empty, then we'll
+        do the releaseDataSource later in decreaseLoadCount.
+        (WebDocumentLoaderMac::increaseLoadCount): Call retainDataSource unconditionally
+        rather than calling HardRetain only if the old set of resources was empty.
+        (WebDocumentLoaderMac::decreaseLoadCount): Call releaseDataSource if m_loadingResources
+        is empty and we're not attached to a frame. If we are attached to a frame, then
+        we'll do the releaseDataSource later in detachFromFrame.
+        (WebDocumentLoaderMac::retainDataSource): Added. Calls CFRetain, but only if the data
+        source is not already retained (according to the boolean).
+        (WebDocumentLoaderMac::releaseDataSource): Added. Calls CFRelease, but only if the data
+        source is currently retained (according to the boolean).
+        (WebDocumentLoaderMac::detachDataSource): Added. Sets m_dataSource to nil. Since this
+        is only called from WebDataSource's dealloc and finalize methods, it won't ever be called
+        when the m_isDataSourceRetained boolean is true.
+
 2007-07-03  Darin Adler  <darin@apple.com>
 
         - forgot to check in one file in the fix for <rdar://problem/5307880>
index 39c9201c3480d7ecfe5f864b3ca0bc31b6e934f4..43f10ba738d0b864dfa92a51aadaa4252b37e395 100644 (file)
 
 using namespace WebCore;
 
-@interface WebDataSourcePrivate : NSObject
-{
-    @public
-    
-    WebDocumentLoaderMac *loader;
+@interface WebDataSourcePrivate : NSObject {
+@public
+    WebDocumentLoaderMac* loader;
    
     id <WebDocumentRepresentation> representation;
     
     WebUnarchivingState *unarchivingState;
     BOOL representationFinishedLoading;
 }
-
 @end
 
 @implementation WebDataSourcePrivate 
@@ -87,7 +84,7 @@ using namespace WebCore;
 - (void)dealloc
 {
     ASSERT(!loader->isLoading());
-
+    loader->detachDataSource();
     loader->deref();
     
     [representation release];
@@ -98,8 +95,10 @@ using namespace WebCore;
 
 - (void)finalize
 {
-    ASSERT(!loader->isLoading());
+    ASSERT_MAIN_THREAD();
 
+    ASSERT(!loader->isLoading());
+    loader->detachDataSource();
     loader->deref();
 
     [super finalize];
index fbb33f2e1efce005225bb2035c4ba991be0da5f6..9acba890ee147485654023158febda207a28b5b1 100644 (file)
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2006 Apple Computer, Inc.  All rights reserved.
+ * Copyright (C) 2006, 2007 Apple Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
@@ -37,12 +37,12 @@ namespace WebCore {
     class ResourceRequest;
 }
 
-class WebDocumentLoaderMac : public WebCore::DocumentLoader
-{
+class WebDocumentLoaderMac : public WebCore::DocumentLoader {
 public:
     WebDocumentLoaderMac(const WebCore::ResourceRequest&, const WebCore::SubstituteData&);
 
     void setDataSource(WebDataSource *, WebView*);
+    void detachDataSource();
     WebDataSource *dataSource() const;
 
     virtual void attachToFrame();
@@ -50,10 +50,14 @@ public:
 
     void increaseLoadCount(unsigned long identifier);
     void decreaseLoadCount(unsigned long identifier);
+
 private:
+    void retainDataSource();
+    void releaseDataSource();
+
     WebDataSource *m_dataSource;
+    bool m_isDataSourceRetained;
     RetainPtr<id> m_resourceLoadDelegate;
     RetainPtr<id> m_downloadDelegate;
-    bool m_hasEverBeenDetached;
     HashSet<unsigned long> m_loadingResources;
 };
index 0b1e64079957651ba11fda06f943ea0fe69f6759..26e60a41ce402f0a6544aaf26c14e476846d4f6e 100644 (file)
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2006 Apple Computer, Inc.  All rights reserved.
+ * Copyright (C) 2006, 2007 Apple Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
 
 #import "WebDocumentLoaderMac.h"
 
-#import <JavaScriptCore/Assertions.h>
-#import <WebCore/SubstituteData.h>
-#import <WebCore/FoundationExtras.h>
-
 #import "WebView.h"
 
 using namespace WebCore;
@@ -39,7 +35,7 @@ using namespace WebCore;
 WebDocumentLoaderMac::WebDocumentLoaderMac(const ResourceRequest& request, const SubstituteData& substituteData)
     : DocumentLoader(request, substituteData)
     , m_dataSource(nil)
-    , m_hasEverBeenDetached(false)
+    , m_isDataSourceRetained(false)
 {
 }
 
@@ -60,13 +56,17 @@ static inline bool needsAppKitWorkaround(WebView *webView)
 void WebDocumentLoaderMac::setDataSource(WebDataSource *dataSource, WebView *webView)
 {
     ASSERT(!m_dataSource);
-    HardRetain(dataSource);
+    ASSERT(!m_isDataSourceRetained);
+
     m_dataSource = dataSource;
-    
+    retainDataSource();
+
     m_resourceLoadDelegate = [webView resourceLoadDelegate];
     m_downloadDelegate = [webView downloadDelegate];
     
-    // Possibly work around a bug in Tiger AppKit where timers do not fire sometimes
+    // Work around a bug in Tiger AppKit's use of WebKit. The particular idiom used
+    // won't allow the timer to run, so deferring the main resource load with a timer
+    // causes a delay until something else wakes the run loop.
     // See <rdar://problem/5266289>
     if (needsAppKitWorkaround(webView))
         m_deferMainResourceDataLoad = false;
@@ -80,42 +80,64 @@ WebDataSource *WebDocumentLoaderMac::dataSource() const
 void WebDocumentLoaderMac::attachToFrame()
 {
     DocumentLoader::attachToFrame();
-    ASSERT(m_loadingResources.isEmpty());
 
-    if (m_hasEverBeenDetached)
-        HardRetain(m_dataSource);
+    retainDataSource();
 }
 
 void WebDocumentLoaderMac::detachFromFrame()
 {
     DocumentLoader::detachFromFrame();
-  
-    m_hasEverBeenDetached = true;
-    HardRelease(m_dataSource);
+
+    if (m_loadingResources.isEmpty())
+        releaseDataSource();
+
+    // FIXME: What prevents the data source from getting deallocated while the
+    // frame is not attached?
 }
 
 void WebDocumentLoaderMac::increaseLoadCount(unsigned long identifier)
 {
     ASSERT(m_dataSource);
-    
+
     if (m_loadingResources.contains(identifier))
         return;
-
-    if (m_loadingResources.isEmpty())
-        HardRetain(m_dataSource);
-
     m_loadingResources.add(identifier);
+
+    retainDataSource();
 }
 
 void WebDocumentLoaderMac::decreaseLoadCount(unsigned long identifier)
 {
     ASSERT(m_loadingResources.contains(identifier));
-    
     m_loadingResources.remove(identifier);
     
     if (m_loadingResources.isEmpty()) {
         m_resourceLoadDelegate = 0;
         m_downloadDelegate = 0;
-        HardRelease(m_dataSource);
+        if (!frame())
+            releaseDataSource();
     }
 }
+
+void WebDocumentLoaderMac::retainDataSource()
+{
+    if (m_isDataSourceRetained || !m_dataSource)
+        return;
+    m_isDataSourceRetained = true;
+    CFRetain(m_dataSource);
+}
+
+void WebDocumentLoaderMac::releaseDataSource()
+{
+    if (!m_isDataSourceRetained)
+        return;
+    ASSERT(m_dataSource);
+    m_isDataSourceRetained = false;
+    CFRelease(m_dataSource);
+}
+
+void WebDocumentLoaderMac::detachDataSource()
+{
+    ASSERT(!m_isDataSourceRetained);
+    m_dataSource = nil;
+}