Web Inspector: InspectorInstrumentation::frameDestroyed is called after m_page has...
authorpfeldman@chromium.org <pfeldman@chromium.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 14 Sep 2011 14:14:37 +0000 (14:14 +0000)
committerpfeldman@chromium.org <pfeldman@chromium.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 14 Sep 2011 14:14:37 +0000 (14:14 +0000)
https://bugs.webkit.org/show_bug.cgi?id=67997

We should not instrument frameDestroyed event from within Frame's destructor
since frame's m_page pointer is likely to be 0 by that time and appropriate
instrumenting agent won't be found. As a result, stale frame with its id
end up in the inspector.

This change wipes out frame binding from the inspector upon detach rather
than destroy + adds an assertion into the inspector agents lookup with 0 page.

Reviewed by Tony Gentilcore.

* inspector/InspectorInstrumentation.cpp:
(WebCore::InspectorInstrumentation::frameDetachedImpl):
(WebCore::InspectorInstrumentation::instrumentingAgentsForPage):
* inspector/InspectorInstrumentation.h:
(WebCore::InspectorInstrumentation::frameWindowDiscarded):
(WebCore::InspectorInstrumentation::domContentLoadedEventFired):
(WebCore::InspectorInstrumentation::loadEventFired):
(WebCore::InspectorInstrumentation::frameDetached):
(WebCore::InspectorInstrumentation::didCommitLoad):
* inspector/InspectorPageAgent.cpp:
(WebCore::InspectorPageAgent::frameDetached):
* inspector/InspectorPageAgent.h:
* loader/FrameLoader.cpp:
(WebCore::FrameLoader::detachFromParent):
* page/Frame.cpp:
(WebCore::Frame::~Frame):
(WebCore::Frame::detachFromPage):
(WebCore::Frame::transferChildFrameToNewDocument):
* page/Frame.h:

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

Source/WebCore/ChangeLog
Source/WebCore/inspector/InspectorInstrumentation.cpp
Source/WebCore/inspector/InspectorInstrumentation.h
Source/WebCore/inspector/InspectorPageAgent.cpp
Source/WebCore/inspector/InspectorPageAgent.h
Source/WebCore/page/Frame.cpp

index fdf6794..a30ccd8 100644 (file)
@@ -1,3 +1,38 @@
+2011-09-13  Pavel Feldman  <pfeldman@google.com>
+
+        Web Inspector: InspectorInstrumentation::frameDestroyed is called after m_page has been reset.
+        https://bugs.webkit.org/show_bug.cgi?id=67997
+
+        We should not instrument frameDestroyed event from within Frame's destructor
+        since frame's m_page pointer is likely to be 0 by that time and appropriate
+        instrumenting agent won't be found. As a result, stale frame with its id
+        end up in the inspector.
+
+        This change wipes out frame binding from the inspector upon detach rather
+        than destroy + adds an assertion into the inspector agents lookup with 0 page.
+
+        Reviewed by Tony Gentilcore.
+
+        * inspector/InspectorInstrumentation.cpp:
+        (WebCore::InspectorInstrumentation::frameDetachedImpl):
+        (WebCore::InspectorInstrumentation::instrumentingAgentsForPage):
+        * inspector/InspectorInstrumentation.h:
+        (WebCore::InspectorInstrumentation::frameWindowDiscarded):
+        (WebCore::InspectorInstrumentation::domContentLoadedEventFired):
+        (WebCore::InspectorInstrumentation::loadEventFired):
+        (WebCore::InspectorInstrumentation::frameDetached):
+        (WebCore::InspectorInstrumentation::didCommitLoad):
+        * inspector/InspectorPageAgent.cpp:
+        (WebCore::InspectorPageAgent::frameDetached):
+        * inspector/InspectorPageAgent.h:
+        * loader/FrameLoader.cpp:
+        (WebCore::FrameLoader::detachFromParent):
+        * page/Frame.cpp:
+        (WebCore::Frame::~Frame):
+        (WebCore::Frame::detachFromPage):
+        (WebCore::Frame::transferChildFrameToNewDocument):
+        * page/Frame.h:
+
 2011-09-14  Sheriff Bot  <webkit.review.bot@gmail.com>
 
         Unreviewed, rolling out r95080.
index 47a2650..c3379db 100644 (file)
@@ -660,12 +660,6 @@ void InspectorInstrumentation::didCommitLoadImpl(InstrumentingAgents* instrument
         pageAgent->frameNavigated(loader);
 }
 
-void InspectorInstrumentation::frameDestroyedImpl(InstrumentingAgents* instrumentingAgents, Frame* frame)
-{
-    if (InspectorPageAgent* inspectorPageAgent = instrumentingAgents->inspectorPageAgent())
-        inspectorPageAgent->frameDestroyed(frame);
-}
-
 void InspectorInstrumentation::loaderDetachedFromFrameImpl(InstrumentingAgents* instrumentingAgents, DocumentLoader* loader)
 {
     if (InspectorPageAgent* inspectorPageAgent = instrumentingAgents->inspectorPageAgent())
@@ -883,6 +877,8 @@ InspectorTimelineAgent* InspectorInstrumentation::retrieveTimelineAgent(const In
 
 InstrumentingAgents* InspectorInstrumentation::instrumentingAgentsForPage(Page* page)
 {
+    // We should not be getting 0 page here, but we are not quite ready to remove the test below.
+    ASSERT(page);
     if (!page)
         return 0;
     return instrumentationForPage(page);
index af41e8f..ee56cc7 100644 (file)
@@ -142,7 +142,6 @@ public:
     static void loadEventFired(Frame*, const KURL&);
     static void frameDetachedFromParent(Frame*);
     static void didCommitLoad(Frame*, DocumentLoader*);
-    static void frameDestroyed(Frame*);
     static void loaderDetachedFromFrame(Frame*, DocumentLoader*);
 
     static InspectorInstrumentationCookie willWriteHTML(Document*, unsigned int length, unsigned int startLine);
@@ -272,7 +271,6 @@ private:
     static void loadEventFiredImpl(InstrumentingAgents*, Frame*, const KURL&);
     static void frameDetachedFromParentImpl(InstrumentingAgents*, Frame*);
     static void didCommitLoadImpl(InstrumentingAgents*, Page*, DocumentLoader*);
-    static void frameDestroyedImpl(InstrumentingAgents*, Frame*);
     static void loaderDetachedFromFrameImpl(InstrumentingAgents*, DocumentLoader*);
 
     static InspectorInstrumentationCookie willWriteHTMLImpl(InstrumentingAgents*, unsigned int length, unsigned int startLine);
@@ -408,7 +406,10 @@ inline void InspectorInstrumentation::didInvalidateStyleAttr(Document* document,
 inline void InspectorInstrumentation::frameWindowDiscarded(Frame* frame, DOMWindow* domWindow)
 {
 #if ENABLE(INSPECTOR)
-    if (InstrumentingAgents* instrumentingAgents = instrumentingAgentsForFrame(frame))
+    Page* page = frame->page();
+    if (!page)
+        return; // Entire page being destroyed.
+    if (InstrumentingAgents* instrumentingAgents = instrumentingAgentsForPage(page))
         frameWindowDiscardedImpl(instrumentingAgents, domWindow);
 #endif
 }
@@ -863,7 +864,10 @@ inline void InspectorInstrumentation::didReceiveScriptResponse(ScriptExecutionCo
 inline void InspectorInstrumentation::domContentLoadedEventFired(Frame* frame, const KURL& url)
 {
 #if ENABLE(INSPECTOR)
-    if (InstrumentingAgents* instrumentingAgents = instrumentingAgentsForFrame(frame))
+    Page* page = frame->page();
+    if (!page)
+        return; // DOMContentLoaded event triggered post frame detach.
+    if (InstrumentingAgents* instrumentingAgents = instrumentingAgentsForPage(page))
         domContentLoadedEventFiredImpl(instrumentingAgents, frame, url);
 #endif
 }
@@ -871,7 +875,10 @@ inline void InspectorInstrumentation::domContentLoadedEventFired(Frame* frame, c
 inline void InspectorInstrumentation::loadEventFired(Frame* frame, const KURL& url)
 {
 #if ENABLE(INSPECTOR)
-    if (InstrumentingAgents* instrumentingAgents = instrumentingAgentsForFrame(frame))
+    Page* page = frame->page();
+    if (!page)
+        return; // Load event triggered post frame detach.
+    if (InstrumentingAgents* instrumentingAgents = instrumentingAgentsForPage(page))
         loadEventFiredImpl(instrumentingAgents, frame, url);
 #endif
 }
@@ -887,24 +894,14 @@ inline void InspectorInstrumentation::frameDetachedFromParent(Frame* frame)
 inline void InspectorInstrumentation::didCommitLoad(Frame* frame, DocumentLoader* loader)
 {
 #if ENABLE(INSPECTOR)
-    if (!frame)
-        return;
     Page* page = frame->page();
     if (!page)
-        return;
+        return; // Commit load triggered post frame detach.
     if (InstrumentingAgents* instrumentingAgents = instrumentingAgentsForPage(page))
         didCommitLoadImpl(instrumentingAgents, page, loader);
 #endif
 }
 
-inline void InspectorInstrumentation::frameDestroyed(Frame* frame)
-{
-#if ENABLE(INSPECTOR)
-    if (InstrumentingAgents* instrumentingAgents = instrumentingAgentsForFrame(frame))
-        frameDestroyedImpl(instrumentingAgents, frame);
-#endif
-}
-
 inline void InspectorInstrumentation::loaderDetachedFromFrame(Frame* frame, DocumentLoader* loader)
 {
 #if ENABLE(INSPECTOR)
index 0b55f24..bddbb54 100644 (file)
@@ -558,7 +558,12 @@ void InspectorPageAgent::frameNavigated(DocumentLoader* loader)
 
 void InspectorPageAgent::frameDetached(Frame* frame)
 {
-    m_frontend->frameDetached(frameId(frame));
+    HashMap<Frame*, String>::iterator iterator = m_frameToIdentifier.find(frame);
+    if (iterator != m_frameToIdentifier.end()) {
+        m_frontend->frameDetached(iterator->second);
+        m_identifierToFrame.remove(iterator->second);
+        m_frameToIdentifier.remove(iterator);
+    }
 }
 
 Frame* InspectorPageAgent::mainFrame()
@@ -596,15 +601,6 @@ String InspectorPageAgent::loaderId(DocumentLoader* loader)
     return identifier;
 }
 
-void InspectorPageAgent::frameDestroyed(Frame* frame)
-{
-    HashMap<Frame*, String>::iterator iterator = m_frameToIdentifier.find(frame);
-    if (iterator != m_frameToIdentifier.end()) {
-        m_identifierToFrame.remove(iterator->second);
-        m_frameToIdentifier.remove(iterator);
-    }
-}
-
 void InspectorPageAgent::loaderDetachedFromFrame(DocumentLoader* loader)
 {
     HashMap<DocumentLoader*, String>::iterator iterator = m_loaderToIdentifier.find(loader);
index 89bc9bf..c8dd0de 100644 (file)
@@ -102,7 +102,6 @@ public:
     void loadEventFired();
     void frameNavigated(DocumentLoader*);
     void frameDetached(Frame*);
-    void frameDestroyed(Frame*);
     void loaderDetachedFromFrame(DocumentLoader*);
 
     // Inspector Controller API
index e891e87..756f5b7 100644 (file)
@@ -238,8 +238,6 @@ Frame::~Frame()
     for (HashSet<FrameDestructionObserver*>::iterator it = m_destructionObservers.begin(); it != stop; ++it)
         (*it)->frameDestroyed();
 
-    InspectorInstrumentation::frameDestroyed(this);
-
     if (m_view) {
         m_view->hide();
         m_view->clearFrame();