639985 - Geolocation/Offline Storage notifications pop down on Google Reader tab when loading Google Maps in another tab

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect, P2)

Description

[Aakash Desai [:aakashd]]

Build Id:
Mozilla/5.0 (Android; Linux armv71; rv:2.0b13pre) Gecko/20110308 Firefox/4.0b13pre Fennec/4.0b6pre

Steps to Reproduce:
1. Go to reader.google.com
2. Open a new tab
3. Go to maps.google.com in the new tab
4. Go back to the tab with reader.google.com loaded

Actual Results:
The geolocation share prompt pops down in the reader.google.com tab

Expected Results:
The geolocation share prompt pops down in the maps.google.com tab

Comment 1

[Wesley Johnston (:wesj)]

Attachment: Testcase

I think this is because of e10s. In the parent, we don't have access to the window, and have no idea who fired the request:

http://mxr.mozilla.org/mobile-browser/source/components/ContentPermissionPrompt.js#52

so we just use the selected tab. Testcase fires a geolocation prompt on a 5 sec delay. Click the button, and then switch to a different tab. Notification will show up in the wrong tab.

Comment 2

[Wesley Johnston (:wesj)]

Attachment: Patch v1

request.element is a reference to the browser we care about.

Comment 3

[Doug Turner (:dougt)]

Comment on attachment 517942 [details] [diff] [review]
Patch v1

if you have a geo request in the chrome process, request.element will be null.  did you check that case?

Comment 4

[Wesley Johnston (:wesj)]

Attachment: Patch v2

Getting a geo prompt from the chrome process is pretty tough for us (chrome pages throw when calling navigator.geolocation). Talked to dougt and he is filing bugs for the problems.

Patch should work in either case though. Adding mfinkle to review.

Comment 5

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 518089 [details] [diff] [review]
Patch v2


>+  getBrowserForWindow: function getBrowserForWindow(aWindow) {
>+    for (let i = 0; i < this.browsers.length; i++) {
>+      if (this.browsers[i].contentWindow == aWindow)
>+        return this.browsers[i];
>+    }
>+    return null;
>+  },


  getBrowserForWindowId: function getBrowserForWindowId(aWindowId) {
    for (let i = 0; i < this.browsers.length; i++) {
      if (this.browsers[i].contentWindowId == aWindowId)
        return this.browsers[i];
    }
    return null;
  },

where windowId is:

window.QueryInterface(Ci.nsIInterfaceRequestor).getInterface(Ci.nsIDOMWindowUtils).currentInnerWindowID

r+ with that change

Comment 6

[Wesley Johnston (:wesj)]

Attachment: Patch v3

Thanks finkle.

Comment 7

[Doug Turner (:dougt)]

Comment on attachment 518099 [details] [diff] [review]
Patch v3

+    for (let i = 0; i < this.browsers.length; i++) {

isn't this expensive?

Comment 8

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to comment #7)
> Comment on attachment 518099 [details] [diff] [review]
> Patch v3
> 
> +    for (let i = 0; i < this.browsers.length; i++) {
> 
> isn't this expensive?

Not really, but we could use this.tabs, like we do in the other enumerators

Comment 9

[Wesley Johnston (:wesj)]

pushed: http://hg.mozilla.org/mobile-browser/rev/c21d6f5fa876

Comment 10

[Aakash Desai [:aakashd]]

verified FIXED on build:
Mozilla/5.0 (Android; Linux armv71; rv:2.0b13pre) Gecko/20110311 Firefox/4.0b13pre Fennec/4.0b6pre

Bug 640307 is still happening, so taking it out of the blocked list.