Attachment 341051 Details for Bug 185712 – [Patch] Remove NavigationAction::sourceDocument()

[patch] [Patch] Remove NavigationAction::sourceDocument()

WIP-RemoveExplicitStrongReference_1.patch (text/plain), 12.43 KB, created by Daniel Bates on 2018-05-22 17:33:22 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Daniel Bates

Created: 2018-05-22 17:33:22 PDT

Size: 12.43 KB

patch

obsolete

>From 08e3911b72338855d9d18c399ef4662008b2cd06 Mon Sep 17 00:00:00 2001
>From: Daniel Bates <dabates@apple.com>
>Date: Tue, 22 May 2018 16:15:10 -0700
>Subject: [PATCH] NavigationAction should not hold explicit ref to Document
>
>---
> Source/WebCore/loader/NavigationAction.cpp         | 27 +++++++++++++---------
> Source/WebCore/loader/NavigationAction.h           | 27 +++++++++++++++-------
> .../WebCoreSupport/WebFrameLoaderClient.cpp        | 19 +++++++--------
> 3 files changed, 45 insertions(+), 28 deletions(-)
>
>diff --git a/Source/WebCore/loader/NavigationAction.cpp b/Source/WebCore/loader/NavigationAction.cpp
>index 8fcdb486109..4431af46938 100644
>--- a/Source/WebCore/loader/NavigationAction.cpp
>+++ b/Source/WebCore/loader/NavigationAction.cpp
>@@ -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,17 @@
> #include "Document.h"
> #include "Event.h"
> #include "FrameLoader.h"
>+#include "FrameLoaderClient.h"
> #include "HistoryItem.h"
> 
> namespace WebCore {
> 
>+NavigationAction::Requester::Requester(Document& document)
>+    : m_url { URL { document.url() } }
>+    , m_pageIDAndFrameIDPair { std::make_pair(document.frame()->loader().client().pageID().value_or(0), document.frame()->loader().client().frameID().value_or(0)) }
>+{
>+}
>+
> NavigationAction::NavigationAction() = default;
> NavigationAction::~NavigationAction() = default;
> 
>@@ -45,22 +52,20 @@ NavigationAction::NavigationAction(NavigationAction&&) = default;
> NavigationAction& NavigationAction::operator=(const NavigationAction&) = default;
> NavigationAction& NavigationAction::operator=(NavigationAction&&) = default;
> 
>-static bool shouldTreatAsSameOriginNavigation(Document& source, const URL& url)
>+static bool shouldTreatAsSameOriginNavigation(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 }
>     , m_initiatedByMainFrame { initiatedByMainFrame }
>     , m_event { event }
>     , m_downloadAttribute { downloadAttribute }
>-    , m_treatAsSameOriginNavigation { shouldTreatAsSameOriginNavigation(source, resourceRequest.url()) }
>+    , m_treatAsSameOriginNavigation { shouldTreatAsSameOriginNavigation(requester, resourceRequest.url()) }
> {
> }
> 
>@@ -77,15 +82,15 @@ static NavigationType navigationType(FrameLoadType frameLoadType, bool isFormSub
>     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 }
>     , m_initiatedByMainFrame { initiatedByMainFrame }
>     , m_event { event }
>     , m_downloadAttribute { downloadAttribute }
>-    , m_treatAsSameOriginNavigation { shouldTreatAsSameOriginNavigation(source, resourceRequest.url()) }
>+    , m_treatAsSameOriginNavigation { shouldTreatAsSameOriginNavigation(requester, resourceRequest.url()) }
> {
> }
> 
>diff --git a/Source/WebCore/loader/NavigationAction.h b/Source/WebCore/loader/NavigationAction.h
>index 234108b23f0..137df0d79ec 100644
>--- a/Source/WebCore/loader/NavigationAction.h
>+++ b/Source/WebCore/loader/NavigationAction.h
>@@ -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
>@@ -54,9 +54,22 @@ public:
>     NavigationAction(NavigationAction&&);
>     NavigationAction& operator=(NavigationAction&&);
> 
>+    using PageIDAndFrameIDPair = std::pair<uint64_t /* pageID */, uint64_t /* frameID */>;
>+    class Requester {
>+    public:
>+        Requester(Document&);
>+        const URL& url() const { return m_url; }
>+        uint64_t pageID() const { return m_pageIDAndFrameIDPair.first; }
>+        uint64_t frameID() const { return m_pageIDAndFrameIDPair.second; }
>+    private:
>+        URL m_url;
>+        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 +77,6 @@ public:
>     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 +90,14 @@ public:
>     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 +107,7 @@ private:
>     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;
> };
> 
>diff --git a/Source/WebKit/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp b/Source/WebKit/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp
>index d0fe918fb2b..ce7864830ed 100644
>--- a/Source/WebKit/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp
>+++ b/Source/WebKit/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp
>@@ -842,16 +842,17 @@ void WebFrameLoaderClient::dispatchDecidePolicyForNavigationAction(const Navigat
>     
>     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 = SecurityOrigin::create(requester.url())->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();
>@@ -890,7 +891,7 @@ void WebFrameLoaderClient::dispatchDecidePolicyForNavigationAction(const Navigat
>         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 @@ void WebFrameLoaderClient::dispatchDecidePolicyForNavigationAction(const Navigat
>     }
> 
>     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, { }, { });
> }
> 
>-- 
>2.16.1 (Apple Git-102)
>

Actions: View | Formatted Diff | Diff

Attachments on bug 185712: 341045 | 341051 | 341055 | 341056 | 341059 | 341060 | 341065 | 341220