Closed Bug 639985 Opened 9 years ago Closed 9 years ago

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

Categories

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

ARM
Android
defect

Tracking

(fennec-)

VERIFIED FIXED
Tracking Status
fennec - ---

People

(Reporter: aakashd, Assigned: wesj)

Details

Attachments

(2 files, 2 obsolete files)

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
Attached file 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.
Attached patch Patch v1 (obsolete) — Splinter Review
request.element is a reference to the browser we care about.
Assignee: nobody → wjohnston
Attachment #517942 - Flags: review?(doug.turner)
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?
Attached patch Patch v2 (obsolete) — Splinter Review
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.
Attachment #517942 - Attachment is obsolete: true
Attachment #517942 - Flags: review?(doug.turner)
Attachment #518089 - Flags: review?(mark.finkle)
Attachment #518089 - Flags: review?(doug.turner)
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
Attachment #518089 - Flags: review?(mark.finkle) → review+
Attached patch Patch v3Splinter Review
Thanks finkle.
Attachment #518089 - Attachment is obsolete: true
Attachment #518089 - Flags: review?(doug.turner)
Attachment #518099 - Flags: review?(doug.turner)
tracking-fennec: ? → 2.0-
Comment on attachment 518099 [details] [diff] [review]
Patch v3

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

isn't this expensive?
(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
Attachment #518099 - Flags: review?(doug.turner) → review+
Attachment #518099 - Flags: approval2.0+
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
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.
No longer blocks: 640307
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.