[haiku-webkit-commits] r299 - in webkit/trunk/WebKit/haiku: API WebCoreSupport

  • From: webkit@xxxxxxxxxxxxxxx
  • To: haiku-webkit-commits@xxxxxxxxxxxxx
  • Date: Mon, 08 Mar 2010 20:37:21 +0000

Author: stippi
Date: Mon Mar  8 20:37:20 2010
New Revision: 299
URL: http://mmlr.dyndns.org/changeset/299

Log:
Finally wrapped my head around the ref counting problem of WebCore::Frame
instances. I've confirmed everything working and compared reference counts
with the Gtk port. Now all frames are correctly free'd. Introduced BWebFrame::
Shutdown(), to ballance allocations in the same class file.

Modified:
   webkit/trunk/WebKit/haiku/API/WebFrame.cpp
   webkit/trunk/WebKit/haiku/API/WebFrame.h
   webkit/trunk/WebKit/haiku/API/WebFramePrivate.h
   webkit/trunk/WebKit/haiku/API/WebPage.cpp
   webkit/trunk/WebKit/haiku/API/WebPage.h
   webkit/trunk/WebKit/haiku/WebCoreSupport/FrameLoaderClientHaiku.cpp

Modified: webkit/trunk/WebKit/haiku/API/WebFrame.cpp
==============================================================================
--- webkit/trunk/WebKit/haiku/API/WebFrame.cpp  Mon Mar  8 13:20:09 2010        
(r298)
+++ webkit/trunk/WebKit/haiku/API/WebFrame.cpp  Mon Mar  8 20:37:20 2010        
(r299)
@@ -56,42 +56,39 @@
 
 using namespace WebCore;
 
-#if TRACE_SHUTDOWN_PROCESS
-#define TRACE_SHUTDOWN(x...) printf(x)
-#else
-#define TRACE_SHUTDOWN(x...) do {} while (false)
-#endif
-
 BWebFrame::BWebFrame(BWebPage* webPage, BWebFrame* parentFrame, 
WebFramePrivate* data)
     : fTextMagnifier(1.0)
     , fIsEditable(false)
     , fTitle(0)
     , fData(data)
 {
-TRACE_SHUTDOWN("%p->BWebFrame::BWebFrame()\n", this);
        fData->loaderClient = new WebCore::FrameLoaderClientHaiku(webPage, 
this);
-    fData->frame = WebCore::Frame::create(fData->page, fData->ownerElement,
+    RefPtr<WebCore::Frame> frame = WebCore::Frame::create(fData->page, 
fData->ownerElement,
         fData->loaderClient);
+    // We don't keep the reference to the Frame, see WebFramePrivate.h.
+    fData->frame = frame.get();
 
     fData->frame->tree()->setName(fData->name);
     if (parentFrame)
         parentFrame->Frame()->tree()->appendChild(fData->frame);
 
-       // TODO: We will be left with an extra reference to the WebCore::Frame, 
but I have
-       // not yet fully understood how to get rid of it in the proper way.
-
     fData->frame->init();
 }
 
 BWebFrame::~BWebFrame()
 {
-TRACE_SHUTDOWN("%p->BWebFrame::~BWebFrame()\n", this);
-       // NOTE: This is currently never called, because we hage one
-       // reference too many on the WebCore::Frame. See above.
-    fData->frame->loader()->cancelAndClear();
+    delete fData->loaderClient;
+    delete fData;
+}
 
-       delete fData;
-TRACE_SHUTDOWN("%p->BWebFrame::~BWebFrame() - done\n", this);
+void
+BWebFrame::Shutdown()
+{
+       // The private method is only invoked from the FrameLoaderClient, as
+       // no one else keeps track of a BWebFrame object's lifetime, we tie
+       // to the WebCore::Frame/FrameLoader lifetime and shutdown via
+       // the FrameLoaderClient::frameLoaderDestroyed() hook.
+       delete this;
 }
 
 void BWebFrame::SetListener(const BMessenger& listener)
@@ -298,7 +295,7 @@
     if (view && view->layoutPending())
         view->layout();
 
-    return externalRepresentation(fData->frame.get());
+    return externalRepresentation(fData->frame);
 }
 
 bool BWebFrame::FindString(const char* string, bool forward, bool 
caseSensitive, bool wrapSelection, bool startInSelection)
@@ -380,5 +377,5 @@
 
 WebCore::Frame* BWebFrame::Frame() const
 {
-    return fData->frame.get();
+    return fData->frame;
 }

Modified: webkit/trunk/WebKit/haiku/API/WebFrame.h
==============================================================================
--- webkit/trunk/WebKit/haiku/API/WebFrame.h    Mon Mar  8 13:20:09 2010        
(r298)
+++ webkit/trunk/WebKit/haiku/API/WebFrame.h    Mon Mar  8 20:37:20 2010        
(r299)
@@ -46,8 +46,6 @@
 
 class BWebFrame {
 public:
-                                                               ~BWebFrame();
-
                        void                            SetListener(const 
BMessenger& listener);
 
                        void                            LoadURL(BString url);
@@ -114,6 +112,9 @@
                                                                
BWebFrame(BWebPage* webPage,
                                                                        
BWebFrame* parentFrame,
                                                                        
WebFramePrivate* data);
+                                                               ~BWebFrame();
+
+                       void                            Shutdown();
 
                        void                            LoadURL(WebCore::KURL);
                        WebCore::Frame*         Frame() const;

Modified: webkit/trunk/WebKit/haiku/API/WebFramePrivate.h
==============================================================================
--- webkit/trunk/WebKit/haiku/API/WebFramePrivate.h     Mon Mar  8 13:20:09 
2010        (r298)
+++ webkit/trunk/WebKit/haiku/API/WebFramePrivate.h     Mon Mar  8 20:37:20 
2010        (r299)
@@ -56,7 +56,13 @@
     WebCore::String requestedURL;
     WebCore::HTMLFrameOwnerElement* ownerElement;
     WebCore::Page* page;
-    WTF::RefPtr<WebCore::Frame> frame;
+    // NOTE: We don't keep a reference pointer for the WebCore::Frame, since
+    // that will leave us with one too many references, which will in turn
+    // prevent the shutdown mechanism from working, since that one is only
+    // triggered from the FrameLoader destructor, i.e. when there are no more
+    // references around. (FrameLoader and Frame used to be one class, they
+    // can be considered as one object as far as object life-time goes.)
+    WebCore::Frame* frame;
     WebCore::FrameLoaderClientHaiku* loaderClient;
 };
 

Modified: webkit/trunk/WebKit/haiku/API/WebPage.cpp
==============================================================================
--- webkit/trunk/WebKit/haiku/API/WebPage.cpp   Mon Mar  8 13:20:09 2010        
(r298)
+++ webkit/trunk/WebKit/haiku/API/WebPage.cpp   Mon Mar  8 20:37:20 2010        
(r299)
@@ -80,12 +80,6 @@
 #include <wtf/Assertions.h>
 #include <wtf/Threading.h>
 
-#if TRACE_SHUTDOWN_PROCESS
-#define TRACE_SHUTDOWN(x...) printf(x)
-#else
-#define TRACE_SHUTDOWN(x...) do {} while (false)
-#endif
-
 /*
      The basic idea here is to dispatch all public methods to the BLooper
      to which the handler is attached (should be the be_app), such that
@@ -189,7 +183,6 @@
 
 BWebPage::~BWebPage()
 {
-TRACE_SHUTDOWN("%p->BWebPage::~BWebPage()\n", this);
        // We need to make sure there are no more timers running, since those
        // arrive to a different, global handler (the timer handler), and the
        // timer functions would then operate on stale pointers.
@@ -198,9 +191,10 @@
     if (m_mainFrame && m_mainFrame->Frame())
         m_mainFrame->Frame()->loader()->detachFromParent();
 
-// NOTE: The m_webFrame member should be deleted by the FrameLoaderClientHaiku,
-// but currently, we have an extra reference to the WebCore::Frame, which needs
-// to be fixed.
+    // NOTE: The m_webFrame member will be deleted by the
+    // FrameLoaderClientHaiku, when the WebCore::Frame/FrameLoader instance is
+    // free'd. For sub-frames, we don't maintain them anyway, and for the
+    // main frame, the same mechanism is used.
     delete m_settings;
     delete m_page;
     // Deleting the BWebView is deferred here to keep it alive in
@@ -209,7 +203,6 @@
     // deleted itself before we reach the shutdown message, there would be
     // a race condition and chance to operate on a stale BWebView pointer.
     delete m_webView;
-TRACE_SHUTDOWN("%p->BWebPage::~BWebPage() - done\n", this);
 }
 
 // #pragma mark - public

Modified: webkit/trunk/WebKit/haiku/API/WebPage.h
==============================================================================
--- webkit/trunk/WebKit/haiku/API/WebPage.h     Mon Mar  8 13:20:09 2010        
(r298)
+++ webkit/trunk/WebKit/haiku/API/WebPage.h     Mon Mar  8 20:37:20 2010        
(r299)
@@ -65,8 +65,6 @@
        B_DOWNLOAD_REMOVED              = 'dwnr'
 };
 
-#define TRACE_SHUTDOWN_PROCESS 0
-
 typedef enum {
        B_WEBKIT_CACHE_MODEL_DOCUMENT_VIEWER = 0,
        B_WEBKIT_CACHE_MODEL_WEB_BROWSER

Modified: webkit/trunk/WebKit/haiku/WebCoreSupport/FrameLoaderClientHaiku.cpp
==============================================================================
--- webkit/trunk/WebKit/haiku/WebCoreSupport/FrameLoaderClientHaiku.cpp Mon Mar 
 8 13:20:09 2010        (r298)
+++ webkit/trunk/WebKit/haiku/WebCoreSupport/FrameLoaderClientHaiku.cpp Mon Mar 
 8 20:37:20 2010        (r299)
@@ -71,12 +71,7 @@
 #include <Message.h>
 #include <MimeType.h>
 #include <String.h>
-
-#if TRACE_SHUTDOWN_PROCESS
-#define TRACE_SHUTDOWN(x...) printf(x)
-#else
-#define TRACE_SHUTDOWN(x...) do {} while (false)
-#endif
+#include <debugger.h>
 
 namespace WebCore {
 
@@ -86,7 +81,6 @@
     , m_messenger()
     , m_loadingErrorPage(false)
 {
-TRACE_SHUTDOWN("%p->FrameLoaderClientHaiku::FrameLoaderClientHaiku(%p)\n", 
this, webPage);
     ASSERT(m_webPage);
     ASSERT(m_webFrame);
 }
@@ -98,14 +92,10 @@
 
 void FrameLoaderClientHaiku::frameLoaderDestroyed()
 {
-TRACE_SHUTDOWN("%p->FrameLoaderClientHaiku::frameLoaderDestroyed()\n", this);
     // No one else feels responsible for the BWebFrame instance that created 
us.
-    // Even cleaner would be introducing a BWebFrame method to forward this
-    // notification, so that BWebFrame deletes itself there, and us along,
-    // doesn't really matter though.
-    delete m_webFrame;
-    delete this;
-TRACE_SHUTDOWN("%p->FrameLoaderClientHaiku::frameLoaderDestroyed() - done\n", 
this);
+    // Through Shutdown(), we initiate the deletion sequence, after this 
method returns,
+    // this object will be gone.
+    m_webFrame->Shutdown();
 }
 
 bool FrameLoaderClientHaiku::hasWebView() const
@@ -887,6 +877,9 @@
     data->page = m_webPage->page();
 
     BWebFrame* frame = new BWebFrame(m_webPage, m_webFrame, data);
+    // The ownership of "frame" is implicitely transferred to the 
FrameLoadClientHaiku
+    // instance which is created in the BWebFrame consructor.
+
     // As long as we don't return the Frame, we are responsible for deleting 
it.
     RefPtr<Frame> childFrame = frame->Frame();
 

Other related posts:

  • » [haiku-webkit-commits] r299 - in webkit/trunk/WebKit/haiku: API WebCoreSupport - webkit