1020179 - Move PContentPermissionRequest to be managed by PContent

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Kan-Ru Chen [:kanru] (UTC+9)]

nsContentPermissionHelper assumes it lives in the chrome process. Actually permission check is done per-process basis so it make sense to move it to PContent.

Comment 1

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: WIP - let PContent manage PContentPermissionRequest

Need find a way to obtain an representative element to initialize ContentPermissionRequestParent. In the past we use frameElement in TabParent, however, there is no such element in parent process for nested-oop scenario.

Comment 2

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Per discussion with @kanru, we need to maintain a tree structure of the representative elements (RemoteFrameElement) for all the oop iframe in b2g process. In that case, ContentParent can use the corresponding element in permission request.

Comment 3

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Bug 1045975 will make my life easier for moving PContentPermissionRequest from PBrowser to PContent.

Comment 4

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

The nsIContentPermissionRequest.window and .element is used for:
1. get corresponding chrome window for the requesting window
2. get corresponding tab for the requesting window
3. check the existence/visibility of the requesting window

For usage #1 and #2, we can always provide the window of the top most iframe of the requesting window as long as we don't allow chrome window to live on content process. For b2g, we don't really use the chrome window but SystemAppProxy therefore we can ignore this attribute.

For usage #3, we need to introduce a new attribute to provide the information because we don't have all the window/frame information in chrome process under nested OOP scenario.

Here is the IDL I have in mind:
interface nsIContentPermissionRequester : nsISupports {
  // the information we want to expose while doing permission prompt
  readonly attribute jsval onvisibilitychange;
  boolean isVisible();
};

interface nsIContentPermissionRequest : nsISupports {
  readonly attribute nsIArray types;
  readonly attribute nsIPrincipal principal;

  // For in-process case, provide the requesting content window directly
  // For oop case, provide the content window of the top most iframe of the requesting window
  readonly attribute nsIDOMWindow window;

  // expose the required information of requesting window
  readonly attribute nsIContentPermissionRequester requester;

  void cancel();
  void allow([optional] in jsval choices); // {"type1": "choice1", "type2": "choiceA"}
};

@mbrubeck, @fabrice, @mfinkle, and @felipc, do you think the interface I proposed is enough for implementing permission prompt on Firefox/Fennec/B2G/Metro in order to support nested oop?

Comment 5

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

In order to mapping the requesting window to top most iframe (TabParent), we need to maintain a tree structure of the opened remote TabParent as mentioned in bug 1020172 comment #1.

Comment 6

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: WIP - let PContent manage PContentPermissionRequest, v2

update WIP because dependent bug 1045975 has landed.

Comment 7

[:Felipe Gomes (needinfo for replies!)]

Sending my feedback request to Blake who probably knows more about permission prompts on Desktop

Comment 8

[Jenny Liu[:jennyliu]]

We may not be able land it within v2.1 sprint 3, move to v2.2.

Comment 9

[[:fabrice] Fabrice Desré]

Shih-Chiang - the looks fine to me, but I'm not yet very familiar on how the nested OOP setup differs from the "simple" OOP setup. Is there something I could read about that? It seems that we now create TabParent in the child?

Comment 10

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

(In reply to Fabrice Desré [:fabrice] from comment #9)
> Shih-Chiang - the looks fine to me, but I'm not yet very familiar on how the
> nested OOP setup differs from the "simple" OOP setup. Is there something I
> could read about that? It seems that we now create TabParent in the child?
Yes, TabParent will be created in content process now, which managed by PContentBridge instead of PContent. Here is the steps for setting up nested OOP.

1. content process ask chrome process to create a child process via PContent.
2. chrome process create the child process and setup the PContentBridge between these two content processes
3. content process create TabParent/TabChild over PContentBridge.

@kanru, do you have any document that explain nested OOP in detail?

Comment 11

[Kan-Ru Chen [:kanru] (UTC+9)]

The document is lagging a bit behind but you could find the information on this wiki

https://wiki.mozilla.org/Nested_Content_Processes

I'll update it with more details.

Comment 12

[Matt Brubeck (:mbrubeck)]

I see no particular problems here for Metro.

Comment 13

[Kai-Chih Hu [:khu]]

Use feature-b2g:2.2? rather than just 2.2.

Comment 14

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

The IDL looks reasonable to me, but Fennec doesn't use e10s yet, so as long as the changes continue to work well for non-e10s Fennec will be fine.

Comment 15

[Kershaw Chang [:kershaw]]

(In reply to Shih-Chiang Chien [:schien] (UTC+8) from comment #4)
> The nsIContentPermissionRequest.window and .element is used for:
> 1. get corresponding chrome window for the requesting window
> 2. get corresponding tab for the requesting window
> 3. check the existence/visibility of the requesting window
> 
> For usage #1 and #2, we can always provide the window of the top most iframe
> of the requesting window as long as we don't allow chrome window to live on
> content process. For b2g, we don't really use the chrome window but
> SystemAppProxy therefore we can ignore this attribute.
> 
> For usage #3, we need to introduce a new attribute to provide the
> information because we don't have all the window/frame information in chrome
> process under nested OOP scenario.
> 
> Here is the IDL I have in mind:
> interface nsIContentPermissionRequester : nsISupports {
>   // the information we want to expose while doing permission prompt
>   readonly attribute jsval onvisibilitychange;
>   boolean isVisible();
> };
> 
> interface nsIContentPermissionRequest : nsISupports {
>   readonly attribute nsIArray types;
>   readonly attribute nsIPrincipal principal;
> 
>   // For in-process case, provide the requesting content window directly
>   // For oop case, provide the content window of the top most iframe of the
> requesting window
>   readonly attribute nsIDOMWindow window;
> 
>   // expose the required information of requesting window
>   readonly attribute nsIContentPermissionRequester requester;
> 
>   void cancel();
>   void allow([optional] in jsval choices); // {"type1": "choice1", "type2":
> "choiceA"}
> };
> 

Hi SC,

Could you explain more why we need this idl change?
Can we just migrate PContentPermissionRequest from PBrowser to PContent?

Thanks.

Comment 16

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

(In reply to Kershaw Chang [:kershaw] from comment #15)
> (In reply to Shih-Chiang Chien [:schien] (UTC+8) from comment #4)
> > The nsIContentPermissionRequest.window and .element is used for:
> > 1. get corresponding chrome window for the requesting window
> > 2. get corresponding tab for the requesting window
> > 3. check the existence/visibility of the requesting window
> > 
> > For usage #1 and #2, we can always provide the window of the top most iframe
> > of the requesting window as long as we don't allow chrome window to live on
> > content process. For b2g, we don't really use the chrome window but
> > SystemAppProxy therefore we can ignore this attribute.
> > 
> > For usage #3, we need to introduce a new attribute to provide the
> > information because we don't have all the window/frame information in chrome
> > process under nested OOP scenario.
> > 
> > Here is the IDL I have in mind:
> > interface nsIContentPermissionRequester : nsISupports {
> >   // the information we want to expose while doing permission prompt
> >   readonly attribute jsval onvisibilitychange;
> >   boolean isVisible();
> > };
> > 
> > interface nsIContentPermissionRequest : nsISupports {
> >   readonly attribute nsIArray types;
> >   readonly attribute nsIPrincipal principal;
> > 
> >   // For in-process case, provide the requesting content window directly
> >   // For oop case, provide the content window of the top most iframe of the
> > requesting window
> >   readonly attribute nsIDOMWindow window;
> > 
> >   // expose the required information of requesting window
> >   readonly attribute nsIContentPermissionRequester requester;
> > 
> >   void cancel();
> >   void allow([optional] in jsval choices); // {"type1": "choice1", "type2":
> > "choiceA"}
> > };
> > 
> 
> Hi SC,
> 
> Could you explain more why we need this idl change?
> Can we just migrate PContentPermissionRequest from PBrowser to PContent?
> 
> Thanks.

Two reasons:
1. There is no way to implement nsIContentPermissionRequest.element for grandchild process in chrome process because the corresponding frameElement only existed in grandchild process and its parent process. So the first modification is to remove this field and use |window| attribute under all circumstances.

2. There is one existing code that need visibility of the frame, which cannot be provided by |window| attribute [1]. So I added the |requester| for querying frame-related information. In nested OOP scenario, there should be one IPC channel created between grandchild process and chrome process automatically.

Full version of explanation in comment #4.

[1] http://dxr.mozilla.org/mozilla-central/source/b2g/components/ContentPermissionPrompt.js#319

Comment 17

[Kershaw Chang [:kershaw]]

Attachment: Let WyciwygChannel support nested oop iframe - V3

Hi SC,

Could you help to take a look at this patch first?
It seems that the element is only used for getting the visibility of the remote window, so I just add one function in idl to do this.

Thanks.

Comment 18

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Comment on attachment 8526617 [details] [diff] [review]
Let WyciwygChannel support nested oop iframe - V3

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

See my comments below and you'll need to check all ContentPermissionPrompt implementations on all product.
Firefox: http://dxr.mozilla.org/mozilla-central/source/browser/components/nsBrowserGlue.js#2002
Fennec: http://dxr.mozilla.org/mozilla-central/source/mobile/android/components/ContentPermissionPrompt.js#31
WebRT: http://dxr.mozilla.org/mozilla-central/source/webapprt/ContentPermission.js#17
Metro: http://dxr.mozilla.org/mozilla-central/source/browser/metro/components/ContentPermissionPrompt.js#27

::: b2g/components/ContentPermissionPrompt.js
@@ +325,4 @@
>  
> +        // Monitor the frame visibility and cancel the request if the frame goes
> +        // away but the request is still here.
> +        frame.addEventListener("mozbrowservisibilitychange", onVisibilityChange);

|frame| will not existed after nsIContentPermissionRequest.element is removed. The visibility change detection is for dismissing prompt UI while user switch app to background because permission prompt UI is a system-wise overlay (see Bug 832412). I'm not sure if that's still true for current Gaia. Ask @alive about the Gaia restriction and Ask @jenny for UX input if we want to change the behavior.

::: dom/base/nsDocument.cpp
@@ +11776,5 @@
>  
> +NS_IMETHODIMP
> +nsPointerLockPermissionRequest::GetVisibility(nsIContentPermissionRequestCallback *aCallback)
> +{
> +  return NS_OK;

The visibility handling for non-oop case should be the same for all PermissionRequest (except for the proxy one). You can create a base class for GetVisibility and should be the similar to RemotePermissionRequest::RecvGetVisibility.

::: dom/interfaces/base/nsIContentPermissionPrompt.idl
@@ +65,1 @@
>    readonly attribute nsIDOMElement element;

|element| should be removed according to comment #4.

::: dom/ipc/ContentParent.cpp
@@ +4393,5 @@
> +    ContentProcessManager* cpm = ContentProcessManager::GetSingleton();
> +    TabId tabId = aTabId;
> +    ContentParentId cpId = this->ChildID();
> +
> +    while (true) {

|while (true)| always feels creepy to me. Try if you can make it into for loop or |while(stop criteria)|.

@@ +4410,5 @@
> +        cpId = parentCpId;
> +        tabId = openerTabId;
> +    }
> +
> +    ContentParent* cp = cpm->GetContentProcessById(cpId);

Use smart pointer as you can.

@@ +4418,5 @@
> +        if (tab->GetTabId() == tabId) {
> +            return nsContentPermissionUtils::CreateContentPermissionRequestParent(
> +                aRequests, tab->GetOwnerElement(), aPrincipal);
> +        }
> +    }

How about creating a |GetTabParent(cpId, tabId)| in ContentProcessManager and do following things:

  nsRefPtr<TabParent> tp = cpm->GetTabParent(cpId, tabId);

  if (!tp) {
    MOZ_ASSERT(false);
    return nullptr;
  }

  return nsContentPermissionUtils::CreateContentPermissionRequestParent(aRequest, tp->GetOwnerElement(), aPrincipal);

I always prefer early return style.

Comment 19

[Kershaw Chang [:kershaw]]

(In reply to Shih-Chiang Chien [:schien] (UTC+8) from comment #18)
> Comment on attachment 8526617 [details] [diff] [review]
> Let WyciwygChannel support nested oop iframe - V3
> 
> Review of attachment 8526617 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> See my comments below and you'll need to check all ContentPermissionPrompt
> implementations on all product.
> Firefox:
> http://dxr.mozilla.org/mozilla-central/source/browser/components/
> nsBrowserGlue.js#2002
> Fennec:
> http://dxr.mozilla.org/mozilla-central/source/mobile/android/components/
> ContentPermissionPrompt.js#31
> WebRT:
> http://dxr.mozilla.org/mozilla-central/source/webapprt/ContentPermission.
> js#17
> Metro:
> http://dxr.mozilla.org/mozilla-central/source/browser/metro/components/
> ContentPermissionPrompt.js#27

Thanks. I will check these implementations.
> 
> ::: b2g/components/ContentPermissionPrompt.js
> @@ +325,4 @@
> >  
> > +        // Monitor the frame visibility and cancel the request if the frame goes
> > +        // away but the request is still here.
> > +        frame.addEventListener("mozbrowservisibilitychange", onVisibilityChange);
> 
> |frame| will not existed after nsIContentPermissionRequest.element is
> removed. The visibility change detection is for dismissing prompt UI while
> user switch app to background because permission prompt UI is a system-wise
> overlay (see Bug 832412). I'm not sure if that's still true for current
> Gaia. Ask @alive about the Gaia restriction and Ask @jenny for UX input if
> we want to change the behavior.

nsIContentPermissionRequest.element is only implemented in nsContentPermissionRequestProxy::GetElement which is already used for current oop case. I think we should keep nsIContentPermissionRequest.element as possible as we can, since we don't want to change the current behavior, do we?
I think the problem is can we use the element of 1st level frame for nested oop case. I think I can create a test app to test this.
 
> 
> ::: dom/base/nsDocument.cpp
> @@ +11776,5 @@
> >  
> > +NS_IMETHODIMP
> > +nsPointerLockPermissionRequest::GetVisibility(nsIContentPermissionRequestCallback *aCallback)
> > +{
> > +  return NS_OK;
> 
> The visibility handling for non-oop case should be the same for all
> PermissionRequest (except for the proxy one). You can create a base class
> for GetVisibility and should be the similar to
> RemotePermissionRequest::RecvGetVisibility.

It seems the visibility handling is only used in b2g and oop case. Do we have to also implement GetVisibility for non-oop case?

> 
> ::: dom/interfaces/base/nsIContentPermissionPrompt.idl
> @@ +65,1 @@
> >    readonly attribute nsIDOMElement element;
> 
> |element| should be removed according to comment #4.

Do we have a strong reason for removing this?

Comment 20

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

(In reply to Kershaw Chang [:kershaw] from comment #19)
> (In reply to Shih-Chiang Chien [:schien] (UTC+8) from comment #18)
> > Comment on attachment 8526617 [details] [diff] [review]
> > Let WyciwygChannel support nested oop iframe - V3
> > 
> > Review of attachment 8526617 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > See my comments below and you'll need to check all ContentPermissionPrompt
> > implementations on all product.
> > Firefox:
> > http://dxr.mozilla.org/mozilla-central/source/browser/components/
> > nsBrowserGlue.js#2002
> > Fennec:
> > http://dxr.mozilla.org/mozilla-central/source/mobile/android/components/
> > ContentPermissionPrompt.js#31
> > WebRT:
> > http://dxr.mozilla.org/mozilla-central/source/webapprt/ContentPermission.
> > js#17
> > Metro:
> > http://dxr.mozilla.org/mozilla-central/source/browser/metro/components/
> > ContentPermissionPrompt.js#27
> 
> Thanks. I will check these implementations.
> > 
> > ::: b2g/components/ContentPermissionPrompt.js
> > @@ +325,4 @@
> > >  
> > > +        // Monitor the frame visibility and cancel the request if the frame goes
> > > +        // away but the request is still here.
> > > +        frame.addEventListener("mozbrowservisibilitychange", onVisibilityChange);
> > 
> > |frame| will not existed after nsIContentPermissionRequest.element is
> > removed. The visibility change detection is for dismissing prompt UI while
> > user switch app to background because permission prompt UI is a system-wise
> > overlay (see Bug 832412). I'm not sure if that's still true for current
> > Gaia. Ask @alive about the Gaia restriction and Ask @jenny for UX input if
> > we want to change the behavior.
> 
> nsIContentPermissionRequest.element is only implemented in
> nsContentPermissionRequestProxy::GetElement which is already used for
> current oop case. I think we should keep nsIContentPermissionRequest.element
> as possible as we can, since we don't want to change the current behavior,
> do we?
> I think the problem is can we use the element of 1st level frame for nested
> oop case. I think I can create a test app to test this.
>  
The problem is we are technically unable to keep the same behavior of |element| in nested-oop case. The corresponding iframe only existed in that content process and its direct parent process and I think making non-oop, 1st level oop, nested oop using different API will cause huge maintain effort. I would like to see a general solution for all three cases and end the pain.
> > 
> > ::: dom/base/nsDocument.cpp
> > @@ +11776,5 @@
> > >  
> > > +NS_IMETHODIMP
> > > +nsPointerLockPermissionRequest::GetVisibility(nsIContentPermissionRequestCallback *aCallback)
> > > +{
> > > +  return NS_OK;
> > 
> > The visibility handling for non-oop case should be the same for all
> > PermissionRequest (except for the proxy one). You can create a base class
> > for GetVisibility and should be the similar to
> > RemotePermissionRequest::RecvGetVisibility.
> 
> It seems the visibility handling is only used in b2g and oop case. Do we
> have to also implement GetVisibility for non-oop case?
The visibility handling is b2g only and it is a bug that we only cover OOP case. We definitely want to fix it as well in this bug.
> 
> > 
> > ::: dom/interfaces/base/nsIContentPermissionPrompt.idl
> > @@ +65,1 @@
> > >    readonly attribute nsIDOMElement element;
> > 
> > |element| should be removed according to comment #4.
> 
> Do we have a strong reason for removing this?
Yes and see my comment above.

Comment 21

[Kershaw Chang [:kershaw]]

Attachment: Let PContent manage PContentPermissionRequest, v4

Hi SC,

Could you help to look at this patch?

Thanks.

Comment 22

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Comment on attachment 8535521 [details] [diff] [review]
Let PContent manage PContentPermissionRequest, v4

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

Please see my comment below.

::: b2g/components/ContentPermissionPrompt.js
@@ +316,5 @@
> +        // Monitor the frame visibility and cancel the request if the frame goes
> +        // away but the request is still here.
> +        request.requester.onVisibilityChange = {
> +          notifyVisibility: function(isVisible) {
> +            if (isVisible === true)

|if (isVisible)| is enough.

@@ +435,1 @@
>      while (targetWindow.realFrameElement) {

Do we still need to iterate through window? I thought |request.window.realFrameElement| will give you the topmost frame in system app already.

::: dom/base/nsContentPermissionHelper.cpp
@@ +25,5 @@
>  #include "nsJSUtils.h"
>  #include "nsISupportsPrimitives.h"
>  #include "nsServiceManagerUtils.h"
> +#include "nsIDocument.h"
> +#include "nsIDOMEventListener.h"

You wont' need this if you include it in nsContentPermissionHelper.h as my comment.

@@ +294,5 @@
>  
> +nsContentPermissionRequester::nsContentPermissionRequester(nsPIDOMWindow* aWindow)
> +  : mWindow(aWindow)
> +{
> +  MOZ_COUNT_CTOR(nsContentPermissionRequester);

You don't need this because nsContentPermissionRequester inherits nsISupports.

@@ +299,5 @@
> +}
> +
> +nsContentPermissionRequester::~nsContentPermissionRequester()
> +{
> +  MOZ_COUNT_DTOR(nsContentPermissionRequester);

You don't need this, either.

@@ +303,5 @@
> +  MOZ_COUNT_DTOR(nsContentPermissionRequester);
> +
> +  if (!mWindow) {
> +    return;
> +  }

MOZ_ASSERT(mWindow)?

@@ +310,5 @@
> +  if (!doc) {
> +    return;
> +  }
> +
> +  nsCOMPtr<EventTarget> target = do_QueryInterface(doc);

do_QueryInterface(mWindow->GetDoc());?

@@ +325,5 @@
> +nsContentPermissionRequester::GetVisibility(nsIContentPermissionRequestCallback* aCallback)
> +{
> +  NS_ENSURE_ARG_POINTER(aCallback);
> +
> +  if (!mWindow) {

Looks like we don't expect nsContentPermissionRequester can be used without a vaild window. We can add |MOZ_ASSERT(mWindow)| in constructor and all other function that reference to it.
If you want to use weak reference on mWindow as my comment, you'll have to keep these reference check.

@@ +357,5 @@
> +
> +  mOnChangeCallback = aCallback;
> +
> +  doc->AddSystemEventListener(NS_LITERAL_STRING("visibilitychange"),
> +                              this, /* listener */

I'll use style like |/* listener = */ this,|.

@@ +403,5 @@
> +nsRemoteContentPermissionRequester::nsRemoteContentPermissionRequester(ContentPermissionRequestParent* aParent)
> +  : mParent(aParent)
> +  , mWaitGettingResult(false)
> +{
> +  MOZ_COUNT_CTOR(nsRemoteContentPermissionRequester);

you don't need this.

@@ +408,5 @@
> +}
> +
> +nsRemoteContentPermissionRequester::~nsRemoteContentPermissionRequester()
> +{
> +  MOZ_COUNT_DTOR(nsRemoteContentPermissionRequester);

you don't need this.

@@ +522,5 @@
> +
> +  nsCOMPtr<nsIDOMWindow> window = do_QueryInterface(frame->OwnerDoc()->GetWindow());
> +  NS_ENSURE_TRUE(window, NS_ERROR_FAILURE);
> +
> +  NS_IF_ADDREF(*aRequestingWindow = window);

Use |window.forget(aRequestingWindow);| to transfer ownership instead of doing ref counting manually.

@@ +625,5 @@
>  
> +void
> +nsContentPermissionRequestProxy::NotifyVisibility(const bool& aIsVisible)
> +{
> +  if (mRequester) {

use |MOZ_ASSERT(mRequester)| should be fine.

@@ +632,5 @@
> +}
> +
> +NS_IMETHODIMP
> +nsContentPermissionRequestProxy::GetRequester(nsIContentPermissionRequester * *aRequester)
> +{

NS_ENSURE_ARG_POINTER(aRequester);

@@ +659,5 @@
> +}
> +
> +RemotePermissionRequest::~RemotePermissionRequest()
> +{
> +  MOZ_ASSERT(!mIPCOpen, "Protocol must not be open when RemotePermissionRequest is destroyed.");

|MOZ_ASSERT(!mIPCOpen)| is enough.

@@ +666,5 @@
> +  if (!doc) {
> +    return;
> +  }
> +
> +  nsCOMPtr<EventTarget> target = do_QueryInterface(doc);

nsCOMPtr<EventTarget> target = do_QueryInterface(mWindow->GetDoc());
MOZ_ASSERT(target);

Early return destructor isn't really useful IMO. Or, you can just do |if (target) { ... }|.

::: dom/base/nsContentPermissionHelper.h
@@ +8,5 @@
>  #include "nsIContentPermissionPrompt.h"
>  #include "nsTArray.h"
>  #include "nsIMutableArray.h"
>  #include "mozilla/dom/PContentPermissionRequestChild.h"
> +#include "mozilla/DOMEventTargetHelper.h"

I didn't see |DOMEventTargetHelper| beeing used in this file. |#include "nsIDOMEventListener.h"| should be enough.

@@ +16,5 @@
>  #undef LoadImage
>  
>  class nsPIDOMWindow;
>  class nsContentPermissionRequestProxy;
> +class nsIDocShell;

I didn't see |nsIDocShell| being used in this file.

@@ +93,5 @@
> +
> +protected:
> +  virtual ~nsContentPermissionRequester();
> +
> +  nsCOMPtr<nsPIDOMWindow> mWindow;

maybe we should use weak reference, i.e. nsWeakPtr, to prevent reference cycle.

@@ +138,5 @@
>    // Non-owning pointer to the ContentPermissionRequestParent object which owns this proxy.
>    mozilla::dom::ContentPermissionRequestParent* mParent;
>    nsTArray<mozilla::dom::PermissionRequest> mPermissionRequests;
> +
> +  nsCOMPtr<nsIContentPermissionRequester> mRequester;

I think you can use |nsRefPtr<nsRemoteContentPermissionRequest>| here directly. Therefore, you can remove |notifyVisibilityResult| from IDL because it is just a private interface between RequestProxy and RemoteRequester. You'll be able to remove the nsContentPermissionRequester::NotifyVisibilityResult because it does nothing.

@@ +183,5 @@
>    nsCOMPtr<nsPIDOMWindow>               mWindow;
>    bool                                  mIPCOpen;
>  };
>  
> +

nit: remove this extra empty line.

::: dom/interfaces/base/nsIContentPermissionPrompt.idl
@@ +69,5 @@
>   * Interface allows access to a content to request
>   * permission to perform a privileged operation such as
>   * geolocation.
>   */
>  [scriptable, uuid(69a39d88-d1c4-4ba9-9b19-bafc7a1bb783)]

need to change uuid after modifying this interface.

@@ +90,3 @@
>     */
>    readonly attribute nsIDOMWindow window;
> +  readonly attribute nsIContentPermissionRequester requester;

Please add comment for this new attribute.

::: dom/ipc/ContentParent.cpp
@@ +4406,5 @@
> +        tabId = openerTabId;
> +    }
> +
> +    nsRefPtr<TabParent> tp = cpm->GetTabParentByProcessAndTabId(cpId, tabId);
> +    if (!tp) {

simply use |MOZ_ASSERT(!tp)| if we think this should never happen in theory.

::: dom/ipc/ContentProcessManager.cpp
@@ +279,5 @@
>  
> +TabParent*
> +ContentProcessManager::GetTabParentByProcessAndTabId(const ContentParentId& aChildCpId,
> +                                                     const TabId& aChildTabId)
> +{

You can think about how to add ref count inside this function and return |already_AddRefed<TabParent>|. This can ensure ref count is correct for each instance you return.

::: dom/ipc/PContent.ipdl
@@ +832,5 @@
> +     *
> +     * @param aRequests
> +     *   The array of permissions to request.
> +     * @param aPrincipal
> +     *   The principal of the request.

should add |tabId| in comment as well.

::: dom/ipc/PContentPermissionRequest.ipdl
@@ +18,3 @@
>  
>  child:
> +	GetVisibility();

nit: alignment

::: dom/media/MediaPermissionGonk.cpp
@@ +274,5 @@
> +  nsCOMPtr<nsPIDOMWindow> window = static_cast<nsPIDOMWindow*>
> +      (nsGlobalWindow::GetInnerWindowWithId(mRequest->InnerWindowID()));
> +
> +  nsCOMPtr<nsIContentPermissionRequester> requester =
> +    new nsContentPermissionRequester(window);

You shouldn't create a new instance for every |GetRequester| because it don't match with the semantic of |attribute| in IDL. You'll have to fix in other permission request implementations as well.

Comment 23

[Kershaw Chang [:kershaw]]

(In reply to Shih-Chiang Chien [:schien] (UTC+8) from comment #22)
> Comment on attachment 8535521 [details] [diff] [review]
> Let PContent manage PContentPermissionRequest, v4
> 
> Review of attachment 8535521 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Please see my comment below.
> 

Thanks for your feedback! I'll create the next version asap.

> 
> @@ +93,5 @@
> > +
> > +protected:
> > +  virtual ~nsContentPermissionRequester();
> > +
> > +  nsCOMPtr<nsPIDOMWindow> mWindow;
> 
> maybe we should use weak reference, i.e. nsWeakPtr, to prevent reference
> cycle.
> 
I don't quite understand why we need nsWeakPtr for mWindow. It seems we don't have a reference cycle here?
Could you explain more?
Thanks.

Comment 24

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

(In reply to Kershaw Chang [:kershaw] from comment #23)
> (In reply to Shih-Chiang Chien [:schien] (UTC+8) from comment #22)
> > @@ +93,5 @@
> > > +
> > > +protected:
> > > +  virtual ~nsContentPermissionRequester();
> > > +
> > > +  nsCOMPtr<nsPIDOMWindow> mWindow;
> > 
> > maybe we should use weak reference, i.e. nsWeakPtr, to prevent reference
> > cycle.
> > 
> I don't quite understand why we need nsWeakPtr for mWindow. It seems we
> don't have a reference cycle here?
> Could you explain more?
> Thanks.

Reference cycle is created while registering system listener.

>nsGlobalWindow -> nsDocument -> EventListenerManager -> CallbackObjectHolder -> nsContentPermissionRequester
>   ^_________________________________________________________________________________|

You can resolve this by either making nsContentPermissionRequester cycle collectable or breaking the cycle as soon as possible.

Comment 25

[Kershaw Chang [:kershaw]]

> @@ +435,1 @@
> >      while (targetWindow.realFrameElement) {
> 
> Do we still need to iterate through window? I thought
> |request.window.realFrameElement| will give you the topmost frame in system
> app already.
> 
I think we still need to do this for in-process app, am I right?

Comment 26

[Kershaw Chang [:kershaw]]

Attachment: Let PContent manage PContentPermissionRequest, v5

Hi SC,

Please help to look at this patch. 

Thanks.

Comment 27

[Kershaw Chang [:kershaw]]

Attachment: interdiff for v5 and v4

Comment 28

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Comment on attachment 8537108 [details] [diff] [review]
Let PContent manage PContentPermissionRequest, v5

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

Mostly good with following comments. You should separate this patch into 5 parts: platform, firefox, b2g, android, and metro for assigning different reviewers.

::: dom/base/nsContentPermissionHelper.cpp
@@ +293,5 @@
>  
> +nsContentPermissionRequester::nsContentPermissionRequester(nsPIDOMWindow* aWindow)
> +{
> +  mWindow = do_GetWeakReference(aWindow);
> +  MOZ_ASSERT(mWindow);

MOZ_ASSERT(aWindow) at the begin of this function would be more meaningful.

@@ +299,5 @@
> +
> +nsContentPermissionRequester::~nsContentPermissionRequester()
> +{
> +  nsCOMPtr<nsPIDOMWindow> window = do_QueryReferent(mWindow);
> +  MOZ_ASSERT(window);

mWindow is a weak reference which means you need to tolerate it might be released before you use it.

@@ +305,5 @@
> +  nsCOMPtr<EventTarget> target = do_QueryInterface(window->GetDoc());
> +  if (target) {
> +    target->RemoveSystemEventListener(NS_LITERAL_STRING("visibilitychange"),
> +                                      /* listener */ this,
> +                                      /* use capture */ true);

Removing listener until dtor might be too late because the reference from window will only disappear until the window object being cycle collected. Could we do this right after PermissionRequest object being destroyed?

@@ +316,5 @@
> +  NS_ENSURE_ARG_POINTER(aCallback);
> +
> +  nsCOMPtr<nsPIDOMWindow> window = do_QueryReferent(mWindow);
> +  if (!window) {
> +    MOZ_ASSERT(false);

tolerate weak reference, too.

@@ +342,5 @@
> +    MOZ_ASSERT(false);
> +    return NS_ERROR_FAILURE;
> +  }
> +
> +  nsCOMPtr<nsIDocument> doc = window->GetDoc();

GetDoc() will create a new nsIDocument if no one exists, which might not be the behavior we want. Use GetExtantDoc() instead.

@@ +376,5 @@
> +  }
> +
> +  nsCOMPtr<nsIDocument> doc =
> +    do_QueryInterface(aEvent->InternalDOMEvent()->GetTarget());
> +  if (mOnChangeCallback && doc) {

Shouldn't |doc| always exist while visibility changing? If so, put MOZ_ASSERT(doc).

@@ +624,5 @@
>    : mRequest(aRequest)
>    , mWindow(aWindow)
>    , mIPCOpen(false)
>  {
> +  nsCOMPtr<nsIDocument> doc = mWindow->GetDoc();

GetExtantDoc(), too.

@@ +639,5 @@
> +  MOZ_ASSERT(!mIPCOpen);
> +
> +  nsCOMPtr<EventTarget> target = do_QueryInterface(mWindow->GetDoc());
> +  if (target) {
> +    target->RemoveSystemEventListener(NS_LITERAL_STRING("visibilitychange"),

Removing listener until dtor might be too late because the reference from window will only disappear until the window object being cycle collected. Could we do this right after IPDL being destroyed?

@@ +721,5 @@
> +  }
> +
> +  nsAutoString type;
> +  aEvent->GetType(type);
> +  if (!type.EqualsLiteral("visibilitychange")) {

"visibilitychange" appears multiple times, a const will prevent typo.

::: dom/base/nsContentPermissionHelper.h
@@ +80,5 @@
>    static nsresult
>    AskPermission(nsIContentPermissionRequest* aRequest, nsPIDOMWindow* aWindow);
>  };
>  
> +class nsContentPermissionRequester : public nsIContentPermissionRequester

append |MOZ_FINAL| if you don't expect anyone will inherit this class.

@@ +88,5 @@
> +  NS_DECL_ISUPPORTS
> +  NS_DECL_NSICONTENTPERMISSIONREQUESTER
> +  NS_DECL_NSIDOMEVENTLISTENER
> +
> +  nsContentPermissionRequester(nsPIDOMWindow* aWindow);

add |explicit| prefix.

@@ +90,5 @@
> +  NS_DECL_NSIDOMEVENTLISTENER
> +
> +  nsContentPermissionRequester(nsPIDOMWindow* aWindow);
> +
> +protected:

use private if there is no subclass.

@@ +97,5 @@
> +  nsWeakPtr mWindow;
> +  nsCOMPtr<nsIContentPermissionRequestCallback> mOnChangeCallback;
> +};
> +
> +class nsRemoteContentPermissionRequester : public nsIContentPermissionRequester

The |Remote| prefix has been used for class in content process in this file, see RemtoePermissionRequest. Using ContentPermissionRequesterProxy will make it consistent.
BTW, this class should only be used by nsContentPermissionRequestProxy, right? You can consider make it an inner class for preventing misused.

@@ +103,5 @@
> +public:
> +  NS_DECL_ISUPPORTS
> +  NS_DECL_NSICONTENTPERMISSIONREQUESTER
> +
> +  nsRemoteContentPermissionRequester(ContentPermissionRequestParent* aParent)

add |explicit|, too.

@@ +107,5 @@
> +  nsRemoteContentPermissionRequester(ContentPermissionRequestParent* aParent)
> +    : mParent(aParent)
> +    , mWaitGettingResult(false) {}
> +  void NotifyVisibilityResult(const bool& aIsVisible);
> +protected:

private here, too. (And extra line before it)

@@ +138,1 @@
>   private:

extra line between these two lines.

::: dom/ipc/ContentProcessManager.cpp
@@ +284,5 @@
> +  MOZ_ASSERT(NS_IsMainThread());
> +
> +  auto iter = mContentParentMap.find(aChildCpId);
> +  if (NS_WARN_IF(iter == mContentParentMap.end())) {
> +    ASSERT_UNLESS_FUZZING();

Why not use MOZ_ASSERT?

Comment 29

[Kershaw Chang [:kershaw]]

(In reply to Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) from comment #28)
> Comment on attachment 8537108 [details] [diff] [review]
> Let PContent manage PContentPermissionRequest, v5
> 
> Review of attachment 8537108 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Mostly good with following comments. You should separate this patch into 5
> parts: platform, firefox, b2g, android, and metro for assigning different
> reviewers.
> 
Thanks fro your feedback. I'll upload the next version later.


> @@ +305,5 @@
> > +  nsCOMPtr<EventTarget> target = do_QueryInterface(window->GetDoc());
> > +  if (target) {
> > +    target->RemoveSystemEventListener(NS_LITERAL_STRING("visibilitychange"),
> > +                                      /* listener */ this,
> > +                                      /* use capture */ true);
> 
> Removing listener until dtor might be too late because the reference from
> window will only disappear until the window object being cycle collected.
> Could we do this right after PermissionRequest object being destroyed?
> 
I also plan to remove the listener when the callback is set to null.
BTW, I also put the event handling code into another listener object for saving duplicate code in next version.


> ::: dom/ipc/ContentProcessManager.cpp
> @@ +284,5 @@
> > +  MOZ_ASSERT(NS_IsMainThread());
> > +
> > +  auto iter = mContentParentMap.find(aChildCpId);
> > +  if (NS_WARN_IF(iter == mContentParentMap.end())) {
> > +    ASSERT_UNLESS_FUZZING();
> 
> Why not use MOZ_ASSERT?
Please see Bug 1020172 comment #73.

Comment 30

[Kershaw Chang [:kershaw]]

Attachment: Part1: Add new function to get TabParent in ContentProcessManager

Comment 31

[Kershaw Chang [:kershaw]]

Attachment: Part2: Let PContent manage PContentPermissionRequest, v6

Comment 32

[Kershaw Chang [:kershaw]]

Attachment: Part3: ContentPermissionPrompt change in b2g

Comment 33

[Kershaw Chang [:kershaw]]

Attachment: Part4: ContentPermissionPrompt change in browser

Comment 34

[Kershaw Chang [:kershaw]]

Attachment: Part5: ContentPermissionPrompt change in metro

Comment 35

[Kershaw Chang [:kershaw]]

Attachment: Part6: ContentPermissionPrompt change in android

Comment 36

[Kershaw Chang [:kershaw]]

Hi SC,

Could you help to look at these patches?
Summary of changes:
1. Last comments are addressed.
2. Add a new listener to handle "visibilitychange" event.
3. Allow to set null to visibility change callback.

Thanks.

Comment 37

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Comment on attachment 8542878 [details] [diff] [review]
Part1: Add new function to get TabParent in ContentProcessManager

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

::: dom/ipc/ContentProcessManager.h
@@ +16,5 @@
>  
>  namespace mozilla {
>  namespace dom {
>  class ContentParent;
> +class Tabparent;

TabParent? You probably don't need this if your patch pass compilation.

Comment 38

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Comment on attachment 8542879 [details] [diff] [review]
Part2: Let PContent manage PContentPermissionRequest, v6

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

::: dom/base/nsContentPermissionHelper.cpp
@@ +98,5 @@
> +  }
> +
> +  nsCOMPtr<EventTarget> target = do_QueryInterface(window->GetExtantDoc());
> +  if (target) {
> +    target->RemoveSystemEventListener(NS_LITERAL_STRING(kVisibilityChange),

I'm not sure if we need to prevent calling |RemoveSystemEventListener| multiple times.

@@ +114,5 @@
> +already_AddRefed<nsIContentPermissionRequestCallback>
> +VisibilityChangeListener::GetCallback()
> +{
> +  if (mCallback) {
> +    return mCallback.forget();

Calling |forget()| directly on mCallback will make mCallback null. If you want copy the reference you'll need to do
  nsCOMPtr<nsIContentPermissionRequestCallback> callback;
  return callback.forget();

Or, rename it to |TakeCallback| if you intend to transfer the ownership.

::: dom/base/nsContentPermissionHelper.h
@@ +16,5 @@
>  #undef LoadImage
>  
>  class nsPIDOMWindow;
>  class nsContentPermissionRequestProxy;
> +class VisibilityChangeListener;

add extra line between separating class declaration and comment below.

::: dom/base/nsDocument.cpp
@@ +12000,5 @@
> +nsPointerLockPermissionRequest::GetRequester(nsIContentPermissionRequester** aRequester)
> +{
> +  NS_ENSURE_ARG_POINTER(aRequester);
> +
> +  NS_IF_ADDREF(*aRequester = mRequester);

I would prefer using |nsCOMPtr::forget| to do ownership transferring instead of manually adding reference count, but the choice is up to you.

ex:
nsCOMPtr<nsIContentPermissionRequester> = mRequester;
mRequester.forget(aRequester);

::: dom/ipc/ContentParent.cpp
@@ +4578,5 @@
> +        tabId = openerTabId;
> +    }
> +
> +    nsRefPtr<TabParent> tp = cpm->GetTabParentByProcessAndTabId(cpId, tabId);
> +    MOZ_ASSERT(tp);

Using ASSERT_UNLESS_FUZZING here according to Bug 1020172 comment #73? If so, we'll still need early return while |tp| is null.

::: dom/media/MediaPermissionGonk.cpp
@@ +158,5 @@
>    }
> +
> +  nsCOMPtr<nsPIDOMWindow> window = static_cast<nsPIDOMWindow*>
> +    (nsGlobalWindow::GetInnerWindowWithId(mRequest->InnerWindowID()));
> +  mRequester = new nsContentPermissionRequester(window);

|mRequester = new nsContentPermissionRequester(GetOwner())|?

Comment 39

[Kershaw Chang [:kershaw]]

(In reply to Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) from comment #38)
> Comment on attachment 8542879 [details] [diff] [review]
> Part2: Let PContent manage PContentPermissionRequest, v6
> 
> Review of attachment 8542879 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/base/nsContentPermissionHelper.cpp
> @@ +98,5 @@
> > +  }
> > +
> > +  nsCOMPtr<EventTarget> target = do_QueryInterface(window->GetExtantDoc());
> > +  if (target) {
> > +    target->RemoveSystemEventListener(NS_LITERAL_STRING(kVisibilityChange),
> 
> I'm not sure if we need to prevent calling |RemoveSystemEventListener|
> multiple times.
> 
I've done a test on my flame. It seems ok to call RemoveSystemEventListener multiple times.

Comment 40

[Kershaw Chang [:kershaw]]

Attachment: Part1: Add new function to get TabParent in ContentProcessManager - V2

Comment 41

[Kershaw Chang [:kershaw]]

Attachment: Part2: Let PContent manage PContentPermissionRequest, v7

Hi SC,

Would you like to take a look again?
Summary of changes:
1. Fix your last feedback.
2. Move the declaration of VisibilityChangeListener to nsContentPermissionHelper.h to pass compilation.

Thanks.

Comment 42

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Comment on attachment 8544422 [details] [diff] [review]
Part2: Let PContent manage PContentPermissionRequest, v7

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

f+ with some suggestion. You may start the r? procedure in your next revision. :)

::: dom/base/nsContentPermissionHelper.cpp
@@ +414,5 @@
> +  callback.forget(aCallback);
> +  return NS_OK;
> +}
> +
> +NS_IMPL_ISUPPORTS(nsContentPermissionRequester, nsIContentPermissionRequester)

I'll prefer moving this line before all other function definitions of nsContentPermissionRequester.

@@ +466,5 @@
> +    mOnChangeCallback->NotifyVisibility(aIsVisible);
> +  }
> +}
> +
> +NS_IMPL_ISUPPORTS(nsContentPermissionRequestProxy::nsContentPermissionRequesterProxy,

Move it before other function definitions of nsContentPermissionRequesterProxy.

@@ +639,5 @@
> +  mRequester->NotifyVisibilityResult(aIsVisible);
> +}
> +
> +NS_IMETHODIMP
> +nsContentPermissionRequestProxy::GetRequester(nsIContentPermissionRequester * *aRequester)

nit: nsIContentPermissionRequester** aRequester

@@ +643,5 @@
> +nsContentPermissionRequestProxy::GetRequester(nsIContentPermissionRequester * *aRequester)
> +{
> +  NS_ENSURE_ARG_POINTER(aRequester);
> +
> +  NS_IF_ADDREF(*aRequester = mRequester);

using nsCOMPtr.forget() technique here.

::: dom/base/nsContentPermissionHelper.h
@@ +37,5 @@
> +  void SetCallback(nsIContentPermissionRequestCallback* aCallback);
> +  already_AddRefed<nsIContentPermissionRequestCallback> GetCallback();
> +
> +  NS_DECL_ISUPPORTS
> +  NS_DECL_NSIDOMEVENTLISTENER

I'd prefer move NS_DECL_xxx to the beginning of public block.

::: dom/geolocation/nsGeolocation.cpp
@@ +499,5 @@
> +nsGeolocationRequest::GetRequester(nsIContentPermissionRequester** aRequester)
> +{
> +  NS_ENSURE_ARG_POINTER(aRequester);
> +
> +  NS_IF_ADDREF(*aRequester = mRequester);

nsCOMPtr.forget technique here, too.

Comment 43

[Kershaw Chang [:kershaw]]

Attachment: Part1: Add new function to get TabParent in ContentProcessManager

Hi Kyle,

Please take a look at this patch.

Thanks.

Comment 44

[Kershaw Chang [:kershaw]]

Attachment: Part2: Let PContent manage PContentPermissionRequest, v8

Hi Fabrice,

Please help to review this patch.

Thanks.

Comment 45

[Kershaw Chang [:kershaw]]

@mbrubeck, @fabrice, @mfinkle, and @felipc,

Please help to review the changes related to B2G/Firefox/Fennec/Metro.
Thanks.

Comment 46

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 8547314 [details] [diff] [review]
Part1: Add new function to get TabParent in ContentProcessManager

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

r=me

Comment 47

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8542882 [details] [diff] [review]
Part4: ContentPermissionPrompt change in browser

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

Sorry, this patch doesn't look right to me. In Firefox desktop, this nsBrowserGlue code is running in the parent (UI) process.  And the function _getBrowserForRequest should return the <browser> element which generated this request, because the permission requests are tied to a specific tab.  You can't replace it with the top-level window (which is the chrome Firefox window) because of that.

The parent process can't see anything deeper than the <browser> element (because all of the content windows are in the child process), so you've got to still keep a reference to the element that originated the request.

Comment 48

[Kershaw Chang [:kershaw]]

(In reply to :Felipe Gomes from comment #47)
> Comment on attachment 8542882 [details] [diff] [review]
> Part4: ContentPermissionPrompt change in browser
> 
> Review of attachment 8542882 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Sorry, this patch doesn't look right to me. In Firefox desktop, this
> nsBrowserGlue code is running in the parent (UI) process.  And the function
> _getBrowserForRequest should return the <browser> element which generated
> this request, because the permission requests are tied to a specific tab. 
> You can't replace it with the top-level window (which is the chrome Firefox
> window) because of that.
> 
> The parent process can't see anything deeper than the <browser> element
> (because all of the content windows are in the child process), so you've got
> to still keep a reference to the element that originated the request.

Hi Felipe,
Thanks for your feedback. I'll add the element back in nsIContentPermissionRequest interface.

Comment 49

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

(In reply to Kershaw Chang [:kershaw] from comment #48)
> (In reply to :Felipe Gomes from comment #47)
> > Comment on attachment 8542882 [details] [diff] [review]
> > Part4: ContentPermissionPrompt change in browser
> > 
> > Review of attachment 8542882 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Sorry, this patch doesn't look right to me. In Firefox desktop, this
> > nsBrowserGlue code is running in the parent (UI) process.  And the function
> > _getBrowserForRequest should return the <browser> element which generated
> > this request, because the permission requests are tied to a specific tab. 
> > You can't replace it with the top-level window (which is the chrome Firefox
> > window) because of that.
> > 
> > The parent process can't see anything deeper than the <browser> element
> > (because all of the content windows are in the child process), so you've got
> > to still keep a reference to the element that originated the request.
> 
> Hi Felipe,
> Thanks for your feedback. I'll add the element back in
> nsIContentPermissionRequest interface.

Hi Felipe, I think there is a misunderstanding about the "window" attribute here. The "window" attribute still represents the inner-most element in parent process. The logic in this patch should be the same as before.

Hi Kershaw, please let me know if I misunderstand you patch.

Comment 50

[:Felipe Gomes (needinfo for replies!)]

(In reply to Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) from comment #49)
> (In reply to Kershaw Chang [:kershaw] from comment #48)
> > (In reply to :Felipe Gomes from comment #47)
> > > Comment on attachment 8542882 [details] [diff] [review]
> > > Part4: ContentPermissionPrompt change in browser
> > > 
> > > Review of attachment 8542882 [details] [diff] [review]:
> > > -----------------------------------------------------------------
> > > 
> > > Sorry, this patch doesn't look right to me. In Firefox desktop, this
> > > nsBrowserGlue code is running in the parent (UI) process.  And the function
> > > _getBrowserForRequest should return the <browser> element which generated
> > > this request, because the permission requests are tied to a specific tab. 
> > > You can't replace it with the top-level window (which is the chrome Firefox
> > > window) because of that.
> > > 
> > > The parent process can't see anything deeper than the <browser> element
> > > (because all of the content windows are in the child process), so you've got
> > > to still keep a reference to the element that originated the request.
> > 
> > Hi Felipe,
> > Thanks for your feedback. I'll add the element back in
> > nsIContentPermissionRequest interface.
> 
> Hi Felipe, I think there is a misunderstanding about the "window" attribute
> here. The "window" attribute still represents the inner-most element in
> parent process. The logic in this patch should be the same as before.
> 
> Hi Kershaw, please let me know if I misunderstand you patch.

Hi schien, I guess I don't understand how that can be true. What exactly is this element? The `window` and all of the content elements are in the child process. I don't know of any object in the parent that represents the content, can be QI'ed to nsIDocShell, and has the correct .chromeEventHandler property returning a browser element.  Unless this works due to the use of CPOWs, but we shouldn't use them.

Comment 51

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

(In reply to :Felipe Gomes from comment #50)
> (In reply to Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) from comment
> #49)
> > (In reply to Kershaw Chang [:kershaw] from comment #48)
> > > (In reply to :Felipe Gomes from comment #47)
> > > > Comment on attachment 8542882 [details] [diff] [review]
> > > > Part4: ContentPermissionPrompt change in browser
> > > > 
> > > > Review of attachment 8542882 [details] [diff] [review]:
> > > > -----------------------------------------------------------------
> > > > 
> > > > Sorry, this patch doesn't look right to me. In Firefox desktop, this
> > > > nsBrowserGlue code is running in the parent (UI) process.  And the function
> > > > _getBrowserForRequest should return the <browser> element which generated
> > > > this request, because the permission requests are tied to a specific tab. 
> > > > You can't replace it with the top-level window (which is the chrome Firefox
> > > > window) because of that.
> > > > 
> > > > The parent process can't see anything deeper than the <browser> element
> > > > (because all of the content windows are in the child process), so you've got
> > > > to still keep a reference to the element that originated the request.
> > > 
> > > Hi Felipe,
> > > Thanks for your feedback. I'll add the element back in
> > > nsIContentPermissionRequest interface.
> > 
> > Hi Felipe, I think there is a misunderstanding about the "window" attribute
> > here. The "window" attribute still represents the inner-most element in
> > parent process. The logic in this patch should be the same as before.
> > 
> > Hi Kershaw, please let me know if I misunderstand you patch.
> 
> Hi schien, I guess I don't understand how that can be true. What exactly is
> this element? The `window` and all of the content elements are in the child
> process. I don't know of any object in the parent that represents the
> content, can be QI'ed to nsIDocShell, and has the correct
> .chromeEventHandler property returning a browser element.  Unless this works
> due to the use of CPOWs, but we shouldn't use them.

Yeah you're right. I was trying to find a common element in chrome process that can represents the permission requester in both e10s and non-e10s cases. So, how about returning the inner-most nsIDocShell in chrome process for that request?

Comment 52

[Kershaw Chang [:kershaw]]

(In reply to Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) from comment #51)
> (In reply to :Felipe Gomes from comment #50)
> > (In reply to Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) from comment
> > #49)
> > > (In reply to Kershaw Chang [:kershaw] from comment #48)
> > > > (In reply to :Felipe Gomes from comment #47)
> > > > > Comment on attachment 8542882 [details] [diff] [review]
> > > > > Part4: ContentPermissionPrompt change in browser
> > > > > 
> > > > > Review of attachment 8542882 [details] [diff] [review]:
> > > > > -----------------------------------------------------------------
> > > > > 
> > > > > Sorry, this patch doesn't look right to me. In Firefox desktop, this
> > > > > nsBrowserGlue code is running in the parent (UI) process.  And the function
> > > > > _getBrowserForRequest should return the <browser> element which generated
> > > > > this request, because the permission requests are tied to a specific tab. 
> > > > > You can't replace it with the top-level window (which is the chrome Firefox
> > > > > window) because of that.
> > > > > 
> > > > > The parent process can't see anything deeper than the <browser> element
> > > > > (because all of the content windows are in the child process), so you've got
> > > > > to still keep a reference to the element that originated the request.
> > > > 
> > > > Hi Felipe,
> > > > Thanks for your feedback. I'll add the element back in
> > > > nsIContentPermissionRequest interface.
> > > 
> > > Hi Felipe, I think there is a misunderstanding about the "window" attribute
> > > here. The "window" attribute still represents the inner-most element in
> > > parent process. The logic in this patch should be the same as before.
> > > 
> > > Hi Kershaw, please let me know if I misunderstand you patch.
> > 
> > Hi schien, I guess I don't understand how that can be true. What exactly is
> > this element? The `window` and all of the content elements are in the child
> > process. I don't know of any object in the parent that represents the
> > content, can be QI'ed to nsIDocShell, and has the correct
> > .chromeEventHandler property returning a browser element.  Unless this works
> > due to the use of CPOWs, but we shouldn't use them.
> 
> Yeah you're right. I was trying to find a common element in chrome process
> that can represents the permission requester in both e10s and non-e10s
> cases. So, how about returning the inner-most nsIDocShell in chrome process
> for that request?

For e10s, I think nsIDocShell is in child process, am I right?
It seems there is no common element can represent the permission requester in both e10s and non-e10s. Since we already stuck in this for a while, I'd plan to keep the origin way: frame element for e10s and window for non-e10s.

Comment 53

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

You can get corresponding docshell in parent side via following code:

nsRefPtr<TabParent> tp = ...
nsCOMPtr<nsIFrameLoaderOwner> frameLoaderOwner = do_QueryInterface(tp->GetOwnerElement());
nsRefPtr<nsFrameLoader> frameLoader = frameLoaderOwner->GetFrameLoader();
nsCOMPtr<nsIDocShell> docshell = frameLoader->GetExistingDocShell();

Comment 54

[[:fabrice] Fabrice Desré]

Comment on attachment 8547315 [details] [diff] [review]
Part2: Let PContent manage PContentPermissionRequest, v8

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

Kershaw, I'd rather have Ben take a look at these changes.

Comment 55

[:Felipe Gomes (needinfo for replies!)]

(In reply to Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) from comment #51)
> Yeah you're right. I was trying to find a common element in chrome process
> that can represents the permission requester in both e10s and non-e10s
> cases. So, how about returning the inner-most nsIDocShell in chrome process
> for that request?

I don't think that will work. The chrome process doesn't have a useful inner-most nsIDocShell that you can use to differentiate between tabs. If you find something that works, sure, I'm all for it. But note that the function you're modifying in Firefox's front-end is called _getBrowserForRequest. So ultimately that function must return a proper <browser> representing where the request came from.
I suspect that if you start from scratch, what you'll end up with is something analogous to the .request that you are trying to remove :)

Comment 56

[:Felipe Gomes (needinfo for replies!)]

(In reply to :Felipe Gomes from comment #55)
> I suspect that if you start from scratch, what you'll end up with is
> something analogous to the .request that you are trying to remove :)

sry, .element

Comment 57

[[:fabrice] Fabrice Desré]

Comment on attachment 8542881 [details] [diff] [review]
Part3: ContentPermissionPrompt change in b2g

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

lgtm, but I'd really like us to add tests.

Comment 58

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

@felipe, thanks for the explanation. My understanding of Firefox e10s might be totally wrong. @kershaw, you can go ahead with the decision you know that actually works. :)

Comment 59

[Kershaw Chang [:kershaw]]

Attachment: Let PContent manage PContentPermissionRequest, v9

Hi Ben,

Could you please review this v9 patch? The only difference with the previous version is that element is added back in nsIContentPermissionRequest interface.

Thanks.

Comment 60

[Kershaw Chang [:kershaw]]

Attachment: ContentPermissionPrompt change in b2g - v2

Hi Fabrice,

Would you like to take a look again? The difference is about the request.element.

Thanks.

Comment 61

[Kershaw Chang [:kershaw]]

(In reply to Fabrice Desré [:fabrice] from comment #57)
> Comment on attachment 8542881 [details] [diff] [review]
> Part3: ContentPermissionPrompt change in b2g
> 
> Review of attachment 8542881 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> lgtm, but I'd really like us to add tests.

Thanks for reviewing. I'll start to study how to add tests.

Comment 62

[[:fabrice] Fabrice Desré]

Kershaw, even if you have r+ on the patches, please write tests before landing. We had too many issues with regressions around this code in the past.

Comment 63

[Kershaw Chang [:kershaw]]

Attachment: Let PContent manage PContentPermissionRequest, v10

Rebase and fix some errors found on test run.

Comment 64

[Kershaw Chang [:kershaw]]

Attachment: Test case for visibilitychange

Fabrice, please take a look at the patch that tests whether the prompt is canceled when the iframe is invisible.

Test result on b2g emulator:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=992a6b1c441f

Comment 65

[[:fabrice] Fabrice Desré]

Comment on attachment 8560296 [details] [diff] [review]
Test case for visibilitychange

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

r=me with nits addressed. thanks for doing that!

::: b2g/components/test/mochitest/test_permission_visibilitychange.html
@@ +18,5 @@
> +var gUrl = SimpleTest.getTestFileURL("permission_handler_chrome.js");
> +var gScript = SpecialPowers.loadChromeScript(gUrl);
> +
> +function testDone() {
> +  gScript.sendAsyncMessage('teardown', '');

nit: stick to double quotes for strings everywhere in this file.

::: testing/mochitest/b2g_start_script.js
@@ +66,5 @@
>    container.parentNode.appendChild(popupIframe);
>  }
>  container.addEventListener('mozbrowseropenwindow', openWindow);
> +container.addEventListener('mozbrowsershowmodalprompt', function (e) {
> +  if (e.detail.message == "setVisible::false") {

nit: it looks like we are using single quotes in this part of the file.

Comment 66

[Kershaw Chang [:kershaw]]

Attachment: Patch3: Test case for visibilitychange, r=fabrice

Fix nit.

Comment 67

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 8560291 [details] [diff] [review]
Let PContent manage PContentPermissionRequest, v10

Apologies, I'm heading out for about a week, so passing to khuey. If you can't get to it just bounce back to me and I'll grab it as soon as I get back.

Comment 68

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 8560291 [details] [diff] [review]
Let PContent manage PContentPermissionRequest, v10

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

Sorry for the delays here.  r=me with comments.

::: dom/base/nsContentPermissionHelper.cpp
@@ +42,5 @@
> +  MOZ_ASSERT(aWindow);
> +
> +  mWindow = do_GetWeakReference(aWindow);
> +  nsCOMPtr<nsPIDOMWindow> window = do_QueryReferent(mWindow);
> +  if (window) {

Why don't you just use aWindow directly?

@@ +69,5 @@
> +
> +  if (mCallback) {
> +    mCallback->NotifyVisibility(!doc->Hidden());
> +  }
> +  return NS_OK;

nit: \n before return

::: dom/base/nsContentPermissionHelper.h
@@ +135,3 @@
>   private:
> +  class nsContentPermissionRequesterProxy MOZ_FINAL : public nsIContentPermissionRequester
> +  {

everything in this class should be two spaces to the left

@@ +138,5 @@
> +    public:
> +      NS_DECL_ISUPPORTS
> +      NS_DECL_NSICONTENTPERMISSIONREQUESTER
> +
> +      explicit nsContentPermissionRequesterProxy(mozilla::dom::ContentPermissionRequestParent* aParent)

Maybe put a using declaration in this class so you don't have to fully qualify this here?

@@ +147,5 @@
> +
> +    private:
> +      virtual ~nsContentPermissionRequesterProxy() {}
> +
> +      mozilla::dom::ContentPermissionRequestParent* mParent;

and here?

::: dom/ipc/ContentChild.cpp
@@ +2551,5 @@
> +
> +bool
> +ContentChild::DeallocPContentPermissionRequestChild(PContentPermissionRequestChild* actor)
> +{
> +    RemotePermissionRequest* child = static_cast<RemotePermissionRequest*>(actor);

You could just use auto, but it doesn't really matter.

Comment 69

[Kershaw Chang [:kershaw]]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #68)
> Comment on attachment 8560291 [details] [diff] [review]
> Let PContent manage PContentPermissionRequest, v10
> 
> Review of attachment 8560291 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Sorry for the delays here.  r=me with comments.
> 
Thanks for reviewing this.

> @@ +138,5 @@
> > +    public:
> > +      NS_DECL_ISUPPORTS
> > +      NS_DECL_NSICONTENTPERMISSIONREQUESTER
> > +
> > +      explicit nsContentPermissionRequesterProxy(mozilla::dom::ContentPermissionRequestParent* aParent)
> 
> Maybe put a using declaration in this class so you don't have to fully
> qualify this here?

Do you mean something like this?

> +    public:
> +      using namespace mozilla::dom;
> +
> +      NS_DECL_ISUPPORTS
> +      NS_DECL_NSICONTENTPERMISSIONREQUESTER
> +
> +      explicit nsContentPermissionRequesterProxy(mozilla::dom::ContentPermissionRequestParent* aParent)

But, it seems illegal to declare "using namespace" within class. Can I just put using declaration outside the class scope?

Comment 70

[Kershaw Chang [:kershaw]]

After migrating PContentPermissionRequest from PBrowser to PContent, it can not pass a mochitest chrome test with e10s enabled. The failure shows that there are leaks before the content process is terminated. It seems that e10s also has some similar issues before [1].
I found that the shutdown leaks are collected before the content process is ended. At that time, PContentPermissionRequest is not deleted. I think we are not really leaked, since PContentPermissionRequest will be released when PContent is destroyed. A possible solution is to delete the PContentPermissionRequest when the tab it belongs is closed.

Try push:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=80490f1d215f

[1] https://dxr.mozilla.org/mozilla-central/search?q=%22leaked+until+shutdown%22&case=false

Comment 71

[Kershaw Chang [:kershaw]]

Attachment: Bug 1020179 - Close undeleted ContentPermissionRequest when the tab is destroying

Please take a look at comment #70.
Kyle, do you think it makes sense to delete the unclosed ContentPermissionRequest when the tab is about destroying?

Thanks.

Comment 72

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 8583649 [details] [diff] [review]
Bug 1020179 - Close undeleted ContentPermissionRequest when the tab is destroying

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

nsContentPermissionRequestProxy already checks mParent->IsBeingDestroyed() before doing anything, so what's the point in tearing down the actor manually?

::: dom/base/nsContentPermissionHelper.cpp
@@ +289,5 @@
>  }
>  
> +/* static */
> +std::map<PContentPermissionRequestParent*, TabId>
> +nsContentPermissionUtils::mContentPermissionRequestParentMap;

a static std::map is going to create static initializers that we don't want.

Comment 73

[Kershaw Chang [:kershaw]]

(In reply to Kyle Huey [:khuey] ([e-mail]) (PTO starting 4/1, back 4/8) from comment #72)
> Comment on attachment 8583649 [details] [diff] [review]
> Bug 1020179 - Close undeleted ContentPermissionRequest when the tab is
> destroying
> 
> Review of attachment 8583649 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> nsContentPermissionRequestProxy already checks mParent->IsBeingDestroyed()
> before doing anything, so what's the point in tearing down the actor
> manually?

Because the shutdown leak collection for mochietst browser is done before child process end [1]. Moving PContentPermissionRequest from PBrowser to PContent causes some false leaks, since PContentPermissionRequest's lifetime is now tied to PContent not PBrowser.
Besides manually tearing down the actor when the tab is destroyed, do we have a better solution?

[1] https://dxr.mozilla.org/mozilla-central/source/testing/mochitest/browser-test.js#636

Comment 74

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Ok, that sounds fine to me.

Comment 75

[Kershaw Chang [:kershaw]]

Attachment: Close undeleted ContentPermissionRequest when the tab is destroying

Put the static map into an anonymous namespace.

Comment 76

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 8588976 [details] [diff] [review]
Close undeleted ContentPermissionRequest when the tab is destroying

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

::: dom/base/nsContentPermissionHelper.cpp
@@ +295,5 @@
> +  MOZ_ASSERT(NS_IsMainThread());
> +  static std::map<PContentPermissionRequestParent*, TabId> sPermissionRequestParentMap;
> +  return sPermissionRequestParentMap;
> +}
> +} // anonymous namespace

You don't really need the anonymous namespace here, putting it in the function is enough.

Comment 77

[Kershaw Chang [:kershaw]]

Attachment: Part 1: Let PContent manage PContentPermissionRequest, v13, r=fabrice, khuey

Rebase and carry reviewer's name.

Comment 78

[Kershaw Chang [:kershaw]]

Attachment: ContentPermissionPrompt change in b2g, r=fabrice

Carry reviewer's name.

Comment 79

[Kershaw Chang [:kershaw]]

Attachment: Patch3: Test case for visibilitychange, r=fabrice

Rebase

Comment 80

[Kershaw Chang [:kershaw]]

Test run looks good.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b100770bfcbf

Comment 81

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7471229f0a79
https://hg.mozilla.org/integration/mozilla-inbound/rev/8bdd951da164
https://hg.mozilla.org/integration/mozilla-inbound/rev/873428bfc723

Comment 82

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7471229f0a79
https://hg.mozilla.org/mozilla-central/rev/8bdd951da164
https://hg.mozilla.org/mozilla-central/rev/873428bfc723

Comment 83

[Daniel Holbert [:dholbert]]

Comment on attachment 8592047 [details] [diff] [review]
Part 1: Let PContent manage PContentPermissionRequest, v13, r=fabrice, khuey

>+++ b/dom/devicestorage/nsDeviceStorage.cpp
>+  NS_IMETHODIMP GetRequester(nsIContentPermissionRequester** aRequester)
>+  {
>+    NS_ENSURE_ARG_POINTER(aRequester);
>+
>+    nsCOMPtr<nsIContentPermissionRequester> requester = mRequester;
>+    requester.forget(aRequester);
>+    return NS_OK;
>+  }
>+
> private:
>   ~DeviceStorageRequest() {}

This was missing an 'override' annotation, which causes clang 3.6 &
newer to spam a "-Winconsistent-missing-override" build warning, which
busts --enable-warnings-as-errors builds (with clang 3.6+).

Additionally, this shouldn't use NS_IMETHODIMP; that's only to be used for a method implementation *outside of a class definition*. This should've used NS_IMETHOD instead. (They expand to almost the same thing; NS_IMETHOD just has 'virtual', which doesn't work outside of a class definition, which is why the *IMP version exists.)  See bug 1026350 comment 77 through 81 for more details.

I pushed a followup to fix both of these issues, with blanket r+ that ehsan granted me for 'override' fixes over in bug 1126447 comment 2:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e2fd54cbb309

Comment 84

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e2fd54cbb309