Bug 202338 - Switch to WKShareSheet for WKPDFView
Summary: Switch to WKShareSheet for WKPDFView
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Megan Gardner
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-09-27 17:15 PDT by Megan Gardner
Modified: 2019-10-07 15:53 PDT (History)
4 users (show)

See Also:


Attachments
Patch (3.99 KB, patch)
2019-09-27 17:18 PDT, Megan Gardner
no flags Details | Formatted Diff | Diff
Patch (5.02 KB, patch)
2019-09-27 17:31 PDT, Megan Gardner
no flags Details | Formatted Diff | Diff
Patch (4.99 KB, patch)
2019-10-07 12:02 PDT, Megan Gardner
no flags Details | Formatted Diff | Diff
Patch for landing (5.02 KB, patch)
2019-10-07 15:12 PDT, Megan Gardner
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Megan Gardner 2019-09-27 17:15:54 PDT
Switch to WKShareSheet for WKPDFView
Comment 1 Megan Gardner 2019-09-27 17:18:38 PDT
Created attachment 379775 [details]
Patch
Comment 2 Megan Gardner 2019-09-27 17:31:07 PDT
Created attachment 379778 [details]
Patch
Comment 3 Daniel Bates 2019-09-29 10:04:52 PDT
Comment on attachment 379778 [details]
Patch

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

This is a good first stab. It looks like this patch has a use-after-free because:

1. The allocated WKShareSheet is assigned to a local RetainPtr.

2. _shareSheet is never assigned a non-nil value

> Source/WebKit/UIProcess/ios/WKPDFView.mm:560
> +    if (_shareSheet)

Ok as-is. The optimal solution would be to unconditionally call -dismiss because:

1. Sending a message to a possible nil object is okay in Obj-C.
2. Because of (1) a branch can be removed.
Comment 4 Megan Gardner 2019-10-07 12:02:20 PDT
Created attachment 380342 [details]
Patch
Comment 5 Tim Horton 2019-10-07 14:18:56 PDT
Comment on attachment 380342 [details]
Patch

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

> Source/WebKit/ChangeLog:10
> +        us to eliminate this mostly unused class.

Now entirely unused?

> Source/WebKit/UIProcess/ios/WKPDFView.h:32
> +@interface WKPDFView : WKApplicationStateTrackingView <WKWebViewContentProvider, WKShareSheetDelegate>

Keep these sorted :P

> Source/WebKit/UIProcess/ios/WKPDFView.mm:564
> +    [_shareSheet presentWithParameters:shareData inRect: { [[_hostViewController view] convertRect:boundingRect toView:webView] } completionHandler:[] (bool success) { }];

Is there not a way to pass a null completion handler? Maybe not.
Comment 6 Megan Gardner 2019-10-07 15:11:21 PDT
Comment on attachment 380342 [details]
Patch

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

>> Source/WebKit/UIProcess/ios/WKPDFView.mm:564
>> +    [_shareSheet presentWithParameters:shareData inRect: { [[_hostViewController view] convertRect:boundingRect toView:webView] } completionHandler:[] (bool success) { }];
> 
> Is there not a way to pass a null completion handler? Maybe not.

I took this from the calls in WKContentViewInteraction.
Comment 7 Megan Gardner 2019-10-07 15:12:10 PDT
Created attachment 380366 [details]
Patch for landing
Comment 8 Radar WebKit Bug Importer 2019-10-07 15:13:00 PDT
<rdar://problem/56052711>
Comment 9 WebKit Commit Bot 2019-10-07 15:53:33 PDT
Comment on attachment 380366 [details]
Patch for landing

Clearing flags on attachment: 380366

Committed r250801: <https://trac.webkit.org/changeset/250801>
Comment 10 WebKit Commit Bot 2019-10-07 15:53:34 PDT
All reviewed patches have been landed.  Closing bug.