Changeset 247851 in webkit


Ignore:
Timestamp:
Jul 25, 2019 6:26:24 PM (5 years ago)
Author:
jiewen_tan@apple.com
Message:

WebPageProxy::receivedPolicyDecision should check navigation ID before clear pendingAPIRequest
​https://bugs.webkit.org/show_bug.cgi?id=200108
<rdar://problem/53521238>

Reviewed by Chris Dumez.

Source/WebKit:

Assuming there are two loads happening one after another. There is an issue when clients save
the first decisionHandler and then call WKNavigationActionPolicyCancel for it right after the
second decisionHandler received, -[WKWebView URL] could return a null string even though it is
loading the second one.

To solve that, this patch pairs a navigationID with the pendingAPIRequestURL such that
WebPageProxy::receivedPolicyDecision could clear the pendingAPIRequestURL only if
the passed navigation ID matches the current one.

  • UIProcess/PageLoadState.cpp:

(WebKit::PageLoadState::reset):
(WebKit::PageLoadState::activeURL):
(WebKit::PageLoadState::estimatedProgress):
(WebKit::PageLoadState::pendingAPIRequestURL const):
(WebKit::PageLoadState::pendingAPIRequest const):
(WebKit::PageLoadState::setPendingAPIRequest):
(WebKit::PageLoadState::clearPendingAPIRequest):
(WebKit::PageLoadState::isLoading):
(WebKit::PageLoadState::setPendingAPIRequestURL): Deleted.
(WebKit::PageLoadState::clearPendingAPIRequestURL): Deleted.

  • UIProcess/PageLoadState.h:

(WebKit::PageLoadState::setPendingAPIRequest):
(WebKit::PageLoadState::setPendingAPIRequestURL): Deleted.

  • UIProcess/WebPageProxy.cpp:

(WebKit::WebPageProxy::launchProcessForReload):
(WebKit::WebPageProxy::loadRequestWithNavigationShared):
(WebKit::WebPageProxy::loadFile):
(WebKit::WebPageProxy::loadDataWithNavigationShared):
(WebKit::WebPageProxy::loadAlternateHTML):
(WebKit::WebPageProxy::loadWebArchiveData):
(WebKit::WebPageProxy::reload):
(WebKit::WebPageProxy::goToBackForwardItem):
(WebKit::WebPageProxy::receivedPolicyDecision):
(WebKit::WebPageProxy::continueNavigationInNewProcess):
(WebKit::WebPageProxy::didStartProvisionalLoadForFrameShared):
(WebKit::WebPageProxy::didSameDocumentNavigationForFrame):
(WebKit::WebPageProxy::decidePolicyForNavigationAction):

Tools:

Added an API test.

  • TestWebKitAPI/Tests/WebKitCocoa/DecidePolicyForNavigationAction.mm:

(-[DecidePolicyForNavigationActionController webView:decidePolicyForNavigationAction:decisionHandler:]):
(TEST):

Location:
trunk
Files:
6 edited

Legend:

Unmodified
Added
Removed

  • trunk/Source/WebKit/ChangeLog

    r247842 r247851  
     12019-07-25  Jiewen Tan  <jiewen_tan@apple.com>
     2
     3        WebPageProxy::receivedPolicyDecision should check navigation ID before clear pendingAPIRequest
     4        https://bugs.webkit.org/show_bug.cgi?id=200108
     5        <rdar://problem/53521238>
     6
     7        Reviewed by Chris Dumez.
     8
     9        Assuming there are two loads happening one after another. There is an issue when clients save
     10        the first decisionHandler and then call WKNavigationActionPolicyCancel for it right after the
     11        second decisionHandler received, -[WKWebView URL] could return a null string even though it is
     12        loading the second one.
     13
     14        To solve that, this patch pairs a navigationID with the pendingAPIRequestURL such that
     15        WebPageProxy::receivedPolicyDecision could clear the pendingAPIRequestURL only if
     16        the passed navigation ID matches the current one.
     17
     18        * UIProcess/PageLoadState.cpp:
     19        (WebKit::PageLoadState::reset):
     20        (WebKit::PageLoadState::activeURL):
     21        (WebKit::PageLoadState::estimatedProgress):
     22        (WebKit::PageLoadState::pendingAPIRequestURL const):
     23        (WebKit::PageLoadState::pendingAPIRequest const):
     24        (WebKit::PageLoadState::setPendingAPIRequest):
     25        (WebKit::PageLoadState::clearPendingAPIRequest):
     26        (WebKit::PageLoadState::isLoading):
     27        (WebKit::PageLoadState::setPendingAPIRequestURL): Deleted.
     28        (WebKit::PageLoadState::clearPendingAPIRequestURL): Deleted.
     29        * UIProcess/PageLoadState.h:
     30        (WebKit::PageLoadState::setPendingAPIRequest):
     31        (WebKit::PageLoadState::setPendingAPIRequestURL): Deleted.
     32        * UIProcess/WebPageProxy.cpp:
     33        (WebKit::WebPageProxy::launchProcessForReload):
     34        (WebKit::WebPageProxy::loadRequestWithNavigationShared):
     35        (WebKit::WebPageProxy::loadFile):
     36        (WebKit::WebPageProxy::loadDataWithNavigationShared):
     37        (WebKit::WebPageProxy::loadAlternateHTML):
     38        (WebKit::WebPageProxy::loadWebArchiveData):
     39        (WebKit::WebPageProxy::reload):
     40        (WebKit::WebPageProxy::goToBackForwardItem):
     41        (WebKit::WebPageProxy::receivedPolicyDecision):
     42        (WebKit::WebPageProxy::continueNavigationInNewProcess):
     43        (WebKit::WebPageProxy::didStartProvisionalLoadForFrameShared):
     44        (WebKit::WebPageProxy::didSameDocumentNavigationForFrame):
     45        (WebKit::WebPageProxy::decidePolicyForNavigationAction):
     46
    1472019-07-25  Commit Queue  <commit-queue@webkit.org>
    248

  • trunk/Source/WebKit/UIProcess/PageLoadState.cpp

    r246694 r247851  
    155155    m_uncommittedState.hasInsecureContent = false;
    156156
    157     m_uncommittedState.pendingAPIRequestURL = String();
     157    m_uncommittedState.pendingAPIRequest = { };
    158158    m_uncommittedState.provisionalURL = String();
    159159    m_uncommittedState.url = String();
    … …  
    183183    // even when there's no main frame yet, as it might be the
    184184    // first API request.
    185     if (!data.pendingAPIRequestURL.isNull())
    186         return data.pendingAPIRequestURL;
     185    if (!data.pendingAPIRequest.url.isNull())
     186        return data.pendingAPIRequest.url;
    187187
    188188    if (!data.unreachableURL.isEmpty())
    … …  
    224224double PageLoadState::estimatedProgress(const Data& data)
    225225{
    226     if (!data.pendingAPIRequestURL.isNull())
     226    if (!data.pendingAPIRequest.url.isNull())
    227227        return initialProgressValue;
    228228
    … …  
    237237const String& PageLoadState::pendingAPIRequestURL() const
    238238{
    239     return m_committedState.pendingAPIRequestURL;
     239    return m_committedState.pendingAPIRequest.url;
     240}
     241
     242auto PageLoadState::pendingAPIRequest() const -> const PendingAPIRequest&
     243{
     244    return m_committedState.pendingAPIRequest;
    240245}
    241246
    … …  
    245250}
    246251
    247 void PageLoadState::setPendingAPIRequestURL(const Transaction::Token& token, const String& pendingAPIRequestURL, const URL& resourceDirectoryURL)
    248 {
    249     ASSERT_UNUSED(token, &token.m_pageLoadState == this);
    250     m_uncommittedState.pendingAPIRequestURL = pendingAPIRequestURL;
     252void PageLoadState::setPendingAPIRequest(const Transaction::Token& token, PendingAPIRequest&& pendingAPIRequest, const URL& resourceDirectoryURL)
     253{
     254    ASSERT_UNUSED(token, &token.m_pageLoadState == this);
     255    m_uncommittedState.pendingAPIRequest = WTFMove(pendingAPIRequest);
    251256    m_uncommittedState.resourceDirectoryURL = resourceDirectoryURL;
    252257}
    253258
    254 void PageLoadState::clearPendingAPIRequestURL(const Transaction::Token& token)
    255 {
    256     ASSERT_UNUSED(token, &token.m_pageLoadState == this);
    257     m_uncommittedState.pendingAPIRequestURL = String();
     259void PageLoadState::clearPendingAPIRequest(const Transaction::Token& token)
     260{
     261    ASSERT_UNUSED(token, &token.m_pageLoadState == this);
     262    m_uncommittedState.pendingAPIRequest = { };
    258263}
    259264
    … …  
    411416bool PageLoadState::isLoading(const Data& data)
    412417{
    413     if (!data.pendingAPIRequestURL.isNull())
     418    if (!data.pendingAPIRequest.url.isNull())
    414419        return true;
    415420

  • trunk/Source/WebKit/UIProcess/PageLoadState.h

    r246694 r247851  
    114114    };
    115115
     116    struct PendingAPIRequest {
     117        uint64_t navigationID { 0 };
     118        String url;
     119    };
     120
    116121    void addObserver(Observer&);
    117122    void removeObserver(Observer&);
    … …  
    145150
    146151    const String& pendingAPIRequestURL() const;
    147     void setPendingAPIRequestURL(const Transaction::Token&, const String& pendingAPIRequestURL, const URL& resourceDirectoryPath = { });
    148     void clearPendingAPIRequestURL(const Transaction::Token&);
     152    const PendingAPIRequest& pendingAPIRequest() const;
     153    void setPendingAPIRequest(const Transaction::Token&, PendingAPIRequest&& pendingAPIRequest, const URL& resourceDirectoryPath = { });
     154    void clearPendingAPIRequest(const Transaction::Token&);
    149155
    150156    void didStartProvisionalLoad(const Transaction::Token&, const String& url, const String& unreachableURL);
    … …  
    208214        bool hasInsecureContent;
    209215
    210         String pendingAPIRequestURL;
     216        PendingAPIRequest pendingAPIRequest;
    211217
    212218        String provisionalURL;

  • trunk/Source/WebKit/UIProcess/WebPageProxy.cpp

    r247820 r247851  
    889889    auto navigation = m_navigationState->createReloadNavigation();
    890890
     891    String url = currentURL();
     892    if (!url.isEmpty()) {
     893        auto transaction = m_pageLoadState.transaction();
     894        m_pageLoadState.setPendingAPIRequest(transaction, { navigation->navigationID(), url });
     895    }
     896
    891897    // We allow stale content when reloading a WebProcess that's been killed or crashed.
    892898    m_process->send(Messages::WebPage::GoToBackForwardItem(navigation->navigationID(), m_backForwardList->currentItem()->itemID(), FrameLoadType::IndexedBackForward, ShouldTreatAsContinuingLoad::No, WTF::nullopt), m_pageID);
    … …  
    11161122    auto url = request.url();
    11171123    if (shouldTreatAsContinuingLoad != ShouldTreatAsContinuingLoad::Yes)
    1118         m_pageLoadState.setPendingAPIRequestURL(transaction, url);
     1124        m_pageLoadState.setPendingAPIRequest(transaction, { navigation.navigationID(), url });
    11191125
    11201126    LoadParameters loadParameters;
    … …  
    11691175    auto transaction = m_pageLoadState.transaction();
    11701176
    1171     m_pageLoadState.setPendingAPIRequestURL(transaction, fileURLString, resourceDirectoryURL);
     1177    m_pageLoadState.setPendingAPIRequest(transaction, { navigation->navigationID(), fileURLString }, resourceDirectoryURL);
    11721178
    11731179    String resourceDirectoryPath = resourceDirectoryURL.fileSystemPath();
    … …  
    12131219    auto transaction = m_pageLoadState.transaction();
    12141220
    1215     m_pageLoadState.setPendingAPIRequestURL(transaction, !baseURL.isEmpty() ? baseURL : WTF::blankURL().string());
     1221    m_pageLoadState.setPendingAPIRequest(transaction, { navigation.navigationID(), !baseURL.isEmpty() ? baseURL : WTF::blankURL().string() });
    12161222
    12171223    LoadParameters loadParameters;
    … …  
    12521258    auto transaction = m_pageLoadState.transaction();
    12531259
    1254     m_pageLoadState.setPendingAPIRequestURL(transaction, unreachableURL);
     1260    m_pageLoadState.setPendingAPIRequest(transaction, { 0, unreachableURL });
    12551261    m_pageLoadState.setUnreachableURL(transaction, unreachableURL);
    12561262
    … …  
    12881294
    12891295    auto transaction = m_pageLoadState.transaction();
    1290     m_pageLoadState.setPendingAPIRequestURL(transaction, WTF::blankURL().string());
     1296    m_pageLoadState.setPendingAPIRequest(transaction, { 0, WTF::blankURL().string() });
    12911297
    12921298    LoadParameters loadParameters;
    … …  
    13461352    String url = currentURL();
    13471353    if (!url.isEmpty()) {
    1348         auto transaction = m_pageLoadState.transaction();
    1349         m_pageLoadState.setPendingAPIRequestURL(transaction, url);
    1350 
    13511354        // We may not have an extension yet if back/forward list was reinstated after a WebProcess crash or a browser relaunch
    13521355        maybeInitializeSandboxExtensionHandle(m_process, URL(URL(), url), currentResourceDirectoryURL(), sandboxExtensionHandle);
    … …  
    13571360   
    13581361    auto navigation = m_navigationState->createReloadNavigation();
     1362
     1363    if (!url.isEmpty()) {
     1364        auto transaction = m_pageLoadState.transaction();
     1365        m_pageLoadState.setPendingAPIRequest(transaction, { navigation->navigationID(), url });
     1366    }
    13591367
    13601368    // Store decision to reload without content blockers on the navigation so that we can later set the corresponding
    … …  
    14251433        return launchProcessWithItem(item);
    14261434
    1427     auto transaction = m_pageLoadState.transaction();
    1428 
    1429     m_pageLoadState.setPendingAPIRequestURL(transaction, item.url());
    1430 
    14311435    RefPtr<API::Navigation> navigation;
    14321436    if (!m_backForwardList->currentItem()->itemIsInSameDocument(item))
    14331437        navigation = m_navigationState->createBackForwardNavigation(item, m_backForwardList->currentItem(), frameLoadType);
     1438
     1439    auto transaction = m_pageLoadState.transaction();
     1440    m_pageLoadState.setPendingAPIRequest(transaction, { navigation ? navigation->navigationID() : 0, item.url() });
    14341441
    14351442    m_process->send(Messages::WebPage::GoToBackForwardItem(navigation ? navigation->navigationID() : 0, item.itemID(), frameLoadType, ShouldTreatAsContinuingLoad::No, WTF::nullopt), m_pageID);
    … …  
    28782885    auto transaction = m_pageLoadState.transaction();
    28792886
    2880     if (action == PolicyAction::Ignore && willContinueLoadInNewProcess == WillContinueLoadInNewProcess::No)
    2881         m_pageLoadState.clearPendingAPIRequestURL(transaction);
     2887    if (action == PolicyAction::Ignore && willContinueLoadInNewProcess == WillContinueLoadInNewProcess::No && navigation && navigation->navigationID() == m_pageLoadState.pendingAPIRequest().navigationID)
     2888        m_pageLoadState.clearPendingAPIRequest(transaction);
    28822889
    28832890    DownloadID downloadID = { };
    … …  
    29572964
    29582965        auto transaction = m_pageLoadState.transaction();
    2959         m_pageLoadState.setPendingAPIRequestURL(transaction, item->url());
     2966        m_pageLoadState.setPendingAPIRequest(transaction, { navigation.navigationID(), item->url() });
    29602967
    29612968        m_provisionalPage->goToBackForwardItem(navigation, *item, WTFMove(websitePolicies));
    … …  
    39773984    auto transaction = m_pageLoadState.transaction();
    39783985
    3979     m_pageLoadState.clearPendingAPIRequestURL(transaction);
     3986    m_pageLoadState.clearPendingAPIRequest(transaction);
    39803987
    39813988    if (frame->isMainFrame()) {
    … …  
    44424449    }
    44434450
    4444     m_pageLoadState.clearPendingAPIRequestURL(transaction);
     4451    m_pageLoadState.clearPendingAPIRequest(transaction);
    44454452    frame->didSameDocumentNavigation(url);
    44464453
    … …  
    46244631    auto transaction = m_pageLoadState.transaction();
    46254632
    4626     bool fromAPI = request.url() == m_pageLoadState.pendingAPIRequestURL();
     4633    bool fromAPI = navigationID == m_pageLoadState.pendingAPIRequest().navigationID;
    46274634    if (!fromAPI)
    4628         m_pageLoadState.clearPendingAPIRequestURL(transaction);
     4635        m_pageLoadState.clearPendingAPIRequest(transaction);
    46294636
    46304637    if (!checkURLReceivedFromCurrentOrPreviousWebProcess(process, request.url())) {
    … …  
    46854692    auto listener = makeRef(frame.setUpPolicyListenerProxy([this, protectedThis = makeRef(*this), frame = makeRef(frame), sender = WTFMove(sender), navigation] (PolicyAction policyAction, API::WebsitePolicies* policies, ProcessSwapRequestedByClient processSwapRequestedByClient, RefPtr<SafeBrowsingWarning>&& safeBrowsingWarning) mutable {
    46864693
    4687         auto completionHandler = [this, protectedThis = protectedThis.copyRef(), frame = frame.copyRef(), sender = WTFMove(sender), navigation = WTFMove(navigation), processSwapRequestedByClient, policies = makeRefPtr(policies)] (PolicyAction policyAction) mutable {
     4694        auto completionHandler = [this, protectedThis = protectedThis.copyRef(), frame = frame.copyRef(), sender = WTFMove(sender), navigation, processSwapRequestedByClient, policies = makeRefPtr(policies)] (PolicyAction policyAction) mutable {
    46884695            if (frame->isMainFrame()) {
    46894696                if (!policies) {
    … …  
    47054712            if (frame->isMainFrame() && safeBrowsingWarning->url().isValid()) {
    47064713                auto transaction = m_pageLoadState.transaction();
    4707                 m_pageLoadState.setPendingAPIRequestURL(transaction, safeBrowsingWarning->url());
     4714                m_pageLoadState.setPendingAPIRequest(transaction, { navigation->navigationID(), safeBrowsingWarning->url() });
    47084715                m_pageLoadState.commitChanges();
    47094716            }

  • trunk/Tools/ChangeLog

    r247850 r247851  
     12019-07-24  Jiewen Tan  <jiewen_tan@apple.com>
     2
     3        WebPageProxy::receivedPolicyDecision should check navigation ID before clear pendingAPIRequest
     4        https://bugs.webkit.org/show_bug.cgi?id=200108
     5        <rdar://problem/53521238>
     6
     7        Reviewed by Chris Dumez.
     8
     9        Added an API test.
     10
     11        * TestWebKitAPI/Tests/WebKitCocoa/DecidePolicyForNavigationAction.mm:
     12        (-[DecidePolicyForNavigationActionController webView:decidePolicyForNavigationAction:decisionHandler:]):
     13        (TEST):
     14
    1152019-07-25  Jonathan Bedard  <jbedard@apple.com>
    216

  • trunk/Tools/TestWebKitAPI/Tests/WebKitCocoa/DecidePolicyForNavigationAction.mm

    r242339 r247851  
    3333#import <WebKit/WKProcessPoolPrivate.h>
    3434#import <WebKit/_WKProcessPoolConfiguration.h>
     35#import <wtf/BlockPtr.h>
    3536#import <wtf/RetainPtr.h>
    3637#import <wtf/mac/AppKitCompatibilityDeclarations.h>
    3738
    3839static bool shouldCancelNavigation;
     40static bool shouldDelayDecision;
    3941static bool createdWebView;
    4042static bool decidedPolicy;
    … …  
    4244static RetainPtr<WKNavigationAction> action;
    4345static RetainPtr<WKWebView> newWebView;
     46static BlockPtr<void(WKNavigationActionPolicy)> delayedDecision;
    4447
    4548static NSString *firstURL = @"data:text/html,First";
    … …  
    6366    }
    6467
     68    if (shouldDelayDecision) {
     69        if (delayedDecision)
     70            delayedDecision(WKNavigationActionPolicyCancel);
     71        delayedDecision = makeBlockPtr(decisionHandler);
     72        decidedPolicy = true;
     73        return;
     74    }
     75
    6576    decisionHandler(webView == newWebView.get() ? WKNavigationActionPolicyCancel : WKNavigationActionPolicyAllow);
    6677
    … …  
    592603    newWebView = nullptr;
    593604    action = nullptr;
     605}
     606
     607TEST(WebKit, DelayDecidePolicyForNavigationAction)
     608{
     609    shouldDelayDecision = true;
     610
     611    auto webView = adoptNS([[WKWebView alloc] initWithFrame:NSMakeRect(0, 0, 800, 600)]);
     612    auto controller = adoptNS([[DecidePolicyForNavigationActionController alloc] init]);
     613    [webView setNavigationDelegate:controller.get()];
     614
     615    RetainPtr<NSURL> testURL = [[NSBundle mainBundle] URLForResource:@"simple" withExtension:@"html" subdirectory:@"TestWebKitAPI.resources"];
     616    [webView loadRequest:[NSURLRequest requestWithURL:testURL.get()]];
     617    TestWebKitAPI::Util::run(&decidedPolicy);
     618    [webView loadRequest:[NSURLRequest requestWithURL:testURL.get()]];
     619    TestWebKitAPI::Util::sleep(0.5); // Wait until the pending api request gets clear.
     620    EXPECT_TRUE([[webView URL] isEqual:testURL.get()]);
     621
     622    shouldDelayDecision = false;
     623    delayedDecision(WKNavigationActionPolicyCancel);
    594624}
    595625
Note: See TracChangeset for help on using the changeset viewer.