Diff
Modified: trunk/Source/WebCore/ChangeLog (232215 => 232216)
--- trunk/Source/WebCore/ChangeLog 2018-05-25 23:45:36 UTC (rev 232215)
+++ trunk/Source/WebCore/ChangeLog 2018-05-25 23:48:11 UTC (rev 232216)
@@ -1,3 +1,38 @@
+2018-05-25 Daniel Bates <[email protected]>
+
+ NavigationAction should not hold a strong reference to a Document
+ https://bugs.webkit.org/show_bug.cgi?id=185712
+ <rdar://problem/40320916>
+
+ Reviewed by Brent Fulgham.
+
+ Have NavigationAction store all the relevant details callers need to know about the document
+ that initiated the navigation in an independent data structure, called NavigationAction::Requester,
+ as opposed to holding a RefPtr to the document itself. The benefit of this approach is that it
+ is a step towards ensuring that NavigationAction does not keep the document alive after navigating
+ to a new document given that DocumentLoader stores the NavigationAction for the last navigation.
+
+ * loader/NavigationAction.cpp:
+ (WebCore::NavigationAction::Requester::Requester): Track all relevant details of the document that
+ requested this navigation that are needed to support WebKit API/SPI. We hold the SecurityOrigin in
+ a RefPtr to avoid the need to explicitly define a copy constructor and copy-assignment constructor
+ because Requester needs to be copyable as NavigationAction, which owns a Requester, is copyable.
+ (WebCore::shouldTreatAsSameOriginNavigation): Fix some style nits.
+ (WebCore::NavigationAction::NavigationAction): Instantiate a Requester from the specified document.
+ * loader/NavigationAction.h:
+ (WebCore::NavigationAction::Requester::url const): Added.
+ (WebCore::NavigationAction::Requester::securityOrigin const): Added.
+ (WebCore::NavigationAction::Requester::pageID const): Added.
+ (WebCore::NavigationAction::Requester::frameID const): Added.
+ (WebCore::NavigationAction::requester const): Returns details about the document that requested
+ this navigation, if applicable.
+ (WebCore::NavigationAction::isEmpty const): Update criterion for being empty to consider the
+ requester.
+ (WebCore::NavigationAction::setOpener): Extracted out the datatype of the parameter into a
+ type alias to avoid duplication and updated this code to use the alias.
+ (WebCore::NavigationAction::opener const): Ditto.
+ (WebCore::NavigationAction::sourceDocument const): Deleted.
+
2018-05-25 Jeremy Jones <[email protected]>
Fullscreen element can be clipped by ancestor.
Modified: trunk/Source/WebCore/loader/NavigationAction.cpp (232215 => 232216)
--- trunk/Source/WebCore/loader/NavigationAction.cpp 2018-05-25 23:45:36 UTC (rev 232215)
+++ trunk/Source/WebCore/loader/NavigationAction.cpp 2018-05-25 23:48:11 UTC (rev 232216)
@@ -1,5 +1,5 @@
/*
- * Copyright (C) 2006-2017 Apple Inc. All rights reserved.
+ * Copyright (C) 2006-2018 Apple Inc. All rights reserved.
*
* Redistribution and use in source and binary forms, with or without
* modification, are permitted provided that the following conditions
@@ -32,10 +32,18 @@
#include "Document.h"
#include "Event.h"
#include "FrameLoader.h"
+#include "FrameLoaderClient.h"
#include "HistoryItem.h"
namespace WebCore {
+NavigationAction::Requester::Requester(const Document& document)
+ : m_url { URL { document.url() } }
+ , m_origin { makeRefPtr(document.securityOrigin()) }
+ , m_pageIDAndFrameIDPair { document.frame() ? std::make_pair(document.frame()->loader().client().pageID().value_or(0), document.frame()->loader().client().frameID().value_or(0)) : std::make_pair<uint64_t, uint64_t>(0, 0) }
+{
+}
+
NavigationAction::NavigationAction() = default;
NavigationAction::~NavigationAction() = default;
@@ -45,15 +53,13 @@
NavigationAction& NavigationAction::operator=(const NavigationAction&) = default;
NavigationAction& NavigationAction::operator=(NavigationAction&&) = default;
-static bool shouldTreatAsSameOriginNavigation(Document& source, const URL& url)
+static bool shouldTreatAsSameOriginNavigation(const Document& document, const URL& url)
{
- return url.isBlankURL()
- || url.protocolIsData()
- || (url.protocolIsBlob() && source.securityOrigin().canRequest(url));
+ return url.isBlankURL() || url.protocolIsData() || (url.protocolIsBlob() && document.securityOrigin().canRequest(url));
}
-NavigationAction::NavigationAction(Document& source, const ResourceRequest& resourceRequest, InitiatedByMainFrame initiatedByMainFrame, NavigationType type, ShouldOpenExternalURLsPolicy shouldOpenExternalURLsPolicy, Event* event, const AtomicString& downloadAttribute)
- : m_sourceDocument { makeRefPtr(source) }
+NavigationAction::NavigationAction(Document& requester, const ResourceRequest& resourceRequest, InitiatedByMainFrame initiatedByMainFrame, NavigationType type, ShouldOpenExternalURLsPolicy shouldOpenExternalURLsPolicy, Event* event, const AtomicString& downloadAttribute)
+ : m_requester { requester }
, m_resourceRequest { resourceRequest }
, m_type { type }
, m_shouldOpenExternalURLsPolicy { shouldOpenExternalURLsPolicy }
@@ -60,7 +66,7 @@
, m_initiatedByMainFrame { initiatedByMainFrame }
, m_event { event }
, m_downloadAttribute { downloadAttribute }
- , m_treatAsSameOriginNavigation { shouldTreatAsSameOriginNavigation(source, resourceRequest.url()) }
+ , m_treatAsSameOriginNavigation { shouldTreatAsSameOriginNavigation(requester, resourceRequest.url()) }
{
}
@@ -77,8 +83,8 @@
return NavigationType::Other;
}
-NavigationAction::NavigationAction(Document& source, const ResourceRequest& resourceRequest, InitiatedByMainFrame initiatedByMainFrame, FrameLoadType frameLoadType, bool isFormSubmission, Event* event, ShouldOpenExternalURLsPolicy shouldOpenExternalURLsPolicy, const AtomicString& downloadAttribute)
- : m_sourceDocument { makeRefPtr(source) }
+NavigationAction::NavigationAction(Document& requester, const ResourceRequest& resourceRequest, InitiatedByMainFrame initiatedByMainFrame, FrameLoadType frameLoadType, bool isFormSubmission, Event* event, ShouldOpenExternalURLsPolicy shouldOpenExternalURLsPolicy, const AtomicString& downloadAttribute)
+ : m_requester { requester }
, m_resourceRequest { resourceRequest }
, m_type { navigationType(frameLoadType, isFormSubmission, !!event) }
, m_shouldOpenExternalURLsPolicy { shouldOpenExternalURLsPolicy }
@@ -85,7 +91,7 @@
, m_initiatedByMainFrame { initiatedByMainFrame }
, m_event { event }
, m_downloadAttribute { downloadAttribute }
- , m_treatAsSameOriginNavigation { shouldTreatAsSameOriginNavigation(source, resourceRequest.url()) }
+ , m_treatAsSameOriginNavigation { shouldTreatAsSameOriginNavigation(requester, resourceRequest.url()) }
{
}
Modified: trunk/Source/WebCore/loader/NavigationAction.h (232215 => 232216)
--- trunk/Source/WebCore/loader/NavigationAction.h 2018-05-25 23:45:36 UTC (rev 232215)
+++ trunk/Source/WebCore/loader/NavigationAction.h 2018-05-25 23:48:11 UTC (rev 232216)
@@ -1,5 +1,5 @@
/*
- * Copyright (C) 2006-2017 Apple Inc. All rights reserved.
+ * Copyright (C) 2006-2018 Apple Inc. All rights reserved.
*
* Redistribution and use in source and binary forms, with or without
* modification, are permitted provided that the following conditions
@@ -31,6 +31,7 @@
#include "BackForwardItemIdentifier.h"
#include "FrameLoaderTypes.h"
#include "ResourceRequest.h"
+#include "SecurityOrigin.h"
#include "UserGestureIndicator.h"
#include <wtf/Forward.h>
@@ -54,9 +55,25 @@
NavigationAction(NavigationAction&&);
NavigationAction& operator=(NavigationAction&&);
+ using PageIDAndFrameIDPair = std::pair<uint64_t /* pageID */, uint64_t /* frameID */>;
+ class Requester {
+ public:
+ Requester(const Document&);
+
+ const URL& url() const { return m_url; }
+ const SecurityOrigin& securityOrigin() const { return *m_origin; }
+ uint64_t pageID() const { return m_pageIDAndFrameIDPair.first; }
+ uint64_t frameID() const { return m_pageIDAndFrameIDPair.second; }
+ private:
+ URL m_url;
+ RefPtr<SecurityOrigin> m_origin;
+ PageIDAndFrameIDPair m_pageIDAndFrameIDPair;
+ };
+ const std::optional<Requester>& requester() const { return m_requester; }
+
NavigationAction copyWithShouldOpenExternalURLsPolicy(ShouldOpenExternalURLsPolicy) const;
- bool isEmpty() const { return !m_sourceDocument || m_resourceRequest.url().isEmpty(); }
+ bool isEmpty() const { return !m_requester || m_requester->url().isEmpty() || m_resourceRequest.url().isEmpty(); }
URL url() const { return m_resourceRequest.url(); }
const ResourceRequest& resourceRequest() const { return m_resourceRequest; }
@@ -64,8 +81,6 @@
NavigationType type() const { return m_type; }
const Event* event() const { return m_event.get(); }
- const Document* sourceDocument() const { return m_sourceDocument.get(); }
-
bool processingUserGesture() const { return m_userGestureToken ? m_userGestureToken->processingUserGesture() : false; }
RefPtr<UserGestureToken> userGestureToken() const { return m_userGestureToken; }
@@ -79,14 +94,14 @@
void setIsCrossOriginWindowOpenNavigation(bool value) { m_isCrossOriginWindowOpenNavigation = value; }
bool isCrossOriginWindowOpenNavigation() const { return m_isCrossOriginWindowOpenNavigation; }
- void setOpener(std::optional<std::pair<uint64_t, uint64_t>>&& opener) { m_opener = WTFMove(opener); }
- const std::optional<std::pair<uint64_t, uint64_t>>& opener() const { return m_opener; }
+ void setOpener(std::optional<PageIDAndFrameIDPair>&& opener) { m_opener = WTFMove(opener); }
+ const std::optional<PageIDAndFrameIDPair>& opener() const { return m_opener; }
void setTargetBackForwardItem(HistoryItem&);
const std::optional<BackForwardItemIdentifier>& targetBackForwardItemIdentifier() const { return m_targetBackForwardItemIdentifier; }
private:
- RefPtr<Document> m_sourceDocument;
+ std::optional<Requester> m_requester;
ResourceRequest m_resourceRequest;
NavigationType m_type;
ShouldOpenExternalURLsPolicy m_shouldOpenExternalURLsPolicy;
@@ -96,7 +111,7 @@
AtomicString m_downloadAttribute;
bool m_treatAsSameOriginNavigation;
bool m_isCrossOriginWindowOpenNavigation { false };
- std::optional<std::pair<uint64_t /* pageID */, uint64_t /* frameID */>> m_opener;
+ std::optional<PageIDAndFrameIDPair> m_opener;
std::optional<BackForwardItemIdentifier> m_targetBackForwardItemIdentifier;
};
Modified: trunk/Source/WebKit/ChangeLog (232215 => 232216)
--- trunk/Source/WebKit/ChangeLog 2018-05-25 23:45:36 UTC (rev 232215)
+++ trunk/Source/WebKit/ChangeLog 2018-05-25 23:48:11 UTC (rev 232216)
@@ -1,3 +1,16 @@
+2018-05-25 Daniel Bates <[email protected]>
+
+ NavigationAction should not hold a strong reference to a Document
+ https://bugs.webkit.org/show_bug.cgi?id=185712
+ <rdar://problem/40320916>
+
+ Reviewed by Brent Fulgham.
+
+ Update code to make use of NavigationAction::requester().
+
+ * WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:
+ (WebKit::WebFrameLoaderClient::dispatchDecidePolicyForNavigationAction):
+
2018-05-25 Tim Horton <[email protected]>
Ensure that the Web Content process doesn't sleep during initialization
Modified: trunk/Source/WebKit/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp (232215 => 232216)
--- trunk/Source/WebKit/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp 2018-05-25 23:45:36 UTC (rev 232215)
+++ trunk/Source/WebKit/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp 2018-05-25 23:48:11 UTC (rev 232216)
@@ -842,17 +842,18 @@
uint64_t listenerID = m_frame->setUpPolicyListener(WTFMove(function), WebFrame::ForNavigationAction::Yes);
- ASSERT(navigationAction.sourceDocument());
- const Document& sourceDocument = *navigationAction.sourceDocument();
- RefPtr<WebFrame> originatingFrame = sourceDocument.frame() ? WebFrame::fromCoreFrame(*sourceDocument.frame()) : nullptr;
+ ASSERT(navigationAction.requester());
+ auto requester = navigationAction.requester().value();
FrameInfoData originatingFrameInfoData;
originatingFrameInfoData.isMainFrame = navigationAction.initiatedByMainFrame() == InitiatedByMainFrame::Yes;
- originatingFrameInfoData.request = ResourceRequest(sourceDocument.url());
- originatingFrameInfoData.securityOrigin = sourceDocument.securityOrigin().data();
- if (originatingFrame)
- originatingFrameInfoData.frameID = originatingFrame->frameID();
+ originatingFrameInfoData.request = ResourceRequest { requester.url() };
+ originatingFrameInfoData.securityOrigin = requester.securityOrigin().data();
+ if (requester.frameID() && WebProcess::singleton().webFrame(requester.frameID()))
+ originatingFrameInfoData.frameID = requester.frameID();
+ uint64_t originatingPageID = requester.pageID() && WebProcess::singleton().webPage(requester.pageID()) ? requester.pageID() : 0;
+
NavigationActionData navigationActionData;
navigationActionData.navigationType = action->navigationType();
navigationActionData.modifiers = action->modifiers();
@@ -890,7 +891,7 @@
DownloadID downloadID;
std::optional<WebsitePoliciesData> websitePolicies;
- if (!webPage->sendSync(Messages::WebPageProxy::DecidePolicyForNavigationActionSync(m_frame->frameID(), SecurityOriginData::fromFrame(coreFrame), documentLoader->navigationID(), navigationActionData, originatingFrameInfoData, originatingFrame && originatingFrame->page() ? originatingFrame->page()->pageID() : 0, navigationAction.resourceRequest(), request, listenerID, UserData(WebProcess::singleton().transformObjectsToHandles(userData.get()).get())), Messages::WebPageProxy::DecidePolicyForNavigationActionSync::Reply(newNavigationID, policyAction, downloadID, websitePolicies))) {
+ if (!webPage->sendSync(Messages::WebPageProxy::DecidePolicyForNavigationActionSync(m_frame->frameID(), SecurityOriginData::fromFrame(coreFrame), documentLoader->navigationID(), navigationActionData, originatingFrameInfoData, originatingPageID, navigationAction.resourceRequest(), request, listenerID, UserData(WebProcess::singleton().transformObjectsToHandles(userData.get()).get())), Messages::WebPageProxy::DecidePolicyForNavigationActionSync::Reply(newNavigationID, policyAction, downloadID, websitePolicies))) {
m_frame->didReceivePolicyDecision(listenerID, PolicyAction::Ignore, 0, { }, { });
return;
}
@@ -900,7 +901,7 @@
}
ASSERT(policyDecisionMode == PolicyDecisionMode::Asynchronous);
- if (!webPage->send(Messages::WebPageProxy::DecidePolicyForNavigationAction(m_frame->frameID(), SecurityOriginData::fromFrame(coreFrame), documentLoader->navigationID(), navigationActionData, originatingFrameInfoData, originatingFrame && originatingFrame->page() ? originatingFrame->page()->pageID() : 0, navigationAction.resourceRequest(), request, listenerID, UserData(WebProcess::singleton().transformObjectsToHandles(userData.get()).get()))))
+ if (!webPage->send(Messages::WebPageProxy::DecidePolicyForNavigationAction(m_frame->frameID(), SecurityOriginData::fromFrame(coreFrame), documentLoader->navigationID(), navigationActionData, originatingFrameInfoData, originatingPageID, navigationAction.resourceRequest(), request, listenerID, UserData(WebProcess::singleton().transformObjectsToHandles(userData.get()).get()))))
m_frame->didReceivePolicyDecision(listenerID, PolicyAction::Ignore, 0, { }, { });
}
