Blank page at geolocation and strange behavior

VERIFIED FIXED

Status

Firefox for Android Graveyard
General
VERIFIED FIXED
8 years ago
8 years ago

People

(Reporter: Ioana Chiorean, Assigned: dougt)

Tracking

Trunk
ARM
Android

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

8 years ago
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
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]
************************************************************
Created attachment 479777 [details] [diff] [review]
patch
Attachment #479777 - Flags: review?(mkristoffersen)
Attachment #479777 - Flags: review?(doug.turner)
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.
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.
Created attachment 480096 [details] [diff] [review]
patch v2

Avoid the creation of a WifiGeoPositionObject if we have no valid position
Attachment #479777 - Attachment is obsolete: true
Attachment #480096 - Flags: review?(mkristoffersen)
Attachment #479777 - Flags: review?(mkristoffersen)
Attachment #479777 - Flags: review?(doug.turner)
(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 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).
Attachment #480096 - Flags: review?(mkristoffersen) → review+
(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

8 years ago
The address field is not always present in a location object.
(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).
Created attachment 480211 [details] [diff] [review]
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.
Attachment #480096 - Attachment is obsolete: true
Attachment #480211 - Flags: review?(mkristoffersen)
Comment on attachment 480211 [details] [diff] [review]
patch v3

Good work :)
Attachment #480211 - Flags: review?(mkristoffersen) → review+
(Assignee)

Comment 13

8 years ago
can we get tests for this?
(Assignee)

Updated

8 years ago
tracking-fennec: --- → ?
(Assignee)

Updated

8 years ago
tracking-fennec: ? → 2.0b2+
(Assignee)

Updated

8 years ago
Assignee: nobody → doug.turner
(Assignee)

Comment 14

8 years ago
http://hg.mozilla.org/mozilla-central/rev/b9ed85e1fb55
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
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?
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
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.