Beginning on October 25th, 2016, Persona will no longer be an option for authentication on BMO. For more details see Persona Deprecated.
Last Comment Bug 741131 - Negative timeout values should be clamped to 0, not ignored
: Negative timeout values should be clamped to 0, not ignored
Product: Core
Classification: Components
Component: Geolocation (show other bugs)
: Trunk
: x86 Mac OS X
: -- normal (vote)
: mozilla14
Assigned To: Veeraya Pupatwibul
Depends on:
  Show dependency treegraph
Reported: 2012-03-31 15:48 PDT by Josh Matthews [:jdm]
Modified: 2012-04-14 06:36 PDT (History)
5 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

first patch (865 bytes, patch)
2012-04-02 08:14 PDT, Veeraya Pupatwibul
josh: review+
Details | Diff | Splinter Review
patch (866 bytes, patch)
2012-04-13 03:28 PDT, Veeraya Pupatwibul
josh: review+
josh: feedback+
Details | Diff | Splinter Review

Description Josh Matthews [:jdm] 2012-03-31 15:48:55 PDT
From the spec:

"If a PositionOptions parameter was present, and its timeout attribute was defined to a non-negative value, assign this value to an internal timeout variable. If timeout was defined to a negative value, set the internal timeout variable to 0. If timeout was not specified, set the internal timeout variable to Infinity."

You'll want to update the code in nsGeolocation::SetTimeoutTimer in nsGeolocation.cpp.
Comment 1 Veeraya Pupatwibul 2012-04-02 04:48:14 PDT
I couldn't find nsGeolocation::SetTimeoutTimer in nsGeolocation.cpp.
Do you mean nsGeolocationRequest::SetTimeoutTimer?
Comment 2 Josh Matthews [:jdm] 2012-04-02 07:53:06 PDT
Yes, I did. Sorry!
Comment 3 Veeraya Pupatwibul 2012-04-02 08:14:55 PDT
Created attachment 611459 [details] [diff] [review]
first patch

any comment?:)
Comment 4 Josh Matthews [:jdm] 2012-04-02 08:37:51 PDT
Comment on attachment 611459 [details] [diff] [review]
first patch

Review of attachment 611459 [details] [diff] [review]:

Great; thanks!
Comment 5 Josh Matthews [:jdm] 2012-04-02 08:39:55 PDT
I'll check this patch in with the correct extra information (your name/email, commit message, etc.), but in future could you follow the steps at to make checking your patches in easier?
Comment 7 Veeraya Pupatwibul 2012-04-02 09:08:52 PDT
Okay next time I'll do that! Thanks for the guidance:)
Comment 8 Matt Brubeck (:mbrubeck) 2012-04-02 10:37:04 PDT
Sorry, I had to back out this change:

because it caused some timeouts in automated tests.  Here are some test logs:

The test failures will need to be fixed in order for this patch to land again.  Let us know if you need any help figuring out how to fix the timeouts.  For some information about these tests and how to run them, see:
Comment 9 Veeraya Pupatwibul 2012-04-04 09:10:32 PDT
I need some help figuring how to fix the timeouts. 

It seems like in test_shutdown, timeout is set to 100, so the behavior of the program with and without patch should be the same since the condition timeout < 10 isn't satisfied. I'm not sure why the patch proposed cause the geolocation provider to time out. Could you please give me some hints on how to fix this? Thanks!
Comment 10 Josh Matthews [:jdm] 2012-04-04 11:42:28 PDT
My theory is that NS_SUCCEEDED(mOptions->GetTimeout(&timeout)) succeeds, and timeout contains 0. We previously skipped the timer if this was the case; let's retain an extra check for the if condition of |timeout != 0|, which should leave these tests unaffected.
Comment 11 Josh Matthews [:jdm] 2012-04-12 13:15:21 PDT
Veeraya, if you upload a patch that addresses comment 10, I'll run it through our testing server.
Comment 12 Veeraya Pupatwibul 2012-04-13 03:28:53 PDT
Created attachment 614719 [details] [diff] [review]

Edited according to the comment above. I haven't been able to reproduce the failed tests locally though. Not too sure why. Please help run the patch on the testing server. Thank you!:)
Comment 13 Mozilla RelEng Bot 2012-04-13 08:33:16 PDT
Autoland Patchset:
	Patches: 614719
	Branch: mozilla-central => try
Insufficient permissions to push to try.
Comment 14 Mozilla RelEng Bot 2012-04-13 08:41:28 PDT
Autoland Patchset:
	Patches: 614719
	Branch: mozilla-central => try
Try run started, revision 4d6bb322476e. To cancel or monitor the job, see:
Comment 15 Mozilla RelEng Bot 2012-04-13 15:00:25 PDT
Try run for 4d6bb322476e is complete.
Detailed breakdown of the results available here:
Results (out of 121 total builds):
    exception: 1
    success: 107
    warnings: 13
Builds (or logs if builds failed) available at:
Comment 17 :Ms2ger (⌚ UTC+1/+2) 2012-04-14 06:36:21 PDT

Note You need to log in before you can comment on or make changes to this bug.