Changeset 267352 in webkit


Ignore:
Timestamp:
Sep 21, 2020 12:17:04 PM (4 years ago)
Author:
James Savage
Message:

WKWebView Swift overlay has mis-annotated nullability for evaluateJavaScript
<​http://webkit.org/b/216198>
<rdar://problem/68035950>

Reviewed by Darin Adler.

Due to a mistranslation of evaluateJavaScript, we are vending an API which does not expect
to receive nil as a valid result value. This change fixes the crash, but does not yet fix
the API to have the correct signature. That will come in a later patch.

To fix the crash, we need to produce a valid Result<Any, Error> to pass back to clients.
Fortunately, instead of inventing something clever, we can just use nil. It's valid to box
optional values into Any, and clients can technically retrieve them with the right dynamic cast
as well. Since client code must be using dynamic casting to convert the result Any to a usable
type, and because in the case where we now return a new value at runtime we would have previously
crashed, this shouldn't have any binary compatibility impact either.

To better validate these changes, I also add new unit tests for the conversion of JavaScript
results into Swift values, including a test for the deprecated API.

  • SwiftOverlay/SwiftOverlay/ObjectiveCBlockConversions.swift: Added. For clarity I'm factoring

helper methods into a single namespace, as it also makes a nice place to document their expectations.
(ObjectiveCBlockConversion.exclusive.exclusive(_:)): This is renamed from the free function,

makeResultHandler(_:). It still has the same fatalError (now precondition) as before, but
hopefully a better name to clarify that it expects exactly-one value.

(ObjectiveCBlockConversions.boxingNilAsAnyForCompatibility(_:)): This is a variant of exclusive(_:)

that makes the tradeoff of boxing any nil values as Any to avoid crashing. This is still safe,
since as mentioned our clients will need to cast the value they recieve to do anything with it,
and since the deprecated API expects Any, no one could have been successfully comparing it
to nil today anyways.

  • SwiftOverlay/Tests/JavaScriptToSwiftTypeConversions.swift: Added.

(JavaScriptToSwiftConversions.setUp): Construct a new web view, and add it to a window so that it is

in an expected state. I'm using about:blank as the URL, since page content doesn't matter for
these tests and I want the web content to be ready immediately.

(JavaScriptToSwiftConversions.tearDown): Just perform some window cleanup.
(JavaScriptToSwiftConversions.evaluateJavaScript(_:andExpect:)): Helper method to evaluate script and

check its result. I'm using String.debugDescription because it escapes quotes and special characters
which makes the readout easier to parse.

(JavaScriptToSwiftConversions.testNull): JavaScript's null is actually mapped to NSNull, not nil.
(JavaScriptToSwiftConversions.testInteger): Some standard type coercion tests. The underlying value for

all number types should be NSNumber, so this is actually check against a float or integer type
without issue.

(JavaScriptToSwiftConversions.testDecimal): Ditto.
(JavaScriptToSwiftConversions.testBoolean): Ditto.
(JavaScriptToSwiftConversions.testString): Ditto.
(JavaScriptToSwiftConversions.testArray): Ditto.
(JavaScriptToSwiftConversions.testDictionary): Ditto, only you can't evaluate an object literal directly

so I need to store it in a temporary location first.

(JavaScriptToSwiftConversions.testUndefined): Test our boxing of nil. The exact value matters less than

not crashing at all.

  • SwiftOverlay/WebKitSwiftOverlay.xcodeproj/project.pbxproj: Added new files to project. I kept the

new test file and helper files within the SwiftOverlay group, because they do not contribute any API.

  • UIProcess/API/Cocoa/WebKitSwiftOverlay.swift:

(WKWebView.callAsyncJavaScript(_:arguments:in:in:completionHandler:)): Switch to a conversion which does

not trap on nil.

(WKWebView.createPDF(_:completionHandler:)): Updated to use new helper method name.
(WKWebView.createWebArchiveData(_:)): Ditto.
(WKWebView.evaluateJavaScript(_:in:in:completionHandler:Error:)): See above.
(makeResultHandler(_:): This has been subsumed by ObjCBlockConversions.

Location:
trunk/Source/WebKit
Files:
2 added
3 edited

Legend:

Unmodified
Added
Removed

  • trunk/Source/WebKit/ChangeLog

    r267344 r267352  
     12020-09-21  James Savage  <james.savage@apple.com>
     2
     3        WKWebView Swift overlay has mis-annotated nullability for evaluateJavaScript
     4        <http://webkit.org/b/216198>
     5        <rdar://problem/68035950>
     6
     7        Reviewed by Darin Adler.
     8
     9        Due to a mistranslation of evaluateJavaScript, we are vending an API which does not expect
     10        to receive nil as a valid result value. This change fixes the crash, but does not yet fix
     11        the API to have the correct signature. That will come in a later patch.
     12
     13        To fix the crash, we need to produce a valid Result<Any, Error> to pass back to clients.
     14        Fortunately, instead of inventing something clever, we can just use nil. It's valid to box
     15        optional values into Any, and clients can technically retrieve them with the right dynamic cast
     16        as well. Since client code must be using dynamic casting to convert the result Any to a usable
     17        type, and because in the case where we now return a new value at runtime we would have previously
     18        crashed, this shouldn't have any binary compatibility impact either.
     19
     20        To better validate these changes, I also add new unit tests for the conversion of JavaScript
     21        results into Swift values, including a test for the deprecated API.
     22
     23        * SwiftOverlay/SwiftOverlay/ObjectiveCBlockConversions.swift: Added. For clarity I'm factoring
     24        helper methods into a single namespace, as it also makes a nice place to document their expectations.
     25        (ObjectiveCBlockConversion.exclusive.exclusive(_:)): This is renamed from the free function,
     26            makeResultHandler(_:). It still has the same fatalError (now precondition) as before, but
     27            hopefully a better name to clarify that it expects exactly-one value.
     28        (ObjectiveCBlockConversions.boxingNilAsAnyForCompatibility(_:)): This is a variant of exclusive(_:)
     29            that makes the tradeoff of boxing any nil values as Any to avoid crashing. This is still safe,
     30            since as mentioned our clients will need to cast the value they recieve to do anything with it,
     31            and since the deprecated API expects `Any`, no one could have been successfully comparing it
     32            to `nil` today anyways.
     33
     34        * SwiftOverlay/Tests/JavaScriptToSwiftTypeConversions.swift: Added.
     35        (JavaScriptToSwiftConversions.setUp): Construct a new web view, and add it to a window so that it is
     36            in an expected state. I'm using about:blank as the URL, since page content doesn't matter for
     37            these tests and I want the web content to be ready immediately.
     38        (JavaScriptToSwiftConversions.tearDown): Just perform some window cleanup.
     39        (JavaScriptToSwiftConversions.evaluateJavaScript(_:andExpect:)): Helper method to evaluate script and
     40            check its result. I'm using String.debugDescription because it escapes quotes and special characters
     41            which makes the readout easier to parse.
     42        (JavaScriptToSwiftConversions.testNull): JavaScript's null is actually mapped to NSNull, not nil.
     43        (JavaScriptToSwiftConversions.testInteger): Some standard type coercion tests. The underlying value for
     44            all number types should be NSNumber, so this is actually check against a float or integer type
     45            without issue.
     46        (JavaScriptToSwiftConversions.testDecimal): Ditto.
     47        (JavaScriptToSwiftConversions.testBoolean): Ditto.
     48        (JavaScriptToSwiftConversions.testString): Ditto.
     49        (JavaScriptToSwiftConversions.testArray): Ditto.
     50        (JavaScriptToSwiftConversions.testDictionary): Ditto, only you can't evaluate an object literal directly
     51            so I need to store it in a temporary location first.
     52        (JavaScriptToSwiftConversions.testUndefined): Test our boxing of nil. The exact value matters less than
     53            not crashing at all.
     54
     55        * SwiftOverlay/WebKitSwiftOverlay.xcodeproj/project.pbxproj: Added new files to project. I kept the
     56            new test file and helper files within the SwiftOverlay group, because they do not contribute any API.
     57
     58        * UIProcess/API/Cocoa/WebKitSwiftOverlay.swift:
     59        (WKWebView.callAsyncJavaScript(_:arguments:in:in:completionHandler:)): Switch to a conversion which does
     60            not trap on nil.
     61        (WKWebView.createPDF(_:completionHandler:)): Updated to use new helper method name.
     62        (WKWebView.createWebArchiveData(_:)): Ditto.
     63        (WKWebView.evaluateJavaScript(_:in:in:completionHandler:Error:)): See above.
     64        (makeResultHandler(_:): This has been subsumed by ObjCBlockConversions.
     65
    1662020-09-21  Kate Cheney  <katherine_cheney@apple.com>
    267

  • trunk/Source/WebKit/SwiftOverlay/WebKitSwiftOverlay.xcodeproj/project.pbxproj

    r262089 r267352  
    1414                B3A5D39223F790DB00B17727 /* WebKitSwiftOverlay.swift in Sources */ = {isa = PBXBuildFile; fileRef = B3A5D39123F790DB00B17727 /* WebKitSwiftOverlay.swift */; };
    1515                B3A5D39323F790DB00B17727 /* WebKitSwiftOverlay.swift in Sources */ = {isa = PBXBuildFile; fileRef = B3A5D39123F790DB00B17727 /* WebKitSwiftOverlay.swift */; };
     16                B3B8FECC250090B1006172CA /* JavaScriptToSwiftTypeConversions.swift in Sources */ = {isa = PBXBuildFile; fileRef = B3B8FECB250090B1006172CA /* JavaScriptToSwiftTypeConversions.swift */; };
     17                B3B8FECD250090B1006172CA /* JavaScriptToSwiftTypeConversions.swift in Sources */ = {isa = PBXBuildFile; fileRef = B3B8FECB250090B1006172CA /* JavaScriptToSwiftTypeConversions.swift */; };
     18                B3B8FEEF2502BAA0006172CA /* ObjectiveCBlockConversions.swift in Sources */ = {isa = PBXBuildFile; fileRef = B3B8FEEE2502BAA0006172CA /* ObjectiveCBlockConversions.swift */; };
     19                B3B8FEF02502BAA0006172CA /* ObjectiveCBlockConversions.swift in Sources */ = {isa = PBXBuildFile; fileRef = B3B8FEEE2502BAA0006172CA /* ObjectiveCBlockConversions.swift */; };
    1620/* End PBXBuildFile section */
    1721
    … …  
    4751                B3A5D39923F790E100B17727 /* install-swiftmodules.sh */ = {isa = PBXFileReference; lastKnownFileType = text.script.sh; path = "install-swiftmodules.sh"; sourceTree = "<group>"; };
    4852                B3A5D39A23F790E700B17727 /* WebKitSwiftOverlayTests-Info.plist */ = {isa = PBXFileReference; lastKnownFileType = text.plist.xml; path = "WebKitSwiftOverlayTests-Info.plist"; sourceTree = "<group>"; };
     53                B3B8FECB250090B1006172CA /* JavaScriptToSwiftTypeConversions.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = JavaScriptToSwiftTypeConversions.swift; sourceTree = "<group>"; };
     54                B3B8FEEE2502BAA0006172CA /* ObjectiveCBlockConversions.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = ObjectiveCBlockConversions.swift; sourceTree = "<group>"; };
    4955/* End PBXFileReference section */
    5056
    … …  
    8793                        children = (
    8894                                B3A5D39923F790E100B17727 /* install-swiftmodules.sh */,
     95                                B3B8FEEE2502BAA0006172CA /* ObjectiveCBlockConversions.swift */,
    8996                                B3A5D39123F790DB00B17727 /* WebKitSwiftOverlay.swift */,
    9097                        );
    … …  
    103110                        isa = PBXGroup;
    104111                        children = (
     112                                B3B8FECB250090B1006172CA /* JavaScriptToSwiftTypeConversions.swift */,
    105113                                B3A5D39A23F790E700B17727 /* WebKitSwiftOverlayTests-Info.plist */,
    106114                                B3A5D38823F78F5400B17727 /* WebKitTests.swift */,
    … …  
    345353                        buildActionMask = 2147483647;
    346354                        files = (
     355                                B3B8FEEF2502BAA0006172CA /* ObjectiveCBlockConversions.swift in Sources */,
    347356                                B3A5D39223F790DB00B17727 /* WebKitSwiftOverlay.swift in Sources */,
    348357                        );
    … …  
    353362                        buildActionMask = 2147483647;
    354363                        files = (
     364                                B3B8FEF02502BAA0006172CA /* ObjectiveCBlockConversions.swift in Sources */,
    355365                                B3A5D39323F790DB00B17727 /* WebKitSwiftOverlay.swift in Sources */,
    356366                        );
    … …  
    361371                        buildActionMask = 2147483647;
    362372                        files = (
     373                                B3B8FECC250090B1006172CA /* JavaScriptToSwiftTypeConversions.swift in Sources */,
    363374                                B3A5D38F23F78F5400B17727 /* WebKitTests.swift in Sources */,
    364375                        );
    … …  
    369380                        buildActionMask = 2147483647;
    370381                        files = (
     382                                B3B8FECD250090B1006172CA /* JavaScriptToSwiftTypeConversions.swift in Sources */,
    371383                                B3A5D39023F78F5400B17727 /* WebKitTests.swift in Sources */,
    372384                        );

  • trunk/Source/WebKit/UIProcess/API/Cocoa/WebKitSwiftOverlay.swift

    r263793 r267352  
    3737extension WKWebView {
    3838    public func callAsyncJavaScript(_ functionBody: String, arguments: [String:Any] = [:], in frame: WKFrameInfo? = nil, in contentWorld: WKContentWorld, completionHandler: ((Result<Any, Error>) -> Void)? = nil) {
    39         __callAsyncJavaScript(functionBody, arguments: arguments, inFrame: frame, in: contentWorld, completionHandler: completionHandler.map(makeResultHandler))
     39        __callAsyncJavaScript(functionBody, arguments: arguments, inFrame: frame, in: contentWorld, completionHandler: completionHandler.map(ObjCBlockConversion.boxingNilAsAnyForCompatibility))
    4040    }
    4141
    4242    public func createPDF(configuration: WKPDFConfiguration = .init(), completionHandler: @escaping (Result<Data, Error>) -> Void) {
    43         __createPDF(with: configuration, completionHandler: makeResultHandler(completionHandler))
     43        __createPDF(with: configuration, completionHandler: ObjCBlockConversion.exclusive(completionHandler))
    4444    }
    4545
    4646    public func createWebArchiveData(completionHandler: @escaping (Result<Data, Error>) -> Void) {
    47         __createWebArchiveData(completionHandler: makeResultHandler(completionHandler))
     47        __createWebArchiveData(completionHandler: ObjCBlockConversion.exclusive(completionHandler))
    4848    }
    4949
    5050    public func evaluateJavaScript(_ javaScript: String, in frame: WKFrameInfo? = nil, in contentWorld: WKContentWorld, completionHandler: ((Result<Any, Error>) -> Void)? = nil) {
    51         __evaluateJavaScript(javaScript, inFrame: frame, in: contentWorld, completionHandler: completionHandler.map(makeResultHandler))
     51        __evaluateJavaScript(javaScript, inFrame: frame, in: contentWorld, completionHandler: completionHandler.map(ObjCBlockConversion.boxingNilAsAnyForCompatibility))
    5252    }
    5353
    … …  
    5656    }
    5757}
    58 
    59 func makeResultHandler<Success, Failure>(_ handler: @escaping (Result<Success, Failure>) -> Void) -> (Success?, Failure?) -> Void {
    60     return { success, failure in
    61         if let success = success {
    62             handler(.success(success))
    63         } else if let failure = failure {
    64             handler(.failure(failure))
    65         } else {
    66             fatalError("Bug in WebKit: Received neither result or failure.")
    67         }
    68     }
    69 }
Note: See TracChangeset for help on using the changeset viewer.