Changeset 229828 in webkit


Ignore:
Timestamp:
Mar 21, 2018 2:19:06 PM (6 years ago)
Author:
Chris Dumez
Message:

ScrollViewInsetTests.RestoreInitialContentOffsetAfterCrash API test is failing with async delegates
​https://bugs.webkit.org/show_bug.cgi?id=183787

Reviewed by Wenson Hsieh.

Source/WebCore:

  • loader/FrameLoader.cpp:

(WebCore::FrameLoader::continueLoadAfterNavigationPolicy):

  • loader/FrameLoaderClient.h:

Source/WebKit:

Without asynchronous policy delegates, when the client requests a navigation, we would:

  1. Do a synchronous navigation policy check
  2. If the client allows the navigation, start the provisional load

Starting the provisional load would freeze the layer tree until first meaningful
layout via WebFrameLoaderClient::provisionalLoadStarted() -> WebPage::didStartPageTransition().

When constructing a WebView and then requesting a load right away. This would make sure
we do not commit a layer tree for the initial about:blank page because the layer tree
would be frozen until we have something meaningful to show for the following load.

However, with asynchronous policy delegates, we are able to do a layer tree commit
during the asynchronous navigation policy check because the layer tree is not frozen
yet (provisional load has not started) and the process is not stuck on synchronous
IPC. When constructing a WebView and then requesting a load right away, this would
allow a layer tree commit for about:blank to happen before we've even started the
load. This would cause some API tests to fail on iOS.

To address the issue, we try and maintain pre-existing behavior by freezing the
layer tree during navigation policy decision.

  • WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:

(WebKit::WebFrameLoaderClient::dispatchDecidePolicyForNavigationAction):
(WebKit::WebFrameLoaderClient::didDecidePolicyForNavigationAction):

  • WebProcess/WebCoreSupport/WebFrameLoaderClient.h:
  • WebProcess/WebPage/WebPage.cpp:

(WebKit::WebPage::didStartNavigationPolicyCheck):
(WebKit::WebPage::didCompleteNavigationPolicyCheck):

  • WebProcess/WebPage/WebPage.h:

Tools:

Add API test coverage.

  • TestWebKitAPI/Tests/ios/ScrollViewInsetTests.mm:

(-[AsyncPolicyDelegateForInsetTest webView:didFinishNavigation:]):
(-[AsyncPolicyDelegateForInsetTest webView:decidePolicyForNavigationAction:decisionHandler:]):
(-[AsyncPolicyDelegateForInsetTest webView:decidePolicyForNavigationResponse:decisionHandler:]):
(-[AsyncPolicyDelegateForInsetTest webViewWebContentProcessDidTerminate:]):
(TestWebKitAPI::TEST):

Location:
trunk
Files:
10 edited

Legend:

Unmodified
Added
Removed

  • trunk/Source/WebCore/ChangeLog

    r229824 r229828  
     12018-03-21  Chris Dumez  <cdumez@apple.com>
     2
     3        ScrollViewInsetTests.RestoreInitialContentOffsetAfterCrash API test is failing with async delegates
     4        https://bugs.webkit.org/show_bug.cgi?id=183787
     5
     6        Reviewed by Wenson Hsieh.
     7
     8        * loader/FrameLoader.cpp:
     9        (WebCore::FrameLoader::continueLoadAfterNavigationPolicy):
     10        * loader/FrameLoaderClient.h:
     11
    1122018-03-21  Eric Carlson  <eric.carlson@apple.com>
    213

  • trunk/Source/WebCore/loader/FrameLoader.cpp

    r229778 r229828  
    31683168    ASSERT(m_policyDocumentLoader || !m_provisionalDocumentLoader->unreachableURL().isEmpty());
    31693169
     3170    m_client.didDecidePolicyForNavigationAction();
     3171
    31703172    bool isTargetItem = history().provisionalItem() ? history().provisionalItem()->isTargetItem() : false;
    31713173

  • trunk/Source/WebCore/loader/FrameLoaderClient.h

    r228942 r229828  
    191191    virtual void dispatchDecidePolicyForNewWindowAction(const NavigationAction&, const ResourceRequest&, FormState*, const String& frameName, FramePolicyFunction&&) = 0;
    192192    virtual void dispatchDecidePolicyForNavigationAction(const NavigationAction&, const ResourceRequest&, bool didReceiveRedirectResponse, FormState*, FramePolicyFunction&&) = 0;
     193    virtual void didDecidePolicyForNavigationAction() { }
    193194    virtual void cancelPolicyCheck() = 0;
    194195

  • trunk/Source/WebKit/ChangeLog

    r229823 r229828  
     12018-03-21  Chris Dumez  <cdumez@apple.com>
     2
     3        ScrollViewInsetTests.RestoreInitialContentOffsetAfterCrash API test is failing with async delegates
     4        https://bugs.webkit.org/show_bug.cgi?id=183787
     5
     6        Reviewed by Wenson Hsieh.
     7
     8        Without asynchronous policy delegates, when the client requests a navigation, we would:
     9        1. Do a synchronous navigation policy check
     10        2. If the client allows the navigation, start the provisional load
     11
     12        Starting the provisional load would freeze the layer tree until first meaningful
     13        layout via WebFrameLoaderClient::provisionalLoadStarted() -> WebPage::didStartPageTransition().
     14
     15        When constructing a WebView and then requesting a load right away. This would make sure
     16        we do not commit a layer tree for the initial about:blank page because the layer tree
     17        would be frozen until we have something meaningful to show for the following load.
     18
     19        However, with asynchronous policy delegates, we are able to do a layer tree commit
     20        during the asynchronous navigation policy check because the layer tree is not frozen
     21        yet (provisional load has not started) and the process is not stuck on synchronous
     22        IPC. When constructing a WebView and then requesting a load right away, this would
     23        allow a layer tree commit for about:blank to happen before we've even started the
     24        load. This would cause some API tests to fail on iOS.
     25
     26        To address the issue, we try and maintain pre-existing behavior by freezing the
     27        layer tree during navigation policy decision.
     28
     29        * WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:
     30        (WebKit::WebFrameLoaderClient::dispatchDecidePolicyForNavigationAction):
     31        (WebKit::WebFrameLoaderClient::didDecidePolicyForNavigationAction):
     32        * WebProcess/WebCoreSupport/WebFrameLoaderClient.h:
     33        * WebProcess/WebPage/WebPage.cpp:
     34        (WebKit::WebPage::didStartNavigationPolicyCheck):
     35        (WebKit::WebPage::didCompleteNavigationPolicyCheck):
     36        * WebProcess/WebPage/WebPage.h:
     37
    1382018-03-21  Brent Fulgham  <bfulgham@apple.com>
    239

  • trunk/Source/WebKit/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp

    r229764 r229828  
    826826    }
    827827
     828    m_isDecidingNavigationPolicyDecision = true;
     829    if (m_frame->isMainFrame())
     830        webPage->didStartNavigationPolicyCheck();
     831
    828832    // Always ignore requests with empty URLs.
    829833    if (request.isEmpty()) {
    … …  
    897901}
    898902
     903void WebFrameLoaderClient::didDecidePolicyForNavigationAction()
     904{
     905    if (!m_isDecidingNavigationPolicyDecision)
     906        return;
     907
     908    m_isDecidingNavigationPolicyDecision = false;
     909
     910    if (!m_frame || !m_frame->isMainFrame())
     911        return;
     912
     913    if (auto* webPage = m_frame->page())
     914        webPage->didCompleteNavigationPolicyCheck();
     915}
     916
    899917void WebFrameLoaderClient::cancelPolicyCheck()
    900918{
     919    if (m_isDecidingNavigationPolicyDecision)
     920        didDecidePolicyForNavigationAction();
     921
    901922    m_frame->invalidatePolicyListener();
    902923}
    … …  
    13111332    if (!webPage)
    13121333        return;
     1334
     1335    ASSERT(!m_isDecidingNavigationPolicyDecision);
    13131336
    13141337    if (m_frame->isMainFrame()) {

  • trunk/Source/WebKit/WebProcess/WebCoreSupport/WebFrameLoaderClient.h

    r228942 r229828  
    127127    void dispatchDecidePolicyForNewWindowAction(const WebCore::NavigationAction&, const WebCore::ResourceRequest&, WebCore::FormState*, const String& frameName, WebCore::FramePolicyFunction&&) final;
    128128    void dispatchDecidePolicyForNavigationAction(const WebCore::NavigationAction&, const WebCore::ResourceRequest&, bool didReceiveRedirectResponse, WebCore::FormState*, WebCore::FramePolicyFunction&&) final;
     129    void didDecidePolicyForNavigationAction() final;
    129130    void cancelPolicyCheck() final;
    130131   
    … …  
    276277    RefPtr<PluginView> m_pluginView;
    277278    bool m_hasSentResponseToPluginView;
     279    bool m_isDecidingNavigationPolicyDecision { false };
    278280    bool m_didCompletePageTransition;
    279281    bool m_frameHasCustomContentProvider;

  • trunk/Source/WebKit/WebProcess/WebPage/WebPage.cpp

    r229778 r229828  
    28422842}
    28432843
     2844void WebPage::didStartNavigationPolicyCheck()
     2845{
     2846    m_drawingArea->setLayerTreeStateIsFrozen(true);
     2847}
     2848
     2849void WebPage::didCompleteNavigationPolicyCheck()
     2850{
     2851    m_drawingArea->setLayerTreeStateIsFrozen(false);
     2852}
     2853
    28442854void WebPage::didStartPageTransition()
    28452855{

  • trunk/Source/WebKit/WebProcess/WebPage/WebPage.h

    r229511 r229828  
    305305    bool handleEditingKeyboardEvent(WebCore::KeyboardEvent*);
    306306
     307    void didStartNavigationPolicyCheck();
     308    void didCompleteNavigationPolicyCheck();
    307309    void didStartPageTransition();
    308310    void didCompletePageTransition();

  • trunk/Tools/ChangeLog

    r229811 r229828  
     12018-03-21  Chris Dumez  <cdumez@apple.com>
     2
     3        ScrollViewInsetTests.RestoreInitialContentOffsetAfterCrash API test is failing with async delegates
     4        https://bugs.webkit.org/show_bug.cgi?id=183787
     5
     6        Reviewed by Wenson Hsieh.
     7
     8        Add API test coverage.
     9
     10        * TestWebKitAPI/Tests/ios/ScrollViewInsetTests.mm:
     11        (-[AsyncPolicyDelegateForInsetTest webView:didFinishNavigation:]):
     12        (-[AsyncPolicyDelegateForInsetTest webView:decidePolicyForNavigationAction:decisionHandler:]):
     13        (-[AsyncPolicyDelegateForInsetTest webView:decidePolicyForNavigationResponse:decisionHandler:]):
     14        (-[AsyncPolicyDelegateForInsetTest webViewWebContentProcessDidTerminate:]):
     15        (TestWebKitAPI::TEST):
     16
    1172018-03-21  Chris Dumez  <cdumez@apple.com>
    218

  • trunk/Tools/TestWebKitAPI/Tests/ios/ScrollViewInsetTests.mm

    r229641 r229828  
    3333#import "UIKitSPI.h"
    3434#import <WebKit/WKWebViewPrivate.h>
     35
     36@interface AsyncPolicyDelegateForInsetTest : NSObject<WKNavigationDelegate> {
     37    @public BOOL _navigationComplete;
     38}
     39@property (nonatomic, copy) void (^webContentProcessDidTerminate)(WKWebView *);
     40@end
     41
     42@implementation AsyncPolicyDelegateForInsetTest
     43
     44- (void)webView:(WKWebView *)webView didFinishNavigation:(null_unspecified WKNavigation *)navigation
     45{
     46    _navigationComplete = YES;
     47}
     48
     49- (void)webView:(WKWebView *)webView decidePolicyForNavigationAction:(WKNavigationAction *)navigationAction decisionHandler:(void (^)(WKNavigationActionPolicy))decisionHandler
     50{
     51    int64_t deferredWaitTime = 100 * NSEC_PER_MSEC;
     52    dispatch_time_t when = dispatch_time(DISPATCH_TIME_NOW, deferredWaitTime);
     53    dispatch_after(when, dispatch_get_main_queue(), ^{
     54        decisionHandler(WKNavigationActionPolicyAllow);
     55    });
     56}
     57
     58- (void)webView:(WKWebView *)webView decidePolicyForNavigationResponse:(WKNavigationResponse *)navigationResponse decisionHandler:(void (^)(WKNavigationResponsePolicy))decisionHandler
     59{
     60    int64_t deferredWaitTime = 100 * NSEC_PER_MSEC;
     61    dispatch_time_t when = dispatch_time(DISPATCH_TIME_NOW, deferredWaitTime);
     62    dispatch_after(when, dispatch_get_main_queue(), ^{
     63        decisionHandler(WKNavigationResponsePolicyAllow);
     64    });
     65}
     66
     67- (void)webViewWebContentProcessDidTerminate:(WKWebView *)webView
     68{
     69    if (_webContentProcessDidTerminate)
     70        _webContentProcessDidTerminate(webView);
     71}
     72@end
    3573
    3674namespace TestWebKitAPI {
    … …  
    126164}
    127165
     166TEST(ScrollViewInsetTests, RestoreInitialContentOffsetAfterCrashWithAsyncPolicyDelegates)
     167{
     168    auto delegate = adoptNS([[AsyncPolicyDelegateForInsetTest alloc] init]);
     169    auto webView = adoptNS([[TestWKWebView alloc] initWithFrame:CGRectMake(0, 0, 320, viewHeight)]);
     170    [webView scrollView].contentInset = UIEdgeInsetsMake(400, 0, 0, 0);
     171    [webView setNavigationDelegate:delegate.get()];
     172    delegate->_navigationComplete = NO;
     173    NSURL *testResourceURL = [[[NSBundle mainBundle] bundleURL] URLByAppendingPathComponent:@"TestWebKitAPI.resources"];
     174    [webView loadHTMLString:veryTallDocumentMarkup baseURL:testResourceURL];
     175    Util::run(&delegate->_navigationComplete);
     176
     177    CGPoint initialContentOffset = [webView scrollView].contentOffset;
     178    __block CGPoint contentOffsetAfterCrash = CGPointZero;
     179    __block bool done = false;
     180    [delegate setWebContentProcessDidTerminate:^(WKWebView *webView) {
     181        contentOffsetAfterCrash = webView.scrollView.contentOffset;
     182        done = true;
     183    }];
     184
     185    [webView _killWebContentProcessAndResetState];
     186    Util::run(&done);
     187
     188    EXPECT_EQ(initialContentOffset.x, contentOffsetAfterCrash.x);
     189    EXPECT_EQ(initialContentOffset.y, contentOffsetAfterCrash.y);
     190    EXPECT_EQ(0, initialContentOffset.x);
     191    EXPECT_EQ(-400, initialContentOffset.y);
     192}
     193
    128194} // namespace TestWebKitAPI
    129195
Note: See TracChangeset for help on using the changeset viewer.