894938 - Content permission prompts don't work when prompt requestor is an iframe

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

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

The given URL requests permission for geolocation and notifications from an iframe, but neither display. Using the iframe URL itself (http://sjeng.org/mozilla/geo.html), both prompts display.

I think the issue is here:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/components/ContentPermissionPrompt.js#61

We use request.window, which is the iframe in this case. But the lookup for the tab assumes the window is the top level DOM window. We shold use request.window.top

Testing a patch now.

Comment 1

[Gian-Carlo Pascutto [:gcp]]

Attachment: Patch 1. Use window.top

Comment 2

[Gian-Carlo Pascutto [:gcp]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/0126c6d7c95a

Comment 3

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0126c6d7c95a

Comment 4

[Gian-Carlo Pascutto [:gcp]]

Comment on attachment 777155 [details] [diff] [review]
Patch 1. Use window.top

[Approval Request Comment]
Bug caused by (feature/regressing bug #): We've had this for ages.
User impact if declined: Features which require user permission won't work in iframes.
Testing completed (on m-c, etc.): Landed on m-c a few days ago.
Risk to taking this patch (and alternatives if risky): Content permission prompts might break in other situations. Given how long this bug has existed, it's not critical.

Comment 5

[bhavana bajaj [:bajaj]]

[I'll request holding off landing Bug 885768, given the below comments are resolved]

(In reply to Gian-Carlo Pascutto (:gcp) from comment #4)
> Comment on attachment 777155 [details] [diff] [review]
> Patch 1. Use window.top
> 
> [Approval Request Comment]
> Bug caused by (feature/regressing bug #): We've had this for ages.
> User impact if declined: Features which require user permission won't work
> in iframes.
> Testing completed (on m-c, etc.): Landed on m-c a few days ago.
> Risk to taking this patch (and alternatives if risky): Content permission
> prompts might break in other situations.
If we can identify the situations and help QA with a test plan then I think this is OK to uplift with that confidence.Else looks like there is a chance we may regress something more important than this once this lands given the risk here ? Can you help identify those cases ? Else this should ride the trains given how long this bug has been there as you suggest.
> Given how long this bug has
> existed, it's not critical.

Comment 6

[Gian-Carlo Pascutto [:gcp]]

>If we can identify the situations and help QA with a test plan then I think this is OK to uplift with that confidence.

I don't know of any such situations - if I did I would've fixed them in the first place! I simply tried to identify the worst possible outcome here. So to clarify: to the best of our knowledge this patch fixes something without breaking anything. However, we're known to be fallible so the worst possible thing that could happen is...

Comment 7

[bhavana bajaj [:bajaj]]

needinfo'ing finkle to get his insight here :

mfinkle I was considering this as this blocks 885768, but given the risk based on comment#5/6, how do you feel about landing these on trains given they are edge-casey based on my read of the bugs or is there a product push to have them which would be the only reason for consideration ?

Comment 8

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

bhavana - I think the risk with this patch is really low. I feel comfortable with taking it on Aurora. I have a hard time coming up with possible regressions. It might be possible that the fix is not 100% complete (although I think this fixes all cases I can think of right now), but I can't see how this would make the current functionality any worse.

You have my blessing for uplift.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/142db87e69e4

Comment 10

[Ioana Chiorean]

verified on:
Galaxy Nexus - Android 4.3
Firefox 24.0b2
Firefox 25.0a2 
Firefox  26.0a1