1102060 - PScreenManager::ScreenForBrowser doesn't work in nested content process

Status: RESOLVED FIXED

(Core :: DOM: Content Processes, defect)

Description

[Kershaw Chang [:kershaw]]

prio(high) sync ScreenForBrowser(PBrowser aBrowser)
      returns (ScreenDetails screen,
               bool success);

Since the TabParent may live in content process now, PBrowser may have to change to PBrowserOrId for above IPC.

Comment 1

[C.M.Chang[:chunmin]]

Attachment: bug-1102060-fix.patch

Comment 2

[Kershaw Chang [:kershaw]]

Comment on attachment 8545106 [details] [diff] [review]
bug-1102060-fix.patch

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

Overall looks ok, but you have to watch out those nits.

::: dom/ipc/ContentProcessManager.cpp
@@ +280,5 @@
> +ContentProcessManager::GetTabParentByProcessAndTabId(const ContentParentId& aChildCpId,
> +                                                     const TabId& aChildTabId)
> +{
> +  MOZ_ASSERT(NS_IsMainThread());
> +  

nit: trailing space

@@ +286,5 @@
> +  if (NS_WARN_IF(iter == mContentParentMap.end())) {
> +    ASSERT_UNLESS_FUZZING();
> +    return nullptr;
> +  }
> +  

nit: trailing space

::: dom/ipc/ContentProcessManager.h
@@ +108,5 @@
>     */
>    bool GetRemoteFrameOpenerTabId(const ContentParentId& aChildCpId,
>                                   const TabId& aChildTabId,
>                                   /*out*/ TabId* aOpenerTabId);
> + 

nit: trailing space

@@ +115,5 @@
> +   */
> +  already_AddRefed<TabParent>
> +  GetTabParentByProcessAndTabId(const ContentParentId& aChildCpId,
> +                                const TabId& aChildTabId);
> +  

nit: trailing space

::: dom/ipc/ScreenManagerParent.cpp
@@ +130,5 @@
>  
>    // Find the mWidget associated with the tabparent, and then return
>    // the nsIScreen it's on.
> +  ContentParent* cp = static_cast<ContentParent*>(this->Manager());
> +  nsRefPtr<TabParent> tabParent = GetTopLevelTabParentByProcessAndTabId(cp->ChildID(), aTabId);

This line is too long.

@@ +169,5 @@
> +{
> +  MOZ_ASSERT(NS_IsMainThread());
> +
> +  ContentProcessManager* cpm = ContentProcessManager::GetSingleton();
> + 

nit: trailing space

@@ +171,5 @@
> +
> +  ContentProcessManager* cpm = ContentProcessManager::GetSingleton();
> + 
> +  // To get the upper ContentParentId by the current ContentParentId
> +  ContentParentId upperCpId;

parentCpId is better

@@ +172,5 @@
> +  ContentProcessManager* cpm = ContentProcessManager::GetSingleton();
> + 
> +  // To get the upper ContentParentId by the current ContentParentId
> +  ContentParentId upperCpId;
> +  

nit: trailing space

@@ +182,5 @@
> +  if(upperCpId == 0){
> +    // This content parent is already on top level
> +    return cpm->GetTabParentByProcessAndTabId(aChildCpId, aChildTabId);
> +  }
> + 

nit: trailing space

@@ +184,5 @@
> +    return cpm->GetTabParentByProcessAndTabId(aChildCpId, aChildTabId);
> +  }
> + 
> +  // To get the upper TabParentId by the current ContentParentId and TabId
> +  TabId upperTabId;

openerTabId

::: dom/ipc/ScreenManagerParent.h
@@ +45,5 @@
>                                      ScreenDetails* aRetVal,
>                                      bool* aSuccess);
>  
>   private:
> +  /** 

nit: trailing space

@@ +50,5 @@
> +   * Get the TabParent on top level by the given content process and tab id.
> +   */
> +  already_AddRefed<TabParent>
> +  GetTopLevelTabParentByProcessAndTabId(const ContentParentId& aChildCpId, 
> +                                        const TabId& aChildTabId);

I am thinking to move this function to ContentProcessManager.h, because bug 1020179 also needs to get the top level TabParent. I will ask reviewer's thought and let you know.

@@ +51,5 @@
> +   */
> +  already_AddRefed<TabParent>
> +  GetTopLevelTabParentByProcessAndTabId(const ContentParentId& aChildCpId, 
> +                                        const TabId& aChildTabId);
> + 

nit: trailing space

Comment 3

[C.M.Chang[:chunmin]]

Attachment: bug-123456-fix-v2.patch

Rename some variables in ScreenManagerParent.cpp and clear the nits

Comment 4

[C.M.Chang[:chunmin]]

Attachment: bug-1102060-fix-v2.patch

Rename the variables and clear the nits

Comment 5

[C.M.Chang[:chunmin]]

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

Comment 6

[C.M.Chang[:chunmin]]

Attachment: Part2: Change the way to send/rcv for ScreenForBrowser and add new function to get top level TabParent in ScreenManagerParent

Comment 7

[Kershaw Chang [:kershaw]]

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

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

Please also move GetTopLevelTabParentByProcessAndTabId into ContentProcessManager so bug 1020179 can also reuse this function.
Thanks.

::: dom/ipc/ContentProcessManager.cpp
@@ +275,5 @@
>  
>    return true;
>  }
>  
> +already_AddRefed<TabParent>

You have to include TabParent.h in this cpp.

@@ +288,5 @@
> +    return nullptr;
> +  }
> +
> +  const InfallibleTArray<PBrowserParent*>& browsers =
> +  iter->second.mCp->ManagedPBrowserParent();

nit: add two spaces indent here

Comment 8

[Kershaw Chang [:kershaw]]

Comment on attachment 8549332 [details] [diff] [review]
Part2: Change the way to send/rcv for ScreenForBrowser and add new function to get top level TabParent in ScreenManagerParent

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

::: dom/ipc/ScreenManagerParent.cpp
@@ +189,5 @@
> +    currentTabId = openerTabId;
> +
> +    // Get the ContentParentId and TabId on upper level
> +    if(!cpm->GetParentProcessId(currentCpId, &parentCpId)
> +        || !cpm->GetRemoteFrameOpenerTabId(currentCpId, currentTabId, &openerTabId)){

nit: !cpm->GetParentProcessId(...) ||
       !cpm->GetRemoteFrameOpenerTabId(...)

Comment 9

[Kershaw Chang [:kershaw]]

(In reply to Kershaw Chang [:kershaw] from comment #8)
> Comment on attachment 8549332 [details] [diff] [review]
> Part2: Change the way to send/rcv for ScreenForBrowser and add new function
> to get top level TabParent in ScreenManagerParent
> 
> Review of attachment 8549332 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/ipc/ScreenManagerParent.cpp
> @@ +189,5 @@
> > +    currentTabId = openerTabId;
> > +
> > +    // Get the ContentParentId and TabId on upper level
> > +    if(!cpm->GetParentProcessId(currentCpId, &parentCpId)
> > +        || !cpm->GetRemoteFrameOpenerTabId(currentCpId, currentTabId, &openerTabId)){
> 
> nit: !cpm->GetParentProcessId(...) ||
>        !cpm->GetRemoteFrameOpenerTabId(...)
Remember to make these two conditions aligned.

Comment 10

[C.M.Chang[:chunmin]]

Attachment: Part1: Add two new functions to get TabParent in ContentProcessManager by given ContentParentId and TabId

Comment 11

[C.M.Chang[:chunmin]]

Attachment: Part2: Change the way to send/rcv for ScreenForBrowser

Comment 12

[C.M.Chang[:chunmin]]

Attachment: Part1: Add two functions to get TabParent in ContentProcessManager by given ContentParentId and TabId

Comment 13

[C.M.Chang[:chunmin]]

Attachment: Part2: Change the way to send/rcv for ScreenForBrowser

Comment 14

[Kershaw Chang [:kershaw]]

Hi Kyle,

Could you help to review the part 1 patch? The function GetTabParentByProcessAndTabId is actually the same as the patch 1 in bug 1020179. We add another function GetTopLevelTabParentByProcessAndTabId to get the top level TabParent lived in b2g process.

Thanks.

Comment 15

[Kershaw Chang [:kershaw]]

@chunmin,
You may need to rebase the part 2 patch since Bug 1113006 is landed.

Comment 16

[C.M.Chang[:chunmin]]

Attachment: Bug 1102060 - Part2: Change the way to send/rcv for ScreenForBrowser

@Kershaw: Thank you for the reminder :)

Comment 17

[C.M.Chang[:chunmin]]

Hi, Mike
Could you have time to review the patch part2? I will appreciate your help.

Comment 18

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

Comment on attachment 8550050 [details] [diff] [review]
Part1: Add two functions to get TabParent in ContentProcessManager by given ContentParentId and TabId

I'm on vacation for two weeks, so 302 kanru.

Comment 19

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

Comment on attachment 8550050 [details] [diff] [review]
Part1: Add two functions to get TabParent in ContentProcessManager by given ContentParentId and TabId

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

Overall looks good. I want to take a second look with the styles nits fixed.

::: dom/ipc/ContentProcessManager.cpp
@@ +305,5 @@
> +ContentProcessManager::GetTopLevelTabParentByProcessAndTabId(const ContentParentId& aChildCpId,
> +                                                             const TabId& aChildTabId)
> +{
> +  MOZ_ASSERT(NS_IsMainThread());
> +  

nit: trailing whitespace

@@ +317,5 @@
> +  TabId openerTabId = aChildTabId;
> +
> +  // Stop this loop when the upper ContentParentId of
> +  // the current ContentParentId is chrome(ContentParentId = 0).
> +  do{

nit: insert a space between do and {

@@ +324,5 @@
> +    currentTabId = openerTabId;
> +
> +    // Get the ContentParentId and TabId on upper level
> +    if(!GetParentProcessId(currentCpId, &parentCpId) || 
> +       !GetRemoteFrameOpenerTabId(currentCpId, currentTabId, &openerTabId)){

nit: trailing whitespace, insert a space between if and (, ) and {

@@ +327,5 @@
> +    if(!GetParentProcessId(currentCpId, &parentCpId) || 
> +       !GetRemoteFrameOpenerTabId(currentCpId, currentTabId, &openerTabId)){
> +      return nullptr;
> +    }
> +  }while(parentCpId);

nit: insert a space between }, while and (

::: dom/ipc/ContentProcessManager.h
@@ +111,5 @@
>                                   /*out*/ TabId* aOpenerTabId);
>  
> +  /**
> +   * Get the TabParent by the given content process and tab id.
> +   */

document that this function returns nullptr when TabParent couldn't be found via aChildTabId (probably because the TabParent is not in the chrome process)

@@ +118,5 @@
> +                                const TabId& aChildTabId);
> +
> +  /**
> +   * Get the TabParent on top level by the given content process and tab id.
> +   */

Maybe you could explain a bit more about what is a top level TabParent, its relation to the aChildTabId.

Comment 20

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8552135 [details] [diff] [review]
Bug 1102060 - Part2: Change the way to send/rcv for ScreenForBrowser

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

This looks OK to me, but you might want another reviewer, since I'm only about 50% confident with this stuff.

Comment 21

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8552135 [details] [diff] [review]
Bug 1102060 - Part2: Change the way to send/rcv for ScreenForBrowser

>-  TabParent* tabParent = static_cast<TabParent*>(aBrowser);
>+  ContentParent* cp = static_cast<ContentParent*>(this->Manager());
>+  ContentProcessManager* cpm = ContentProcessManager::GetSingleton();
>+  nsRefPtr<TabParent> tabParent =
>+    cpm->GetTopLevelTabParentByProcessAndTabId(cp->ChildID(), aTabId);
(random note, our naming for parent/childID is so inconsistent :/ )

Comment 22

[C.M.Chang[:chunmin]]

Attachment: Part1: Add two new functions to get TabParent in ContentProcessManager by given ContentParentId and TabId

Hi, Kanru
Thanks for your help. I add a little more comments about top level TabParent. However, I assume the target audience already has the knowledge of IPC and nested oop. If the comment isn't clear enough, I could write some more. Could you give me some suggestions?

Comment 23

[C.M.Chang[:chunmin]]

Mike, Olli, Really thanks for your review :)

Comment 24

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

Comment on attachment 8555057 [details] [diff] [review]
Part1: Add two new functions to get TabParent in ContentProcessManager by given ContentParentId and TabId

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

The comment is much clearer now, thanks!

::: dom/ipc/ContentProcessManager.cpp
@@ +320,5 @@
> +    currentCpId = parentCpId;
> +    currentTabId = openerTabId;
> +
> +    // Get the ContentParentId and TabId on upper level
> +    if(!GetParentProcessId(currentCpId, &parentCpId) ||

nit: insert a space between if and (

Comment 25

[C.M.Chang[:chunmin]]

Attachment: [v4]Part1: Add two new functions to get TabParent in ContentProcessManager by given ContentParentId and TabId

Hi, Kanru,
I must thank you for your patient reviewing. I correct the style issue and rewrite the comment of GetTopLevelTabParentByProcessAndTabId again.

Comment 26

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

Comment on attachment 8555745 [details] [diff] [review]
[v4]Part1: Add two new functions to get TabParent in ContentProcessManager by given ContentParentId and TabId

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

::: dom/ipc/ContentProcessManager.cpp
@@ +321,5 @@
> +    currentTabId = openerTabId;
> +
> +    // Get the ContentParentId and TabId on upper level
> +    if (!GetParentProcessId(currentCpId, &parentCpId) ||
> +       !GetRemoteFrameOpenerTabId(currentCpId, currentTabId, &openerTabId)) {

nit: indent

Comment 27

[C.M.Chang[:chunmin]]

Attachment: [v5]Part1: Add two new functions to get TabParent in ContentProcessManager by given ContentParentId and TabId

Comment 28

[C.M.Chang[:chunmin]]

From try server test, there is some timeout issues: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9f7113e41292

Comment 29

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

(In reply to Chun-Min Chang[:chunmin] from comment #28)
> From try server test, there is some timeout issues:
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=9f7113e41292

I re-triggered the tests, they looks green now.

Comment 30

[C.M.Chang[:chunmin]]

Attachment: [v6]Part1: Add two new functions to get TabParent in ContentProcessManager by given ContentParentId and TabId r=kanru

Comment 31

[C.M.Chang[:chunmin]]

Attachment: [v6] Part2: Change the way to send/rcv for ScreenForBrowser r=smaug

Comment 32

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fd8292a2ed7b
https://hg.mozilla.org/integration/mozilla-inbound/rev/e66dca57463b

Comment 33

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/fd8292a2ed7b
https://hg.mozilla.org/mozilla-central/rev/e66dca57463b