Changeset 229722 in webkit


Ignore:
Timestamp:
Mar 19, 2018 3:30:57 PM (6 years ago)
Author:
Chris Dumez
Message:

WebKit.WebsitePoliciesAutoplayQuirks API test times out with async policy delegates
​https://bugs.webkit.org/show_bug.cgi?id=183702
<rdar://problem/38566060>

Reviewed by Alex Christensen.

Source/WebCore:

The issue is that the test calls loadHTMLString then loadRequest right after, without
waiting for the first load to complete first. loadHTMLString is special as it relies
on substitute data and which schedules a timer to commit the data. When doing the
navigation policy check for the following loadRequest(), the substitute data timer
would fire and commit its data and load. This would in turn cancel the pending
navigation policy check for the loadRequest().

With sync policy delegates, this is not an issue because we take care of stopping
all loaders when receiving the policy decision, which happens synchronously. However,
when the policy decision happens asynchronously, the pending substitute data load
does not get cancelled in time and it gets committed.

To address the issue, we now cancel any pending provisional load before doing the
navigation policy check.

Test: fast/loader/inner-iframe-loads-data-url-into-parent-on-unload-crash-async-delegate.html

  • loader/FrameLoader.cpp:

(WebCore::FrameLoader::clearProvisionalLoadForPolicyCheck):

  • loader/FrameLoader.h:
  • loader/PolicyChecker.cpp:

(WebCore::PolicyChecker::checkNavigationPolicy):
Cancel any pending provisional load before starting the navigation policy check. This call
needs to be here rather than in the call site of policyChecker().checkNavigationPolicy()
because there is code in PolicyChecker::checkNavigationPolicy() which relies on
FrameLoader::activeDocumentLoader().
Also, we only cancel the provisional load if there is a policy document loader. In some
rare cases (when we receive a redirect after navigation policy has been decided for the
initial request), the provisional document loader needs to receive navigation policy
decisions so we cannot clear the provisional document loader in such case.

Tools:

Add API test coverage.

  • TestWebKitAPI/Tests/WebKitCocoa/WebsitePolicies.mm:

(-[AsyncAutoplayPoliciesDelegate webView:decidePolicyForNavigationAction:decisionHandler:]):
(-[AsyncAutoplayPoliciesDelegate _webView:decidePolicyForNavigationAction:decisionHandler:]):
(-[AsyncAutoplayPoliciesDelegate _webView:handleAutoplayEvent:withFlags:]):
(TEST):

LayoutTests:

Add variant of fast/loader/inner-iframe-loads-data-url-into-parent-on-unload-crash.html with async navigation
delegate since the previous iteration of this patch broke this test case.

  • fast/loader/inner-iframe-loads-data-url-into-parent-on-unload-crash-async-delegate-expected.txt: Added.
  • fast/loader/inner-iframe-loads-data-url-into-parent-on-unload-crash-async-delegate.html: Added.
Location:
trunk
Files:
2 added
7 edited

Legend:

Unmodified
Added
Removed

  • trunk/LayoutTests/ChangeLog

    r229699 r229722  
     12018-03-19  Chris Dumez  <cdumez@apple.com>
     2
     3        WebKit.WebsitePoliciesAutoplayQuirks API test times out with async policy delegates
     4        https://bugs.webkit.org/show_bug.cgi?id=183702
     5        <rdar://problem/38566060>
     6
     7        Reviewed by Alex Christensen.
     8
     9        Add variant of fast/loader/inner-iframe-loads-data-url-into-parent-on-unload-crash.html with async navigation
     10        delegate since the previous iteration of this patch broke this test case.
     11
     12        * fast/loader/inner-iframe-loads-data-url-into-parent-on-unload-crash-async-delegate-expected.txt: Added.
     13        * fast/loader/inner-iframe-loads-data-url-into-parent-on-unload-crash-async-delegate.html: Added.
     14
    1152018-03-17  Jiewen Tan  <jiewen_tan@apple.com>
    216

  • trunk/Source/WebCore/ChangeLog

    r229714 r229722  
     12018-03-19  Chris Dumez  <cdumez@apple.com>
     2
     3        WebKit.WebsitePoliciesAutoplayQuirks API test times out with async policy delegates
     4        https://bugs.webkit.org/show_bug.cgi?id=183702
     5        <rdar://problem/38566060>
     6
     7        Reviewed by Alex Christensen.
     8
     9        The issue is that the test calls loadHTMLString then loadRequest right after, without
     10        waiting for the first load to complete first. loadHTMLString is special as it relies
     11        on substitute data and which schedules a timer to commit the data. When doing the
     12        navigation policy check for the following loadRequest(), the substitute data timer
     13        would fire and commit its data and load. This would in turn cancel the pending
     14        navigation policy check for the loadRequest().
     15
     16        With sync policy delegates, this is not an issue because we take care of stopping
     17        all loaders when receiving the policy decision, which happens synchronously. However,
     18        when the policy decision happens asynchronously, the pending substitute data load
     19        does not get cancelled in time and it gets committed.
     20
     21        To address the issue, we now cancel any pending provisional load before doing the
     22        navigation policy check.
     23
     24        Test: fast/loader/inner-iframe-loads-data-url-into-parent-on-unload-crash-async-delegate.html
     25
     26        * loader/FrameLoader.cpp:
     27        (WebCore::FrameLoader::clearProvisionalLoadForPolicyCheck):
     28        * loader/FrameLoader.h:
     29        * loader/PolicyChecker.cpp:
     30        (WebCore::PolicyChecker::checkNavigationPolicy):
     31        Cancel any pending provisional load before starting the navigation policy check. This call
     32        needs to be here rather than in the call site of policyChecker().checkNavigationPolicy()
     33        because there is code in PolicyChecker::checkNavigationPolicy() which relies on
     34        FrameLoader::activeDocumentLoader().
     35        Also, we only cancel the provisional load if there is a policy document loader. In some
     36        rare cases (when we receive a redirect after navigation policy has been decided for the
     37        initial request), the provisional document loader needs to receive navigation policy
     38        decisions so we cannot clear the provisional document loader in such case.
     39
    1402018-03-19  Eric Carlson  <eric.carlson@apple.com>
    241

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

    r229704 r229722  
    15451545}
    15461546
     1547void FrameLoader::clearProvisionalLoadForPolicyCheck()
     1548{
     1549    if (!m_policyDocumentLoader || !m_provisionalDocumentLoader)
     1550        return;
     1551
     1552    m_provisionalDocumentLoader->stopLoading();
     1553    setProvisionalDocumentLoader(nullptr);
     1554}
     1555
    15471556void FrameLoader::reportLocalLoadFailed(Frame* frame, const String& url)
    15481557{

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

    r229341 r229722  
    142142    bool closeURL();
    143143    void cancelAndClear();
     144    void clearProvisionalLoadForPolicyCheck();
    144145    // FIXME: clear() is trying to do too many things. We should break it down into smaller functions (ideally with fewer raw Boolean parameters).
    145146    void clear(Document* newDocument, bool clearWindowProperties = true, bool clearScriptObjects = true, bool clearFrameView = true);

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

    r229133 r229722  
    145145#endif
    146146
     147    m_frame.loader().clearProvisionalLoadForPolicyCheck();
     148
    147149    m_delegateIsDecidingNavigationPolicy = true;
    148150    String suggestedFilename = action.downloadAttribute().isEmpty() ? nullAtom() : action.downloadAttribute();

  • trunk/Tools/ChangeLog

    r229712 r229722  
     12018-03-19  Chris Dumez  <cdumez@apple.com>
     2
     3        WebKit.WebsitePoliciesAutoplayQuirks API test times out with async policy delegates
     4        https://bugs.webkit.org/show_bug.cgi?id=183702
     5        <rdar://problem/38566060>
     6
     7        Reviewed by Alex Christensen.
     8
     9        Add API test coverage.
     10
     11        * TestWebKitAPI/Tests/WebKitCocoa/WebsitePolicies.mm:
     12        (-[AsyncAutoplayPoliciesDelegate webView:decidePolicyForNavigationAction:decisionHandler:]):
     13        (-[AsyncAutoplayPoliciesDelegate _webView:decidePolicyForNavigationAction:decisionHandler:]):
     14        (-[AsyncAutoplayPoliciesDelegate _webView:handleAutoplayEvent:withFlags:]):
     15        (TEST):
     16
    1172018-03-19  Daniel Bates  <dabates@apple.com>
    218

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

    r229704 r229722  
    189189        websitePolicies.autoplayPolicy = _autoplayPolicyForURL(navigationAction.request.URL);
    190190    decisionHandler(WKNavigationActionPolicyAllow, websitePolicies);
     191}
     192
     193#if PLATFORM(MAC)
     194- (void)_webView:(WKWebView *)webView handleAutoplayEvent:(_WKAutoplayEvent)event withFlags:(_WKAutoplayEventFlags)flags
     195{
     196    receivedAutoplayEventFlags = flags;
     197    receivedAutoplayEvent = event;
     198}
     199#endif
     200
     201@end
     202
     203@interface AsyncAutoplayPoliciesDelegate : NSObject <WKNavigationDelegate, WKUIDelegatePrivate>
     204@property (nonatomic, copy) _WKWebsiteAutoplayPolicy(^autoplayPolicyForURL)(NSURL *);
     205@property (nonatomic, copy) _WKWebsiteAutoplayQuirk(^allowedAutoplayQuirksForURL)(NSURL *);
     206@end
     207
     208@implementation AsyncAutoplayPoliciesDelegate
     209
     210- (void)webView:(WKWebView *)webView decidePolicyForNavigationAction:(WKNavigationAction *)navigationAction decisionHandler:(void (^)(WKNavigationActionPolicy))decisionHandler
     211{
     212    // _webView:decidePolicyForNavigationAction:decisionHandler: should be called instead if it is implemented.
     213    EXPECT_TRUE(false);
     214    decisionHandler(WKNavigationActionPolicyAllow);
     215}
     216
     217- (void)_webView:(WKWebView *)webView decidePolicyForNavigationAction:(WKNavigationAction *)navigationAction decisionHandler:(void (^)(WKNavigationActionPolicy, _WKWebsitePolicies *))decisionHandler
     218{
     219    dispatch_async(dispatch_get_main_queue(), ^{
     220        _WKWebsitePolicies *websitePolicies = [[[_WKWebsitePolicies alloc] init] autorelease];
     221        if (_allowedAutoplayQuirksForURL)
     222            websitePolicies.allowedAutoplayQuirks = _allowedAutoplayQuirksForURL(navigationAction.request.URL);
     223        if (_autoplayPolicyForURL)
     224            websitePolicies.autoplayPolicy = _autoplayPolicyForURL(navigationAction.request.URL);
     225        decisionHandler(WKNavigationActionPolicyAllow, websitePolicies);
     226    });
    191227}
    192228
    … …  
    658694    [webView stringByEvaluatingJavaScript:@"play()"];
    659695    [webView waitForMessage:@"playing"];
     696}
     697
     698TEST(WebKit, WebsitePoliciesAutoplayQuirksAsyncPolicyDelegate)
     699{
     700    auto configuration = adoptNS([[WKWebViewConfiguration alloc] init]);
     701    auto webView = adoptNS([[TestWKWebView alloc] initWithFrame:NSMakeRect(0, 0, 800, 600) configuration:configuration.get()]);
     702
     703    auto delegate = adoptNS([[AsyncAutoplayPoliciesDelegate alloc] init]);
     704    [webView setNavigationDelegate:delegate.get()];
     705
     706    WKRetainPtr<WKPreferencesRef> preferences(AdoptWK, WKPreferencesCreate());
     707    WKPreferencesSetNeedsSiteSpecificQuirks(preferences.get(), true);
     708    WKPageGroupSetPreferences(WKPageGetPageGroup([webView _pageForTesting]), preferences.get());
     709
     710    NSURLRequest *requestWithAudio = [NSURLRequest requestWithURL:[[NSBundle mainBundle] URLForResource:@"autoplay-check" withExtension:@"html" subdirectory:@"TestWebKitAPI.resources"]];
     711
     712    [delegate setAllowedAutoplayQuirksForURL:^_WKWebsiteAutoplayQuirk(NSURL *url) {
     713        return _WKWebsiteAutoplayQuirkSynthesizedPauseEvents;
     714    }];
     715    [delegate setAutoplayPolicyForURL:^(NSURL *) {
     716        return _WKWebsiteAutoplayPolicyDeny;
     717    }];
     718    [webView loadRequest:requestWithAudio];
     719    [webView waitForMessage:@"did-not-play"];
     720    [webView waitForMessage:@"on-pause"];
     721
     722    receivedAutoplayEvent = std::nullopt;
     723    [webView loadHTMLString:@"" baseURL:nil];
     724
     725    NSURLRequest *requestWithAudioInFrame = [NSURLRequest requestWithURL:[[NSBundle mainBundle] URLForResource:@"autoplay-check-in-iframe" withExtension:@"html" subdirectory:@"TestWebKitAPI.resources"]];
     726
     727    [delegate setAllowedAutoplayQuirksForURL:^_WKWebsiteAutoplayQuirk(NSURL *url) {
     728        if ([url.lastPathComponent isEqualToString:@"autoplay-check-frame.html"])
     729            return _WKWebsiteAutoplayQuirkInheritedUserGestures;
     730
     731        return _WKWebsiteAutoplayQuirkSynthesizedPauseEvents | _WKWebsiteAutoplayQuirkInheritedUserGestures;
     732    }];
     733    [delegate setAutoplayPolicyForURL:^(NSURL *) {
     734        return _WKWebsiteAutoplayPolicyDeny;
     735    }];
     736    [webView loadRequest:requestWithAudioInFrame];
     737    [webView waitForMessage:@"did-not-play"];
     738    [webView waitForMessage:@"on-pause"];
    660739}
    661740#endif // PLATFORM(MAC)
Note: See TracChangeset for help on using the changeset viewer.