Attachment 341220 Details for Bug 185712 – Patch

[patch] Patch

bug-185712-20180524132048.patch (text/plain), 15.29 KB, created by Daniel Bates on 2018-05-24 13:20:49 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Daniel Bates

Created: 2018-05-24 13:20:49 PDT

Size: 15.29 KB

patch

obsolete

>Subversion Revision: 232043
>diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
>index 6b89d43b0f2fc11d8dc1337d423c96937c09e5be..014d7200e49aac33bae7b194502d25ddc35da56a 100644
>--- a/Source/WebCore/ChangeLog
>+++ b/Source/WebCore/ChangeLog
>@@ -1,3 +1,35 @@
>+2018-05-23  Daniel Bates  <dabates@apple.com>
>+
>+        NavigationAction should not hold a strong reference to a Document
>+        https://bugs.webkit.org/show_bug.cgi?id=185712
>+        <rdar://problem/40320916>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        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.
>+        (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::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-21  Chris Dumez  <cdumez@apple.com>
> 
>         File's structured serialization should serialize lastModified attribute
>diff --git a/Source/WebKit/ChangeLog b/Source/WebKit/ChangeLog
>index 8aac8f8fa819153b3fb018b7d86e31b8b8f7162d..aa3a4c6c49c0fcc5c63bede280c2dd43009127d0 100644
>--- a/Source/WebKit/ChangeLog
>+++ b/Source/WebKit/ChangeLog
>@@ -1,3 +1,16 @@
>+2018-05-23  Daniel Bates  <dabates@apple.com>
>+
>+        NavigationAction should not hold a strong reference to a Document
>+        https://bugs.webkit.org/show_bug.cgi?id=185712
>+        <rdar://problem/40320916>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Update code to make use of NavigationAction::requester().
>+
>+        * WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:
>+        (WebKit::WebFrameLoaderClient::dispatchDecidePolicyForNavigationAction):
>+
> 2018-05-21  Aditya Keerthi  <akeerthi@apple.com>
> 
>         [iOS] Click events only fire once when editing
>diff --git a/Source/WebCore/loader/NavigationAction.cpp b/Source/WebCore/loader/NavigationAction.cpp
>index 8fcdb4861098bc54973885456a588e7875b2897c..fdc63d7449c6d5a5eb05065eb1ceefc6d1a1c228 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 { 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,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 234108b23f078018d462b2d6079382fb529f4fcf..137df0d79ecc507c8c010da4aa5e4a1b9ad4dd77 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 d0fe918fb2b7f896050626aaafa089fae6e97bcc..ce7864830ed673431159e7c60484fd23697c0516 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, { }, { });
> }
>

Flags:

bfulgham: review+

Actions: View | Formatted Diff | Diff

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