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

VERIFIED FIXED

Status

()

Core
Geolocation
--
critical
VERIFIED FIXED
9 years ago
7 years ago

People

(Reporter: Régis Caspar, Assigned: dougt)

Tracking

({crash, testcase, verified1.9.1})

1.9.1 Branch
crash, testcase, verified1.9.1
Points:
---
Bug Flags:
blocking1.9.1.1 -
wanted1.9.1.x +

Firefox Tracking Flags

(status1.9.1 .2-fixed)

Details

(crash signature)

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

9 years ago
Created attachment 388135 [details]
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

9 years ago
Crashes on Linux too. Also crashes in 3.5 release, not just trunk.
bp-82cb127c-5e49-4d13-bbb1-1346b2090712
OS: Windows Vista → All
Hardware: x86 → All
Version: Trunk → 1.9.1 Branch

Comment 2

9 years ago
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

9 years ago
Created attachment 388203 [details] [diff] [review]
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.
Assignee: nobody → timeless
Status: NEW → ASSIGNED
Attachment #388203 - Flags: review?(doug.turner)
(Reporter)

Comment 4

9 years ago
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)
(Assignee)

Comment 5

9 years ago
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?
(Assignee)

Updated

9 years ago
Flags: blocking1.9.1.1?
(Assignee)

Comment 6

9 years ago
Created attachment 388278 [details] [diff] [review]
patch v.1

asserts if null is passed as the success callback.
Assignee: timeless → doug.turner
Attachment #388203 - Attachment is obsolete: true
Attachment #388278 - Flags: review?
Attachment #388203 - Flags: review?(doug.turner)
NS_ENSURE_ARG_POINTER ? Isn't this what it was created for?
(Assignee)

Comment 8

9 years ago
@natch sure.
(Assignee)

Updated

9 years ago
Attachment #388278 - Flags: review? → review?(jst)

Comment 9

9 years ago
note that i claim dougt's approach is invalid.

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

Updated

9 years ago
Attachment #388278 - Flags: review?(jst) → review-
(Assignee)

Comment 10

9 years ago
(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.
(Assignee)

Comment 11

9 years ago
Created attachment 388304 [details] [diff] [review]
patch v.2
Attachment #388278 - Attachment is obsolete: true
Attachment #388304 - Flags: superreview?(jst)
Attachment #388304 - Flags: review?(jst)

Comment 12

9 years ago
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.
(Assignee)

Comment 13

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

Comment 14

9 years ago
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.
status1.9.1: --- → wanted
Flags: wanted1.9.1.x+
Flags: blocking1.9.1.1?
Flags: blocking1.9.1.1-
(Assignee)

Comment 15

9 years ago
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

Updated

9 years ago
Attachment #388304 - Flags: superreview?(jst)
Attachment #388304 - Flags: superreview+
Attachment #388304 - Flags: review?(jst)
Attachment #388304 - Flags: review+
(Assignee)

Comment 16

9 years ago
http://hg.mozilla.org/mozilla-central/rev/38ffa7e4351f
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Flags: blocking1.9.2?
Resolution: --- → FIXED
(Reporter)

Comment 17

9 years ago
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
Status: RESOLVED → VERIFIED
Doug, if this patch applies to 1.9.1, please request approval1.9.1.2 on it.
(Assignee)

Updated

9 years ago
Attachment #388304 - Flags: approval1.9.1.2?
Attachment #388304 - Flags: approval1.9.1.2? → approval1.9.1.2+
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.
(Assignee)

Comment 20

9 years ago
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/5745be5d071c
status1.9.1: wanted → .2-fixed
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.
Keywords: verified1.9.1
(Assignee)

Updated

9 years ago
Flags: blocking1.9.2?
Crash Signature: [@ nsGeolocationRequest::SendLocation(nsIDOMGeoPosition*) ]
You need to log in before you can comment on or make changes to this bug.