Diff
Modified: trunk/Source/WebCore/ChangeLog (167850 => 167851)
--- trunk/Source/WebCore/ChangeLog 2014-04-27 02:48:19 UTC (rev 167850)
+++ trunk/Source/WebCore/ChangeLog 2014-04-27 04:09:45 UTC (rev 167851)
@@ -1,3 +1,19 @@
+2014-04-26 Darin Adler <[email protected]>
+
+ 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 <[email protected]>
Local files should not be allowed to read pasteboard data during drag
Modified: trunk/Source/WebCore/loader/FrameLoader.cpp (167850 => 167851)
--- trunk/Source/WebCore/loader/FrameLoader.cpp 2014-04-27 02:48:19 UTC (rev 167850)
+++ trunk/Source/WebCore/loader/FrameLoader.cpp 2014-04-27 04:09:45 UTC (rev 167851)
@@ -3426,14 +3426,15 @@
{
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();
}
}
@@ -3441,7 +3442,7 @@
if (isDocumentSandboxed(openerFrame, SandboxPopups)) {
// 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;
}
// FIXME: Setting the referrer should be the caller's responsibility.
@@ -3453,29 +3454,42 @@
Page* oldPage = openerFrame->page();
if (!oldPage)
- return 0;
+ return nullptr;
- NavigationAction action(requestWithReferrer.resourceRequest());
- Page* page = oldPage->chrome().createWindow(openerFrame, requestWithReferrer, features, action);
+ Page* page = oldPage->chrome().createWindow(openerFrame, requestWithReferrer, features, NavigationAction(requestWithReferrer.resourceRequest()));
if (!page)
- return 0;
+ return nullptr;
- page->mainFrame().loader().forceSandboxFlags(openerFrame->document()->sandboxFlags());
+ 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);
// 'x' and 'y' specify the location of the window, while 'width' and 'height'
// specify the size of the viewport. We can only resize the window, so adjust
// 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();
FloatRect windowRect = page->chrome().windowRect();
@@ -3492,6 +3506,8 @@
// Ensure non-NaN values, minimum size as well as being within valid screen area.
FloatRect newWindowRect = DOMWindow::adjustWindowRect(page, windowRect);
+ if (!frame->page())
+ return nullptr;
page->chrome().setWindowRect(newWindowRect);
#else
// On iOS, width and height refer to the viewport dimensions.
@@ -3501,13 +3517,15 @@
arguments.width = features.width;
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();
}
} // namespace WebCore
Modified: trunk/Source/WebCore/loader/SubframeLoader.cpp (167850 => 167851)
--- trunk/Source/WebCore/loader/SubframeLoader.cpp 2014-04-27 02:48:19 UTC (rev 167850)
+++ trunk/Source/WebCore/loader/SubframeLoader.cpp 2014-04-27 04:09:45 UTC (rev 167851)
@@ -322,6 +322,9 @@
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)
frame->navigationScheduler().scheduleLocationChange(m_frame.document()->securityOrigin(), url.string(), m_frame.loader().outgoingReferrer(), lockHistory, lockBackForwardList);
Modified: trunk/Source/WebCore/page/DOMWindow.cpp (167850 => 167851)
--- trunk/Source/WebCore/page/DOMWindow.cpp 2014-04-27 02:48:19 UTC (rev 167850)
+++ trunk/Source/WebCore/page/DOMWindow.cpp 2014-04-27 04:09:45 UTC (rev 167851)
@@ -1891,19 +1891,20 @@
return;
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;
+
// We want a new history item if we are processing a user gesture.
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);
}
void DOMWindow::printErrorMessage(const String& message)
@@ -1988,7 +1989,7 @@
if (!completedURL.isEmpty() && !completedURL.isValid()) {
// 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;
}
// For whatever reason, Firefox uses the first frame to determine the outgoingReferrer. We replicate that behavior here.
@@ -2003,7 +2004,7 @@
bool created;
RefPtr<Frame> newFrame = WebCore::createWindow(activeFrame, openerFrame, frameRequest, windowFeatures, created);
if (!newFrame)
- return 0;
+ return nullptr;
newFrame->loader().setOpener(openerFrame);
newFrame->page()->setOpenedByDOM();
@@ -2032,24 +2033,24 @@
DOMWindow& activeWindow, DOMWindow& firstWindow)
{
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()) {
// Because FrameTree::find() returns true for empty strings, we must check for empty frame names.
// 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();
else if (frameName == "_parent") {
@@ -2060,9 +2061,11 @@
}
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))
return targetFrame->document()->domWindow();
@@ -2080,7 +2083,7 @@
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;
}
void DOMWindow::showModalDialog(const String& urlString, const String& dialogFeaturesString, DOMWindow& activeWindow, DOMWindow& firstWindow, std::function<void (DOMWindow&)> prepareDialogFunction)
