[GeoLocation] invalidate timer after success or error callback

[henter]

Just trying to getCurrentPosition , and found the errorBlock of location request in timeout handler would cause red error like this:

2015-05-10 17:50:39.607 [warn][tid:com.facebook.React.JavaScript] "Warning: Cannot find callback with CBID 5. Native module may have invoked both the success callback and the error callback."
2015-05-10 17:50:39.610 [error][tid:com.facebook.React.JavaScript] "Error: null is not an object (evaluating 'cb.apply')
 stack: 
  _invokeCallback  index.ios.bundle:7593
  <unknown>        index.ios.bundle:7656
  <unknown>        index.ios.bundle:7648
  perform          index.ios.bundle:6157
  batchedUpdates   index.ios.bundle:13786
  batchedUpdates   index.ios.bundle:4689
  <unknown>        index.ios.bundle:7647
  applyWithGuard   index.ios.bundle:882
  guardReturn      index.ios.bundle:7421
  processBatch     index.ios.bundle:7646
 URL: http://192.168.100.182:8081/index.ios.bundle
 line: 7593
 message: null is not an object (evaluating 'cb.apply')"

I'm not sure if there is a better way to avoid multiple callback which caused the error,
but I don't think that the errorBlock should be called in timeout handler.
So, just remove it.

update: to invalidate the timer after success or error callback.

I just use the demo code of Geolocation

[ide]

This leaks memory on the JS side if the request times out and will also cause the JS app to hang forever if the callback isn't invoked. Better to cancel the timer if the location request invokes either its success or error callback.

[henter]

@ide yeah, that's a great idea.
code updated. :-)

[ide]

@henter this looks good but can you please verify that the NSTimer is scheduled and invalidated on the same run loop (check with https://developer.apple.com/library/mac/documentation/CoreFoundation/Reference/CFRunLoopRef/index.html#//apple_ref/c/func/CFRunLoopGetCurrent)?

Apple says:

you should always call the invalidate method from the same thread on which the timer was installed

In -[RCTLocationRequest dealloc] you should check _timeoutTimer.valid since I believe it will crash the app if a timer is invalidated twice.

Also please squash your commits (git rebase -i) into one commit so that it's easier to review and bisect.

Otherwise, this PR looks good to me.

[henter]

@ide thanks. code updated. :-)

[ide]

Slightly prefer _timeoutTimer.valid since it's documented that way here: https://developer.apple.com/library/mac/documentation/Cocoa/Reference/Foundation/Classes/NSTimer_Class/index.html#//apple_ref/occ/instp/NSTimer/valid

[ide]

Left one small comment if you want to update it but will mark this as accepted.

[henter]

updated.

[ide]

@vjeux if I say:
@facebook-github-bot import
will it work? ;)

[vjeux]

You need it in its own line. It actually may work with you since you have a fb employee vc

[ide]

@facebook-github-bot import

[reconbot]

Looking forward to see this land. 👍

[vjeux]

nit: space after if and space after (

[henter]

code updated. @ide @vjeux

[ide]

Thanks @henter. This looks fine to me.

[vjeux]

@facebook-github-bot import

[facebook-github-bot]

Thanks for importing. If you are an FB employee go to https://our.intern.facebook.com/intern/opensource/github/pull_request/1402019833455864/int_phab to review.

[sahrens]

The new invalidate calls shouldn't be necessary because this should be invalidating the timers because all the requests should be dealloc'd at this point. If that's not happening, there is probably a retain cycle or something? cc @nicklockwood

[sahrens]

Did you have a repro case that failed 100% without this PR and 0% with it? I'm afraid there might be something else going wrong.

[sahrens]

ping @ide, @henter on my comment - am I missing something?

[ide]

@sahrens My view on this is: covering up the retain cycle (if there is one) is bad and we should fix that. I do think the explicit -[invalidate] calls clarify the intent here, since we want to stop the timer whether or not the request is deallocated.

I haven't profiled the code for leaks but it looks like there is a cycle between request.timer and timer.userInfo (which is the request object). A nice thing about calling -[NSTimer invalidate] is that it clears the strong references to the timer's target and userInfo properties, which should break the cycle.

[sahrens]

Cool, thanks - I'll bring this in.

On May 22, 2015, at 4:52 PM, James Ide [email protected] wrote:

@sahrens My view on this is: covering up the retain cycle (if there is one) is bad and we should fix that. I do think the explicit -[invalidate] calls clarify the intent here, since we want to stop the timer whether or not the request is deallocated.

I haven't profiled the code for leaks but it looks like there is a cycle between request.timer and timer.userInfo (which is the request object). A nice thing about calling -[NSTimer invalidate] is that it clears the strong references to the timer's target and userInfo properties, which should break the cycle.

—
Reply to this email directly or view it on GitHub.

[justinhj]

I've updated to the latest publicly available React native, but this crash still happens using the demo code provided.

[aphillipo]

Still seems to be broken for me too. Did this make it into the release, should I use beta/github react-native from npm?