Changeset 167851 in webkit

Timestamp:

Apr 26, 2014 9:09:45 PM (10 years ago)

Author:

Darin Adler

Message:

Frame and page lifetime fixes in WebCore::createWindow
​https://bugs.webkit.org/show_bug.cgi?id=132089

Reviewed by Sam Weinig.

Speculative fix because I was unable to reproduce the crash that was
reported with the test case attached to this bug.

• loader/FrameLoader.cpp:

(WebCore::createWindow): Changed code to remove the assumption that calls
out will not destroy the page or frame. Use RefPtr for the frame, and
added early exits if frame->page() becomes null at any point before we
use a page pointer.

Location:

trunk/Source/WebCore

Files:

• ChangeLog (modified) (1 diff)
• loader/FrameLoader.cpp (modified) (6 diffs)
• loader/SubframeLoader.cpp (modified) (1 diff)
• page/DOMWindow.cpp (modified) (8 diffs)

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2014-04-26  Darin Adler  <darin@apple.com>
+
+        Frame and page lifetime fixes in WebCore::createWindow
+        https://bugs.webkit.org/show_bug.cgi?id=132089
+
+        Reviewed by Sam Weinig.
+
+        Speculative fix because I was unable to reproduce the crash that was
+        reported with the test case attached to this bug.
+
+        * loader/FrameLoader.cpp:
+        (WebCore::createWindow): Changed code to remove the assumption that calls
+        out will not destroy the page or frame. Use RefPtr for the frame, and
+        added early exits if frame->page() becomes null at any point before we
+        use a page pointer.
+
 2014-04-26  Alexey Proskuryakov  <ap@apple.com>
 

trunk/Source/WebCore/loader/FrameLoader.cpp

@@ -3427 +3427 @@
     ASSERT(!features.dialog || request.frameName().isEmpty());
 
+    created = false;
+
     if (!request.frameName().isEmpty() && request.frameName() != "_blank") {
-        if (Frame* frame = lookupFrame->loader().findFrameForNavigation(request.frameName(), openerFrame->document())) {
+        if (RefPtr<Frame> frame = lookupFrame->loader().findFrameForNavigation(request.frameName(), openerFrame->document())) {
             if (request.frameName() != "_self") {
                 if (Page* page = frame->page())
                     page->chrome().focus();
             }
-            created = false;
-            return frame;
+            return frame.release();
         }
     }
@@ -3442 +3443 @@
         // FIXME: This message should be moved off the console once a solution to https://bugs.webkit.org/show_bug.cgi?id=103274 exists.
         openerFrame->document()->addConsoleMessage(MessageSource::Security, MessageLevel::Error, "Blocked opening '" + request.resourceRequest().url().stringCenterEllipsizedToLength() + "' in a new window because the request was made in a sandboxed frame whose 'allow-popups' permission is not set.");
-        return 0;
+        return nullptr;
     }
 
@@ -3454 +3455 @@
     Page* oldPage = openerFrame->page();
     if (!oldPage)
-        return 0;
-
-    NavigationAction action(requestWithReferrer.resourceRequest());
-    Page* page = oldPage->chrome().createWindow(openerFrame, requestWithReferrer, features, action);
+        return nullptr;
+
+    Page* page = oldPage->chrome().createWindow(openerFrame, requestWithReferrer, features, NavigationAction(requestWithReferrer.resourceRequest()));
     if (!page)
-        return 0;
-
-    page->mainFrame().loader().forceSandboxFlags(openerFrame->document()->sandboxFlags());
+        return nullptr;
+
+    RefPtr<Frame> frame = &page->mainFrame();
+
+    frame->loader().forceSandboxFlags(openerFrame->document()->sandboxFlags());
 
     if (request.frameName() != "_blank")
-        page->mainFrame().tree().setName(request.frameName());
+        frame->tree().setName(request.frameName());
 
     page->chrome().setToolbarsVisible(features.toolBarVisible || features.locationBarVisible);
+
+    if (!frame->page())
+        return nullptr;
     page->chrome().setStatusbarVisible(features.statusBarVisible);
+
+    if (!frame->page())
+        return nullptr;
     page->chrome().setScrollbarsVisible(features.scrollbarsVisible);
+
+    if (!frame->page())
+        return nullptr;
     page->chrome().setMenubarVisible(features.menuBarVisible);
+
+    if (!frame->page())
+        return nullptr;
     page->chrome().setResizable(features.resizable);
 
@@ -3476 +3490 @@
     // for the difference between the window size and the viewport size.
 
-// FIXME: We should reconcile the initialization of viewport arguments between iOS and OpenSource.
+    // FIXME: We should reconcile the initialization of viewport arguments between iOS and non-IOS.
 #if !PLATFORM(IOS)
     FloatSize viewportSize = page->chrome().pageRect().size();
@@ -3493 +3507 @@
     FloatRect newWindowRect = DOMWindow::adjustWindowRect(page, windowRect);
 
+    if (!frame->page())
+        return nullptr;
     page->chrome().setWindowRect(newWindowRect);
 #else
@@ -3502 +3518 @@
     if (features.heightSet && features.height)
         arguments.height = features.height;
-    page->mainFrame().setViewportArguments(arguments);
+    frame->setViewportArguments(arguments);
 #endif
 
+    if (!frame->page())
+        return nullptr;
     page->chrome().show();
 
     created = true;
-    return &page->mainFrame();
+    return frame.release();
 }
 

trunk/Source/WebCore/loader/SubframeLoader.cpp

@@ -323 +323 @@
 Frame* SubframeLoader::loadOrRedirectSubframe(HTMLFrameOwnerElement& ownerElement, const URL& url, const AtomicString& frameName, LockHistory lockHistory, LockBackForwardList lockBackForwardList)
 {
+    if (!url.isValid())
+        return nullptr;
+
     Frame* frame = ownerElement.contentFrame();
     if (frame)

trunk/Source/WebCore/page/DOMWindow.cpp

@@ -1892 +1892 @@
 
     URL completedURL = firstFrame->document()->completeURL(urlString);
-    if (completedURL.isNull())
+    if (!completedURL.isValid())
         return;
 
     if (isInsecureScriptAccess(activeWindow, completedURL))
+        return;
+
+    Frame* referrerFrame = activeDocument->frame();
+    if (!referrerFrame)
         return;
 
@@ -1901 +1905 @@
     LockHistory lockHistory = (locking != LockHistoryBasedOnGestureState || !ScriptController::processingUserGesture()) ? LockHistory::Yes : LockHistory::No;
     LockBackForwardList lockBackForwardList = (locking != LockHistoryBasedOnGestureState) ? LockBackForwardList::Yes : LockBackForwardList::No;
-    m_frame->navigationScheduler().scheduleLocationChange(activeDocument->securityOrigin(),
-        // FIXME: What if activeDocument()->frame() is 0?
-        completedURL, activeDocument->frame()->loader().outgoingReferrer(),
-        lockHistory, lockBackForwardList);
+    m_frame->navigationScheduler().scheduleLocationChange(activeDocument->securityOrigin(), completedURL, referrerFrame->loader().outgoingReferrer(), lockHistory, lockBackForwardList);
 }
 
@@ -1989 +1990 @@
         // Don't expose client code to invalid URLs.
         activeWindow.printErrorMessage("Unable to open a window with invalid URL '" + completedURL.string() + "'.\n");
-        return 0;
+        return nullptr;
     }
 
@@ -2004 +2005 @@
     RefPtr<Frame> newFrame = WebCore::createWindow(activeFrame, openerFrame, frameRequest, windowFeatures, created);
     if (!newFrame)
-        return 0;
+        return nullptr;
 
     newFrame->loader().setOpener(openerFrame);
@@ -2033 +2034 @@
 {
     if (!isCurrentlyDisplayedInFrame())
-        return 0;
+        return nullptr;
     Document* activeDocument = activeWindow.document();
     if (!activeDocument)
-        return 0;
+        return nullptr;
     Frame* firstFrame = firstWindow.frame();
     if (!firstFrame)
-        return 0;
+        return nullptr;
 
     if (!firstWindow.allowPopUp()) {
@@ -2045 +2046 @@
         // Otherwise, illegitimate window.open() calls with no name will pass right through the popup blocker.
         if (frameName.isEmpty() || !m_frame->tree().find(frameName))
-            return 0;
+            return nullptr;
     }
 
     // Get the target frame for the special cases of _top and _parent.
     // In those cases, we schedule a location change right now and return early.
-    Frame* targetFrame = 0;
+    Frame* targetFrame = nullptr;
     if (frameName == "_top")
         targetFrame = &m_frame->tree().top();
@@ -2061 +2062 @@
     if (targetFrame) {
         if (!activeDocument->canNavigate(targetFrame))
-            return 0;
+            return nullptr;
 
         URL completedURL = firstFrame->document()->completeURL(urlString);
+        if (!completedURL.isValid())
+            return nullptr;
 
         if (targetFrame->document()->domWindow()->isInsecureScriptAccess(activeWindow, completedURL))
@@ -2081 +2084 @@
     WindowFeatures windowFeatures(windowFeaturesString);
     RefPtr<Frame> result = createWindow(urlString, frameName, windowFeatures, activeWindow, firstFrame, m_frame);
-    return result ? result->document()->domWindow() : 0;
+    return result ? result->document()->domWindow() : nullptr;
 }