Regression (r240046): [PSON] Spurious changes to [WKWebView url] and [WKWebView loading] after [WKWebView loadRequest]

[Ali Juma]

After r240046, on a cross-origin [WKWebView loadRequest], [WKWebView url] will first change to the new URL, then *change back* to the old URL, then change again to the new URL. Similarly, [WKWebView loading] will become true, then become false, and then become true again.

More specifically, we get the following sequence:
1) [WKWebView loadRequest] --> [WKWebView ur] becomes the request URL and [WKWebView loading] becomes true
2) WebPageProxy::receivedPolicyDecision --> [WKWebView url] reverts to the previous URL and [WKWebView loading] becomes false
3) WebPageProxy::didStartProvisionalLoadForFrameShared --> [WKWebView url] becomes the requested URL again, and [WKWebView loading] becomes true again

The extra changes in (2) and (3) are confusing to embedders (like Chrome) that are doing KVO on [WKWebView url] and [WKWebView loading], since the change in (2) makes it look like the load was aborted.

The cause of these changes is that in r240046, WebPageProxy::receivedNavigationPolicyDecision sets policyAction to Ignore (instead of Suspend) on navigations which trigger process swaps, which then makes WebPageProxy::receivedPolicyDecision call PageLoadState::clearPendingAPIRequestURL, and this has the effect of reverting [WKWebView url] to the old URL and making [WKWebView loading] false.

Navigations initiated by [WKWebView goToBackForwardListItem] also have this broken behavior.

[Radar WebKit Bug Importer]

<rdar://problem/47635348>

[Chris Dumez]

Thanks for reporting this Ali, will fix ASAP.

[Chris Dumez]

Ali, how/when is it observable by the client exactly?

I tried to write the following API test but it is passing:
TEST(ProcessSwap, LoadingStateAfterPolicyDecision)
{
    auto processPoolConfiguration = adoptNS([[_WKProcessPoolConfiguration alloc] init]);
    processPoolConfiguration.get().processSwapsOnNavigation = YES;
    auto processPool = adoptNS([[WKProcessPool alloc] _initWithConfiguration:processPoolConfiguration.get()]);

    auto webViewConfiguration = adoptNS([[WKWebViewConfiguration alloc] init]);
    [webViewConfiguration setProcessPool:processPool.get()];
    auto handler = adoptNS([[PSONScheme alloc] init]);
    [webViewConfiguration setURLSchemeHandler:handler.get() forURLScheme:@"PSON"];

    auto webView = adoptNS([[WKWebView alloc] initWithFrame:NSMakeRect(0, 0, 800, 600) configuration:webViewConfiguration.get()]);
    auto navigationDelegate = adoptNS([[PSONNavigationDelegate alloc] init]);
    [webView setNavigationDelegate:navigationDelegate.get()];

    [webView configuration].preferences.safeBrowsingEnabled = NO;

    [webView loadRequest:[NSURLRequest requestWithURL:[NSURL URLWithString:@"pson://www.webkit.org/main.html"]]];
    TestWebKitAPI::Util::run(&done);
    done = false;

    didStartProvisionalLoad = false;
    [webView loadRequest:[NSURLRequest requestWithURL:[NSURL URLWithString:@"pson://www.apple.com/main.html"]]];
    EXPECT_WK_STREQ(@"pson://www.apple.com/main.html", [[webView URL] absoluteString]);
    EXPECT_TRUE([webView isLoading]);

    navigationDelegate->decidePolicyForNavigationAction = ^(WKNavigationAction *, void (^decisionHandler)(WKNavigationActionPolicy)) {
        EXPECT_WK_STREQ(@"pson://www.apple.com/main.html", [[webView URL] absoluteString]);
        EXPECT_TRUE([webView isLoading]);

        decisionHandler(WKNavigationActionPolicyAllow);

        EXPECT_WK_STREQ(@"pson://www.apple.com/main.html", [[webView URL] absoluteString]);
        EXPECT_TRUE([webView isLoading]);
    };

    TestWebKitAPI::Util::run(&didStartProvisionalLoad);
    didStartProvisionalLoad = false;

    EXPECT_WK_STREQ(@"pson://www.apple.com/main.html", [[webView URL] absoluteString]);
    EXPECT_TRUE([webView isLoading]);

    TestWebKitAPI::Util::run(&done);
    done = false;

    EXPECT_WK_STREQ(@"pson://www.apple.com/main.html", [[webView URL] absoluteString]);
    EXPECT_FALSE([webView isLoading]);
}

[Chris Dumez]

(In reply to Chris Dumez from comment #3)
> Ali, how/when is it observable by the client exactly?
> 
> I tried to write the following API test but it is passing:
> TEST(ProcessSwap, LoadingStateAfterPolicyDecision)
> {
>     auto processPoolConfiguration = adoptNS([[_WKProcessPoolConfiguration
> alloc] init]);
>     processPoolConfiguration.get().processSwapsOnNavigation = YES;
>     auto processPool = adoptNS([[WKProcessPool alloc]
> _initWithConfiguration:processPoolConfiguration.get()]);
> 
>     auto webViewConfiguration = adoptNS([[WKWebViewConfiguration alloc]
> init]);
>     [webViewConfiguration setProcessPool:processPool.get()];
>     auto handler = adoptNS([[PSONScheme alloc] init]);
>     [webViewConfiguration setURLSchemeHandler:handler.get()
> forURLScheme:@"PSON"];
> 
>     auto webView = adoptNS([[WKWebView alloc] initWithFrame:NSMakeRect(0, 0,
> 800, 600) configuration:webViewConfiguration.get()]);
>     auto navigationDelegate = adoptNS([[PSONNavigationDelegate alloc] init]);
>     [webView setNavigationDelegate:navigationDelegate.get()];
> 
>     [webView configuration].preferences.safeBrowsingEnabled = NO;
> 
>     [webView loadRequest:[NSURLRequest requestWithURL:[NSURL
> URLWithString:@"pson://www.webkit.org/main.html"]]];
>     TestWebKitAPI::Util::run(&done);
>     done = false;
> 
>     didStartProvisionalLoad = false;
>     [webView loadRequest:[NSURLRequest requestWithURL:[NSURL
> URLWithString:@"pson://www.apple.com/main.html"]]];
>     EXPECT_WK_STREQ(@"pson://www.apple.com/main.html", [[webView URL]
> absoluteString]);
>     EXPECT_TRUE([webView isLoading]);
> 
>     navigationDelegate->decidePolicyForNavigationAction =
> ^(WKNavigationAction *, void (^decisionHandler)(WKNavigationActionPolicy)) {
>         EXPECT_WK_STREQ(@"pson://www.apple.com/main.html", [[webView URL]
> absoluteString]);
>         EXPECT_TRUE([webView isLoading]);
> 
>         decisionHandler(WKNavigationActionPolicyAllow);
> 
>         EXPECT_WK_STREQ(@"pson://www.apple.com/main.html", [[webView URL]
> absoluteString]);
>         EXPECT_TRUE([webView isLoading]);
>     };
> 
>     TestWebKitAPI::Util::run(&didStartProvisionalLoad);
>     didStartProvisionalLoad = false;
> 
>     EXPECT_WK_STREQ(@"pson://www.apple.com/main.html", [[webView URL]
> absoluteString]);
>     EXPECT_TRUE([webView isLoading]);
> 
>     TestWebKitAPI::Util::run(&done);
>     done = false;
> 
>     EXPECT_WK_STREQ(@"pson://www.apple.com/main.html", [[webView URL]
> absoluteString]);
>     EXPECT_FALSE([webView isLoading]);
> }

Nevermind, I can reproduce using KVO for "loading" key instead.

[Chris Dumez]

Created attachment 360473 [details]
Patch

[EWS Watchlist]

Attachment 360473 [details] did not pass style-queue:


ERROR: Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:3475:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 1 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Ali Juma]

Thanks for uploading a fix Chris!

(In reply to Chris Dumez from comment #4)
> Nevermind, I can reproduce using KVO for "loading" key instead.

Yeah, this is observable by the client when doing KVO on 'loading' or on 'url'.

[Alex Christensen]

Comment on attachment 360473 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=360473&action=review

> Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:3476
> +    EXPECT_WK_STREQ(@"loading", keyPath);

Should the test observe url also?

[Chris Dumez]

Comment on attachment 360473 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=360473&action=review

>> Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:3476
>> +    EXPECT_WK_STREQ(@"loading", keyPath);
> 
> Should the test observe url also?

Why not. Let me look up how to do that.

[Chris Dumez]

Created attachment 360487 [details]
Patch

[Chris Dumez]

(In reply to Chris Dumez from comment #9)
> Comment on attachment 360473 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=360473&action=review
> 
> >> Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:3476
> >> +    EXPECT_WK_STREQ(@"loading", keyPath);
> > 
> > Should the test observe url also?
> 
> Why not. Let me look up how to do that.

Done.

[EWS Watchlist]

Attachment 360487 [details] did not pass style-queue:


ERROR: Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:3476:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 1 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Chris Dumez]

Comment on attachment 360487 [details]
Patch

Clearing flags on attachment: 360487

Committed r240675: <https://trac.webkit.org/changeset/240675>

[Chris Dumez]

All reviewed patches have been landed.  Closing bug.