600816 - Blank page at geolocation and strange behavior

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Ioana Chiorean]

Build Identifier: Mozilla/5.0 (Android; Linux armv7l; en-US; rv:2.0b7pre)
Gecko/20090929 Firefox/4.0b7pre Fennec/4.0b2pre 
Device: Motorola Droid 2.2 

Steps to reproduce:
1. Open Fennec -> go to http://people.mozilla.org/~jmaher/wifi.html
2. Tab URL bar and choose again http://people.mozilla.org/~jmaher/wifi.html
3. Choose share our location

Expected result:
- the page should load without problems, our location should display

Actual result:
- after choosing share location, nothing is displayed
- the page is centered under the blue pop up , you have to drag the page down to see it

Comment 1

[[:fabrice] Fabrice Desré]

On a debug build we have this exception raised :

************************************************************
* Call to xpconnect wrapped JSObject produced this error:  *
[Exception... "'[JavaScript Error: "(void 0) is undefined" {file: "file:///home/ifda7415/dev/mozilla-central/obj-i686-pc-linux-gnu/dist/bin/components/NetworkGeolocationProvider.js" line: 84}]' when calling method: [nsIDOMEventListener::handleEvent]"  nsresult: "0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)"  location: "<unknown>"  data: yes]
************************************************************

Comment 2

[[:fabrice] Fabrice Desré]

Attachment: patch

Comment 3

[Mike Kristoffersen (:MikeK)]

While I'm sure this one fixes the problem and might be a valid check in it self, wouldn't it be better to check for a valid location object in the function that calls this one? 

To me it looks like it just forwards the decision that "no location" is an invalid location to the geolocation service, where it could have bailed out much earlier.

I might be wrong thou, I'm really not experienced in looking at JS code.

Comment 4

[[:fabrice] Fabrice Desré]

Mike, so you would prefer to test for error condition here :
http://mxr.mozilla.org/mozilla-central/source/dom/system/NetworkGeolocationProvider.js#395 ?
I agree it would make more sense to not update at all if we don't have a valid position to transmit.

Comment 5

[[:fabrice] Fabrice Desré]

Attachment: patch v2

Avoid the creation of a WifiGeoPositionObject if we have no valid position

Comment 6

[Mike Kristoffersen (:MikeK)]

(In reply to comment #5)
> Created attachment 480096 [details] [diff] [review]
> patch v2
> 
> Avoid the creation of a WifiGeoPositionObject if we have no valid position

Maybe we could even move the test above the " var address = null;" in line 386, as I assume that will also fail if response.location is null...

Comment 7

[Mike Kristoffersen (:MikeK)]

Comment on attachment 480096 [details] [diff] [review]
patch v2

Patch is fine - assuming it has been tested that it fixes the problem actually described in this bug?

(You can create a new patch with my previous comment if you like).

Comment 8

[[:fabrice] Fabrice Desré]

(In reply to comment #6)
> (In reply to comment #5)
> > Created attachment 480096 [details] [diff] [review] [details]
> > patch v2
> > 
> > Avoid the creation of a WifiGeoPositionObject if we have no valid position
> 
> Maybe we could even move the test above the " var address = null;" in line 386,
> as I assume that will also fail if response.location is null...

You're right, which means that the WifiGeoPositionObject(location, address) constructor is strange : it allows address to be null but not location, although this can not happen this address is a location field...

Shouldn't we have rather WifiGeoPositionObject(location) only and use the address field from here?

Comment 9

[Josh Matthews [:jdm]]

The address field is not always present in a location object.

Comment 10

[Mike Kristoffersen (:MikeK)]

(In reply to comment #9)
> The address field is not always present in a location object.

I think the point is that "address = response.location.address", so it gets the address two times, or more explicit:

var newLocation = new WifiGeoPositionObject(response.location, response.location.address);

So we can move the test of the presence of a address field inside the constructor, and we don't need to test it, if the location it self is null.

(In reply to comment #8)
> (In reply to comment #6)
> Shouldn't we have rather WifiGeoPositionObject(location) only and use the
> address field from here?

Sounds like a good idea to me :)  If you do this, can you send it for review again, want to see how you handle the cases where response.location is null or response.location.address is null.

I'll be out of town this weekend, but I'll be back Sunday evening (GMT).

Comment 11

[[:fabrice] Fabrice Desré]

Attachment: patch v3

With this patch :
- if location == null, we do nothing
- if location != null, we use location.address if it's available, and send a location update

Works fine for me here.

Comment 12

[Mike Kristoffersen (:MikeK)]

Comment on attachment 480211 [details] [diff] [review]
patch v3

Good work :)

Comment 13

[Doug Turner (:dougt)]

can we get tests for this?

Comment 14

[Doug Turner (:dougt)]

http://hg.mozilla.org/mozilla-central/rev/b9ed85e1fb55

Comment 15

[Aakash Desai [:aakashd]]

It takes 30 seconds or more for the geolocation prompt to come up again on reload on build:

Mozilla/5.0 (Android; Linux armv71; rv:2.0b8pre) Gecko/20101011 Namoroka/4.0b8pre Fennec/4.0b2pre


Is this intended?

Comment 16

[Aakash Desai [:aakashd]]

verified FIXED on builds:

Mozilla/5.0 (maemol Linux armv7l; rv:2.0b7pre) Gecko/20101029 Firefox/4.0b8pre Fennec/4.0b2

Mozilla/5.0 (android Linux armv7l; rv:2.0b7pre) Gecko/20101029 Firefox/4.0b8pre Fennec/4.0b2