[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 notifications@github.com 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?