Changeset 34271 in webkit

Timestamp:

May 30, 2008 10:37:03 PM (16 years ago)

Author:

mjs@apple.com

Message:

2008-05-30 Maciej Stachowiak <​mjs@apple.com>

Reviewed by Oliver (earlier version reviewed by Alexey).

• speculative fix for "REGRESSION(r34143?): Frequent crash while browsing" ​https://bugs.webkit.org/show_bug.cgi?id=19285

("This Time for Sure" Edition)

I'm pretty sure this fixes it but I have not been able to
reproduce and am unsure if my theory of the bug is right.

I belive the bug was because JSDOMWindowBase accessed
JSDOMWindowShell in its destructor to remove itself from a
hashtable, but GC destructor order is not guaranteed, so the
hashtable may have been freed already. This patch changes things
so that a non-GC object (the KJSProxy) does the tracking of live
window objects for a frame. JSDOMWindowBase can null check the frame
pointer to verify if it is still good.

In addition, we must create a similar setup between DOMWindow and
Frame; since the DOMWindow of a given frame can now change over
time, we must ensure that the Frame disconnects every live
DOMWindow when destroyed, not just the last.

• bindings/js/JSDOMWindowBase.cpp: (WebCore::JSDOMWindowBase::~JSDOMWindowBase):
• bindings/js/JSDOMWindowShell.cpp: (WebCore::JSDOMWindowShell::JSDOMWindowShell):
• bindings/js/JSDOMWindowShell.h: (WebCore::JSDOMWindowShell::setWindow):
• bindings/js/kjs_proxy.cpp: (WebCore::KJSProxy::clear): (WebCore::KJSProxy::initScript): (WebCore::KJSProxy::updateDocument):
• bindings/js/kjs_proxy.h: (WebCore::KJSProxy::clearFormerWindow):
• page/DOMWindow.cpp: (WebCore::DOMWindow::~DOMWindow):
• page/Frame.cpp: (WebCore::Frame::~Frame): (WebCore::Frame::setDocument): (WebCore::Frame::clearDOMWindow): (WebCore::Frame::clearFormerDOMWindow):
• page/Frame.h:
• page/FramePrivate.h:

Location:

trunk/WebCore

Files:

• ChangeLog (modified) (1 diff)
• bindings/js/JSDOMWindowBase.cpp (modified) (1 diff)
• bindings/js/JSDOMWindowShell.cpp (modified) (2 diffs)
• bindings/js/JSDOMWindowShell.h (modified) (2 diffs)
• bindings/js/kjs_proxy.cpp (modified) (3 diffs)
• bindings/js/kjs_proxy.h (modified) (2 diffs)
• page/DOMWindow.cpp (modified) (1 diff)
• page/Frame.cpp (modified) (4 diffs)
• page/Frame.h (modified) (1 diff)
• page/FramePrivate.h (modified) (1 diff)

trunk/WebCore/ChangeLog

@@ +1 @@
+2008-05-30  Maciej Stachowiak  <mjs@apple.com>
+
+        Reviewed by Oliver (earlier version reviewed by Alexey).
+
+        - speculative fix for "REGRESSION(r34143?): Frequent crash while browsing"
+        https://bugs.webkit.org/show_bug.cgi?id=19285
+        
+        ("This Time for Sure" Edition)
+
+        I'm pretty sure this fixes it but I have not been able to
+        reproduce and am unsure if my theory of the bug is right.
+
+        I belive the bug was because JSDOMWindowBase accessed
+        JSDOMWindowShell in its destructor to remove itself from a
+        hashtable, but GC destructor order is not guaranteed, so the
+        hashtable may have been freed already. This patch changes things
+        so that a non-GC object (the KJSProxy) does the tracking of live
+        window objects for a frame. JSDOMWindowBase can null check the frame
+        pointer to verify if it is still good.
+        
+        In addition, we must create a similar setup between DOMWindow and
+        Frame; since the DOMWindow of a given frame can now change over
+        time, we must ensure that the Frame disconnects every live
+        DOMWindow when destroyed, not just the last.
+
+        * bindings/js/JSDOMWindowBase.cpp:
+        (WebCore::JSDOMWindowBase::~JSDOMWindowBase):
+        * bindings/js/JSDOMWindowShell.cpp:
+        (WebCore::JSDOMWindowShell::JSDOMWindowShell):
+        * bindings/js/JSDOMWindowShell.h:
+        (WebCore::JSDOMWindowShell::setWindow):
+        * bindings/js/kjs_proxy.cpp:
+        (WebCore::KJSProxy::clear):
+        (WebCore::KJSProxy::initScript):
+        (WebCore::KJSProxy::updateDocument):
+        * bindings/js/kjs_proxy.h:
+        (WebCore::KJSProxy::clearFormerWindow):
+        * page/DOMWindow.cpp:
+        (WebCore::DOMWindow::~DOMWindow):
+        * page/Frame.cpp:
+        (WebCore::Frame::~Frame):
+        (WebCore::Frame::setDocument):
+        (WebCore::Frame::clearDOMWindow):
+        (WebCore::Frame::clearFormerDOMWindow):
+        * page/Frame.h:
+        * page/FramePrivate.h:
+
 2008-05-30  Dan Bernstein  <mitz@apple.com>
 

trunk/WebCore/bindings/js/JSDOMWindowBase.cpp

@@ -203 +203 @@
 JSDOMWindowBase::~JSDOMWindowBase()
 {
-    d->m_shell->clearFormerWindow(asJSDOMWindow(this));
+    if (m_impl->frame())
+        m_impl->frame()->scriptProxy()->clearFormerWindow(asJSDOMWindow(this));
 
     clearAllTimeouts();

trunk/WebCore/bindings/js/JSDOMWindowShell.cpp

@@ -45 +45 @@
     : Base(jsNull())
     , m_window(0)
-    , m_liveFormerWindows(new HashSet<JSDOMWindow*>)
 {
     m_window = new JSDOMWindow(domWindow, this);
@@ -146 +145 @@
 
 
-void JSDOMWindowShell::updateDocument()
-{
-    m_window->updateDocument();
-    HashSet<JSDOMWindow*>::iterator end = m_liveFormerWindows->end();
-    for (HashSet<JSDOMWindow*>::iterator it = m_liveFormerWindows->begin(); it != end; ++it)
-        (*it)->updateDocument();
-}
-
 // ----
 // Conversion methods

trunk/WebCore/bindings/js/JSDOMWindowShell.h

@@ -48 +48 @@
         {
             ASSERT_ARG(window, window);
-            m_liveFormerWindows->add(m_window);
             m_window = window;
             setPrototype(window->prototype());
@@ -77 +76 @@
         void disconnectFrame();
         void clear();
-        void updateDocument();
-
-        void clearFormerWindow(JSDOMWindow* window) { m_liveFormerWindows->remove(window); }
 
     private:
         JSDOMWindow* m_window;
-        HashSet<JSDOMWindow*>* m_liveFormerWindows;
     };
 

trunk/WebCore/bindings/js/kjs_proxy.cpp

@@ -113 +113 @@
     JSLock lock;
     m_windowShell->window()->clear();
+    m_liveFormerWindows.add(m_windowShell->window());
     m_windowShell->setWindow(new JSDOMWindow(m_frame->domWindow(), m_windowShell));
     if (Page* page = m_frame->page()) {
@@ -156 +157 @@
 
     m_windowShell = new JSDOMWindowShell(m_frame->domWindow());
-    m_windowShell->updateDocument();
+    updateDocument();
 
     if (Page* page = m_frame->page()) {
@@ -211 +212 @@
 }
 
+void KJSProxy::updateDocument()
+{
+    JSLock lock;
+    if (m_windowShell)
+        m_windowShell->window()->updateDocument();
+    HashSet<JSDOMWindow*>::iterator end = m_liveFormerWindows.end();
+    for (HashSet<JSDOMWindow*>::iterator it = m_liveFormerWindows.begin(); it != end; ++it)
+        (*it)->updateDocument();
+}
+
+
 } // namespace WebCore

trunk/WebCore/bindings/js/kjs_proxy.h

@@ -78 +78 @@
     bool isPaused() const { return m_paused; }
 
+    void clearFormerWindow(JSDOMWindow* window) { m_liveFormerWindows.remove(window); }
+    void updateDocument();
+
 private:
     void initScriptIfNeeded()
@@ -87 +90 @@
 
     KJS::ProtectedPtr<JSDOMWindowShell> m_windowShell;
+    HashSet<JSDOMWindow*> m_liveFormerWindows;
     Frame* m_frame;
     int m_handlerLineno;

trunk/WebCore/page/DOMWindow.cpp

@@ -147 +147 @@
 DOMWindow::~DOMWindow()
 {
+    if (m_frame)
+        m_frame->clearFormerDOMWindow(this);
 }
 

trunk/WebCore/page/Frame.cpp

@@ -182 +182 @@
     if (d->m_domWindow)
         d->m_domWindow->disconnectFrame();
+
+    HashSet<DOMWindow*>::iterator end = d->m_liveFormerWindows.end();
+    for (HashSet<DOMWindow*>::iterator it = d->m_liveFormerWindows.begin(); it != end; ++it)
+        (*it)->disconnectFrame();
             
     if (d->m_view) {
@@ -261 +265 @@
 
     // Update the cached 'document' property, which is now stale.
-    if (d->m_doc && d->m_jscript.haveWindowShell()) {
-        JSLock lock;
-        d->m_jscript.windowShell()->updateDocument();
-    }
+    d->m_jscript.updateDocument();
 }
 
@@ -1129 +1130 @@
 void Frame::clearDOMWindow()
 {
-    if (d->m_domWindow)
+    if (d->m_domWindow) {
+        d->m_liveFormerWindows.add(d->m_domWindow.get());
         d->m_domWindow->clear();
+    }
     d->m_domWindow = 0;
 }
@@ -1709 +1712 @@
 
     return d->m_domWindow.get();
+}
+
+void Frame::clearFormerDOMWindow(DOMWindow* window)
+{
+    d->m_liveFormerWindows.remove(window);    
 }
 

trunk/WebCore/page/Frame.h

@@ -97 +97 @@
 
     DOMWindow* domWindow() const;
+    void clearFormerDOMWindow(DOMWindow*);
     Editor* editor() const;
     EventHandler* eventHandler() const;

trunk/WebCore/page/FramePrivate.h

@@ -75 +75 @@
         FrameLoader m_loader;
         RefPtr<DOMWindow> m_domWindow;
+        HashSet<DOMWindow*> m_liveFormerWindows;
 
         HTMLFrameOwnerElement* m_ownerElement;