Bug 137770 – Various crashes in ViewGestureControllerIOS when closing a tab while a swipe gesture is in progress

• Component: WebKit2
• Status: RESOLVED FIXED

[Tim Horton]

A multitude of crashes are possible here:

1. We can attempt to access a deleted ViewGestureController from inside a CA callback.
2. We can attempt to access a deleted WebBackForwardListItem from inside a CA callback.
3. We can attempt to dereference a null DrawingArea from a page that's on its way down.
4. UIKit can become fairly unhappy with dangling '_UINavigationInteractiveTransitionBase's.

Let's fix these things.

<rdar://problem/17916459>

[Tim Horton]

Created attachment 239935 [details]
patch

[WebKit Commit Bot]

Attachment 239935 [details] did not pass style-queue:


ERROR: Source/WebKit2/UIProcess/ios/ViewGestureControllerIOS.mm:241:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebKit2/UIProcess/ios/ViewGestureControllerIOS.mm:246:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebKit2/UIProcess/mac/ViewGestureController.h:43:  _UIViewControllerOneToOneTransitionContext is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebKit2/UIProcess/mac/ViewGestureController.h:44:  _UIViewControllerTransitionContext is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 4 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 239935 [details]
patch

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

> Source/WebKit2/UIProcess/ios/ViewGestureControllerIOS.mm:97
> +    _backTransitionController = nullptr;
> +    _forwardTransitionController = nullptr;

Isn’t nil more appropriate for these, since the underlying type is an id?

I’m not convinced that we should clear these. Doing so doesn’t really guarantee anything except that if -directionForTransition: were to be called it would return the wrong answer.

[Simon Fraser (smfr)]

Comment on attachment 239935 [details]
patch

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

> Source/WebKit2/ChangeLog:9
> +        Reviewed by NOBODY (OOPS!).
> +
> +        * UIProcess/ios/ViewGestureControllerIOS.mm:

I would like to read a summary of the issue and how you fixed it here.

[Tim Horton]

(In reply to comment #3)
> Comment on attachment 239935 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=239935&action=review
> 
> > Source/WebKit2/UIProcess/ios/ViewGestureControllerIOS.mm:97
> > +    _backTransitionController = nullptr;
> > +    _forwardTransitionController = nullptr;
> 
> Isn’t nil more appropriate for these, since the underlying type is an id?
> 
> I’m not convinced that we should clear these. Doing so doesn’t really
> guarantee anything except that if -directionForTransition: were to be called
> it would return the wrong answer.

A valid point. This wasn't the key to the crash in the end, so I guess there's no point. We agree that the ViewGestureController pointer should be cleared, though?

(In reply to comment #4)
> Comment on attachment 239935 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=239935&action=review
> 
> > Source/WebKit2/ChangeLog:9
> > +        Reviewed by NOBODY (OOPS!).
> > +
> > +        * UIProcess/ios/ViewGestureControllerIOS.mm:
> 
> I would like to read a summary of the issue and how you fixed it here.

There are four issues and four fixes, they're detailed individually below :D "this avoids"x3 + "null-check". But I will add a summary of some sort.

[Tim Horton]

http://trac.webkit.org/changeset/174788