Closed
Bug 503750
Opened 15 years ago
Closed 15 years ago
Crash when parameters of navigator.geolocation.getCurrentPosition are null [@ nsGeolocationRequest::SendLocation(nsIDOMGeoPosition*) ]
Categories
(Core :: DOM: Geolocation, defect)
Tracking
()
VERIFIED
FIXED
Tracking | Status | |
---|---|---|
status1.9.1 | --- | .2-fixed |
People
(Reporter: regis.caspar+bz, Assigned: dougt)
Details
(Keywords: crash, testcase, verified1.9.1)
Crash Data
Attachments
(2 files, 2 obsolete files)
951 bytes,
text/html
|
Details | |
1.50 KB,
patch
|
jst
:
review+
jst
:
superreview+
samuel.sidler+old
:
approval1.9.1.2+
|
Details | Diff | Splinter Review |
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•15 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
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.
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.
Reporter | ||
Comment 4•15 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•15 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•15 years ago
|
Flags: blocking1.9.1.1?
Assignee | ||
Comment 6•15 years ago
|
||
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)
Comment 7•15 years ago
|
||
NS_ENSURE_ARG_POINTER ? Isn't this what it was created for?
Assignee | ||
Comment 8•15 years ago
|
||
@natch sure.
Assignee | ||
Updated•15 years ago
|
Attachment #388278 -
Flags: review? → review?(jst)
note that i claim dougt's approach is invalid. nothing in the spec says i can't do: stupid.getCurrentPosition(null, errorCallback);
Assignee | ||
Updated•15 years ago
|
Attachment #388278 -
Flags: review?(jst) → review-
Assignee | ||
Comment 10•15 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•15 years ago
|
||
Attachment #388278 -
Attachment is obsolete: true
Attachment #388304 -
Flags: superreview?(jst)
Attachment #388304 -
Flags: review?(jst)
Comment 12•15 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•15 years ago
|
||
i want to report the error to javascript; this is what the patch does.
Comment 14•15 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.
Updated•15 years ago
|
Assignee | ||
Comment 15•15 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•15 years ago
|
Attachment #388304 -
Flags: superreview?(jst)
Attachment #388304 -
Flags: superreview+
Attachment #388304 -
Flags: review?(jst)
Attachment #388304 -
Flags: review+
Assignee | ||
Comment 16•15 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/38ffa7e4351f
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Flags: blocking1.9.2?
Resolution: --- → FIXED
Reporter | ||
Comment 17•15 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
Comment 18•15 years ago
|
||
Doug, if this patch applies to 1.9.1, please request approval1.9.1.2 on it.
Assignee | ||
Updated•15 years ago
|
Attachment #388304 -
Flags: approval1.9.1.2?
Updated•15 years ago
|
Attachment #388304 -
Flags: approval1.9.1.2? → approval1.9.1.2+
Comment 19•15 years ago
|
||
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•15 years ago
|
||
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/5745be5d071c
Comment 21•15 years ago
|
||
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•15 years ago
|
Flags: blocking1.9.2?
Updated•13 years ago
|
Crash Signature: [@ nsGeolocationRequest::SendLocation(nsIDOMGeoPosition*) ]
You need to log in
before you can comment on or make changes to this bug.
Description
•