⚠ Archived content — this site is no longer maintained.   Current WebKit documentation is at docs.webkit.org.

Changeset 203541 in webkit


Ignore:
Timestamp:
Jul 21, 2016, 5:11:14 PM (10 years ago)
Author:
commit-queue@webkit.org
Message:

[iOS] Apps using WKWebView will crash if they set the scroll view's delegate and don't nil it out later
​https://bugs.webkit.org/show_bug.cgi?id=159980
rdar://problem/27450825

Patch by Chelsea Pugh <​cpugh@apple.com> on 2016-07-21
Reviewed by Dan Bernstein.

Source/WebKit2:

The root cause of this crash is that we are not abiding the UIScrollView API that the scroll view
delegate property should be weak. If setters of this delegate do not know that, since the WKWebView
exposes the scroll view as a UIScrollView, they may forget to nil out the delegate they set and will
then crash.

  • UIProcess/ios/WKScrollView.mm:

(-[WKScrollViewDelegateForwarder methodSignatureForSelector:]): Get a RetainPtr holding the
external delegate and use where needed.
(-[WKScrollViewDelegateForwarder respondsToSelector:]): Ditto.
(-[WKScrollViewDelegateForwarder forwardInvocation:]): Ditto.
(-[WKScrollViewDelegateForwarder forwardingTargetForSelector:]): Ditto. When returning a reference
to the external delegate, get a retained and autoreleased reference so the caller needn't release
the object when done.
(-[WKScrollView delegate]): Ditto.
(-[WKScrollView _updateDelegate]): Get a RetainPtr holding the external delegate that can be
used throughout this method. Use the RetainPtr to get the external delegate for setting super's
delegate as well as creating the delegate forwarder.
(-[WKScrollView setDelegate:]): Get a RetainPtr holding the external delegate and use its value for
comparison to the object we are setting the external delegate to.

Tools:

  • TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj:
  • TestWebKitAPI/Tests/ios/WKScrollViewDelegateCrash.mm: Added.

(-[TestDelegateForScrollView dealloc]): Update delegateIsDeallocated to true so that we can tell
when our delegate has hit -dealloc.
(TestWebKitAPI::TEST): Ensure that after an object has been set as the scroll view's delegate,
and has then been deallocated, that the scroll view's delegate is nil and the deallocated delegate
will not be messaged.

Location:
trunk
Files:
1 added
4 edited

Legend:

Unmodified
Added
Removed

  • trunk/Source/WebKit2/ChangeLog

    r203520 r203541  
     12016-07-21  Chelsea Pugh  <cpugh@apple.com>
     2
     3        [iOS] Apps using WKWebView will crash if they set the scroll view's delegate and don't nil it out later
     4        https://bugs.webkit.org/show_bug.cgi?id=159980
     5        rdar://problem/27450825
     6
     7        Reviewed by Dan Bernstein.
     8
     9        The root cause of this crash is that we are not abiding the UIScrollView API that the scroll view
     10        delegate property should be weak. If setters of this delegate do not know that, since the WKWebView
     11        exposes the scroll view as a UIScrollView, they may forget to nil out the delegate they set and will
     12        then crash.
     13
     14        * UIProcess/ios/WKScrollView.mm:
     15        (-[WKScrollViewDelegateForwarder methodSignatureForSelector:]): Get a RetainPtr holding the
     16        external delegate and use where needed.
     17        (-[WKScrollViewDelegateForwarder respondsToSelector:]): Ditto.
     18        (-[WKScrollViewDelegateForwarder forwardInvocation:]): Ditto.
     19        (-[WKScrollViewDelegateForwarder forwardingTargetForSelector:]): Ditto. When returning a reference
     20        to the external delegate, get a retained and autoreleased reference so the caller needn't release
     21        the object when done.
     22        (-[WKScrollView delegate]): Ditto.
     23        (-[WKScrollView _updateDelegate]): Get a RetainPtr holding the external delegate that can be
     24        used throughout this method. Use the RetainPtr to get the external delegate for setting super's
     25        delegate as well as creating the delegate forwarder.
     26        (-[WKScrollView setDelegate:]): Get a RetainPtr holding the external delegate and use its value for
     27        comparison to the object we are setting the external delegate to.
     28
    1292016-07-21  Myles C. Maxfield  <mmaxfield@apple.com>
    230

  • trunk/Source/WebKit2/UIProcess/ios/WKScrollView.mm

    r188541 r203541  
    3030
    3131#import "WKWebViewInternal.h"
     32#import "WeakObjCPtr.h"
    3233#import <WebCore/CoreGraphicsSPI.h>
     34
     35using namespace WebKit;
    3336
    3437@interface UIScrollView (UIScrollViewInternalHack)
    … …  
    4447@implementation WKScrollViewDelegateForwarder {
    4548    WKWebView *_internalDelegate;
    46     id <UIScrollViewDelegate> _externalDelegate;
     49    WeakObjCPtr<id <UIScrollViewDelegate>> _externalDelegate;
    4750}
    4851
    … …  
    5962- (NSMethodSignature *)methodSignatureForSelector:(SEL)aSelector
    6063{
     64    auto externalDelegate = _externalDelegate.get();
    6165    NSMethodSignature *signature = [super methodSignatureForSelector:aSelector];
    6266    if (!signature)
    6367        signature = [(NSObject *)_internalDelegate methodSignatureForSelector:aSelector];
    6468    if (!signature)
    65         signature = [(NSObject *)_externalDelegate methodSignatureForSelector:aSelector];
     69        signature = [(NSObject *)externalDelegate methodSignatureForSelector:aSelector];
    6670    return signature;
    6771}
    … …  
    6973- (BOOL)respondsToSelector:(SEL)aSelector
    7074{
    71     return [super respondsToSelector:aSelector] || [_internalDelegate respondsToSelector:aSelector] || [_externalDelegate respondsToSelector:aSelector];
     75    return [super respondsToSelector:aSelector] || [_internalDelegate respondsToSelector:aSelector] || [_externalDelegate.get() respondsToSelector:aSelector];
    7276}
    7377
    7478- (void)forwardInvocation:(NSInvocation *)anInvocation
    7579{
     80    auto externalDelegate = _externalDelegate.get();
    7681    SEL aSelector = [anInvocation selector];
    7782    BOOL internalDelegateWillRespond = [_internalDelegate respondsToSelector:aSelector];
    78     BOOL externalDelegateWillRespond = [_externalDelegate respondsToSelector:aSelector];
     83    BOOL externalDelegateWillRespond = [externalDelegate respondsToSelector:aSelector];
    7984
    8085    if (internalDelegateWillRespond && externalDelegateWillRespond)
    … …  
    8489        [anInvocation invokeWithTarget:_internalDelegate];
    8590    if (externalDelegateWillRespond)
    86         [anInvocation invokeWithTarget:_externalDelegate];
     91        [anInvocation invokeWithTarget:externalDelegate.get()];
    8792
    8893    if (internalDelegateWillRespond && externalDelegateWillRespond)
    … …  
    96101{
    97102    BOOL internalDelegateWillRespond = [_internalDelegate respondsToSelector:aSelector];
    98     BOOL externalDelegateWillRespond = [_externalDelegate respondsToSelector:aSelector];
     103    BOOL externalDelegateWillRespond = [_externalDelegate.get() respondsToSelector:aSelector];
    99104
    100105    if (internalDelegateWillRespond && !externalDelegateWillRespond)
    101106        return _internalDelegate;
    102107    if (externalDelegateWillRespond && !internalDelegateWillRespond)
    103         return _externalDelegate;
     108        return _externalDelegate.getAutoreleased();
    104109    return nil;
    105110}
    … …  
    108113
    109114@implementation WKScrollView {
    110     id <UIScrollViewDelegate> _externalDelegate;
     115    WeakObjCPtr<id <UIScrollViewDelegate>> _externalDelegate;
    111116    WKScrollViewDelegateForwarder *_delegateForwarder;
    112117}
    … …  
    133138- (void)setDelegate:(id <UIScrollViewDelegate>)delegate
    134139{
    135     if (_externalDelegate == delegate)
     140    if (_externalDelegate.get().get() == delegate)
    136141        return;
    137142    _externalDelegate = delegate;
    … …  
    141146- (id <UIScrollViewDelegate>)delegate
    142147{
    143     return _externalDelegate;
     148    return _externalDelegate.getAutoreleased();
    144149}
    145150
    … …  
    148153    WKScrollViewDelegateForwarder *oldForwarder = _delegateForwarder;
    149154    _delegateForwarder = nil;
    150     if (!_externalDelegate)
     155    auto externalDelegate = _externalDelegate.get();
     156    if (!externalDelegate)
    151157        [super setDelegate:_internalDelegate];
    152158    else if (!_internalDelegate)
    153         [super setDelegate:_externalDelegate];
     159        [super setDelegate:externalDelegate.get()];
    154160    else {
    155         _delegateForwarder = [[WKScrollViewDelegateForwarder alloc] initWithInternalDelegate:_internalDelegate externalDelegate:_externalDelegate];
     161        _delegateForwarder = [[WKScrollViewDelegateForwarder alloc] initWithInternalDelegate:_internalDelegate externalDelegate:externalDelegate.get()];
    156162        [super setDelegate:_delegateForwarder];
    157163    }

  • trunk/Tools/ChangeLog

    r203540 r203541  
     12016-07-21  Chelsea Pugh  <cpugh@apple.com>
     2
     3        [iOS] Apps using WKWebView will crash if they set the scroll view's delegate and don't nil it out later
     4        https://bugs.webkit.org/show_bug.cgi?id=159980
     5        rdar://problem/27450825
     6
     7        Reviewed by Dan Bernstein.
     8
     9        * TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj:
     10        * TestWebKitAPI/Tests/ios/WKScrollViewDelegateCrash.mm: Added.
     11        (-[TestDelegateForScrollView dealloc]): Update delegateIsDeallocated to true so that we can tell
     12        when our delegate has hit -dealloc.
     13        (TestWebKitAPI::TEST): Ensure that after an object has been set as the scroll view's delegate,
     14        and has then been deallocated, that the scroll view's delegate is nil and the deallocated delegate
     15        will not be messaged.
     16
    1172016-07-21  Myles C. Maxfield  <mmaxfield@apple.com>
    218

  • trunk/Tools/TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj

    r203508 r203541  
    115115                5C9E59421D3EB5AC00E3C62E /* ApplicationCache.db-shm in Copy Resources */ = {isa = PBXBuildFile; fileRef = 5C9E593F1D3EB1DE00E3C62E /* ApplicationCache.db-shm */; };
    116116                5C9E59431D3EB5AC00E3C62E /* ApplicationCache.db-wal in Copy Resources */ = {isa = PBXBuildFile; fileRef = 5C9E59401D3EB1DE00E3C62E /* ApplicationCache.db-wal */; };
     117                5E4B1D2E1D404C6100053621 /* WKScrollViewDelegateCrash.mm in Sources */ = {isa = PBXBuildFile; fileRef = 5E4B1D2C1D404C6100053621 /* WKScrollViewDelegateCrash.mm */; };
    117118                764322D71B61CCC30024F801 /* WordBoundaryTypingAttributes.mm in Sources */ = {isa = PBXBuildFile; fileRef = 764322D51B61CCA40024F801 /* WordBoundaryTypingAttributes.mm */; };
    118119                7673499D1930C5BB00E44DF9 /* StopLoadingDuringDidFailProvisionalLoad_bundle.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 7673499A1930182E00E44DF9 /* StopLoadingDuringDidFailProvisionalLoad_bundle.cpp */; };
    … …  
    804805                5C9E593F1D3EB1DE00E3C62E /* ApplicationCache.db-shm */ = {isa = PBXFileReference; lastKnownFileType = file; path = "ApplicationCache.db-shm"; sourceTree = "<group>"; };
    805806                5C9E59401D3EB1DE00E3C62E /* ApplicationCache.db-wal */ = {isa = PBXFileReference; lastKnownFileType = file; path = "ApplicationCache.db-wal"; sourceTree = "<group>"; };
     807                5E4B1D2C1D404C6100053621 /* WKScrollViewDelegateCrash.mm */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.objcpp; name = WKScrollViewDelegateCrash.mm; path = ../ios/WKScrollViewDelegateCrash.mm; sourceTree = "<group>"; };
    806808                7560917719259C59009EF06E /* MemoryCacheAddImageToCacheIOS.mm */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.objcpp; path = MemoryCacheAddImageToCacheIOS.mm; sourceTree = "<group>"; };
    807809                75F3133F18C171B70041CAEC /* EphemeralSessionPushStateNoHistoryCallback.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; path = EphemeralSessionPushStateNoHistoryCallback.cpp; sourceTree = "<group>"; };
    … …  
    12481250                                93E943F11CD3E87E00AC08C2 /* VideoControlsManager.mm */,
    12491251                                2D00065D1C1F58940088E6A7 /* WKPDFViewResizeCrash.mm */,
     1252                                5E4B1D2C1D404C6100053621 /* WKScrollViewDelegateCrash.mm */,
    12501253                                7C417F311D19E14800B8EF53 /* WKWebViewDefaultNavigationDelegate.mm */,
    12511254                                0F3B94A51A77266C00DE3272 /* WKWebViewEvaluateJavaScript.mm */,
    … …  
    21332136                                7CCE7EE01A411A9A00447C4C /* EditorCommands.mm in Sources */,
    21342137                                7CCE7EBF1A411A7E00447C4C /* ElementAtPointInWebFrame.mm in Sources */,
     2138                                5E4B1D2E1D404C6100053621 /* WKScrollViewDelegateCrash.mm in Sources */,
    21352139                                7CCE7EEF1A411AE600447C4C /* EphemeralSessionPushStateNoHistoryCallback.cpp in Sources */,
    21362140                                7CCE7EF01A411AE600447C4C /* EvaluateJavaScript.cpp in Sources */,
Note: See TracChangeset for help on using the changeset viewer.