Skip to content

Commit

Permalink
[iOS] Crash bug fix for WebStateImpl being null during download process
Browse files Browse the repository at this point in the history
This is a merge to M103. 

There's currently a bug where a user crashes during the download
process due to WebState being null. Reproducing the crash through a
unit/eg test is currently difficult since the cause of the crash seems
to be time dependent hence why there isn't a unit test accompanied with
the fix to test if it works. Existing unit tests have also been
updated to pass the tests since we can't replicate a delegate call since
the delegate is set to nil. This way, unit tests can still pass as if
the delegate existed and wasn't prematurely cancelled.

(cherry picked from commit 13df7d1)

Fixed: 1325108
Change-Id: I72a54fcddde20d95061642539c3553b0e3c219ef
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3648169
Commit-Queue: Joemer Ramos <joemerramos@chromium.org>
Reviewed-by: Sylvain Defresne <sdefresne@chromium.org>
Cr-Original-Commit-Position: refs/heads/main@{#1004248}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3661421
Commit-Queue: Sylvain Defresne <sdefresne@chromium.org>
Auto-Submit: Joemer Ramos <joemerramos@chromium.org>
Cr-Commit-Position: refs/branch-heads/5060@{#217}
Cr-Branched-From: b83393d-refs/heads/main@{#1002911}
  • Loading branch information
Joemer Ramos authored and Chromium LUCI CQ committed May 24, 2022
1 parent 7b0821e commit 1865175
Show file tree
Hide file tree
Showing 4 changed files with 72 additions and 17 deletions.
2 changes: 1 addition & 1 deletion ios/web/download/download_native_task_bridge.h
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ using NativeDownloadTaskCompleteCallback =

// Used to set response url, content length, mimetype and http response headers
// in CRWWkNavigationHandler so method can interact with WKWebView.
- (void)onDownloadNativeTaskBridgeReadyForDownload:
- (BOOL)onDownloadNativeTaskBridgeReadyForDownload:
(DownloadNativeTaskBridge*)bridge API_AVAILABLE(ios(15));

// Calls CRWWKNavigationHandlerDelegate to resume download using |webView|
Expand Down
4 changes: 3 additions & 1 deletion ios/web/download/download_native_task_bridge.mm
Original file line number Diff line number Diff line change
Expand Up @@ -153,7 +153,9 @@ - (void)download:(WKDownload*)download
handler(_urlForDownload);
} else {
_startDownloadBlock = handler;
[_delegate onDownloadNativeTaskBridgeReadyForDownload:self];
if (![_delegate onDownloadNativeTaskBridgeReadyForDownload:self]) {
[self cancel];
}
}
}

Expand Down
14 changes: 11 additions & 3 deletions ios/web/navigation/crw_wk_navigation_handler.mm
Original file line number Diff line number Diff line change
Expand Up @@ -1043,8 +1043,17 @@ - (void)webView:(WKWebView*)webView
delegate:self]];
}

- (void)onDownloadNativeTaskBridgeReadyForDownload:
// Used to set response url, content length, mimetype and http response headers
// in CRWWkNavigationHandler so method can interact with WKWebView. Returns NO
// if the download cannot be started.
- (BOOL)onDownloadNativeTaskBridgeReadyForDownload:
(DownloadNativeTaskBridge*)bridge API_AVAILABLE(ios(15)) {
__attribute__((objc_precise_lifetime))
DownloadNativeTaskBridge* nativeTaskBridge = bridge;
[_nativeTaskBridges removeObject:bridge];
if (!self.webStateImpl)
return NO;

const GURL responseURL = net::GURLWithNSURL(bridge.response.URL);
const int64_t contentLength = bridge.response.expectedContentLength;
const std::string MIMEType =
Expand All @@ -1066,8 +1075,7 @@ - (void)onDownloadNativeTaskBridgeReadyForDownload:
->CreateNativeDownloadTask(self.webStateImpl, [NSUUID UUID].UUIDString,
responseURL, HTTPMethod, contentDisposition,
contentLength, MIMEType, bridge);

[_nativeTaskBridges removeObject:bridge];
return YES;
}

- (void)resumeDownloadNativeTask:(NSData*)data
Expand Down
69 changes: 57 additions & 12 deletions ios/web/test/fakes/fake_native_task_bridge.mm
Original file line number Diff line number Diff line change
Expand Up @@ -6,12 +6,17 @@

#import "base/callback.h"
#import "base/strings/sys_string_conversions.h"
#import "net/base/net_errors.h"

#if !defined(__has_feature) || !__has_feature(objc_arc)
#error "This file requires ARC support."
#endif

@implementation FakeNativeTaskBridge
@implementation FakeNativeTaskBridge {
void (^_startDownloadBlock)(NSURL*);
BOOL _observingDownloadProgress;
NativeDownloadTaskCompleteCallback _completeCallback;
}

@synthesize download = _download;
@synthesize progress = _progress;
Expand All @@ -30,41 +35,81 @@ - (instancetype)initWithDownload:(WKDownload*)download
}

- (void)cancel {
[super cancel];
if (_startDownloadBlock) {
_startDownloadBlock(nil);
}
_progress = [NSProgress progressWithTotalUnitCount:0];
}

- (void)startDownload:(const base::FilePath&)path
progressCallback:(NativeDownloadTaskProgressCallback)progressCallback
responseCallback:(NativeDownloadTaskResponseCallback)responseCallback
completeCallback:(NativeDownloadTaskCompleteCallback)completeCallback {
[super startDownload:path
progressCallback:std::move(progressCallback)
responseCallback:std::move(responseCallback)
completeCallback:std::move(completeCallback)];
_completeCallback = std::move(completeCallback);
[self startObservingDownloadProgress];
_startDownloadBlock(self.urlForDownload);
_startDownloadBlock = nil;

// Simulates completing a download progress
_progress = [NSProgress progressWithTotalUnitCount:100];
}

- (void)dealloc {
[self stopObservingDownloadProgress];
}

#pragma mark - Private methods

- (void)downloadInitialized API_AVAILABLE(ios(15)) {
// Instantiates _startDownloadBlock, so when we call
// startDownload:progressionHandler:completionHandler method, the block is
// initialized.
__weak FakeNativeTaskBridge* weakSelf = self;
[super download:_download
decideDestinationUsingResponse:_response
suggestedFilename:_suggestedFilename
completionHandler:^void(NSURL* url) {
[weakSelf destinationDecided:url];
}];
void (^handler)(NSURL* destination) = ^void(NSURL* url) {
[weakSelf destinationDecided:url];
};

if (self.urlForDownload) {
// Resuming a download.
[self startObservingDownloadProgress];
handler(self.urlForDownload);
} else {
_startDownloadBlock = handler;
}
}

- (void)destinationDecided:(NSURL*)url API_AVAILABLE(ios(15)) {
_calledStartDownloadBlock = YES;
[self downloadDidFinish:_download];
}

#pragma mark - Private methods

- (void)startObservingDownloadProgress {
DCHECK(!_observingDownloadProgress);

_observingDownloadProgress = YES;
[self.progress addObserver:self
forKeyPath:@"fractionCompleted"
options:NSKeyValueObservingOptionNew
context:nil];
}

- (void)stopObservingDownloadProgress {
if (_observingDownloadProgress) {
_observingDownloadProgress = NO;
[self.progress removeObserver:self
forKeyPath:@"fractionCompleted"
context:nil];
}
}

- (void)downloadDidFinish:(WKDownload*)download API_AVAILABLE(ios(15)) {
[self stopObservingDownloadProgress];
if (!_completeCallback.is_null()) {
web::DownloadResult download_result(net::OK);
std::move(_completeCallback).Run(download_result);
}
}

@end

0 comments on commit 1865175

Please sign in to comment.