<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.1//EN"
"http://www.w3.org/TR/xhtml11/DTD/xhtml11.dtd">
<html xmlns="http://www.w3.org/1999/xhtml">
<head><meta http-equiv="content-type" content="text/html; charset=utf-8" />
<title>[174788] trunk/Source/WebKit2</title>
</head>
<body>
<style type="text/css"><!--
#msg dl.meta { border: 1px #006 solid; background: #369; padding: 6px; color: #fff; }
#msg dl.meta dt { float: left; width: 6em; font-weight: bold; }
#msg dt:after { content:':';}
#msg dl, #msg dt, #msg ul, #msg li, #header, #footer, #logmsg { font-family: verdana,arial,helvetica,sans-serif; font-size: 10pt; }
#msg dl a { font-weight: bold}
#msg dl a:link { color:#fc3; }
#msg dl a:active { color:#ff0; }
#msg dl a:visited { color:#cc6; }
h3 { font-family: verdana,arial,helvetica,sans-serif; font-size: 10pt; font-weight: bold; }
#msg pre { overflow: auto; background: #ffc; border: 1px #fa0 solid; padding: 6px; }
#logmsg { background: #ffc; border: 1px #fa0 solid; padding: 1em 1em 0 1em; }
#logmsg p, #logmsg pre, #logmsg blockquote { margin: 0 0 1em 0; }
#logmsg p, #logmsg li, #logmsg dt, #logmsg dd { line-height: 14pt; }
#logmsg h1, #logmsg h2, #logmsg h3, #logmsg h4, #logmsg h5, #logmsg h6 { margin: .5em 0; }
#logmsg h1:first-child, #logmsg h2:first-child, #logmsg h3:first-child, #logmsg h4:first-child, #logmsg h5:first-child, #logmsg h6:first-child { margin-top: 0; }
#logmsg ul, #logmsg ol { padding: 0; list-style-position: inside; margin: 0 0 0 1em; }
#logmsg ul { text-indent: -1em; padding-left: 1em; }#logmsg ol { text-indent: -1.5em; padding-left: 1.5em; }
#logmsg > ul, #logmsg > ol { margin: 0 0 1em 0; }
#logmsg pre { background: #eee; padding: 1em; }
#logmsg blockquote { border: 1px solid #fa0; border-left-width: 10px; padding: 1em 1em 0 1em; background: white;}
#logmsg dl { margin: 0; }
#logmsg dt { font-weight: bold; }
#logmsg dd { margin: 0; padding: 0 0 0.5em 0; }
#logmsg dd:before { content:'\00bb';}
#logmsg table { border-spacing: 0px; border-collapse: collapse; border-top: 4px solid #fa0; border-bottom: 1px solid #fa0; background: #fff; }
#logmsg table th { text-align: left; font-weight: normal; padding: 0.2em 0.5em; border-top: 1px dotted #fa0; }
#logmsg table td { text-align: right; border-top: 1px dotted #fa0; padding: 0.2em 0.5em; }
#logmsg table thead th { text-align: center; border-bottom: 1px solid #fa0; }
#logmsg table th.Corner { text-align: left; }
#logmsg hr { border: none 0; border-top: 2px dashed #fa0; height: 1px; }
#header, #footer { color: #fff; background: #636; border: 1px #300 solid; padding: 6px; }
#patch { width: 100%; }
#patch h4 {font-family: verdana,arial,helvetica,sans-serif;font-size:10pt;padding:8px;background:#369;color:#fff;margin:0;}
#patch .propset h4, #patch .binary h4 {margin:0;}
#patch pre {padding:0;line-height:1.2em;margin:0;}
#patch .diff {width:100%;background:#eee;padding: 0 0 10px 0;overflow:auto;}
#patch .propset .diff, #patch .binary .diff {padding:10px 0;}
#patch span {display:block;padding:0 10px;}
#patch .modfile, #patch .addfile, #patch .delfile, #patch .propset, #patch .binary, #patch .copfile {border:1px solid #ccc;margin:10px 0;}
#patch ins {background:#dfd;text-decoration:none;display:block;padding:0 10px;}
#patch del {background:#fdd;text-decoration:none;display:block;padding:0 10px;}
#patch .lines, .info {color:#888;background:#fff;}
--></style>
<div id="msg">
<dl class="meta">
<dt>Revision</dt> <dd><a href="http://trac.webkit.org/projects/webkit/changeset/174788">174788</a></dd>
<dt>Author</dt> <dd>timothy_horton@apple.com</dd>
<dt>Date</dt> <dd>2014-10-16 12:53:12 -0700 (Thu, 16 Oct 2014)</dd>
</dl>
<h3>Log Message</h3>
<pre>Various crashes in ViewGestureControllerIOS when closing a tab while a swipe gesture is in progress
https://bugs.webkit.org/show_bug.cgi?id=137770
<rdar://problem/17916459>
Reviewed by Dan Bernstein.
When tearing down a WKWebView in the middle of a swipe gesture, a variety of
UI process crashes were observed. First, two uses of potentially deleted objects
(the WebBackForwardListItem and ViewGestureController), which were fixed by
extending the object's lifetime and checking for its liveness, respectively.
Second, a potential null-deref of DrawingArea if the timing of endSwipeGesture
vs. page teardown was such that DrawingArea was null but everything else was in line.
Lastly, another case of messaging a potentially deleted object (specifically,
the _UIViewControllerTransitionContext's animator) in a callback from CA, which
was fixed by nulling out the animator (and a few other properties) when tearing
down the ViewGestureController.
* UIProcess/ios/ViewGestureControllerIOS.mm:
(-[WKSwipeTransitionController invalidate]):
Clear the soon-to-be-invalid ViewGestureController pointer.
(WebKit::ViewGestureController::~ViewGestureController):
Call [WKSwipeTransitionController invalidate] upon destruction.
Clear our transition context's interactor and animator, and inform it that
the transition is not in flight. This avoids a crash when calling back
to the already-destroyed animator later.
(WebKit::ViewGestureController::beginSwipeGesture):
Keep a reference to the target WebBackForwardListItem; this avoids
it being deleted between here and the transition completion block firing.
Look up the ViewGestureController by pageID, just like we do in endSwipeGesture,
to avoid situations where the callback fires after the WKWebView/ViewGestureController
have gone away.
Hold on to our _UIViewControllerOneToOneTransitionContext, so that we can do the
aforementioned clearing upon deallocation.
(WebKit::ViewGestureController::endSwipeGesture):
Null check the DrawingArea. If it is null, instead of doing our normal delayed logic
for swipe snapshot teardown, just put things back together immediately.
(WebKit::ViewGestureController::removeSwipeSnapshot):
Clear m_swipeTransitionContext.</pre>
<h3>Modified Paths</h3>
<ul>
<li><a href="#trunkSourceWebKit2ChangeLog">trunk/Source/WebKit2/ChangeLog</a></li>
<li><a href="#trunkSourceWebKit2UIProcessiosViewGestureControllerIOSmm">trunk/Source/WebKit2/UIProcess/ios/ViewGestureControllerIOS.mm</a></li>
<li><a href="#trunkSourceWebKit2UIProcessmacViewGestureControllerh">trunk/Source/WebKit2/UIProcess/mac/ViewGestureController.h</a></li>
</ul>
</div>
<div id="patch">
<h3>Diff</h3>
<a id="trunkSourceWebKit2ChangeLog"></a>
<div class="modfile"><h4>Modified: trunk/Source/WebKit2/ChangeLog (174787 => 174788)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/WebKit2/ChangeLog 2014-10-16 19:45:31 UTC (rev 174787)
+++ trunk/Source/WebKit2/ChangeLog 2014-10-16 19:53:12 UTC (rev 174788)
</span><span class="lines">@@ -1,3 +1,50 @@
</span><ins>+2014-10-16 Tim Horton <timothy_horton@apple.com>
+
+ Various crashes in ViewGestureControllerIOS when closing a tab while a swipe gesture is in progress
+ https://bugs.webkit.org/show_bug.cgi?id=137770
+ <rdar://problem/17916459>
+
+ Reviewed by Dan Bernstein.
+
+ When tearing down a WKWebView in the middle of a swipe gesture, a variety of
+ UI process crashes were observed. First, two uses of potentially deleted objects
+ (the WebBackForwardListItem and ViewGestureController), which were fixed by
+ extending the object's lifetime and checking for its liveness, respectively.
+ Second, a potential null-deref of DrawingArea if the timing of endSwipeGesture
+ vs. page teardown was such that DrawingArea was null but everything else was in line.
+ Lastly, another case of messaging a potentially deleted object (specifically,
+ the _UIViewControllerTransitionContext's animator) in a callback from CA, which
+ was fixed by nulling out the animator (and a few other properties) when tearing
+ down the ViewGestureController.
+
+ * UIProcess/ios/ViewGestureControllerIOS.mm:
+ (-[WKSwipeTransitionController invalidate]):
+ Clear the soon-to-be-invalid ViewGestureController pointer.
+
+ (WebKit::ViewGestureController::~ViewGestureController):
+ Call [WKSwipeTransitionController invalidate] upon destruction.
+ Clear our transition context's interactor and animator, and inform it that
+ the transition is not in flight. This avoids a crash when calling back
+ to the already-destroyed animator later.
+
+ (WebKit::ViewGestureController::beginSwipeGesture):
+ Keep a reference to the target WebBackForwardListItem; this avoids
+ it being deleted between here and the transition completion block firing.
+
+ Look up the ViewGestureController by pageID, just like we do in endSwipeGesture,
+ to avoid situations where the callback fires after the WKWebView/ViewGestureController
+ have gone away.
+
+ Hold on to our _UIViewControllerOneToOneTransitionContext, so that we can do the
+ aforementioned clearing upon deallocation.
+
+ (WebKit::ViewGestureController::endSwipeGesture):
+ Null check the DrawingArea. If it is null, instead of doing our normal delayed logic
+ for swipe snapshot teardown, just put things back together immediately.
+
+ (WebKit::ViewGestureController::removeSwipeSnapshot):
+ Clear m_swipeTransitionContext.
+
</ins><span class="cx"> 2014-10-16 Antti Koivisto <antti@apple.com>
</span><span class="cx">
</span><span class="cx"> REGRESSION (r173356): Downloading a disk image appends ".txt" to it
</span></span></pre></div>
<a id="trunkSourceWebKit2UIProcessiosViewGestureControllerIOSmm"></a>
<div class="modfile"><h4>Modified: trunk/Source/WebKit2/UIProcess/ios/ViewGestureControllerIOS.mm (174787 => 174788)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/WebKit2/UIProcess/ios/ViewGestureControllerIOS.mm 2014-10-16 19:45:31 UTC (rev 174787)
+++ trunk/Source/WebKit2/UIProcess/ios/ViewGestureControllerIOS.mm 2014-10-16 19:53:12 UTC (rev 174788)
</span><span class="lines">@@ -51,6 +51,7 @@
</span><span class="cx">
</span><span class="cx"> @interface WKSwipeTransitionController : NSObject <_UINavigationInteractiveTransitionBaseDelegate>
</span><span class="cx"> - (instancetype)initWithViewGestureController:(WebKit::ViewGestureController*)gestureController gestureRecognizerView:(UIView *)gestureRecognizerView;
</span><ins>+- (void)invalidate;
</ins><span class="cx"> @end
</span><span class="cx">
</span><span class="cx"> @interface _UIViewControllerTransitionContext (WKDetails)
</span><span class="lines">@@ -90,6 +91,11 @@
</span><span class="cx"> return self;
</span><span class="cx"> }
</span><span class="cx">
</span><ins>+- (void)invalidate
+{
+ _gestureController = nullptr;
+}
+
</ins><span class="cx"> - (WebKit::ViewGestureController::SwipeDirection)directionForTransition:(_UINavigationInteractiveTransitionBase *)transition
</span><span class="cx"> {
</span><span class="cx"> return transition == _backTransitionController ? WebKit::ViewGestureController::SwipeDirection::Left : WebKit::ViewGestureController::SwipeDirection::Right;
</span><span class="lines">@@ -145,6 +151,10 @@
</span><span class="cx">
</span><span class="cx"> ViewGestureController::~ViewGestureController()
</span><span class="cx"> {
</span><ins>+ [m_swipeTransitionContext _setTransitionIsInFlight:NO];
+ [m_swipeTransitionContext _setInteractor:nil];
+ [m_swipeTransitionContext _setAnimator:nil];
+ [m_swipeInteractiveTransitionDelegate invalidate];
</ins><span class="cx"> viewGestureControllersForAllPages().remove(m_webPageProxy.pageID());
</span><span class="cx"> }
</span><span class="cx">
</span><span class="lines">@@ -177,7 +187,7 @@
</span><span class="cx"> if (m_webPageProxyForBackForwardListForCurrentSwipe != &m_webPageProxy)
</span><span class="cx"> backForwardList.currentItem()->setSnapshot(m_webPageProxy.backForwardList().currentItem()->snapshot());
</span><span class="cx">
</span><del>- WebBackForwardListItem* targetItem = direction == SwipeDirection::Left ? backForwardList.backItem() : backForwardList.forwardItem();
</del><ins>+ RefPtr<WebBackForwardListItem> targetItem = direction == SwipeDirection::Left ? backForwardList.backItem() : backForwardList.forwardItem();
</ins><span class="cx">
</span><span class="cx"> CGRect liveSwipeViewFrame = [m_liveSwipeView frame];
</span><span class="cx">
</span><span class="lines">@@ -215,26 +225,31 @@
</span><span class="cx"> UINavigationControllerOperation transitionOperation = direction == SwipeDirection::Left ? UINavigationControllerOperationPop : UINavigationControllerOperationPush;
</span><span class="cx"> RetainPtr<_UINavigationParallaxTransition> animationController = adoptNS([[_UINavigationParallaxTransition alloc] initWithCurrentOperation:transitionOperation]);
</span><span class="cx">
</span><del>- RetainPtr<_UIViewControllerOneToOneTransitionContext> transitionContext = adoptNS([[_UIViewControllerOneToOneTransitionContext alloc] init]);
- [transitionContext _setFromViewController:targettedViewController.get()];
- [transitionContext _setToViewController:snapshotViewController.get()];
- [transitionContext _setContainerView:m_transitionContainerView.get()];
- [transitionContext _setFromStartFrame:liveSwipeViewFrame];
- [transitionContext _setToEndFrame:liveSwipeViewFrame];
- [transitionContext _setToStartFrame:CGRectZero];
- [transitionContext _setFromEndFrame:CGRectZero];
- [transitionContext _setAnimator:animationController.get()];
- [transitionContext _setInteractor:transition];
- [transitionContext _setTransitionIsInFlight:YES];
- [transitionContext _setInteractiveUpdateHandler:^(BOOL finish, CGFloat percent, BOOL transitionCompleted, _UIViewControllerTransitionContext *) {
</del><ins>+ m_swipeTransitionContext = adoptNS([[_UIViewControllerOneToOneTransitionContext alloc] init]);
+ [m_swipeTransitionContext _setFromViewController:targettedViewController.get()];
+ [m_swipeTransitionContext _setToViewController:snapshotViewController.get()];
+ [m_swipeTransitionContext _setContainerView:m_transitionContainerView.get()];
+ [m_swipeTransitionContext _setFromStartFrame:liveSwipeViewFrame];
+ [m_swipeTransitionContext _setToEndFrame:liveSwipeViewFrame];
+ [m_swipeTransitionContext _setToStartFrame:CGRectZero];
+ [m_swipeTransitionContext _setFromEndFrame:CGRectZero];
+ [m_swipeTransitionContext _setAnimator:animationController.get()];
+ [m_swipeTransitionContext _setInteractor:transition];
+ [m_swipeTransitionContext _setTransitionIsInFlight:YES];
+ [m_swipeTransitionContext _setInteractiveUpdateHandler:^(BOOL finish, CGFloat percent, BOOL transitionCompleted, _UIViewControllerTransitionContext *) {
</ins><span class="cx"> if (finish)
</span><span class="cx"> m_webPageProxyForBackForwardListForCurrentSwipe->navigationGestureWillEnd(transitionCompleted, *targetItem);
</span><span class="cx"> }];
</span><del>- [transitionContext _setCompletionHandler:^(_UIViewControllerTransitionContext *context, BOOL didComplete) { endSwipeGesture(targetItem, context, !didComplete); }];
- [transitionContext _setInteractiveUpdateHandler:^(BOOL, CGFloat, BOOL, _UIViewControllerTransitionContext *) { }];
</del><ins>+ uint64_t pageID = m_webPageProxy.pageID();
+ [m_swipeTransitionContext _setCompletionHandler:^(_UIViewControllerTransitionContext *context, BOOL didComplete) {
+ auto gestureControllerIter = viewGestureControllersForAllPages().find(pageID);
+ if (gestureControllerIter != viewGestureControllersForAllPages().end())
+ gestureControllerIter->value->endSwipeGesture(targetItem.get(), context, !didComplete);
+ }];
+ [m_swipeTransitionContext _setInteractiveUpdateHandler:^(BOOL, CGFloat, BOOL, _UIViewControllerTransitionContext *) { }];
</ins><span class="cx">
</span><span class="cx"> [transition setAnimationController:animationController.get()];
</span><del>- [transition startInteractiveTransition:transitionContext.get()];
</del><ins>+ [transition startInteractiveTransition:m_swipeTransitionContext.get()];
</ins><span class="cx">
</span><span class="cx"> m_activeGestureType = ViewGestureType::Swipe;
</span><span class="cx"> }
</span><span class="lines">@@ -275,12 +290,17 @@
</span><span class="cx"> m_webPageProxyForBackForwardListForCurrentSwipe->navigationGestureDidEnd(true, *targetItem);
</span><span class="cx"> m_webPageProxyForBackForwardListForCurrentSwipe->goToBackForwardItem(targetItem);
</span><span class="cx">
</span><del>- uint64_t pageID = m_webPageProxy.pageID();
- m_webPageProxy.drawingArea()->dispatchAfterEnsuringDrawing([pageID] (CallbackBase::Error error) {
- auto gestureControllerIter = viewGestureControllersForAllPages().find(pageID);
- if (gestureControllerIter != viewGestureControllersForAllPages().end())
- gestureControllerIter->value->willCommitPostSwipeTransitionLayerTree(error == CallbackBase::Error::None);
- });
</del><ins>+ if (auto drawingArea = m_webPageProxy.drawingArea()) {
+ uint64_t pageID = m_webPageProxy.pageID();
+ drawingArea->dispatchAfterEnsuringDrawing([pageID] (CallbackBase::Error error) {
+ auto gestureControllerIter = viewGestureControllersForAllPages().find(pageID);
+ if (gestureControllerIter != viewGestureControllersForAllPages().end())
+ gestureControllerIter->value->willCommitPostSwipeTransitionLayerTree(error == CallbackBase::Error::None);
+ });
+ } else {
+ removeSwipeSnapshot();
+ return;
+ }
</ins><span class="cx">
</span><span class="cx"> m_swipeWatchdogTimer.startOneShot(swipeSnapshotRemovalWatchdogDuration.count());
</span><span class="cx"> }
</span><span class="lines">@@ -338,6 +358,8 @@
</span><span class="cx">
</span><span class="cx"> m_webPageProxyForBackForwardListForCurrentSwipe->navigationGestureSnapshotWasRemoved();
</span><span class="cx"> m_webPageProxyForBackForwardListForCurrentSwipe = nullptr;
</span><ins>+
+ m_swipeTransitionContext = nullptr;
</ins><span class="cx"> }
</span><span class="cx">
</span><span class="cx"> } // namespace WebKit
</span></span></pre></div>
<a id="trunkSourceWebKit2UIProcessmacViewGestureControllerh"></a>
<div class="modfile"><h4>Modified: trunk/Source/WebKit2/UIProcess/mac/ViewGestureController.h (174787 => 174788)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/WebKit2/UIProcess/mac/ViewGestureController.h 2014-10-16 19:45:31 UTC (rev 174787)
+++ trunk/Source/WebKit2/UIProcess/mac/ViewGestureController.h 2014-10-16 19:53:12 UTC (rev 174788)
</span><span class="lines">@@ -39,8 +39,9 @@
</span><span class="cx"> OBJC_CLASS UIView;
</span><span class="cx"> OBJC_CLASS WKSwipeTransitionController;
</span><span class="cx"> OBJC_CLASS WKWebView;
</span><ins>+OBJC_CLASS _UINavigationInteractiveTransitionBase;
+OBJC_CLASS _UIViewControllerOneToOneTransitionContext;
</ins><span class="cx"> OBJC_CLASS _UIViewControllerTransitionContext;
</span><del>-OBJC_CLASS _UINavigationInteractiveTransitionBase;
</del><span class="cx"> #else
</span><span class="cx"> OBJC_CLASS NSEvent;
</span><span class="cx"> OBJC_CLASS NSView;
</span><span class="lines">@@ -207,6 +208,7 @@
</span><span class="cx"> RetainPtr<UIView> m_snapshotView;
</span><span class="cx"> RetainPtr<UIView> m_transitionContainerView;
</span><span class="cx"> RetainPtr<WKSwipeTransitionController> m_swipeInteractiveTransitionDelegate;
</span><ins>+ RetainPtr<_UIViewControllerOneToOneTransitionContext> m_swipeTransitionContext;
</ins><span class="cx"> uint64_t m_snapshotRemovalTargetRenderTreeSize;
</span><span class="cx"> bool m_shouldRemoveSnapshotWhenTargetRenderTreeSizeHit;
</span><span class="cx"> WeakObjCPtr<WKWebView> m_alternateBackForwardListSourceView;
</span></span></pre>
</div>
</div>
</body>
</html>