Bug 159980

Summary: [iOS] Apps using WKWebView will crash if they set the scroll view's delegate and don't nil it out later
Product: WebKit Reporter: Chelsea Pugh <cpugh>
Component: WebKit2Assignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: adele, andersca, commit-queue, cpugh, mitz, sam, yongjun_zhang
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch for [iOS] Apps using WKWebView will crash if they set the scroll views
none
v2 patch for [iOS] Apps using WKWebView will crash
mitz: review+, cpugh: commit-queue-
v3 patch for [iOS] Apps using WKWebView will crash if they set the scroll view's delegate and don't nil it out later
none
v3 patch for [iOS] Apps using WKWebView will crash if they set the scroll view's delegate and don't nil it out later none

Description Chelsea Pugh 2016-07-20 11:39:47 PDT
Apps using WKWebView will crash if they set the scroll view's delegate and don't nil it out later. Patch forthcoming.
Comment 1 Chelsea Pugh 2016-07-20 11:45:39 PDT
Created attachment 284134 [details]
Patch for [iOS] Apps using WKWebView will crash if they set the scroll views
Comment 2 mitz 2016-07-20 11:52:09 PDT
Comment on attachment 284134 [details]
Patch for [iOS] Apps using WKWebView will crash if they set the scroll views

I believe that the fundamental problem is that WKScrollView is subclassing UIScrollView and violating its API contract that states that the delegate property is weak. Your patch prevents this from causing a crash in one specific scenario, but I don’t understand why we can’t just fix WKScrollView. It may be as simple as using a WeakObjCPtr for the external delegate (making sure to take a strong reference any time we use it in a method). Am I missing anything?
Comment 3 mitz 2016-07-20 11:53:01 PDT
It also seems like either way you choose to fix it, you should be able to create an API test.
Comment 4 Chelsea Pugh 2016-07-21 14:31:12 PDT
Created attachment 284260 [details]
v2 patch for [iOS] Apps using WKWebView will crash
Comment 5 WebKit Commit Bot 2016-07-21 14:32:12 PDT
Attachment 284260 [details] did not pass style-queue:


ERROR: Source/WebKit2/UIProcess/ios/WKScrollView.mm:31:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 6 mitz 2016-07-21 14:45:39 PDT
Comment on attachment 284260 [details]
v2 patch for [iOS] Apps using WKWebView will crash

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

> Source/WebKit2/UIProcess/ios/WKScrollView.mm:47
> +    WebKit::WeakObjCPtr<id <UIScrollViewDelegate>> _externalDelegate;

I think you can add “using namespace WebKit” to this file and avoid the WebKit:: qualifier here and elsewhere.

> Source/WebKit2/UIProcess/ios/WKScrollView.mm:74
> +    auto externalDelegate = _externalDelegate.get();
> +    return [super respondsToSelector:aSelector] || [_internalDelegate respondsToSelector:aSelector] || [externalDelegate.get() respondsToSelector:aSelector];

Since you’re only using the external delegate once you don’t really have to have this local.

> Source/WebKit2/UIProcess/ios/WKScrollView.mm:140
> +    if (_externalDelegate.get().get() == delegate)

Yikes! I wonder if we should add a == override to WeakObjCPtr.
Comment 7 Chelsea Pugh 2016-07-21 14:56:05 PDT
(In reply to comment #6)
> Comment on attachment 284260 [details]
> v2 patch for [iOS] Apps using WKWebView will crash
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=284260&action=review
> 
> > Source/WebKit2/UIProcess/ios/WKScrollView.mm:47
> > +    WebKit::WeakObjCPtr<id <UIScrollViewDelegate>> _externalDelegate;
> 
> I think you can add “using namespace WebKit” to this file and avoid the
> WebKit:: qualifier here and elsewhere.

Will do

> 
> > Source/WebKit2/UIProcess/ios/WKScrollView.mm:74
> > +    auto externalDelegate = _externalDelegate.get();
> > +    return [super respondsToSelector:aSelector] || [_internalDelegate respondsToSelector:aSelector] || [externalDelegate.get() respondsToSelector:aSelector];
> 
> Since you’re only using the external delegate once you don’t really have to
> have this local.

Okay

> 
> > Source/WebKit2/UIProcess/ios/WKScrollView.mm:140
> > +    if (_externalDelegate.get().get() == delegate)
> 
> Yikes! I wonder if we should add a == override to WeakObjCPtr.

Maybe, but likely out of scope for this patch
Comment 8 Chelsea Pugh 2016-07-21 15:24:35 PDT
Created attachment 284267 [details]
v3 patch for [iOS] Apps using WKWebView will crash if they set the scroll view's delegate and don't nil it out later
Comment 9 Chelsea Pugh 2016-07-21 15:30:09 PDT
Created attachment 284270 [details]
v3 patch for [iOS] Apps using WKWebView will crash if they set the scroll view's delegate and don't nil it out later
Comment 10 WebKit Commit Bot 2016-07-21 16:55:45 PDT
Comment on attachment 284270 [details]
v3 patch for [iOS] Apps using WKWebView will crash if they set the scroll view's delegate and don't nil it out later

Rejecting attachment 284270 [details] from commit-queue.

cpugh@apple.com does not have committer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/contributors.json.

- If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/contributors.json by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your committer rights.
Comment 11 WebKit Commit Bot 2016-07-21 17:12:13 PDT
Comment on attachment 284270 [details]
v3 patch for [iOS] Apps using WKWebView will crash if they set the scroll view's delegate and don't nil it out later

Clearing flags on attachment: 284270

Committed r203541: <http://trac.webkit.org/changeset/203541>
Comment 12 WebKit Commit Bot 2016-07-21 17:12:17 PDT
All reviewed patches have been landed.  Closing bug.