- Revision
- 203541
- Author
- [email protected]
- Date
- 2016-07-21 17:11:14 -0700 (Thu, 21 Jul 2016)
Log 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 <[email protected]> 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.
Modified Paths
Added Paths
Diff
Modified: trunk/Source/WebKit2/ChangeLog (203540 => 203541)
--- trunk/Source/WebKit2/ChangeLog 2016-07-21 23:55:40 UTC (rev 203540)
+++ trunk/Source/WebKit2/ChangeLog 2016-07-22 00:11:14 UTC (rev 203541)
@@ -1,3 +1,31 @@
+2016-07-21 Chelsea Pugh <[email protected]>
+
+ [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
+
+ Reviewed by Dan Bernstein.
+
+ 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.
+
2016-07-21 Myles C. Maxfield <[email protected]>
[iPhone] Playing a video on tudou.com plays only sound, no video
Modified: trunk/Source/WebKit2/UIProcess/ios/WKScrollView.mm (203540 => 203541)
--- trunk/Source/WebKit2/UIProcess/ios/WKScrollView.mm 2016-07-21 23:55:40 UTC (rev 203540)
+++ trunk/Source/WebKit2/UIProcess/ios/WKScrollView.mm 2016-07-22 00:11:14 UTC (rev 203541)
@@ -29,8 +29,11 @@
#if PLATFORM(IOS)
#import "WKWebViewInternal.h"
+#import "WeakObjCPtr.h"
#import <WebCore/CoreGraphicsSPI.h>
+using namespace WebKit;
+
@interface UIScrollView (UIScrollViewInternalHack)
- (CGFloat)_rubberBandOffsetForOffset:(CGFloat)newOffset maxOffset:(CGFloat)maxOffset minOffset:(CGFloat)minOffset range:(CGFloat)range outside:(BOOL *)outside;
@end
@@ -43,7 +46,7 @@
@implementation WKScrollViewDelegateForwarder {
WKWebView *_internalDelegate;
- id <UIScrollViewDelegate> _externalDelegate;
+ WeakObjCPtr<id <UIScrollViewDelegate>> _externalDelegate;
}
- (instancetype)initWithInternalDelegate:(WKWebView <UIScrollViewDelegate> *)internalDelegate externalDelegate:(id <UIScrollViewDelegate>)externalDelegate
@@ -58,24 +61,26 @@
- (NSMethodSignature *)methodSignatureForSelector:(SEL)aSelector
{
+ auto externalDelegate = _externalDelegate.get();
NSMethodSignature *signature = [super methodSignatureForSelector:aSelector];
if (!signature)
signature = [(NSObject *)_internalDelegate methodSignatureForSelector:aSelector];
if (!signature)
- signature = [(NSObject *)_externalDelegate methodSignatureForSelector:aSelector];
+ signature = [(NSObject *)externalDelegate methodSignatureForSelector:aSelector];
return signature;
}
- (BOOL)respondsToSelector:(SEL)aSelector
{
- return [super respondsToSelector:aSelector] || [_internalDelegate respondsToSelector:aSelector] || [_externalDelegate respondsToSelector:aSelector];
+ return [super respondsToSelector:aSelector] || [_internalDelegate respondsToSelector:aSelector] || [_externalDelegate.get() respondsToSelector:aSelector];
}
- (void)forwardInvocation:(NSInvocation *)anInvocation
{
+ auto externalDelegate = _externalDelegate.get();
SEL aSelector = [anInvocation selector];
BOOL internalDelegateWillRespond = [_internalDelegate respondsToSelector:aSelector];
- BOOL externalDelegateWillRespond = [_externalDelegate respondsToSelector:aSelector];
+ BOOL externalDelegateWillRespond = [externalDelegate respondsToSelector:aSelector];
if (internalDelegateWillRespond && externalDelegateWillRespond)
[_internalDelegate _willInvokeUIScrollViewDelegateCallback];
@@ -83,7 +88,7 @@
if (internalDelegateWillRespond)
[anInvocation invokeWithTarget:_internalDelegate];
if (externalDelegateWillRespond)
- [anInvocation invokeWithTarget:_externalDelegate];
+ [anInvocation invokeWithTarget:externalDelegate.get()];
if (internalDelegateWillRespond && externalDelegateWillRespond)
[_internalDelegate _didInvokeUIScrollViewDelegateCallback];
@@ -95,12 +100,12 @@
- (id)forwardingTargetForSelector:(SEL)aSelector
{
BOOL internalDelegateWillRespond = [_internalDelegate respondsToSelector:aSelector];
- BOOL externalDelegateWillRespond = [_externalDelegate respondsToSelector:aSelector];
+ BOOL externalDelegateWillRespond = [_externalDelegate.get() respondsToSelector:aSelector];
if (internalDelegateWillRespond && !externalDelegateWillRespond)
return _internalDelegate;
if (externalDelegateWillRespond && !internalDelegateWillRespond)
- return _externalDelegate;
+ return _externalDelegate.getAutoreleased();
return nil;
}
@@ -107,7 +112,7 @@
@end
@implementation WKScrollView {
- id <UIScrollViewDelegate> _externalDelegate;
+ WeakObjCPtr<id <UIScrollViewDelegate>> _externalDelegate;
WKScrollViewDelegateForwarder *_delegateForwarder;
}
@@ -132,7 +137,7 @@
- (void)setDelegate:(id <UIScrollViewDelegate>)delegate
{
- if (_externalDelegate == delegate)
+ if (_externalDelegate.get().get() == delegate)
return;
_externalDelegate = delegate;
[self _updateDelegate];
@@ -140,7 +145,7 @@
- (id <UIScrollViewDelegate>)delegate
{
- return _externalDelegate;
+ return _externalDelegate.getAutoreleased();
}
- (void)_updateDelegate
@@ -147,12 +152,13 @@
{
WKScrollViewDelegateForwarder *oldForwarder = _delegateForwarder;
_delegateForwarder = nil;
- if (!_externalDelegate)
+ auto externalDelegate = _externalDelegate.get();
+ if (!externalDelegate)
[super setDelegate:_internalDelegate];
else if (!_internalDelegate)
- [super setDelegate:_externalDelegate];
+ [super setDelegate:externalDelegate.get()];
else {
- _delegateForwarder = [[WKScrollViewDelegateForwarder alloc] initWithInternalDelegate:_internalDelegate externalDelegate:_externalDelegate];
+ _delegateForwarder = [[WKScrollViewDelegateForwarder alloc] initWithInternalDelegate:_internalDelegate externalDelegate:externalDelegate.get()];
[super setDelegate:_delegateForwarder];
}
[oldForwarder release];
Modified: trunk/Tools/ChangeLog (203540 => 203541)
--- trunk/Tools/ChangeLog 2016-07-21 23:55:40 UTC (rev 203540)
+++ trunk/Tools/ChangeLog 2016-07-22 00:11:14 UTC (rev 203541)
@@ -1,3 +1,19 @@
+2016-07-21 Chelsea Pugh <[email protected]>
+
+ [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
+
+ Reviewed by Dan Bernstein.
+
+ * 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.
+
2016-07-21 Myles C. Maxfield <[email protected]>
Follow-up patch to r203520
Modified: trunk/Tools/TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj (203540 => 203541)
--- trunk/Tools/TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj 2016-07-21 23:55:40 UTC (rev 203540)
+++ trunk/Tools/TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj 2016-07-22 00:11:14 UTC (rev 203541)
@@ -114,6 +114,7 @@
5C9E59411D3EB5AC00E3C62E /* ApplicationCache.db in Copy Resources */ = {isa = PBXBuildFile; fileRef = 5C9E593E1D3EB1DE00E3C62E /* ApplicationCache.db */; };
5C9E59421D3EB5AC00E3C62E /* ApplicationCache.db-shm in Copy Resources */ = {isa = PBXBuildFile; fileRef = 5C9E593F1D3EB1DE00E3C62E /* ApplicationCache.db-shm */; };
5C9E59431D3EB5AC00E3C62E /* ApplicationCache.db-wal in Copy Resources */ = {isa = PBXBuildFile; fileRef = 5C9E59401D3EB1DE00E3C62E /* ApplicationCache.db-wal */; };
+ 5E4B1D2E1D404C6100053621 /* WKScrollViewDelegateCrash.mm in Sources */ = {isa = PBXBuildFile; fileRef = 5E4B1D2C1D404C6100053621 /* WKScrollViewDelegateCrash.mm */; };
764322D71B61CCC30024F801 /* WordBoundaryTypingAttributes.mm in Sources */ = {isa = PBXBuildFile; fileRef = 764322D51B61CCA40024F801 /* WordBoundaryTypingAttributes.mm */; };
7673499D1930C5BB00E44DF9 /* StopLoadingDuringDidFailProvisionalLoad_bundle.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 7673499A1930182E00E44DF9 /* StopLoadingDuringDidFailProvisionalLoad_bundle.cpp */; };
76E182DD1547569100F1FADD /* WillSendSubmitEvent_Bundle.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 76E182DC1547569100F1FADD /* WillSendSubmitEvent_Bundle.cpp */; };
@@ -803,6 +804,7 @@
5C9E593E1D3EB1DE00E3C62E /* ApplicationCache.db */ = {isa = PBXFileReference; lastKnownFileType = file; path = ApplicationCache.db; sourceTree = "<group>"; };
5C9E593F1D3EB1DE00E3C62E /* ApplicationCache.db-shm */ = {isa = PBXFileReference; lastKnownFileType = file; path = "ApplicationCache.db-shm"; sourceTree = "<group>"; };
5C9E59401D3EB1DE00E3C62E /* ApplicationCache.db-wal */ = {isa = PBXFileReference; lastKnownFileType = file; path = "ApplicationCache.db-wal"; sourceTree = "<group>"; };
+ 5E4B1D2C1D404C6100053621 /* WKScrollViewDelegateCrash.mm */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.objcpp; name = WKScrollViewDelegateCrash.mm; path = ../ios/WKScrollViewDelegateCrash.mm; sourceTree = "<group>"; };
7560917719259C59009EF06E /* MemoryCacheAddImageToCacheIOS.mm */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.objcpp; path = MemoryCacheAddImageToCacheIOS.mm; sourceTree = "<group>"; };
75F3133F18C171B70041CAEC /* EphemeralSessionPushStateNoHistoryCallback.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; path = EphemeralSessionPushStateNoHistoryCallback.cpp; sourceTree = "<group>"; };
764322D51B61CCA40024F801 /* WordBoundaryTypingAttributes.mm */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.objcpp; path = WordBoundaryTypingAttributes.mm; sourceTree = "<group>"; };
@@ -1247,6 +1249,7 @@
7CCB99201D3B41F6003922F6 /* UserInitiatedActionInNavigationAction.mm */,
93E943F11CD3E87E00AC08C2 /* VideoControlsManager.mm */,
2D00065D1C1F58940088E6A7 /* WKPDFViewResizeCrash.mm */,
+ 5E4B1D2C1D404C6100053621 /* WKScrollViewDelegateCrash.mm */,
7C417F311D19E14800B8EF53 /* WKWebViewDefaultNavigationDelegate.mm */,
0F3B94A51A77266C00DE3272 /* WKWebViewEvaluateJavaScript.mm */,
9984FACA1CFFAEEE008D198C /* WKWebViewTextInput.mm */,
@@ -2132,6 +2135,7 @@
7CEFA9661AC0B9E200B910FD /* _WKUserContentExtensionStore.mm in Sources */,
7CCE7EE01A411A9A00447C4C /* EditorCommands.mm in Sources */,
7CCE7EBF1A411A7E00447C4C /* ElementAtPointInWebFrame.mm in Sources */,
+ 5E4B1D2E1D404C6100053621 /* WKScrollViewDelegateCrash.mm in Sources */,
7CCE7EEF1A411AE600447C4C /* EphemeralSessionPushStateNoHistoryCallback.cpp in Sources */,
7CCE7EF01A411AE600447C4C /* EvaluateJavaScript.cpp in Sources */,
7CCE7EF11A411AE600447C4C /* FailedLoad.cpp in Sources */,
Added: trunk/Tools/TestWebKitAPI/Tests/ios/WKScrollViewDelegateCrash.mm (0 => 203541)
--- trunk/Tools/TestWebKitAPI/Tests/ios/WKScrollViewDelegateCrash.mm (rev 0)
+++ trunk/Tools/TestWebKitAPI/Tests/ios/WKScrollViewDelegateCrash.mm 2016-07-22 00:11:14 UTC (rev 203541)
@@ -0,0 +1,68 @@
+/*
+ * Copyright (C) 2016 Apple Inc. All rights reserved.
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions
+ * are met:
+ * 1. Redistributions of source code must retain the above copyright
+ * notice, this list of conditions and the following disclaimer.
+ * 2. Redistributions in binary form must reproduce the above copyright
+ * notice, this list of conditions and the following disclaimer in the
+ * documentation and/or other materials provided with the distribution.
+ *
+ * THIS SOFTWARE IS PROVIDED BY APPLE INC. AND ITS CONTRIBUTORS ``AS IS''
+ * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO,
+ * THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR
+ * PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL APPLE INC. OR ITS CONTRIBUTORS
+ * BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR
+ * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
+ * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS
+ * INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN
+ * CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
+ * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF
+ * THE POSSIBILITY OF SUCH DAMAGE.
+ */
+
+#import "config.h"
+
+#if PLATFORM(IOS)
+
+#import "PlatformUtilities.h"
+#import "Test.h"
+#import <WebKit/WKWebView.h>
+
+static bool delegateIsDeallocated;
+
+@interface TestDelegateForScrollView : NSObject <UIScrollViewDelegate>
+
+@end
+
+@implementation TestDelegateForScrollView
+
+- (void)dealloc
+{
+ delegateIsDeallocated = true;
+ [super dealloc];
+}
+
+@end
+
+namespace TestWebKitAPI {
+
+TEST(WKWebView, WKScrollViewDelegateCrash)
+{
+ WKWebView *webView = [[WKWebView alloc] init];
+ TestDelegateForScrollView *delegateForScrollView = [[TestDelegateForScrollView alloc] init];
+ @autoreleasepool {
+ webView.scrollView.delegate = delegateForScrollView;
+ }
+ delegateIsDeallocated = false;
+ [delegateForScrollView release];
+ TestWebKitAPI::Util::run(&delegateIsDeallocated);
+
+ EXPECT_NULL(webView.scrollView.delegate);
+}
+
+}
+
+#endif
