503750 - Crash when parameters of navigator.geolocation.getCurrentPosition are null [@ nsGeolocationRequest::SendLocation(nsIDOMGeoPosition*) ]

Status: VERIFIED FIXED

(Core :: DOM: Geolocation, defect)

Description

[Régis Caspar]

Attachment: Testcase (safe to open, a confirmation is asked before anything)

Steps to reproduce: see attachment

Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.2a1pre) Gecko/20090711 Minefield/3.6a1pre (.NET CLR 3.5.30729) ID:20090711042715

http://crash-stats.mozilla.com/report/index/91e308c9-88a8-4fb2-89c8-51b4b2090712
With a clean profile:
http://crash-stats.mozilla.com/report/index/f3b9a1b3-2b65-4002-9714-e64bc2090712
http://crash-stats.mozilla.com/report/index/aca035c8-0441-41fa-ac62-e45fc2090712

Comment 1

[Dave Garrett]

Crashes on Linux too. Also crashes in 3.5 release, not just trunk.
bp-82cb127c-5e49-4d13-bbb1-1346b2090712

Comment 2

[timeless]

http://www.w3.org/TR/2009/WD-geolocation-API-20090707/

The getCurrentPosition()  takes one, two or three arguments. When called, it must immediately return and then asynchronously acquire a new Position object. If successful, this method must invoke its associated successCallback argument with a Position object as an argument. If the attempt fails, and the method was invoked with a non-null errorCallback argument, this method must invoke the errorCallback with a PositionError object as an argument. 

The spec doesn't say what to do if the first argument is null.

Comment 3

[timeless]

Attachment: patch

note that we don't simply bail out early, we need to give a chance to pass to the error callback, and we need to record that we didn't time out.

Comment 4

[Régis Caspar]

I tried to narrow down the problem (without success): 
I was thinking about
http://hg.mozilla.org/mozilla-central/annotate/47bfcd275ede/dom/src/geolocation/nsGeolocation.cpp#l328
(mCallback being null)
I tried to patch here with a if not null but with a debug build I saw the
problem seems to be in 
NetworkGeolocationProvider.js when WifiGeoPositionProvider.timer is fired and
WifiGeoPositionProvider.notify is called (then the crash occurs)

Comment 5

[Doug Turner (:dougt)]

I would rather assert if all of the parameters are null/invalid.  timeless, do you want to update, or do you want me to roll a new patch?

Comment 6

[Doug Turner (:dougt)]

Attachment: patch v.1

asserts if null is passed as the success callback.

Comment 7

[Nochum Sossonko [:Natch]]

NS_ENSURE_ARG_POINTER ? Isn't this what it was created for?

Comment 8

[Doug Turner (:dougt)]

@natch sure.

Comment 9

[timeless]

note that i claim dougt's approach is invalid.

nothing in the spec says i can't do:
stupid.getCurrentPosition(null, errorCallback);

Comment 10

[Doug Turner (:dougt)]

(marked - because I wanted to incorporate natch's feedback)

timeless - passing null is stupid as you suggest.  at the end of the day, do you really think supporting such a thing is better than alerting the developer that they are making a programmatic error?

Furthermore, the spec doesn't say alot of things you can and can't do.  I assert that this can be a implementation detail.  We will assert if you pass null.

Comment 11

[Doug Turner (:dougt)]

Attachment: patch v.2

Comment 12

[timeless]

asserts in gecko debug only builds don't help because we're talking about web applications. you need to use the error console.

I could vaguely imagine writing something which only has an error callback.

if you don't do that, you aren't actually reporting the error, you're just wasting time pretending you are.

Comment 13

[Doug Turner (:dougt)]

i want to report the error to javascript; this is what the patch does.

Comment 14

[timeless]

not in any useful way, no.

http://mxr.mozilla.org/mozilla-central/ident?i=ReportUseOfDeprecatedMethod

and if you're going to do it, someone needs to tell the working group....

note that web apps generally do not expect exceptions, so what you're probably doing is causing a number of apps to randomly break. don't do this without standardizing it.

Comment 15

[Doug Turner (:dougt)]

Comment on attachment 388304 [details] [diff] [review]
patch v.2

fwiw, i notified the wg about this and waiting feedback.  we can change the behavior if any different decision is reached.

http://lists.w3.org/Archives/Public/public-geolocation/2009Jul/0010.html

Comment 16

[Doug Turner (:dougt)]

http://hg.mozilla.org/mozilla-central/rev/38ffa7e4351f

Comment 17

[Régis Caspar]

no more crash with testcase and Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.2a1pre) Gecko/20090717 Minefield/3.6a1pre (.NET CLR 3.5.30729) ID:20090717045648 only an error in JS console:
Error: uncaught exception: [Exception... "Component returned failure code: 0x80004003 (NS_ERROR_INVALID_POINTER) [nsIDOMGeoGeolocation.getCurrentPosition]"  nsresult: "0x80004003 (NS_ERROR_INVALID_POINTER)"  location: "JS frame :: https://bug503750.bugzilla.mozilla.org/attachment.cgi?id=388135 :: anonymous :: line 19"  data: no]

Thanks.

=> VERIFIED

Comment 18

[Samuel Sidler (old account; do not CC)]

Doug, if this patch applies to 1.9.1, please request approval1.9.1.2 on it.

Comment 19

[Samuel Sidler (old account; do not CC)]

Comment on attachment 388304 [details] [diff] [review]
patch v.2

Approved for 1.9.1.2. a=ss for release-drivers

Please land on mozilla-1.9.1 and use the ".2-fixed" option of the "status1.9.1" flag.

Comment 20

[Doug Turner (:dougt)]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/5745be5d071c

Comment 21

[juan becerra [:juanb]]

Verified with testcase in comment #0 using: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1.2) Gecko/20090729 Firefox/3.5.2 (.NET CLR 3.5.30729)

3.5 crashes. 3.5.2 does not.