Closed Bug 857330 Opened 7 years ago Closed 6 years ago

Geolocation uses a wrong default value for maximumAge

Categories

(Core :: DOM: Geolocation, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla27

People

(Reporter: ggp, Assigned: tareqakhandaker)

Details

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

Attachments

(2 files, 3 obsolete files)

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.
Doug?
Flags: needinfo?(doug.turner)
Sounds like a bug:

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

ggp, please patch it. :)
Flags: needinfo?(doug.turner)
Component: DOM: Core & HTML → Geolocation
Whiteboard: [mentor=jdm][lang=c++][good first bug]
Attached patch nsGeolocation.patch (obsolete) — Splinter Review
I submitted a patch based on what I understood from the comments above.
Assignee: nobody → tareqakhandaker
Attachment #811040 - Flags: review?(josh)
Comment on attachment 811040 [details] [diff] [review]
nsGeolocation.patch

Correct!
Attachment #811040 - Flags: review?(josh) → review+
Keywords: checkin-needed
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?
Keywords: checkin-needed
Attached patch nsGeolocation.patch (obsolete) — Splinter Review
Sure.
Attachment #811040 - Attachment is obsolete: true
Attachment #811138 - Flags: review?(josh)
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.
Sorry, nevermind. I just re-read the spec, and this is what it says to do. Ignore comment 7.
We can just remove the (b) comment above your changes to reflect what is happening now.
Attached patch nsGeolocation.patch (obsolete) — Splinter Review
I cleaned up the comment a little more since having the a) part by itself looked weird.
Attachment #811138 - Attachment is obsolete: true
Attachment #811138 - Flags: review?(josh)
Attachment #811150 - Flags: review?(josh)
Comment on attachment 811150 [details] [diff] [review]
nsGeolocation.patch

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

Thanks!
Attachment #811150 - Flags: review?(josh) → review+
Keywords: checkin-needed
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
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.
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
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.
Strange, I can't reproduce it any more. I definitely saw it more than once when running the tests earlier though.
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.
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?
That makes sense. We should probably just remove the assertion.
I removed the assertion as well as the immediately above it.

./mach mochitest-plain --repeat 20 dom/tests/mochitest/geolocation passes.
Attachment #811150 - Attachment is obsolete: true
Attachment #811951 - Flags: review?(josh)
(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.
Is there anything else I have to do here?

Thanks,
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 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.
Attachment #811951 - Flags: review?(josh) → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/787a55172184
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
You need to log in before you can comment on or make changes to this bug.