Protect lifetime of frame and frameView objects
authorrniwa@webkit.org <rniwa@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 13 Dec 2019 04:01:35 +0000 (04:01 +0000)
committerrniwa@webkit.org <rniwa@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 13 Dec 2019 04:01:35 +0000 (04:01 +0000)
https://bugs.webkit.org/show_bug.cgi?id=205128

Patch by Jack Lee <shihchieh_lee@apple.com> on 2019-12-12
Reviewed by Ryosuke Niwa.

Could not write a reproducible test case for this.

* page/DOMWindow.cpp:
(WebCore::DOMWindow::focus):
* page/ios/EventHandlerIOS.mm:
(WebCore::EventHandler::focusDocumentView):
* page/mac/EventHandlerMac.mm:
(WebCore::EventHandler::focusDocumentView):

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

Source/WebCore/ChangeLog
Source/WebCore/page/DOMWindow.cpp
Source/WebCore/page/ios/EventHandlerIOS.mm
Source/WebCore/page/mac/EventHandlerMac.mm

index 22df9a5..3c73866 100644 (file)
@@ -1,3 +1,19 @@
+2019-12-12  Jack Lee  <shihchieh_lee@apple.com>
+
+        Protect lifetime of frame and frameView objects
+        https://bugs.webkit.org/show_bug.cgi?id=205128
+
+        Reviewed by Ryosuke Niwa.
+
+        Could not write a reproducible test case for this.
+
+        * page/DOMWindow.cpp:
+        (WebCore::DOMWindow::focus):
+        * page/ios/EventHandlerIOS.mm:
+        (WebCore::EventHandler::focusDocumentView):
+        * page/mac/EventHandlerMac.mm:
+        (WebCore::EventHandler::focusDocumentView):
+
 2019-12-12  Jer Noble  <jer.noble@apple.com>
 
         Add logging for MediaCapabilities.decodeInfo()
index 0568b06..16c1417 100644 (file)
@@ -985,30 +985,27 @@ void DOMWindow::focus(bool allowFocus)
     if (!frame())
         return;
 
-    Page* page = frame()->page();
+    auto protectedFrame = makeRefPtr(frame());
+
+    Page* page = protectedFrame->page();
     if (!page)
         return;
 
-    allowFocus = allowFocus || WindowFocusAllowedIndicator::windowFocusAllowed() || !frame()->settings().windowFocusRestricted();
+    allowFocus = allowFocus || WindowFocusAllowedIndicator::windowFocusAllowed() || !protectedFrame->settings().windowFocusRestricted();
 
     // If we're a top level window, bring the window to the front.
-    if (frame()->isMainFrame() && allowFocus)
+    if (protectedFrame->isMainFrame() && allowFocus)
         page->chrome().focus();
 
-    if (!frame())
-        return;
-
-    if (!frame()->hasHadUserInteraction() && !isSameSecurityOriginAsMainFrame())
+    if (!protectedFrame->hasHadUserInteraction() && !isSameSecurityOriginAsMainFrame())
         return;
 
     // Clear the current frame's focused node if a new frame is about to be focused.
-    Frame* focusedFrame = page->focusController().focusedFrame();
-    if (focusedFrame && focusedFrame != frame())
+    auto focusedFrame = makeRefPtr(page->focusController().focusedFrame());
+    if (focusedFrame && focusedFrame != protectedFrame)
         focusedFrame->document()->setFocusedElement(nullptr);
 
-    // setFocusedElement may clear frame(), so recheck before using it.
-    if (auto* frame = this->frame())
-        frame->eventHandler().focusDocumentView();
+    protectedFrame->eventHandler().focusDocumentView();
 }
 
 void DOMWindow::blur()
index 12bfc53..6ed72cc 100644 (file)
@@ -174,13 +174,16 @@ void EventHandler::focusDocumentView()
     if (!page)
         return;
 
-    Ref<Frame> protectedFrame(m_frame);
-
-    if (FrameView* frameView = m_frame.view()) {
-        if (NSView *documentView = frameView->documentView())
+    if (auto frameView = makeRefPtr(m_frame.view())) {
+        if (NSView *documentView = frameView->documentView()) {
             page->chrome().focusNSView(documentView);
+            // Check page() again because focusNSView can cause reentrancy.
+            if (!m_frame.page())
+                return;
+        }
     }
 
+    RELEASE_ASSERT(page == m_frame.page());
     page->focusController().setFocusedFrame(&m_frame);
 }
 
index 3f981dd..af4d6b3 100644 (file)
@@ -165,11 +165,16 @@ void EventHandler::focusDocumentView()
     if (!page)
         return;
 
-    if (FrameView* frameView = m_frame.view()) {
-        if (NSView *documentView = frameView->documentView())
+    if (auto frameView = makeRefPtr(m_frame.view())) {
+        if (NSView *documentView = frameView->documentView()) {
             page->chrome().focusNSView(documentView);
+            // Check page() again because focusNSView can cause reentrancy.
+            if (!m_frame.page())
+                return;
+        }
     }
 
+    RELEASE_ASSERT(page == m_frame.page());
     page->focusController().setFocusedFrame(&m_frame);
 }