Closed Bug 1238801 Opened 4 years ago Closed 3 years ago

Don't bother setting a timer that will never fire in nsGeolocationRequest::SetTimeoutTimer()

Categories

(Core :: DOM: Geolocation, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox46 --- affected
firefox50 --- fixed

People

(Reporter: mccr8, Assigned: jj, Mentored)

Details

(Whiteboard: [lang=c++][good first bug])

Attachments

(1 file, 8 obsolete files)

The default value for PositionOptions.timeout is 0x7fffffff, which according to bug 741131 is a stand-in for "infinity". It seems like that in that case we should just not bother setting a timer. (Technically a very patient user could specify a timeout of 0x7fffffff and then not get the behavior they expected...)

This block of code is also quite odd:
    if (timeout < 0) {
      timeout = 0;
    } else if (timeout < 10) {
      timeout = 10;
    }
But maybe there's some reason for that.
The negative timeout thing is a relic of older geolocation specifications that used neither unsigned nor [Clamp]: http://dev.w3.org/geo/api/spec-source.html#position-options
The <10 thing originates from http://hg.mozilla.org/mozilla-central/rev/52a9f1cfedaa which was committed before I got involved in the project. I doubt that anybody understands it now :)
There are a few self-contained changes to be made here:
* Updating the PositionOptions definition in Geolocation.webidl to match the specification that I linked in comment 1.
* Removing the check for timeout values that are <0 in nsGeolocation.cpp, since the previous step made that impossible
* Removing the timeout <10 check, because I don't see how it serves any meaningful purpose these days.
Then we need to run the geolocation tests and ensure they still pass: `./mach mochitest dom/tests/mochitest/geolocation`
Mentor: josh
Whiteboard: [lang=c++][good first bug]
Oh, and finally:
* check whether the timeout value equals 0xFFFFFFFF and avoid setting a timer if that's the case
May I be assigned to this bug? I would like to work on it.
Flags: needinfo?(josh)
Assignee: nobody → brindmo
Flags: needinfo?(josh)
Hi, I am new to fixing bugs and I would like to work on the bug can u help in setting the environment
Sorry, someone already claimed this issue in comment 4. You can get help with setting up your environment in the #introduction channel on IRC (https://wiki.mozilla.org/IRC).
Any progress so far?
Flags: needinfo?(brindmo)
Yes, I had some trouble with the development environment, but I should have a patch ready very soon. My apologies for the late response!
Flags: needinfo?(brindmo)
Thank you for your contribution! However, Bugzilla is designed to accept and review patches (ie. the changes to a file), rather than complete files. Could you follow the steps at https://developer.mozilla.org/en-US/docs/Mercurial/Using_Mercurial#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F to create a patch and attach it here instead?
Flags: needinfo?(brindmo)
My apologies. I hope that this is what is expected.
Attachment #8718031 - Attachment is obsolete: true
Flags: needinfo?(brindmo)
That patch is empty, unfortunately. You can see this by clicking on the [details] link.
Comment on attachment 8721142 [details] [diff] [review]
Patch to update PositionOptions in Geolocation webidl to more current specifications.

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

This looks great! I assume `./mach build` still completes successfully?
Attachment #8721142 - Flags: review+
Yes, .mach build still completes successfully.
Josh, it looks like this is ready to be landed or given a try run or something.
Flags: needinfo?(josh)
Brin, were you planning to implement the remaining pieces from comment 2?
Flags: needinfo?(josh) → needinfo?(brindmo)
Attached patch patch.diff (obsolete) — Splinter Review
Attachment #8764354 - Flags: review?(josh)
Comment on attachment 8764354 [details] [diff] [review]
patch.diff

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

Thanks for tackling this Jinank! I have a couple comments that should be addressed before we merge these changes.

::: dom/geolocation/nsGeolocation.cpp
@@ +570,1 @@
>    int32_t timeout;

I think we might as well use mOptions->mTimeout directly rather than introducing this extra variable.

@@ +573,1 @@
>      mTimeoutTimer = do_CreateInstance("@mozilla.org/timer;1");

We should still check if the timeout value equals 0x7fffffff and skip setting the timer if that's the case.
Attachment #8764354 - Flags: review?(josh) → feedback+
Assignee: brindmo → jinank94
:jdm I checked the value of timeout variable no it is not 0x7fffffff. It was 10000 when I printed out that variable.

Thanks for noticing about timeout variable. I will soon fix that up too.
Attached patch patch.diff (obsolete) — Splinter Review
Attachment #8764440 - Flags: review?(dougt)
Attachment #8764440 - Flags: feedback?(josh)
Just to be clear - what was the JS that caused that? I would expect any use of geolocation.getPosition() or geolocation.watchPosition() that doesn't include a {timeout: ####} option argument to have the default value of 0x7fffffff.
Flags: needinfo?(brindmo) → needinfo?(jinank94)
Attachment #8764440 - Flags: review?(josh)
Attachment #8764440 - Flags: review?(dougt)
Attachment #8764440 - Flags: feedback?(josh)
:jdm I tried to print mOptions->mTimeout this variable to know about the value of timeout and then I opened up Google Maps and tried to access my location that might have fired that event.
Flags: needinfo?(jinank94) → needinfo?(josh)
I recommend creating a page where you know precisely what JS it's using; google maps is probably using a real timeout value :)
Flags: needinfo?(josh)
yeah I checked out that it is set 0x7fffffff, so now we could remove setting it
Flags: needinfo?(josh)
I just need one more information yesterday I was trying to find out which mOptions gets set I was not able to find that file could you please help me out with that. [:jdm]
I'm not sure exactly what you're asking. mOptions is set in the nsGeolocationRequest constructor, where it comes from Geolocation::GetCurrentPosition and Geolocation::WatchPosition. Does that answer the question?
Flags: needinfo?(josh)
I was asking that where we set the value for timeout.
Flags: needinfo?(josh)
That is set by generated code in GeolocationBinding.cpp (obj-[something]/dom/bindings/GeolocationBinding.cpp) based on the value that is provided by the JavaScript that invokes geolocation.getCurrentPosition or geolocation.watchPosition.
Flags: needinfo?(josh)
But by removing timeout assignment from Geolocation.webidl gives compilation error so we cannot remove it from there
Flags: needinfo?(josh)
I never said we should remove the timeout default value. I said we should check if the provided timeout equals the default value and skip setting the timer, just like we check for a value of 0.
Flags: needinfo?(josh)
Attached patch patch.diff (obsolete) — Splinter Review
Attachment #8764354 - Attachment is obsolete: true
Attachment #8764440 - Attachment is obsolete: true
Attachment #8764440 - Flags: review?(josh)
Attachment #8765195 - Flags: review?(josh)
Comment on attachment 8765195 [details] [diff] [review]
patch.diff

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

::: dom/geolocation/nsGeolocation.cpp
@@ +571,3 @@
>      mTimeoutTimer = do_CreateInstance("@mozilla.org/timer;1");
>      RefPtr<TimerCallbackHolder> holder = new TimerCallbackHolder(this);
> +    if(mOptions->mTimeout != 0x7fffffff)

Let's add this to the conditions in the previous `if` instead.
Attachment #8765195 - Flags: review?(josh)
Attachment #8721142 - Attachment is obsolete: true
That won't work if we add that condition to the previous if statement then we will not create an instance of mTimeoutTimer but which is needed indeed so I think the added condition location is fine
Flags: needinfo?(josh)
Attached patch patch.diff (obsolete) — Splinter Review
Attachment #8765195 - Attachment is obsolete: true
Attachment #8765283 - Flags: review?(josh)
Comment on attachment 8765283 [details] [diff] [review]
patch.diff

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

Thanks for making these changes! This patch needs a real commit message, like "Bug 1238801 - Avoid setting a geolocation timer for the default timeout value. r=jdm", which you can set using `hg qref -m "..."` (replacing the ... with the commit message). We should also make sure this passes tests - do you have tryserver access? (https://wiki.mozilla.org/ReleaseEngineering/TryServer) If not, please apply for it by following the steps at https://www.mozilla.org/hacking/committer/ .
Attachment #8765283 - Flags: review?(josh) → review+
Flags: needinfo?(josh)
I had access to it before but I kind of forgot the password as I had accessed it a year later.
Do you have any idea about how change my ssh publicid so that I can again get access to try server?
Flags: needinfo?(josh)
https://login.mozilla.com/ is where you can update the SSH key you're using. If you can't remember the password to access that page, then please file a bug in "MOC: Service Requests" asking them to reset your LDAP password.
Flags: needinfo?(josh)
I had pushed that on try server and ran test against all the version
Flags: needinfo?(josh)
This looks good to me; sorry about the delay! Please attach a version of the patch with a commit message like "Bug 1238801 - Don't set a timer for the default geolocation timeout. r=jdm", then add the `checkin-needed` flag to the Keywords field in this bug.
Flags: needinfo?(josh)
Attached patch patch.diff (obsolete) — Splinter Review
Attachment #8765283 - Attachment is obsolete: true
Attachment #8770728 - Flags: checkin+
Done!!
Flags: needinfo?(josh)
Like I said, add checkin-needed to Keywords :)
Flags: needinfo?(josh)
Keywords: checkin-needed
need dom peer review:

dom/webidl/Geolocation.webidl altered in changeset 5ab4ac053870 without DOM peer review
Flags: needinfo?(jinank94)
Keywords: checkin-needed
r=smaug for the .webidl change, since it seems to follow the spec.
Jinank, could you attach a patch with r=jdm,smaug instead of r=jdm?
Attached patch patch.diffSplinter Review
Attachment #8770728 - Attachment is obsolete: true
Flags: needinfo?(jinank94)
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e1b1f1362a6c
Don't set a timer for the default geolocation timeout. r=jdm, r=smaug
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/e1b1f1362a6c
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in before you can comment on or make changes to this bug.