The default bug view has changed. See this FAQ.

Negative timeout values should be clamped to 0, not ignored

RESOLVED FIXED in mozilla14

Status

()

Core
Geolocation
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: jdm, Assigned: Veeraya Pupatwibul)

Tracking

Trunk
mozilla14
x86
Mac OS X
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [mentor=jdm][lang=c++])

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

5 years ago
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.
(Assignee)

Comment 1

5 years ago
I couldn't find nsGeolocation::SetTimeoutTimer in nsGeolocation.cpp.
Do you mean nsGeolocationRequest::SetTimeoutTimer?
(Reporter)

Comment 2

5 years ago
Yes, I did. Sorry!
(Assignee)

Comment 3

5 years ago
Created attachment 611459 [details] [diff] [review]
first patch

any comment?:)
Attachment #611459 - Flags: review?(josh)
(Reporter)

Comment 4

5 years ago
Comment on attachment 611459 [details] [diff] [review]
first patch

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

Great; thanks!
Attachment #611459 - Flags: review?(josh) → review+
(Reporter)

Comment 5

5 years ago
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 https://developer.mozilla.org/en/Creating_a_patch_that_can_be_checked_in to make checking your patches in easier?
(Reporter)

Comment 6

5 years ago
http://hg.mozilla.org/integration/mozilla-inbound/rev/0501a5506c87
Assignee: nobody → pookveeraya
Target Milestone: --- → mozilla14
(Assignee)

Comment 7

5 years ago
Okay next time I'll do that! Thanks for the guidance:)
Sorry, I had to back out this change:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bf983daba97d

because it caused some timeouts in automated tests.  Here are some test logs:
https://tbpl.mozilla.org/php/getParsedLog.php?id=10567251&tree=Mozilla-Inbound

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:
https://developer.mozilla.org/en/Mochitest
Target Milestone: mozilla14 → ---
(Assignee)

Comment 9

5 years ago
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!
(Reporter)

Comment 10

5 years ago
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.
(Reporter)

Comment 11

5 years ago
Veeraya, if you upload a patch that addresses comment 10, I'll run it through our testing server.
(Assignee)

Comment 12

5 years ago
Created attachment 614719 [details] [diff] [review]
patch

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!:)
Attachment #611459 - Attachment is obsolete: true
Attachment #614719 - Flags: feedback?(josh)
(Reporter)

Updated

5 years ago
Attachment #614719 - Flags: feedback?(josh) → feedback+
(Reporter)

Updated

5 years ago
Whiteboard: [mentor=jdm][lang=c++] → [mentor=jdm][lang=c++][autoland-try: -b do -p all -u mochitests -t none]
(Reporter)

Updated

5 years ago
Whiteboard: [mentor=jdm][lang=c++][autoland-try: -b do -p all -u mochitests -t none] → [mentor=jdm][lang=c++][autoland-try:-b do -p all -u mochitests -t none]

Updated

5 years ago
Whiteboard: [mentor=jdm][lang=c++][autoland-try:-b do -p all -u mochitests -t none] → [mentor=jdm][lang=c++][autoland-in-queue]

Comment 13

5 years ago
Autoland Patchset:
	Patches: 614719
	Branch: mozilla-central => try
Insufficient permissions to push to try.

Updated

5 years ago
Whiteboard: [mentor=jdm][lang=c++][autoland-in-queue] → [mentor=jdm][lang=c++]
(Reporter)

Updated

5 years ago
Attachment #614719 - Flags: review+
(Reporter)

Updated

5 years ago
Whiteboard: [mentor=jdm][lang=c++] → [mentor=jdm][lang=c++][autoland-try:-b do -p all -u mochitests -t none]

Updated

5 years ago
Whiteboard: [mentor=jdm][lang=c++][autoland-try:-b do -p all -u mochitests -t none] → [mentor=jdm][lang=c++][autoland-in-queue]

Comment 14

5 years ago
Autoland Patchset:
	Patches: 614719
	Branch: mozilla-central => try
	Destination: http://hg.mozilla.org/try/pushloghtml?changeset=4d6bb322476e
Try run started, revision 4d6bb322476e. To cancel or monitor the job, see: https://tbpl.mozilla.org/?tree=Try&rev=4d6bb322476e

Comment 15

5 years ago
Try run for 4d6bb322476e is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=4d6bb322476e
Results (out of 121 total builds):
    exception: 1
    success: 107
    warnings: 13
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/autolanduser@mozilla.com-4d6bb322476e

Updated

5 years ago
Whiteboard: [mentor=jdm][lang=c++][autoland-in-queue] → [mentor=jdm][lang=c++]
(Reporter)

Comment 16

5 years ago
http://hg.mozilla.org/integration/mozilla-inbound/rev/d50bc535870c
Target Milestone: --- → mozilla14
https://hg.mozilla.org/mozilla-central/rev/d50bc535870c
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.