Bug 191114 - API test WKAttachmentTests.CopyAndPasteBetweenWebViews fails on macOS 10.13
Summary: API test WKAttachmentTests.CopyAndPasteBetweenWebViews fails on macOS 10.13
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Wenson Hsieh
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-10-31 08:18 PDT by Wenson Hsieh
Modified: 2018-11-01 06:35 PDT (History)
11 users (show)

See Also:


Attachments
Speculative fix (2.02 KB, patch)
2018-10-31 08:38 PDT, Wenson Hsieh
no flags Details | Formatted Diff | Diff
Speculative fix (2.28 KB, patch)
2018-10-31 09:18 PDT, Wenson Hsieh
no flags Details | Formatted Diff | Diff
Patch (2.28 KB, patch)
2018-10-31 09:21 PDT, Wenson Hsieh
no flags Details | Formatted Diff | Diff
Patch (1.75 KB, patch)
2018-10-31 17:58 PDT, Wenson Hsieh
dino: review+
ews-watchlist: commit-queue-
Details | Formatted Diff | Diff
Patch for landing (1.84 KB, patch)
2018-10-31 18:34 PDT, Wenson Hsieh
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews121 for ios-simulator-wk2 (2.32 MB, application/zip)
2018-10-31 20:02 PDT, EWS Watchlist
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Wenson Hsieh 2018-10-31 08:18:35 PDT
WKAttachmentTests.CopyAndPasteBetweenWebViews, the API test added with <https://trac.webkit.org/changeset/237624/webkit>, fails on macOS 10.13. It works on macOS 10.12 because USE_SECURE_ARCHIVER_API is 0, so we simply archive the NSFileWrapper; and it works on macOS 10.14 because USE_SECURE_ARCHIVER_API is 1, but NSFileWrapper is secure-coding-compliant. On macOS 10.13, we try to use the secure archiver despite NSFileWrapper not supporting it.
Comment 1 Wenson Hsieh 2018-10-31 08:28:41 PDT
I think we can address this by only requiring secure archival on macOS 10.14+ and iOS 12+
Comment 2 Radar WebKit Bug Importer 2018-10-31 08:29:55 PDT
<rdar://problem/45700410>
Comment 3 Wenson Hsieh 2018-10-31 08:38:53 PDT
Created attachment 353495 [details]
Speculative fix
Comment 4 Wenson Hsieh 2018-10-31 08:39:52 PDT
Comment on attachment 353495 [details]
Speculative fix

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

> Source/WebKit/ChangeLog:13
> +        To fix this, we only use the secure archiver on ⥠macOS 10.14 and ⥠iOS 12.

(Bugzilla garbled ≥ into â¥)
Comment 5 Wenson Hsieh 2018-10-31 09:18:47 PDT
Created attachment 353498 [details]
Speculative fix
Comment 6 Wenson Hsieh 2018-10-31 09:21:05 PDT
Created attachment 353499 [details]
Patch
Comment 7 WebKit Commit Bot 2018-10-31 13:16:57 PDT
Comment on attachment 353499 [details]
Patch

Clearing flags on attachment: 353499

Committed r237648: <https://trac.webkit.org/changeset/237648>
Comment 8 WebKit Commit Bot 2018-10-31 13:16:59 PDT
All reviewed patches have been landed.  Closing bug.
Comment 9 Wenson Hsieh 2018-10-31 16:39:42 PDT
Unfortunately, it appears that this did not work: <https://build.webkit.org/builders/Apple%20High%20Sierra%20Release%20WK2%20(Tests)/builds/7545/steps/run-api-tests/logs/stdio>.

Continuing to investigate...
Comment 10 Wenson Hsieh 2018-10-31 17:58:53 PDT
Created attachment 353563 [details]
Patch
Comment 11 Dean Jackson 2018-10-31 18:02:15 PDT
Comment on attachment 353563 [details]
Patch

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

> Source/WebKit/ChangeLog:9
> +        Followup to r237648: also make sure that we unsecurely unarchive NSFileWrapper on platforms that don't support

typo: unsecurely

> Source/WebKit/UIProcess/API/Cocoa/APIAttachmentCocoa.mm:161
> +    NSFileWrapper *fileWrapper = insecurelyUnarchiveObjectFromData(serializedData.get());

Isn't this insecure? :)
Comment 12 Wenson Hsieh 2018-10-31 18:04:03 PDT
Comment on attachment 353563 [details]
Patch

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

>> Source/WebKit/ChangeLog:9
>> +        Followup to r237648: also make sure that we unsecurely unarchive NSFileWrapper on platforms that don't support
> 
> typo: unsecurely

Oops! Fixed.
Comment 13 Wenson Hsieh 2018-10-31 18:15:04 PDT
Comment on attachment 353563 [details]
Patch

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

>> Source/WebKit/UIProcess/API/Cocoa/APIAttachmentCocoa.mm:161
>> +    NSFileWrapper *fileWrapper = insecurelyUnarchiveObjectFromData(serializedData.get());
> 
> Isn't this insecure? :)

...that's a good point, I think this additionally warrants a check that [fileWrapper isKindOfClass:NSFileWrapper.class]
Comment 14 Wenson Hsieh 2018-10-31 18:34:30 PDT
Created attachment 353568 [details]
Patch for landing
Comment 15 WebKit Commit Bot 2018-10-31 19:11:28 PDT
Comment on attachment 353568 [details]
Patch for landing

Clearing flags on attachment: 353568

Committed r237667: <https://trac.webkit.org/changeset/237667>
Comment 16 EWS Watchlist 2018-10-31 20:01:59 PDT
Comment on attachment 353563 [details]
Patch

Attachment 353563 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/9805037

New failing tests:
http/wpt/mediarecorder/MediaRecorder-mock-dataavailable.html
Comment 17 EWS Watchlist 2018-10-31 20:02:01 PDT
Created attachment 353573 [details]
Archive of layout-test-results from ews121 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.6