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

Status: RESOLVED FIXED

(Core :: DOM: Geolocation, defect)

Description

[Andrew McCreight [:mccr8]]

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.

Comment 1

[Josh Matthews [:jdm]]

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 :)

Comment 2

[Josh Matthews [:jdm]]

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`

Comment 3

[Josh Matthews [:jdm]]

Oh, and finally:
* check whether the timeout value equals 0xFFFFFFFF and avoid setting a timer if that's the case

Comment 4

[Brin Manoogian-O'Dell]

May I be assigned to this bug? I would like to work on it.

Comment 5

[Ma8sa]

Hi, I am new to fixing bugs and I would like to work on the bug can u help in setting the environment

Comment 6

[Josh Matthews [:jdm]]

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).

Comment 7

[Josh Matthews [:jdm]]

Any progress so far?

Comment 8

[Brin Manoogian-O'Dell]

Yes, I had some trouble with the development environment, but I should have a patch ready very soon. My apologies for the late response!

Comment 9

[Brin Manoogian-O'Dell]

Attachment: Updates PositionOptions to meet new specifications

Comment 10

[Josh Matthews [:jdm]]

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?

Comment 11

[Brin Manoogian-O'Dell]

Attachment: Bug 1238801: Update to PositionLocation in the GeoLocation webidl.

My apologies. I hope that this is what is expected.

Comment 12

[Josh Matthews [:jdm]]

That patch is empty, unfortunately. You can see this by clicking on the [details] link.

Comment 13

[Brin Manoogian-O'Dell]

Attachment: Patch to update PositionOptions in Geolocation webidl to more current specifications.

Comment 14

[Josh Matthews [:jdm]]

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?

Comment 15

[Brin Manoogian-O'Dell]

Yes, .mach build still completes successfully.

Comment 16

[Andrew McCreight [:mccr8]]

Josh, it looks like this is ready to be landed or given a try run or something.

Comment 17

[Josh Matthews [:jdm]]

Brin, were you planning to implement the remaining pieces from comment 2?

Comment 18

[Jinank Jain[:jj]]

Attachment: patch.diff

Comment 19

[Josh Matthews [:jdm]]

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.

Comment 20

[Jinank Jain[:jj]]

: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.

Comment 21

[Jinank Jain[:jj]]

Attachment: patch.diff

Comment 22

[Josh Matthews [:jdm]]

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.

Comment 23

[Jinank Jain[:jj]]

: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.

Comment 24

[Josh Matthews [:jdm]]

I recommend creating a page where you know precisely what JS it's using; google maps is probably using a real timeout value :)

Comment 25

[Jinank Jain[:jj]]

yeah I checked out that it is set 0x7fffffff, so now we could remove setting it

Comment 26

[Jinank Jain[:jj]]

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]

Comment 27

[Josh Matthews [: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?

Comment 28

[Jinank Jain[:jj]]

I was asking that where we set the value for timeout.

Comment 29

[Josh Matthews [:jdm]]

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.

Comment 30

[Jinank Jain[:jj]]

But by removing timeout assignment from Geolocation.webidl gives compilation error so we cannot remove it from there

Comment 31

[Josh Matthews [:jdm]]

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.

Comment 32

[Jinank Jain[:jj]]

Attachment: patch.diff

Comment 33

[Josh Matthews [:jdm]]

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.

Comment 34

[Jinank Jain[:jj]]

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

Comment 35

[Jinank Jain[:jj]]

Attachment: patch.diff

Comment 36

[Josh Matthews [:jdm]]

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/ .

Comment 37

[Jinank Jain[:jj]]

I had access to it before but I kind of forgot the password as I had accessed it a year later.

Comment 38

[Jinank Jain[:jj]]

Do you have any idea about how change my ssh publicid so that I can again get access to try server?

Comment 39

[Josh Matthews [:jdm]]

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.

Comment 40

[Jinank Jain[:jj]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=04b1e7fbf58f

https://hg.mozilla.org/try/rev/04b1e7fbf58f

Comment 41

[Jinank Jain[:jj]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e449fd25ff58

Comment 42

[Jinank Jain[:jj]]

I had pushed that on try server and ran test against all the version

Comment 43

[Josh Matthews [:jdm]]

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.

Comment 44

[Jinank Jain[:jj]]

Attachment: patch.diff

Comment 45

[Jinank Jain[:jj]]

Done!!

Comment 46

[Josh Matthews [:jdm]]

Like I said, add checkin-needed to Keywords :)

Comment 47

[Carsten Book [:Tomcat]]

need dom peer review:

dom/webidl/Geolocation.webidl altered in changeset 5ab4ac053870 without DOM peer review

Comment 48

[Olli Pettay [:smaug][[e-mail]]]

r=smaug for the .webidl change, since it seems to follow the spec.

Comment 49

[Josh Matthews [:jdm]]

Jinank, could you attach a patch with r=jdm,smaug instead of r=jdm?

Comment 50

[Jinank Jain[:jj]]

Attachment: patch.diff

Comment 51

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e1b1f1362a6c
Don't set a timer for the default geolocation timeout. r=jdm, r=smaug

Comment 52

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e1b1f1362a6c