Changeset 238381 in webkit


Ignore:
Timestamp:
Nov 19, 2018 2:52:42 PM (5 years ago)
Author:
david_quesada@apple.com
Message:

EXC_BAD_ACCESS when invoking a DownloadProxy's destination decision handler after the download has been canceled
​https://bugs.webkit.org/show_bug.cgi?id=191762
rdar://problem/46151509

Reviewed by Dean Jackson.

Source/WebKit:

When the DownloadClient calls the decision handler with a destination path, check if
m_processPool is null before trying to access its network process. This can happen
if a download is canceled before the client decides its destination.

  • UIProcess/Downloads/DownloadProxy.cpp:

(WebKit::DownloadProxy::decideDestinationWithSuggestedFilenameAsync):

Tools:

  • TestWebKitAPI/Tests/WebKitCocoa/Download.mm:

Enable the Download API test on iOS, since the platform supports downloads. All the
tests pass already, except for one which was written using AppKit-specific code:

TEST(_WKDownload, RedirectedDownload):

Use a more platform-agnostic approach to starting the download in this API test.
Instead of manually triggering an NSMenu item to download a file from a link, the
test will simulate a user-initiated click on the link, and the navigation delegate
will direct the web view to start a download based on the link's navigation action.
Additionally, remove the manual creation of a new NSWindow. TestWKWebView makes its
own UI/NSWindow.

TEST(_WKDownload, DownloadCanceledWhileDecidingDestination):

Add an API test _WKDownload.DownloadCanceledWhileDecidingDestination to simulate the
conditions that would trigger this crash - handling a download's -decideDestination…
delegate call by canceling the download, waiting for the UI process to be notified
that the download has been canceled, and calling the decision handler. This should
not crash.

(-[CancelDownloadWhileDecidingDestinationDelegate _downloadDidFinish:]):
(-[CancelDownloadWhileDecidingDestinationDelegate _download:didFailWithError:]):
(-[CancelDownloadWhileDecidingDestinationDelegate _downloadDidCancel:]):
(-[CancelDownloadWhileDecidingDestinationDelegate _download:decideDestinationWithSuggestedFilename:completionHandler:]):
(-[UIDownloadAsFileTestDelegate _webView:contextMenu:forElement:]): Deleted.

  • TestWebKitAPI/cocoa/TestWKWebView.h:
  • TestWebKitAPI/cocoa/TestWKWebView.mm:

(-[TestWKWebView objectByEvaluatingJavaScriptWithUserGesture:]):

Add a user-initated version of -objectByEvaluatingJavaScript:. This is needed in
order to maintain the behavior of the RedirectedDownload test, which verifies the
state of the _WKDownload.wasUserInitiated property.

Location:
trunk
Files:
6 edited

Legend:

Unmodified
Added
Removed

  • trunk/Source/WebKit/ChangeLog

    r238379 r238381  
     12018-11-19  David Quesada  <david_quesada@apple.com>
     2
     3        EXC_BAD_ACCESS when invoking a DownloadProxy's destination decision handler after the download has been canceled
     4        https://bugs.webkit.org/show_bug.cgi?id=191762
     5        rdar://problem/46151509
     6
     7        Reviewed by Dean Jackson.
     8
     9        When the DownloadClient calls the decision handler with a destination path, check if
     10        m_processPool is null before trying to access its network process. This can happen
     11        if a download is canceled before the client decides its destination.
     12
     13        * UIProcess/Downloads/DownloadProxy.cpp:
     14        (WebKit::DownloadProxy::decideDestinationWithSuggestedFilenameAsync):
     15
    1162018-11-19  Tomoki Imai  <Tomoki.Imai@sony.com>
    217

  • trunk/Source/WebKit/UIProcess/Downloads/DownloadProxy.cpp

    r236153 r238381  
    171171            SandboxExtension::createHandle(destination, SandboxExtension::Type::ReadWrite, sandboxExtensionHandle);
    172172
     173        if (!m_processPool)
     174            return;
     175
    173176        if (auto* networkProcess = m_processPool->networkProcess())
    174177            networkProcess->send(Messages::NetworkProcess::ContinueDecidePendingDownloadDestination(downloadID, destination, sandboxExtensionHandle, allowOverwrite == AllowOverwrite::Yes), 0);

  • trunk/Tools/ChangeLog

    r238380 r238381  
     12018-11-19  David Quesada  <david_quesada@apple.com>
     2
     3        EXC_BAD_ACCESS when invoking a DownloadProxy's destination decision handler after the download has been canceled
     4        https://bugs.webkit.org/show_bug.cgi?id=191762
     5        rdar://problem/46151509
     6
     7        Reviewed by Dean Jackson.
     8
     9        * TestWebKitAPI/Tests/WebKitCocoa/Download.mm:
     10            Enable the Download API test on iOS, since the platform supports downloads. All the
     11            tests pass already, except for one which was written using AppKit-specific code:
     12        TEST(_WKDownload, RedirectedDownload):
     13            Use a more platform-agnostic approach to starting the download in this API test.
     14            Instead of manually triggering an NSMenu item to download a file from a link, the
     15            test will simulate a user-initiated click on the link, and the navigation delegate
     16            will direct the web view to start a download based on the link's navigation action.
     17            Additionally, remove the manual creation of a new NSWindow. TestWKWebView makes its
     18            own UI/NSWindow.
     19        TEST(_WKDownload, DownloadCanceledWhileDecidingDestination):
     20            Add an API test _WKDownload.DownloadCanceledWhileDecidingDestination to simulate the
     21            conditions that would trigger this crash - handling a download's -decideDestination…
     22            delegate call by canceling the download, waiting for the UI process to be notified
     23            that the download has been canceled, and calling the decision handler. This should
     24            not crash.
     25        (-[CancelDownloadWhileDecidingDestinationDelegate _downloadDidFinish:]):
     26        (-[CancelDownloadWhileDecidingDestinationDelegate _download:didFailWithError:]):
     27        (-[CancelDownloadWhileDecidingDestinationDelegate _downloadDidCancel:]):
     28        (-[CancelDownloadWhileDecidingDestinationDelegate _download:decideDestinationWithSuggestedFilename:completionHandler:]):
     29        (-[UIDownloadAsFileTestDelegate _webView:contextMenu:forElement:]): Deleted.
     30        * TestWebKitAPI/cocoa/TestWKWebView.h:
     31        * TestWebKitAPI/cocoa/TestWKWebView.mm:
     32        (-[TestWKWebView objectByEvaluatingJavaScriptWithUserGesture:]):
     33            Add a user-initated version of -objectByEvaluatingJavaScript:. This is needed in
     34            order to maintain the behavior of the RedirectedDownload test, which verifies the
     35            state of the _WKDownload.wasUserInitiated property.
     36
    1372018-11-19  Wenson Hsieh  <wenson_hsieh@apple.com>
    238

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

    r236913 r238381  
    2828
    2929#if WK_API_ENABLED
    30 #if PLATFORM(MAC) // No downloading on iOS
     30#if PLATFORM(MAC) || PLATFORM(IOS)
    3131
    3232#import "PlatformUtilities.h"
    … …  
    4343#import <WebKit/WKWebViewConfiguration.h>
    4444#import <wtf/RetainPtr.h>
    45 #import <wtf/mac/AppKitCompatibilityDeclarations.h>
    4645#import <wtf/text/WTFString.h>
    4746
    … …  
    440439}
    441440
    442 @interface UIDownloadAsFileTestDelegate : NSObject <WKUIDelegatePrivate>
    443 @end
    444 
    445 @implementation UIDownloadAsFileTestDelegate
    446 
    447 IGNORE_WARNINGS_BEGIN("deprecated-implementations")
    448 - (NSMenu *)_webView:(WKWebView *)webView contextMenu:(NSMenu *)menu forElement:(_WKContextMenuElementInfo *)element
    449 IGNORE_WARNINGS_END
    450 {
    451     static const long downloadLinkedFileTag = 2;
    452     auto index = [menu indexOfItemWithTag:downloadLinkedFileTag];
    453     [menu performActionForItemAtIndex:index];
    454     return nil;
    455 }
    456 
    457 @end
    458 
    459441@interface RedirectedDownloadDelegate : NSObject <_WKDownloadDelegate>
    460442@end
    … …  
    518500    isDone = false;
    519501
    520     auto delegate = adoptNS([[UIDownloadAsFileTestDelegate alloc] init]);
    521502    auto configuration = adoptNS([[WKWebViewConfiguration alloc] init]);
    522503    auto webView = adoptNS([[TestWKWebView alloc] initWithFrame:CGRectMake(0, 0, 800, 600) configuration:configuration.get()]);
    523     [webView setUIDelegate:delegate.get()];
    524504    auto downloadDelegate = adoptNS([[RedirectedDownloadDelegate alloc] init]);
    525505    [[[webView configuration] processPool] _setDownloadDelegate:downloadDelegate.get()];
    526506
    527     auto window = adoptNS([[NSWindow alloc] initWithContentRect:[webView frame] styleMask:NSWindowStyleMaskBorderless backing:NSBackingStoreBuffered defer:YES]);
    528     [[window contentView] addSubview:webView.get()];
    529 
    530507    // Do 2 loads in the same view to make sure the redirect chain is properly cleared between loads.
    531508    [webView synchronouslyLoadHTMLString:@"<div>First load</div>"];
    532     [webView synchronouslyLoadHTMLString:@"<a style='display: block; height: 100%; width: 100%' href='http://redirect/?redirect/?pass'>test</a>"];
     509    [webView synchronouslyLoadHTMLString:@"<a id='link' href='http://redirect/?redirect/?pass'>test</a>"];
    533510
    534511    expectedOriginatingWebView = webView.get();
    535512    expectedUserInitiatedState = true;
    536     NSPoint clickPoint = NSMakePoint(100, 100);
    537     [[webView hitTest:clickPoint] mouseDown:[NSEvent mouseEventWithType:NSEventTypeRightMouseDown location:clickPoint modifierFlags:0 timestamp:0 windowNumber:[window windowNumber] context:nil eventNumber:0 clickCount:1 pressure:1]];
    538     [[webView hitTest:clickPoint] mouseUp:[NSEvent mouseEventWithType:NSEventTypeRightMouseUp location:clickPoint modifierFlags:0 timestamp:0 windowNumber:[window windowNumber] context:nil eventNumber:0 clickCount:1 pressure:1]];
     513
     514    auto navigationDelegate = adoptNS([[DownloadNavigationDelegate alloc] init]);
     515    [webView setNavigationDelegate:navigationDelegate.get()];
     516    [webView objectByEvaluatingJavaScriptWithUserGesture:@"document.getElementById('link').click();"];
    539517
    540518    isDone = false;
    … …  
    591569}
    592570
     571static bool downloadHasDecidedDestination;
     572
     573@interface CancelDownloadWhileDecidingDestinationDelegate : NSObject <_WKDownloadDelegate>
     574@end
     575
     576@implementation CancelDownloadWhileDecidingDestinationDelegate
     577- (void)_downloadDidFinish:(_WKDownload *)download
     578{
     579    EXPECT_TRUE(false);
     580    isDone = true;
     581}
     582
     583- (void)_download:(_WKDownload *)download didFailWithError:(NSError *)error
     584{
     585    EXPECT_TRUE(false);
     586    isDone = true;
     587}
     588
     589- (void)_downloadDidCancel:(_WKDownload *)download
     590{
     591    isDone = true;
     592}
     593
     594- (void)_download:(_WKDownload *)download decideDestinationWithSuggestedFilename:(NSString *)filename completionHandler:(void (^)(BOOL allowOverwrite, NSString *destination))completionHandler
     595{
     596    [download cancel];
     597    TestWebKitAPI::Util::run(&isDone);
     598    completionHandler(YES, @"/tmp/WebKitAPITest/_WKDownload");
     599    downloadHasDecidedDestination = true;
     600}
     601@end
     602
     603TEST(_WKDownload, DownloadCanceledWhileDecidingDestination)
     604{
     605    [TestProtocol registerWithScheme:@"http"];
     606
     607    auto navigationDelegate = adoptNS([[DownloadNavigationDelegate alloc] init]);
     608    auto downloadDelegate = adoptNS([[CancelDownloadWhileDecidingDestinationDelegate alloc] init]);
     609
     610    auto webView = adoptNS([[WKWebView alloc] initWithFrame:NSMakeRect(0, 0, 800, 600)]);
     611    [webView setNavigationDelegate:navigationDelegate.get()];
     612    [[[webView configuration] processPool] _setDownloadDelegate:downloadDelegate.get()];
     613
     614    isDone = false;
     615    downloadHasDecidedDestination = false;
     616    [webView loadRequest:[NSURLRequest requestWithURL:[NSURL URLWithString:@"http://pass"]]];
     617
     618    TestWebKitAPI::Util::run(&downloadHasDecidedDestination);
     619
     620    [TestProtocol unregister];
     621}
     622
    593623#endif
    594624#endif

  • trunk/Tools/TestWebKitAPI/cocoa/TestWKWebView.h

    r238188 r238381  
    6666- (void)synchronouslyLoadTestPageNamed:(NSString *)pageName;
    6767- (id)objectByEvaluatingJavaScript:(NSString *)script;
     68- (id)objectByEvaluatingJavaScriptWithUserGesture:(NSString *)script;
    6869- (NSString *)stringByEvaluatingJavaScript:(NSString *)script;
    6970- (void)waitForMessage:(NSString *)message;

  • trunk/Tools/TestWebKitAPI/cocoa/TestWKWebView.mm

    r238380 r238381  
    305305}
    306306
     307- (id)objectByEvaluatingJavaScriptWithUserGesture:(NSString *)script
     308{
     309    bool isWaitingForJavaScript = false;
     310    RetainPtr<id> evalResult;
     311    [self evaluateJavaScript:script completionHandler:[&] (id result, NSError *error) {
     312        evalResult = result;
     313        isWaitingForJavaScript = true;
     314        EXPECT_TRUE(!error);
     315        if (error)
     316            NSLog(@"Encountered error: %@ while evaluating script: %@", error, script);
     317    }];
     318    TestWebKitAPI::Util::run(&isWaitingForJavaScript);
     319    return evalResult.autorelease();
     320}
     321
    307322- (NSString *)stringByEvaluatingJavaScript:(NSString *)script
    308323{
Note: See TracChangeset for help on using the changeset viewer.