857330 - Geolocation uses a wrong default value for maximumAge

Status: RESOLVED FIXED

(Core :: DOM: Geolocation, defect)

Description

[Guilherme Gonçalves [:ggp]]

According to the spec, when no options are provided to a call to getCurrentPosition or watchPosition, the default value for maximumAge should be zero. Our implementation, however, uses the value of 30 seconds whenever no options are provided, or a negative maximumAge is provided.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Doug?

Comment 2

[Doug Turner (:dougt)]

Sounds like a bug:

http://mxr.mozilla.org/mozilla-central/source/dom/src/geolocation/nsGeolocation.cpp#455

ggp, please patch it. :)

Comment 3

[Tareq Khandaker]

Attachment: nsGeolocation.patch

I submitted a patch based on what I understood from the comments above.

Comment 4

[Josh Matthews [:jdm]]

Comment on attachment 811040 [details] [diff] [review]
nsGeolocation.patch

Correct!

Comment 5

[Josh Matthews [:jdm]]

mjh563 pointed out that the value in http://mxr.mozilla.org/mozilla-central/source/dom/webidl/Geolocation.webidl#16 should also change. Could you do that, Tareq?

Comment 6

[Tareq Khandaker]

Attachment: nsGeolocation.patch

Sure.

Comment 7

[Josh Matthews [:jdm]]

Sorry to keep changing the goalposts, but the comment at http://mxr.mozilla.org/mozilla-central/source/dom/src/geolocation/nsGeolocation.cpp#456 was pointed out to me, since it no longer applies. Let's add a new variable called maxCachedAge that is set to maximumAge when maximumAge is greater than 0, and 30s otherwise. Then change the check in canUseCache to refer to maxCachedAge instead of maximumAge.

Comment 8

[Josh Matthews [:jdm]]

Sorry, nevermind. I just re-read the spec, and this is what it says to do. Ignore comment 7.

Comment 9

[Josh Matthews [:jdm]]

We can just remove the (b) comment above your changes to reflect what is happening now.

Comment 10

[Tareq Khandaker]

Attachment: nsGeolocation.patch

I cleaned up the comment a little more since having the a) part by itself looked weird.

Comment 11

[Josh Matthews [:jdm]]

Comment on attachment 811150 [details] [diff] [review]
nsGeolocation.patch

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

Thanks!

Comment 12

[(PTO June 19-July 7) Ryan VanderMeulen]

https://hg.mozilla.org/integration/mozilla-inbound/rev/72a979eba564

Comment 13

[(PTO June 19-July 7) Ryan VanderMeulen]

Backed out.
https://hg.mozilla.org/integration/mozilla-inbound/rev/9d7b0dc99acf

https://tbpl.mozilla.org/php/getParsedLog.php?id=28484639&tree=Mozilla-Inbound

Comment 14

[Tareq Khandaker]

Attachment: mochitest_geolocation.txt

I am not sure what is wrong. I just ran ./mach mochitest-plain dom/tests/mochitest/geolocation | tee ~/mochitest_geolocation.txt on my machine and I attached the results.

I got this at the end:

 0:50.52 161 INFO Passed:  115
 0:50.52 162 INFO Failed:  0
 0:50.52 163 INFO Todo:    0

Comment 15

[Josh Matthews [:jdm]]

I expect you don't have a debug build of Firefox, so the assertion isn't triggering. Skim https://developer.mozilla.org/en-US/docs/Configuring_Build_Options and see the section about --enable-debug.

Comment 16

[Tareq Khandaker]

I updated my .mozconfig to be like so: 

# If ccache was installed via Homebrew:
export PATH="`brew --prefix ccache`/libexec:$PATH"

# Import the stock config for building the browser (Firefox)
. $topsrcdir/browser/config/mozconfig

. $topsrcdir/build/macosx/mozconfig.common

# Define where build files should go. This places them in the directory
# "obj-ff-dbg" under the current source directory
mk_add_options MOZ_OBJDIR=@TOPSRCDIR@/obj-ff-dbg

# -s makes builds quieter by default
# -j4 allows 4 tasks to run in parallel. Set the number to be the amount of
# cores in your machine. 4 is a good number.
mk_add_options MOZ_MAKE_FLAGS="-s -j8"

# Enable debug builds
ac_add_options --enable-debug
ac_add_options --enable-trace-malloc
ac_add_options --enable-accessibility
ac_add_options --enable-signmar

# Treat warnings as errors in directories with FAIL_ON_WARNINGS.
ac_add_options --enable-warnings-as-errors
ac_add_options --with-ccache

# Turn off compiler optimization. This will make applications run slower,
# will allow you to debug the applications under a debugger, like GDB.
ac_add_options --disable-optimize

# autoconf 2.13
mk_add_options AUTOCONF=/usr/local/Cellar/autoconf213/2.13/bin/autoconf213


and I still got 

1:08.45 160 INFO TEST-START | Shutdown
 1:08.45 161 INFO Passed:  115
 1:08.45 162 INFO Failed:  0
 1:08.45 163 INFO Todo:    0

Comment 17

[mjh563]

I can reproduce the assertion failure in a Linux debug build, but only on about 1 in 10 runs at most. It doesn't happen every time.

Try running ./mach mochitest-plain --repeat 20 dom/tests/mochitest/geolocation, to repeat the tests 20 times. I think that should trigger it eventually.

Comment 18

[mjh563]

Strange, I can't reproduce it any more. I definitely saw it more than once when running the tests earlier though.

Comment 19

[Tareq Khandaker]

Thanks! I managed to reproduce the assertion failure using ./mach mochitest-plain --repeat 20 dom/tests/mochitest/geolocation .

I also ran the tests using a maximumAge of 1 instead of 0 (as uint32_t maximumAge = 1; not 1 * PR_MSEC_PER_SE) and it worked all 20 times.

Not sure what to try next.

Comment 20

[Guilherme Gonçalves [:ggp]]

It seems pretty hard to reproduce this locally, so I haven't managed to confirm for sure, but I think I know what's happening.

test_handlerSpinsEventLoop calls getCurrentPosition with no explicit maximumAge, and thus zero is (correctly) used. This prevents us from using the cached position (see canUseCache inside nsGeolocationRequest::Allow), and thus the request is added to the list of allowed requests via a call to Geolocation::NotifyAllowedRequest at the end of nsGeolocationRequest::Allow.

Geolocation::Update gets called when we get a fresh position, and that in turn passes the position to nsGeolocationRequest::Update and then calls Geolocation::RemoveRequest. However, before nsGeolocationRequest::SendLocation gets a chance to run, we get a timeout and Geolocation::Notify gets called. Geolocation::Notify then calls Geolocation::RemoveRequest _again_, which causes the assertion.

The reason why this hadn't happened before is that this test, being run after other geolocation tests, had always used the cached position (which was of course not older than 30 seconds). Thus Geolocation::Update was never called, and Geolocation::RemoveRequest was only ever called once.

jdm, does this sound reasonable to you?

Comment 21

[Josh Matthews [:jdm]]

That makes sense. We should probably just remove the assertion.

Comment 22

[Tareq Khandaker]

Attachment: nsGeolocation.patch

I removed the assertion as well as the immediately above it.

./mach mochitest-plain --repeat 20 dom/tests/mochitest/geolocation passes.

Comment 23

[Guilherme Gonçalves [:ggp]]

(In reply to Josh Matthews [:jdm] from comment #21)
> That makes sense. We should probably just remove the assertion.

Or perhaps cancel the timer on nsGeolocationRequest::Update, before dispatching the runnable? Either way is fine by me.

Comment 24

[Tareq Khandaker]

Is there anything else I have to do here?

Thanks,

Comment 25

[Josh Matthews [:jdm]]

No, I've just been swamped leading up to the Mozilla Summit so I haven't been able to provide feedback yet. No guarantees until Monday, unfortunately, given how intermittent my internet access is.

Comment 26

[Josh Matthews [:jdm]]

Comment on attachment 811951 [details] [diff] [review]
nsGeolocation.patch

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

Comment 23 makes some sense, but separating the logic for cancelling the timer from the logic to reset it makes me worried. I don't think the assertion really helps us that much, so I'm fine with solving this problem by removing it.

Comment 27

[(PTO June 19-July 7) Ryan VanderMeulen]

https://hg.mozilla.org/integration/mozilla-inbound/rev/787a55172184

Comment 28

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/787a55172184