1015894 - Use window.realFrameElement into b2g/components/ContentPermissionPrompt.js to dispatch the permission request to the correct iframe

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Comment 1

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: use.realFrameElement.for.in.process.permission.prompt.patch

Comment 2

[[:fabrice] Fabrice Desré]

Comment on attachment 8428616 [details] [diff] [review]
use.realFrameElement.for.in.process.permission.prompt.patch

Review of attachment 8428616 [details] [diff] [review]:
-----------------------------------------------------------------

::: b2g/components/ContentPermissionPrompt.js
@@ +387,5 @@
> +
> +    let targetElement = request.element;
> +    let targetWindow = request.window || targetElement.ownerDocument.defaultView;
> +    try {
> +      while (targetWindow.realFrameElement) {

Is the only guarantee to not loop forever the xray exception? That looks fragile to me.

::: b2g/components/SystemAppProxy.jsm
@@ +74,5 @@
>        payload = details ? Cu.cloneInto(details, content) : {};
>      }
>  
>      event.initCustomEvent(type, true, false, payload);
> +    (target || content).dispatchEvent(event);

can we check that (target || content) is not null? (we should have checked content before this patch).

Comment 3

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to < away until June 17 > from comment #2)
> Comment on attachment 8428616 [details] [diff] [review]
> use.realFrameElement.for.in.process.permission.prompt.patch
> 
> Review of attachment 8428616 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: b2g/components/ContentPermissionPrompt.js
> @@ +387,5 @@
> > +
> > +    let targetElement = request.element;
> > +    let targetWindow = request.window || targetElement.ownerDocument.defaultView;
> > +    try {
> > +      while (targetWindow.realFrameElement) {
> 
> Is the only guarantee to not loop forever the xray exception? That looks
> fragile to me.
> 

The underlying platform code has been fixed. realFrameElement will just return null now.

> ::: b2g/components/SystemAppProxy.jsm
> @@ +74,5 @@
> >        payload = details ? Cu.cloneInto(details, content) : {};
> >      }
> >  
> >      event.initCustomEvent(type, true, false, payload);
> > +    (target || content).dispatchEvent(event);
> 
> can we check that (target || content) is not null? (we should have checked
> content before this patch).

This has already been validated above. So |content| can't be null.

Comment 4

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: use.realFrameElement.for.in.process.permission.prompt.patch

Asking you as Fabrice is in vacation.

Comment 5

[Alexandre Poirot [:ochameau]]

Comment on attachment 8434089 [details] [diff] [review]
use.realFrameElement.for.in.process.permission.prompt.patch

Review of attachment 8434089 [details] [diff] [review]:
-----------------------------------------------------------------

You would be a gentleman by also adding a test for this new target argument.
It should be matter of dispatching an additional event with a custom target over here:
  http://mxr.mozilla.org/mozilla-central/source/b2g/components/test/mochitest/systemapp_helper.js#121
While listening and asserting event.target over here:
  http://mxr.mozilla.org/mozilla-central/source/b2g/components/test/mochitest/systemapp_helper.js#47

::: b2g/components/ContentPermissionPrompt.js
@@ +389,5 @@
> +    let targetWindow = request.window || targetElement.ownerDocument.defaultView;
> +    while (targetWindow.realFrameElement) {
> +      targetElement = targetWindow.realFrameElement;
> +      targetWindow = targetElement.ownerDocument.defaultView;
> +    }

Using comment and conditional block to distinguish OOP / In-parent would help!
Also I'm not sure this code is expected to work with OOP frame.
If request.element is defined with OOP frame (and request.window is null, which I would expect for OOP frame), targetElement.ownerDocument.defaultView is going to throw.

Here is my understanding of this code:
  let targetElement = request.element; // I imagine if request.element is set, we get the right frame immediately?
  if (!targetElement) {
    // When the request is related to an iframe living in parent process,
    // instead of the frame element, we have its window object
    let targetWindow = request.window; // nsIContentPermissionRequest says request.element and request.window can't be both null, so I'm not sure there is a point in fetching targetElement.ownerDocument.defaultView?
    while (targetWindow.realFrameElement) {
      targetElement = targetWindow.realFrameElement;
      targetWindow = targetElement.ownerDocument.defaultView;
    }
  }

Comment 6

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Alexandre Poirot (:ochameau) from comment #5)
> Comment on attachment 8434089 [details] [diff] [review]
> use.realFrameElement.for.in.process.permission.prompt.patch
> 
> Review of attachment 8434089 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> You would be a gentleman by also adding a test for this new target argument.
> It should be matter of dispatching an additional event with a custom target
> over here:
>  
> http://mxr.mozilla.org/mozilla-central/source/b2g/components/test/mochitest/
> systemapp_helper.js#121
> While listening and asserting event.target over here:
>  
> http://mxr.mozilla.org/mozilla-central/source/b2g/components/test/mochitest/
> systemapp_helper.js#47
> 
> ::: b2g/components/ContentPermissionPrompt.js
> @@ +389,5 @@
> > +    let targetWindow = request.window || targetElement.ownerDocument.defaultView;
> > +    while (targetWindow.realFrameElement) {
> > +      targetElement = targetWindow.realFrameElement;
> > +      targetWindow = targetElement.ownerDocument.defaultView;
> > +    }
> 
> Using comment and conditional block to distinguish OOP / In-parent would
> help!
> Also I'm not sure this code is expected to work with OOP frame.
> If request.element is defined with OOP frame (and request.window is null,
> which I would expect for OOP frame), targetElement.ownerDocument.defaultView
> is going to throw.
> 

It's not. Since targetElement is a <iframe mozbrowser>, targetElement.ownerDocument.defaultView is === to the |window| that owns this <iframe>.

> Here is my understanding of this code:
>   let targetElement = request.element; // I imagine if request.element is
> set, we get the right frame immediately?
>   if (!targetElement) {
>     // When the request is related to an iframe living in parent process,
>     // instead of the frame element, we have its window object
>     let targetWindow = request.window; // nsIContentPermissionRequest says
> request.element and request.window can't be both null, so I'm not sure there
> is a point in fetching targetElement.ownerDocument.defaultView?
>     while (targetWindow.realFrameElement) {
>       targetElement = targetWindow.realFrameElement;
>       targetWindow = targetElement.ownerDocument.defaultView;
>     }
>   }

I need to iterate over windows for the case of the browser. It contains OOP iframes nested into a In-process iframe.

Comment 7

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: use.realFrameElement.for.in.process.permission.prompt.patch

Same with test and more comments.

Comment 8

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: use.realFrameElement.for.in.process.permission.prompt.patch

I made a typo in the test!

Comment 9

[Alexandre Poirot [:ochameau]]

Comment on attachment 8434154 [details] [diff] [review]
use.realFrameElement.for.in.process.permission.prompt.patch

Review of attachment 8434154 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Vivien Nicolas (:vingtetun) (:21) (NOT reading bugmails, needinfo? please) from comment #6)
> > If request.element is defined with OOP frame (and request.window is null,
> > which I would expect for OOP frame), targetElement.ownerDocument.defaultView
> > is going to throw.
> > 
> 
> It's not. Since targetElement is a <iframe mozbrowser>,
> targetElement.ownerDocument.defaultView is === to the |window| that owns
> this <iframe>.

Oh right, I mixed up ownerDocument and contentWindow...

r+ with a green try ;)

Comment 10

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3220b7b2efc9

Comment 11

[Ryan VanderMeulen [:RyanVM]]

This push hit desktop B2G mochitest failures. Not knowing the interdependencies of the push, I backed the entire thing out.
https://hg.mozilla.org/integration/mozilla-inbound/rev/616163ca1aa5

https://tbpl.mozilla.org/php/getParsedLog.php?id=41052634&tree=Mozilla-Inbound

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/0c8f92915f71