Bug 154347 - [iOS WK1] NSTimer in WebGeolocationProviderIOS.mm fires on the main thread without the WebThreadLock
Summary: [iOS WK1] NSTimer in WebGeolocationProviderIOS.mm fires on the main thread wi...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: Other
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: David Kilzer (:ddkilzer)
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2016-02-17 11:22 PST by David Kilzer (:ddkilzer)
Modified: 2016-02-17 14:07 PST (History)
5 users (show)

See Also:


Attachments
Patch v1 (2.86 KB, patch)
2016-02-17 11:31 PST, David Kilzer (:ddkilzer)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description David Kilzer (:ddkilzer) 2016-02-17 11:22:28 PST
The NSTimer in WebGeolocationProviderIOS.mm fires on the main thread without the WebThreadLock.  This causes a fairly rare crash (usually in random WebCore rending code) if the JavaScript that runs after the timer fires causes layout on the main thread.

The fix is simple; fire the NSTimer on the WebThread where it's safe to do the work.
Comment 1 David Kilzer (:ddkilzer) 2016-02-17 11:22:46 PST
<rdar://problem/19360382>
Comment 2 David Kilzer (:ddkilzer) 2016-02-17 11:31:44 PST
Created attachment 271570 [details]
Patch v1
Comment 3 Andy Estes 2016-02-17 11:48:46 PST
Comment on attachment 271570 [details]
Patch v1

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

> Source/WebKit/ios/Misc/WebGeolocationProviderIOS.mm:201
> +        _sendLastPositionAsynchronouslyTimer = [NSTimer timerWithTimeInterval:0 target:self selector:@selector(_handlePendingInitialPosition:) userInfo:nil repeats:NO];

Nit: you could avoid some refcount churn by using NSTimer's designated initializer instead:

_sendLastPositionAsynchronouslyTimer = adoptNS([[NSTimer alloc] initWithFireDate:[NSDate now] ...]);
Comment 4 WebKit Commit Bot 2016-02-17 12:40:26 PST
Comment on attachment 271570 [details]
Patch v1

Clearing flags on attachment: 271570

Committed r196714: <http://trac.webkit.org/changeset/196714>
Comment 5 WebKit Commit Bot 2016-02-17 12:40:30 PST
All reviewed patches have been landed.  Closing bug.
Comment 6 David Kilzer (:ddkilzer) 2016-02-17 12:51:49 PST
(In reply to comment #3)
> Comment on attachment 271570 [details]
> Patch v1
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=271570&action=review
> 
> > Source/WebKit/ios/Misc/WebGeolocationProviderIOS.mm:201
> > +        _sendLastPositionAsynchronouslyTimer = [NSTimer timerWithTimeInterval:0 target:self selector:@selector(_handlePendingInitialPosition:) userInfo:nil repeats:NO];
> 
> Nit: you could avoid some refcount churn by using NSTimer's designated
> initializer instead:
> 
> _sendLastPositionAsynchronouslyTimer = adoptNS([[NSTimer alloc]
> initWithFireDate:[NSDate now] ...]);

Good point!  Won't there be less editing when we enable ARC, though?  :)
Comment 7 Andy Estes 2016-02-17 14:07:01 PST
(In reply to comment #6)
> (In reply to comment #3)
> > Comment on attachment 271570 [details]
> > Patch v1
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=271570&action=review
> > 
> > > Source/WebKit/ios/Misc/WebGeolocationProviderIOS.mm:201
> > > +        _sendLastPositionAsynchronouslyTimer = [NSTimer timerWithTimeInterval:0 target:self selector:@selector(_handlePendingInitialPosition:) userInfo:nil repeats:NO];
> > 
> > Nit: you could avoid some refcount churn by using NSTimer's designated
> > initializer instead:
> > 
> > _sendLastPositionAsynchronouslyTimer = adoptNS([[NSTimer alloc]
> > initWithFireDate:[NSDate now] ...]);
> 
> Good point!  Won't there be less editing when we enable ARC, though?  :)

RetainPtr is already compatible with ARC code (it just ifdefs out ), so either approach would work in ARC without modification (I'm sure we'd eventually remove the RetainPtrs, though).