1020172 - Manage TabParent in chrome process

Status: RESOLVED FIXED

(Core :: DOM: Content Processes, defect)

Description

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

Currently we disable app principal check in RecvSyncMessage, AnswerRpcMessage and RecvAsyncMessage for TabParents that live in content process since a content process cannot kill another content process.

At least we should prevent the message being received if it comes from a suspicious child.

Comment 1

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

THe idea is when a Content Process wants to create a new TabParent it should notify the b2g process. The B2G process will keep a tree of opened [remote] tabparent.

When one of the tabchild sends message to the B2G process, the B2G process could check if the tabchild is the same as created initially.

https://hg.mozilla.org/mozilla-central/file/7fc96293ada8/dom/ipc/ContentParent.cpp#l915

Comment 2

[Jenny Liu[:jennyliu]]

Target at v2.1 sprint 3.

Comment 3

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

Attachment: [IWP] tab-parent-child-share-unique-id.patch

Comment 4

[Jenny Liu[:jennyliu]]

Move to v2.2 since the effort is bigger than we have estimated in the beginning stage.

Comment 5

[Kai-Chih Hu [:khu]]

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

Comment 6

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

Attachment: [WIP] tab-parent-child-share-unique-id.patch

wrap up my current work for @kershaw to follow up.

Comment 7

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

In bug 1020179, I need a function to get the top most TabParent from tabId.

Comment 8

[Kershaw Chang [:kershaw]]

Attachment: Bug-1020172-Manage-TabParent-in-chrome-process-v1.patch

Hi SC and Kan-Ru,

Could you take a look at this patch?
I would like to make sure I am in the right track.

Thanks.

Comment 9

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

Comment on attachment 8482640 [details] [diff] [review]
Bug-1020172-Manage-TabParent-in-chrome-process-v1.patch

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

In general it look good to me. One thing to notice is that ContentProcessManager still not store the information of first level ContentParent (i.e. content process that directly created by chrome process). You'll need to include the corresponding change in next version.

::: dom/ipc/ContentParent.cpp
@@ +961,5 @@
>  
>      ProcessPriority initialPriority = GetInitialProcessPriority(aFrameElement);
>      bool isInContentProcess = (XRE_GetProcessType() != GeckoProcessType_Default);
> +    if (!gContentProcessManager) {
> +        gContentProcessManager = ContentProcessManager::GetSingleton();

Dont' keep another static pointer to a singleton.

@@ +1010,5 @@
> +                                               constructorSender->IsForBrowser());
> +            if (!tabId) {
> +                return nullptr;
> +            }
> +            nsRefPtr<TabParent> tp(new TabParent(constructorSender, tabId,

Maybe we can move the |AllocateTabId| into TabParent/TabChild's constructor or creation method. Therefore, we won't need to add the same code all over the place.

::: dom/ipc/ContentProcessManager.cpp
@@ +55,5 @@
> +                         						 const uint64_t& aId,
> +                         						 const bool& aIsForApp,
> +                         						 const bool& aIsForBrowser)
> +{
> +	MOZ_ASSERT(cIsMainProcess);

You'll need to use MOZ_ASSERT(XRE_GetProcessType() == GeckoProcessType_Default) directly if you want to guarantee to be used in chrome process. Or, you can just maintain the singleton in ContentParent which is guaranteed to be run in chrome process. In addition, you should do MOZ_ASSERT(NS_IsMainThread()) because this data structure is not thread-safe.

@@ +75,5 @@
> +			NS_ERROR("Failed to find parent frame's opener id");
> +			return 0;
> +		}
> +		openerId = remoteFrameIter->second.mOpenerId;
> +	}

Yes, this is exactly what I mean.

::: dom/ipc/ContentProcessManager.h
@@ +19,5 @@
> +class ContentParent;
> +
> +struct RemoteFrameInfo
> +{
> +	uint64_t mOpenerId;

nit: Use space instead of tab.

@@ +50,5 @@
> +	void GetAppIdsByContentProcess(ContentParent* aContent, nsTArray<uint64_t>& aIdArray);
> +
> +private:
> +	static StaticAutoPtr<ContentProcessManager> sSingleton;
> +	uint64_t mId;

You need a |static uint64_t mUniqueId| for ID generation, right?

@@ +52,5 @@
> +private:
> +	static StaticAutoPtr<ContentProcessManager> sSingleton;
> +	uint64_t mId;
> +	std::map<ContentParent*, GrandchildInfo> mGrandchildProcessMap;
> +	std::map<uint64_t, ContentParent*> mContentParentMap;

Maybe we can merge these two map into one:
std::map<uint64_t, ContentProcessInfo> mInfoMap;

struct ContentProcessInfo {
  ContentParent* cp;
  uint64_t parentCpId;
  std::set<uint64_t> mChildrenCpId;
  std::map<uint64_t, RemoteFrameInfo> mRemoteFrames;
};

Store "ChildID" for parentCpId/mChildrenCpId should be safer than store ContentParent* pointer, prevent chrome process to crash while accessing invalid address.

Comment 10

[Kershaw Chang [:kershaw]]

(In reply to Shih-Chiang Chien [:schien] (UTC+8) from comment #9)
> Comment on attachment 8482640 [details] [diff] [review]
> Bug-1020172-Manage-TabParent-in-chrome-process-v1.patch
> 
> Review of attachment 8482640 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> In general it look good to me. One thing to notice is that
> ContentProcessManager still not store the information of first level
> ContentParent (i.e. content process that directly created by chrome
> process). You'll need to include the corresponding change in next version.

Thanks for your good feedback. I'll add this change in next version. 

> 
> @@ +1010,5 @@
> > +                                               constructorSender->IsForBrowser());
> > +            if (!tabId) {
> > +                return nullptr;
> > +            }
> > +            nsRefPtr<TabParent> tp(new TabParent(constructorSender, tabId,
> 
> Maybe we can move the |AllocateTabId| into TabParent/TabChild's constructor
> or creation method. Therefore, we won't need to add the same code all over
> the place.

It seems that we cannot move AllocateTabId into TabParent/TabChild's constructor, because the tab id should be allocated first before creating TabParent/TabChild. Maybe I was wrong. I will keep looking for a way to reduce duplicate code.

> @@ +52,5 @@
> > +private:
> > +	static StaticAutoPtr<ContentProcessManager> sSingleton;
> > +	uint64_t mId;
> > +	std::map<ContentParent*, GrandchildInfo> mGrandchildProcessMap;
> > +	std::map<uint64_t, ContentParent*> mContentParentMap;
> 
> Maybe we can merge these two map into one:
> std::map<uint64_t, ContentProcessInfo> mInfoMap;
> 
> struct ContentProcessInfo {
>   ContentParent* cp;
>   uint64_t parentCpId;
>   std::set<uint64_t> mChildrenCpId;
>   std::map<uint64_t, RemoteFrameInfo> mRemoteFrames;
> };
> 
> Store "ChildID" for parentCpId/mChildrenCpId should be safer than store
> ContentParent* pointer, prevent chrome process to crash while accessing
> invalid address.
Thanks. It's a good advice.

Comment 11

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

Paul, we need to check if this new security design is reasonable. We could schedule a meeting with you to go through the details.

The idea is documented here: https://wiki.mozilla.org/Nested_Content_Processes

Comment 12

[Kershaw Chang [:kershaw]]

Attachment: Manage TabParent in chrome process - v2

Hi SC,

Could you take a look at the v2 patch?
Thanks.

Summary of changes:
1. Save first level TabParent
2. Remove static pointer to ContentProcessManager
3. Add MOZ_ASSERT(NS_IsMainThread()) check
4. Use static uint64_t for tracking tab id
5. Merge two maps into one
6. Store id than ContentParent pointer

Comment 13

[Kershaw Chang [:kershaw]]

I just realized that I did something wrong in this patch.
Sorry for wasting your time.

Comment 14

[Kershaw Chang [:kershaw]]

Attachment: Manage TabParent in chrome process - v3.patch

Hi SC,

Would you like to take a look at the v3 patch?

Thanks.

Comment 15

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

Comment on attachment 8486946 [details] [diff] [review]
Manage TabParent in chrome process - v3.patch

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

CotentParent should not manipulate the data in ContentProcessManager directly. You'll need to expose proper API in ContentProcessManager.

::: dom/ipc/ContentParent.cpp
@@ +905,3 @@
>      }
> +
> +    iter->second.mChildrenCpId.insert(*aId);

These code should be moved to ContentProcessManager.

@@ +917,5 @@
> +
> +    auto iter = cpm->ContentParentMap().find(this->ChildID());
> +    if (iter != cpm->ContentParentMap().end() &&
> +        iter->second.mChildrenCpId.find(cp->ChildID()) !=
> +        iter->second.mChildrenCpId.end()) {

expose proper API in ContentProcessManager.

@@ +926,5 @@
>          return false;
>      }
>  }
>  
> +static nsIDocShell* GetOpenerIdHelper(Element *aFrameElement,uint64_t &aId)

This function returns both docshell and tabId which are logically independent. It'll improve the readability by separating into two functions, e.g. ContentParent::GetOpenerDocShell(Element*) and TabParent::GetTabIdFrom(nsIDocShell*).

::: dom/ipc/ContentProcessManager.h
@@ +38,5 @@
> +public:
> +  static ContentProcessManager* GetSingleton();
> +
> +  std::map<uint64_t, ContentProcessInfo>& ContentParentMap();
> +  std::map<uint64_t, TabParent*>& FirstLvlTabParentMap();

The purpose of ContentProcessManager is to hide these two maps. We should expose more API if necessary.

@@ +56,5 @@
> +private:
> +  static StaticAutoPtr<ContentProcessManager> sSingleton;
> +  static uint64_t mUniqueId;
> +  std::map<uint64_t, ContentProcessInfo> mContentParentMap;
> +  std::map<uint64_t, TabParent*> mFirstLvlTabParentMap;

It'll be better to merge these two maps into one.

::: dom/ipc/TabChild.cpp
@@ +1448,5 @@
>  
> +  IPCTabContext ipcContext(context, mScrolling);
> +
> +  const uint64_t openerId = TabId();
> +  uint64_t tabId = ContentParent::AllocateTabId(openerId,

TabChild should never interact with ContentParent directly. People will get confused about the execution process of this function while reading it.

Comment 16

[Kershaw Chang [:kershaw]]

Attachment: Manage TabParent in chrome process-v4.patch

Hi SC,

Could you please take a look at the v4 patch?
The feedback you mentioned in v3 are included.

Thanks.

Comment 17

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

Comment on attachment 8488530 [details] [diff] [review]
Manage TabParent in chrome process-v4.patch

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

Overall lgtm! You can start to co-work with @kanru, @shianyow, and me to see if |ContentProcessManager| can cover all the use cases in dependent bugs.

::: dom/ipc/ContentParent.cpp
@@ +1506,5 @@
>              sPrivateContent = nullptr;
>          }
>      }
>  
>      mIsAlive = false;

MarkAsDead() can be called from various place. Can you double check if ContentProcessManager::RemoveContentProcess() is invoked at all scenarios? I saw you move the logic into ContentParent::ActorDestroy() but I doubt the equivalence of this change.

@@ +4011,5 @@
> +                             const bool& aIsForApp,
> +                             const bool& aIsForBrowser)
> +{
> +    uint64_t tabId = 0;
> +    if (XRE_GetProcessType() == GeckoProcessType_Default) {

Content process should never call this function so you don't need to check the process type. Also, you can make this function non-static.

@@ +4033,5 @@
> +    return tabId;
> +}
> +
> +/*static*/ void
> +ContentParent::DeallocateTabId(const uint64_t& aTabId, const uint64_t& aCpId)

Same as ContentParent::AllocateTabId().

::: dom/ipc/ContentProcessManager.cpp
@@ +99,5 @@
> +ContentProcessManager::GetAllChildProcessById(const uint64_t& aParentCpId,
> +                                              nsTArray<uint64_t>& aIdArray)
> +{
> +  MOZ_ASSERT(NS_IsMainThread());
> +  NS_PRECONDITION(aIdArray.IsEmpty(), "aIdArray must be empty");

Prefer using |MOZ_ASSERT|.

::: dom/ipc/ContentProcessManager.h
@@ +6,5 @@
> +
> +#ifndef mozilla_dom_ContentProcessManager_h
> +#define mozilla_dom_ContentProcessManager_h
> +
> +#include "base/basictypes.h"

Why need this header file?

@@ +21,5 @@
> +
> +struct RemoteFrameInfo
> +{
> +  uint64_t mOpenerId;
> +  TabContext mContext;

You probably need to include TabContext.h.

@@ +39,5 @@
> +  static ContentProcessManager* GetSingleton();
> +
> +  void AddContentProcess(ContentParent* aCp, const uint64_t& aParentCpId = 0);
> +
> +  void RemoveContentProcess(const uint64_t& aId);

The name |aId| should be unified. Suggest to use |xxxCpId| for representing the ID of ContentParent/Child and |xxxTabId| for the ID of TabParent/Child.

Comment 18

[Kershaw Chang [:kershaw]]

(In reply to Shih-Chiang Chien [:schien] (UTC+8) from comment #17)
> Comment on attachment 8488530 [details] [diff] [review]
> Manage TabParent in chrome process-v4.patch
> 
> Review of attachment 8488530 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Overall lgtm! You can start to co-work with @kanru, @shianyow, and me to see
> if |ContentProcessManager| can cover all the use cases in dependent bugs.

Thanks for your feedback.
Kanru and Shianyow,
Could you please take a look at the APIs that ContentProcessManager provided and let me know if they are enough for you?

> 
> ::: dom/ipc/ContentParent.cpp
> @@ +1506,5 @@
> >              sPrivateContent = nullptr;
> >          }
> >      }
> >  
> >      mIsAlive = false;
> 
> MarkAsDead() can be called from various place. Can you double check if
> ContentProcessManager::RemoveContentProcess() is invoked at all scenarios? I
> saw you move the logic into ContentParent::ActorDestroy() but I doubt the
> equivalence of this change.

MarkAsDead() is called before ActorDestroy(). If we put RemoveContentProcess() before ActorDestroy(), those grandchild processes have no chance to be released. Details can be found in bug 1049354. I plan to also fix in this bug.

> 
> @@ +4011,5 @@
> > +                             const bool& aIsForApp,
> > +                             const bool& aIsForBrowser)
> > +{
> > +    uint64_t tabId = 0;
> > +    if (XRE_GetProcessType() == GeckoProcessType_Default) {
> 
> Content process should never call this function so you don't need to check
> the process type. Also, you can make this function non-static.

AllocateTabId() will be called directly by CreateBrowserOrApp(), which is a static function and also be called in content process.
Making this function static and adding this check can save some code in ContentBridgeParent.

> 
> @@ +4033,5 @@
> > +    return tabId;
> > +}
> > +
> > +/*static*/ void
> > +ContentParent::DeallocateTabId(const uint64_t& aTabId, const uint64_t& aCpId)
> 
> Same as ContentParent::AllocateTabId().
> 

> ::: dom/ipc/ContentProcessManager.h
> @@ +6,5 @@
> > +
> > +#ifndef mozilla_dom_ContentProcessManager_h
> > +#define mozilla_dom_ContentProcessManager_h
> > +
> > +#include "base/basictypes.h"
> 
> Why need this header file?
> 

You are right. I can remove this.
Thanks.

> @@ +21,5 @@
> > +
> > +struct RemoteFrameInfo
> > +{
> > +  uint64_t mOpenerId;
> > +  TabContext mContext;
> 
> You probably need to include TabContext.h.

Yes, I think so. But, I didn't get any complaint about TabContext from my compiler.

> 
> @@ +39,5 @@
> > +  static ContentProcessManager* GetSingleton();
> > +
> > +  void AddContentProcess(ContentParent* aCp, const uint64_t& aParentCpId = 0);
> > +
> > +  void RemoveContentProcess(const uint64_t& aId);
> 
> The name |aId| should be unified. Suggest to use |xxxCpId| for representing
> the ID of ContentParent/Child and |xxxTabId| for the ID of TabParent/Child.

Thanks. I will check namings in next version.

Comment 19

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

Comment on attachment 8488530 [details] [diff] [review]
Manage TabParent in chrome process-v4.patch

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

Looks good. Maybe we could add more integrity check in AllocateTabId().

::: dom/ipc/ContentParent.cpp
@@ +1083,5 @@
>              // succeeds.
>              if (!p->SetPriorityAndCheckIsAlive(initialPriority)) {
>                  p = nullptr;
>              }
> +            ContentProcessManager::GetSingleton()->AddContentProcess(p);

Move this after if (!parent) ?

@@ +1094,5 @@
>                                                 nullptr,
>                                                 &tookPreallocated);
>              MOZ_ASSERT(p);
> +
> +            ContentProcessManager::GetSingleton()->AddContentProcess(p);

And this.

@@ +1103,5 @@
>  
>      if (!parent) {
>          return nullptr;
>      }
>  

Here.

::: dom/ipc/ContentProcessManager.cpp
@@ +35,5 @@
> +void
> +ContentProcessManager::AddContentProcess(ContentParent* aCp,
> +                                         const uint64_t& aParentCpId)
> +{
> +  MOZ_ASSERT(NS_IsMainThread());

MOZ_ASSERT(aCp);

@@ +189,5 @@
> +  }
> +
> +  for (auto it = iter->second.mRemoteFrames.begin();
> +       it != iter->second.mRemoteFrames.end();
> +       it++) {

++iter

@@ +207,5 @@
> +  }
> +
> +  for (auto it = iter->second.mRemoteFrames.begin();
> +       it != iter->second.mRemoteFrames.end();
> +       it++) {

++iter

::: dom/ipc/ContentProcessManager.h
@@ +21,5 @@
> +
> +struct RemoteFrameInfo
> +{
> +  uint64_t mOpenerId;
> +  TabContext mContext;

It didn't fail probably due to Unified source.

::: dom/ipc/TabChild.cpp
@@ +1458,5 @@
> +                                                  &tabId);
> +
> +  nsRefPtr<TabChild> newChild = new TabChild(ContentChild::GetSingleton(), tabId,
> +                                             /* TabContext */ *this, /* chromeFlags */ 0);
> +  if (!NS_SUCCEEDED(newChild->Init())) {

NS_FAILED

::: dom/ipc/TabParent.cpp
@@ +1565,5 @@
> +{
> +  nsCOMPtr<nsITabChild> tabChild(TabChild::GetFrom(docShell));
> +  if (tabChild) {
> +    TabChild* tc = static_cast<TabChild*>(tabChild.get());
> +    if (tc) {

You already checked tabChild, no need to check tc again.

Comment 20

[Kershaw Chang [:kershaw]]

(In reply to Kan-Ru Chen [:kanru] from comment #19)
> Comment on attachment 8488530 [details] [diff] [review]
> Manage TabParent in chrome process-v4.patch
> 
> Review of attachment 8488530 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good. Maybe we could add more integrity check in AllocateTabId().
> 
What kind of integrity check we should add in AllocateTabId()? Do we have to add more checks on TabContext?
BTW, can we remove ChromeFlags, IsForApp, and IsForBrowser from the parameter list of AllocateTabId()? It looks like these parameters are useless now.

Thanks.

Comment 21

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

(In reply to Kershaw Chang [:kershaw] from comment #20)
> (In reply to Kan-Ru Chen [:kanru] from comment #19)
> > Comment on attachment 8488530 [details] [diff] [review]
> > Manage TabParent in chrome process-v4.patch
> > 
> > Review of attachment 8488530 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Looks good. Maybe we could add more integrity check in AllocateTabId().
> > 
> What kind of integrity check we should add in AllocateTabId()? Do we have to
> add more checks on TabContext?

I'm not sure.. I have to think about it.

> BTW, can we remove ChromeFlags, IsForApp, and IsForBrowser from the
> parameter list of AllocateTabId()? It looks like these parameters are
> useless now.

I think these flags were added to save a IPC round-trip. We could save these flags in the info and use them to do integrity check too. But I think they are redundant since we also pass TabContext..

Smaug, do you know how were these flags used?

Comment 22

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

Comment on attachment 8488530 [details] [diff] [review]
Manage TabParent in chrome process-v4.patch

>+    /**
>+     * Tell the chrome process there is an creation of PBrowser.
>+     * return a system-wise unique Id.
>+     */
>+    sync AllocateTabId(uint64_t openerId, IPCTabContext context, uint32_t chromeFlags,
>+                       uint64_t id, bool isForApp, bool isForBrowser)
>+        returns (uint64_t tabId);
At which point is this called? We _must_ not do any sync IPC during a tabchild creation. Sync-ness slows 
down tab creation _significantly_. The fact that we do gfx initialization using sync messaging is a bug, and makes
app startup ~50ms slower, IIRC.

Why isn't TabId allocation merged to PBrowser creation somehow?


See Bug 1003046.

Comment 23

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

See also https://bugzilla.mozilla.org/show_bug.cgi?id=1023980#c0

Comment 24

[Kershaw Chang [:kershaw]]

(In reply to Olli Pettay [:smaug] from comment #22)
> Comment on attachment 8488530 [details] [diff] [review]
> Manage TabParent in chrome process-v4.patch
> 
> >+    /**
> >+     * Tell the chrome process there is an creation of PBrowser.
> >+     * return a system-wise unique Id.
> >+     */
> >+    sync AllocateTabId(uint64_t openerId, IPCTabContext context, uint32_t chromeFlags,
> >+                       uint64_t id, bool isForApp, bool isForBrowser)
> >+        returns (uint64_t tabId);
> At which point is this called? We _must_ not do any sync IPC during a
> tabchild creation. Sync-ness slows 
> down tab creation _significantly_. The fact that we do gfx initialization
> using sync messaging is a bug, and makes
> app startup ~50ms slower, IIRC.
> 
> Why isn't TabId allocation merged to PBrowser creation somehow?
> 
> 
> See Bug 1003046.

Hi Smaug,

AllocateTabId() is called only when a grandchild process want to create a tab. I can move this TabId allocation into RecvCreateChildProcess [1]. This can speed up the app startup time.
Currently, I have no idea how to move TabId allocation into PBrowser creation. Because we must allocate a TabId first, and then assign it to TabParent/TabChild constructor. I'll find if there is a better way to do this.

Thanks.

[1] http://dxr.mozilla.org/mozilla-central/source/dom/ipc/ContentParent.cpp#861

Comment 25

[Shian-Yow Wu [:swu]]

(In reply to Kershaw Chang [:kershaw] from comment #18)
> (In reply to Shih-Chiang Chien [:schien] (UTC+8) from comment #17)
> > Comment on attachment 8488530 [details] [diff] [review]
> > Manage TabParent in chrome process-v4.patch
> > 
> > Review of attachment 8488530 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Overall lgtm! You can start to co-work with @kanru, @shianyow, and me to see
> > if |ContentProcessManager| can cover all the use cases in dependent bugs.
> 
> Thanks for your feedback.
> Kanru and Shianyow,
> Could you please take a look at the APIs that ContentProcessManager provided
> and let me know if they are enough for you?

It's ok for me so far. :)

Comment 26

[Kershaw Chang [:kershaw]]

Attachment: Manage TabParent in chrome process-v5

Comment 27

[Kershaw Chang [:kershaw]]

Hi Kan-Ru,

Would you like to take a look again?

Summary of changes:
1. Some suggestions are addressed.
2. Adjust the naming about xxxCpId and xxxTabId.
3. Preallocate a TabId in RecvCreateChildProcess()

Thanks.

Comment 28

[Kershaw Chang [:kershaw]]

Attachment: Part2 - Replace ManagedPBrowserParent in AppProcessChecker-v1

Hi Kan-Ru,

Would you please also take a look at the part2 patch?

Thanks.

Comment 29

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

Comment on attachment 8491349 [details] [diff] [review]
Manage TabParent in chrome process-v5

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

Given we have so many Id types.. we should perhaps make them incompatible classes so we don't confuse ourselves.

::: dom/ipc/ContentParent.cpp
@@ +857,4 @@
>  bool
>  ContentParent::RecvCreateChildProcess(const IPCTabContext& aContext,
>                                        const hal::ProcessPriority& aPriority,
> +                                      const uint64_t& aOpenerId,

aOpenerTabId and other places.

@@ +1007,5 @@
> +            if (!tabId) {
> +                tabId = AllocateTabId(openerTabId,
> +                                      aContext.AsIPCTabContext(),
> +                                      constructorSender->ChildID());
> +            }

When will this happen? Could you assert that this only happens when we are allocating a second Tab?

@@ +1130,5 @@
> +    if (!tabId) {
> +        tabId = AllocateTabId(openerTabId,
> +                              aContext.AsIPCTabContext(),
> +                              parent->ChildID());
> +    }

Same here.

@@ +1184,5 @@
>  
>  /*static*/ ContentBridgeParent*
>  ContentParent::CreateContentBridgeParent(const TabContext& aContext,
> +                                         const hal::ProcessPriority& aPriority,
> +                                         const uint64_t& openerId,

aOpenerTabId

@@ +4028,5 @@
>    return sIgnoreIPCPrincipal;
>  }
>  
> +/*static*/ uint64_t
> +ContentParent::AllocateTabId(const uint64_t& aOpenerId,

aOpenerTabId

@@ +4030,5 @@
>  
> +/*static*/ uint64_t
> +ContentParent::AllocateTabId(const uint64_t& aOpenerId,
> +                             const IPCTabContext& aContext,
> +                             const uint64_t& aId)

aCpId or aOpenerCpId

Comment 30

[Kershaw Chang [:kershaw]]

(In reply to Kan-Ru Chen [:kanru] from comment #29)
> Comment on attachment 8491349 [details] [diff] [review]
> Manage TabParent in chrome process-v5
> 
> Review of attachment 8491349 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Given we have so many Id types.. we should perhaps make them incompatible
> classes so we don't confuse ourselves.
> 
Thanks for your feedback.
I think you mean using a class to wrap the id, right?
Is there something similar in gecko that I can refer to?

> @@ +1007,5 @@
> > +            if (!tabId) {
> > +                tabId = AllocateTabId(openerTabId,
> > +                                      aContext.AsIPCTabContext(),
> > +                                      constructorSender->ChildID());
> > +            }
> 
> When will this happen? Could you assert that this only happens when we are
> allocating a second Tab?

I think I can move AllocateTabId() into GetNewOrPreallocatedAppProcess() and GetNewOrUsedBrowserProcess(), so we can get a process and a tab id together. However, the only exception is the case when reusing a content process.

Comment 31

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

(In reply to Kershaw Chang [:kershaw] from comment #30)
> (In reply to Kan-Ru Chen [:kanru] from comment #29)
> > Comment on attachment 8491349 [details] [diff] [review]
> > Manage TabParent in chrome process-v5
> > 
> > Review of attachment 8491349 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Given we have so many Id types.. we should perhaps make them incompatible
> > classes so we don't confuse ourselves.
> > 
> Thanks for your feedback.
> I think you mean using a class to wrap the id, right?
> Is there something similar in gecko that I can refer to?

http://mxr.mozilla.org/mozilla-central/source/layout/base/Units.h

But we need nothing fancy here. Just a class that could be converted to a uint32_t.

ex.

template<typename T>
class IdType
{
public:
  explicit IdType(uint32_t aId) : mId(aId) {}
  operator uint32_t() { return mId; }
private:
  uint32_t mId;
};

typedef IdType<TabParent> TabId;
typedef IdType<ContentParent> ContentParentId;

Comment 32

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

Comment on attachment 8491435 [details] [diff] [review]
Part2 - Replace ManagedPBrowserParent in AppProcessChecker-v1

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

Looks good.

Comment 33

[Kershaw Chang [:kershaw]]

Attachment: Patch 1: Manage TabParent in chrome process - v6

Comment 34

[Kershaw Chang [:kershaw]]

Attachment: Patch 2: Replace ManagedPBrowserParent in AppProcessChecker - v1

Comment 35

[Kershaw Chang [:kershaw]]

Attachment: Patch 3: Use TabIdType to replace uint64_t in PBrowserOrId - v1

Comment 36

[Kershaw Chang [:kershaw]]

Hi Kan-Ru,

Would you please take a look at these patches?
Your last feedback is addressed.

Thanks.

Comment 37

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

Comment on attachment 8494357 [details] [diff] [review]
Patch 1: Manage TabParent in chrome process - v6

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

Looks good. Please split the IdType changes and make it Patch 1, that will make review of other changes easier.

The template could be simplified so that it scales better when there are more IdTypes, see below.

::: dom/ipc/IdType.h
@@ +17,5 @@
> +class IdType
> +{
> +
> +  friend struct IPC::ParamTraits<IdType<TabParent> >;
> +  friend struct IPC::ParamTraits<IdType<ContentParent> >;

friend struct IPC::ParamTraits<IdType<T> >;

@@ +40,5 @@
> +  uint64_t mId;
> +};
> +
> +typedef IdType<TabParent> TabIdType;
> +typedef IdType<ContentParent> ContentParentIdType;

I think we could drop the -Type postfix. These could move to TabParent.h and ContentParent.h respectively.

@@ +49,5 @@
> +namespace IPC {
> +
> +template<>
> +struct ParamTraits<mozilla::dom::TabIdType>
> +{

template<typename T>
struct ParamTraits<IdType<T> >
{
  typedef mozilla::dom::IdType<T> ParamType;
  ...
};

Write this once and this template could be used for all IdType<T> types.

Comment 38

[Kershaw Chang [:kershaw]]

(In reply to Kan-Ru Chen [:kanru] from comment #37)
> Comment on attachment 8494357 [details] [diff] [review]
> Patch 1: Manage TabParent in chrome process - v6
> 
> Review of attachment 8494357 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good. Please split the IdType changes and make it Patch 1, that will
> make review of other changes easier.
> 
> The template could be simplified so that it scales better when there are
> more IdTypes, see below.
> 
> ::: dom/ipc/IdType.h
> @@ +17,5 @@
> > +class IdType
> > +{
> > +
> > +  friend struct IPC::ParamTraits<IdType<TabParent> >;
> > +  friend struct IPC::ParamTraits<IdType<ContentParent> >;
> 
> friend struct IPC::ParamTraits<IdType<T> >;
> 
> @@ +40,5 @@
> > +  uint64_t mId;
> > +};
> > +
> > +typedef IdType<TabParent> TabIdType;
> > +typedef IdType<ContentParent> ContentParentIdType;
> 
> I think we could drop the -Type postfix. These could move to TabParent.h and
> ContentParent.h respectively.
If we move the type declaration to TabParent.h, does it mean we also have to include TabParent.h in TabChild.h and also other files that use TabId? I think maybe it's not a good idea.
How about still keeping it in an IdType.h?

Comment 39

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

(In reply to Kershaw Chang [:kershaw] from comment #38)
> If we move the type declaration to TabParent.h, does it mean we also have to
> include TabParent.h in TabChild.h and also other files that use TabId? I
> think maybe it's not a good idea.
> How about still keeping it in an IdType.h?

OK.

Comment 40

[Kershaw Chang [:kershaw]]

Attachment: Patch 1: New IdType for ContentParent and TabParent -v1

Comment 41

[Kershaw Chang [:kershaw]]

Attachment: Patch 2: Manage TabParent in chrome process - v7

Comment 42

[Kershaw Chang [:kershaw]]

Attachment: Patch 3: Replace ManagedPBrowserParent in AppProcessChecker - v1

Comment 43

[Kershaw Chang [:kershaw]]

Attachment: Patch 4: Use TabId to replace uint64_t in PBrowserOrId - v1

Comment 44

[Kershaw Chang [:kershaw]]

Hi Kan-Ru,

I've re-based the patch to the latest code and updated the patches as your feedback.
Would you like to take a look again?

Thanks.

Comment 45

[Kershaw Chang [:kershaw]]

Attachment: Patch 1: Manage TabParent in chrome process - v7

Comment 46

[Kershaw Chang [:kershaw]]

Attachment: Patch 2: Replace ManagedPBrowserParent in AppProcessChecker

Comment 47

[Kershaw Chang [:kershaw]]

Attachment: Patch 3: Use TabId to replace uint64_t in PBrowserOrId

Comment 48

[Kershaw Chang [:kershaw]]

Attachment: Patch 1: Replace process id with ContentParentId type - v1

Comment 49

[Kershaw Chang [:kershaw]]

Attachment: Patch 2: Manage TabParent in chrome process - v8

Comment 50

[Kershaw Chang [:kershaw]]

Attachment: Patch 3: Use TabId to replace uint64_t of PBrowserOrId

Comment 51

[Kershaw Chang [:kershaw]]

Attachment: Patch 4: Replace ManagedPBrowserParent in AppProcessChecker

Comment 52

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

Comment on attachment 8497988 [details] [diff] [review]
Patch 2: Manage TabParent in chrome process - v8

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

::: dom/ipc/TabChild.cpp
@@ +748,5 @@
>      // Pass nullptr to aManager since at this point the TabChild is
>      // not connected to any manager. Any attempt to use the TabChild
>      // in IPC will crash.
>      nsRefPtr<TabChild> tab(new TabChild(nullptr,
> +                                        /* tabId */ TabId(0),

nit: The comment is redundant.

Comment 53

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

(In reply to Kan-Ru Chen [:kanru] from comment #52)
> Comment on attachment 8497988 [details] [diff] [review]
> Patch 2: Manage TabParent in chrome process - v8
> 
> Review of attachment 8497988 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/ipc/TabChild.cpp
> @@ +748,5 @@
> >      // Pass nullptr to aManager since at this point the TabChild is
> >      // not connected to any manager. Any attempt to use the TabChild
> >      // in IPC will crash.
> >      nsRefPtr<TabChild> tab(new TabChild(nullptr,
> > +                                        /* tabId */ TabId(0),
> 
> nit: The comment is redundant.

I mean the /* tabId */ part ;)

Comment 54

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

Comment on attachment 8497987 [details] [diff] [review]
Patch 1: Replace process id with ContentParentId type - v1

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

::: dom/ipc/IdType.h
@@ +16,5 @@
> +class IdType
> +{
> +
> +  friend struct IPC::ParamTraits<IdType<T> >;
> +

We could use >> so
 friend struct IPC::ParamTraits<IdType<T>>;

@@ +45,5 @@
> +
> +namespace IPC {
> +
> +template<typename T>
> +struct ParamTraits<mozilla::dom::IdType<T> >

Same here

struct ParamTraits<mozilla::dom::IdType<T>>

Comment 55

[Kershaw Chang [:kershaw]]

Attachment: Patch 1: Replace process id with ContentParentId type - v2

Comment 56

[Kershaw Chang [:kershaw]]

Attachment: Manage TabParent in chrome process - v9

Hi Kyle,

Could you please take a look at this?

Thanks.

Comment 57

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

Can you explain briefly what the goal of these patches are?

Comment 58

[Kershaw Chang [:kershaw]]

Since the TabParent may live in content process now, we are not able to get the remote tab information by ManagedPBrowserParent [1]. We need to find another way to keep this information in chrome process. The idea is to maintain a tree which saves all necessary information (e.g., TabContext) when a content process wants to create a TabParent.

The detailed information can be found in [2].

[1] http://dxr.mozilla.org/mozilla-central/source/dom/ipc/AppProcessChecker.cpp#174
[2] https://wiki.mozilla.org/Nested_Content_Processes

Comment 59

[Kershaw Chang [:kershaw]]

Hi Kyle,

May I know your initial thoughts on it?

Thanks.

Comment 60

[Kershaw Chang [:kershaw]]

Attachment: Patch 1: Replace process id with ContentParentId type - v3

Rebase and fix error on TrySever

Comment 61

[Kershaw Chang [:kershaw]]

Attachment: Patch 2: Manage TabParent in chrome process - v11

1. Rebase
2. Fix error on TryServer
3. Use PBrowserOrId in PopupIPCTabContext

Comment 62

[Kershaw Chang [:kershaw]]

Attachment: Patch 3: Use TabId to replace uint64_t of PBrowserOrId

Rebase

Comment 63

[Kershaw Chang [:kershaw]]

Attachment: Patch 4: Replace ManagedPBrowserParent in AppProcessChecker

Rebase

Comment 64

[Kershaw Chang [:kershaw]]

Attachment: Patch1 - interdiff of v2 and v3

Comment 65

[Kershaw Chang [:kershaw]]

Attachment: Patch2 - interdiff of v10 and v11

Comment 66

[Kershaw Chang [:kershaw]]

Hi Kyle,

I've fixed some errors on try server and a bug about creating a new tab in TabChild. Please take a look at v11 patches.
I also upload interdiff files to help you to understand the changes of these two versions.

Thanks.

Try Server:
https://tbpl.mozilla.org/?tree=Try&rev=295711e4c3ca

Comment 67

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

Comment on attachment 8503926 [details] [diff] [review]
Patch 1: Replace process id with ContentParentId type - v3

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

::: dom/ipc/nsIContentParent.cpp
@@ +111,4 @@
>                                        const bool& aIsForApp,
>                                        const bool& aIsForBrowser)
>  {
> +  unused << aCpId;

Why did you remove the line for aChromeFlags?

Comment 68

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

Comment on attachment 8503928 [details] [diff] [review]
Patch 2: Manage TabParent in chrome process - v11

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

Please be consistent about naming STL iterator variables |iter| or |it| (I don't care which one you use, but pick one and stick with it).

In a lot of places we treat failing to find the content process in our map less severely than I would expect.  Is that expected to happen?  If so under what circumstances?  If not, let's add some assertions.

I would appreciate an interdiff between this and your next version.

::: dom/ipc/ContentProcessManager.cpp
@@ +4,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +
> +#include "ContentProcessManager.h"

nit: extra \n

@@ +17,5 @@
> +namespace dom {
> +
> +static uint64_t gTabId = 0;
> +
> +/* static */ StaticAutoPtr<ContentProcessManager>

/* static */ on the previous line please.

@@ +26,5 @@
> +
> +/* static */ ContentProcessManager*
> +ContentProcessManager::GetSingleton()
> +{
> +  MOZ_ASSERT(XRE_GetProcessType() == GeckoProcessType_Default);

\n after the assert

@@ +51,5 @@
> +void
> +ContentProcessManager::RemoveContentProcess(const ContentParentId& aChildCpId)
> +{
> +  MOZ_ASSERT(NS_IsMainThread());
> +  mContentParentMap.erase(aChildCpId);

nit: \n after assertions (all over this file, actually)

Assert that erase actually removed something?

@@ +68,5 @@
> +{
> +  MOZ_ASSERT(NS_IsMainThread());
> +  auto iter = mContentParentMap.find(aParentCpId);
> +  if (iter == mContentParentMap.end()) {
> +    MOZ_ASSERT(false, "Parent process should be already in map!");

If this is called with untrusted input (which it looks like this is) it really needs ASSERT_UNLESS_FUZZING.

@@ +77,5 @@
> +}
> +
> +bool
> +ContentProcessManager::GetParentProcessId(const ContentParentId& aChildCpId,
> +                                          /*out*/ContentParentId* aParentCpId)

put some more spaces in here (e.g. /* out */ Cont....

@@ +101,5 @@
> +}
> +
> +void
> +ContentProcessManager::GetAllChildProcessById(const ContentParentId& aParentCpId,
> +                                              nsTArray<ContentParentId>& aIdArray)

Should this just return an nsTArray&&?  Seems like this would be simpler.

@@ +127,5 @@
> +  MOZ_ASSERT(NS_IsMainThread());
> +
> +  auto it = mContentParentMap.find(aChildCpId);
> +  if (NS_WARN_IF(it == mContentParentMap.end())) {
> +    return TabId(0);

Can we ASSERT_UNLESS_FUZZING here?

@@ +138,5 @@
> +  // open a new tab. aOpenerTabId has to be it's parent frame's opener id.
> +  if (appBrowser.type() == IPCTabAppBrowserContext::TPopupIPCTabContext) {
> +    auto remoteFrameIter = it->second.mRemoteFrames.find(aOpenerTabId);
> +    if (remoteFrameIter == it->second.mRemoteFrames.end()) {
> +      NS_ERROR("Failed to find parent frame's opener id.");

This should be ASSERT_UNLESS_FUZZING too, I think (aOpenerTabId is untrusted, right?)

@@ +191,5 @@
> +}
> +
> +void
> +ContentProcessManager::GetAppIdsByContentProcess(const ContentParentId& aChildCpId,
> +                                                 nsTArray<uint64_t>& aIdArray)

This also seems like something that should return nsTArray&&.

@@ +210,5 @@
> +}
> +
> +void
> +ContentProcessManager::GetTabContextByContentProcess(const ContentParentId& aChildCpId,
> +                                                     nsTArray<TabContext>& aContextArray)

And this.

::: dom/ipc/ContentProcessManager.h
@@ +31,5 @@
> +  std::set<ContentParentId> mChildrenCpId;
> +  std::map<TabId, RemoteFrameInfo> mRemoteFrames;
> +};
> +
> +class ContentProcessManager MOZ_FINAL

This header needs a lot more comments.  Please document what these methods are for and how you would use them.

@@ +70,5 @@
> +                                 /*out*/TabId* aOpenerTabId);
> +
> +private:
> +  static StaticAutoPtr<ContentProcessManager> sSingleton;
> +  static TabId mUniqueId;

Why is this static?

@@ +73,5 @@
> +  static StaticAutoPtr<ContentProcessManager> sSingleton;
> +  static TabId mUniqueId;
> +  std::map<ContentParentId, ContentProcessInfo> mContentParentMap;
> +
> +  ContentProcessManager() {};

This needs MOZ_COUNT_CTOR/DTOR.

::: dom/ipc/PBrowserOrIdType.ipdlh
@@ +1,1 @@
> +/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */

Rename this file PBrowserOrId.ipdlh please

@@ +13,5 @@
> +
> +union PBrowserOrId {
> +nullable PBrowser;
> +TabId;
> +};

style nits:

union PBrowserOrId
{
  nullable PBrowser;
  TabId;
};

::: dom/ipc/TabChild.cpp
@@ +841,5 @@
>    }
> +
> +  // preloaded TabChild should not be added to child map
> +  if (mUniqueId) {
> +    NestedTabChildMap()[mUniqueId] = this;

Seems like we should assert that there's not already something in the map.

::: dom/ipc/TabContext.cpp
@@ +269,5 @@
>          }
> +      } else if (ipcContext.opener().type() == PBrowserOrId::TPBrowserChild) {
> +        context = static_cast<TabChild*>(ipcContext.opener().get_PBrowserChild());
> +      } else if (ipcContext.opener().type() == PBrowserOrId::TTabId) {
> +        mInvalidReason = "Not support getting TabContext by ID.";

Can we translate this into something a Gaia developer might understand?  What actually causes us to get here?

::: dom/ipc/nsIContentParent.cpp
@@ +79,5 @@
>    }
>  
>    const PopupIPCTabContext& popupContext = appBrowser.get_PopupIPCTabContext();
> +  if (popupContext.opener().type() != PBrowserOrId::TPBrowserParent) {
> +    NS_ERROR("Unexpected PopupIPCTabContext type.  Aborting AllocPBrowserParent.");

This should also probably be ASSERT_UNLESS_FUZZING (and so should the bit above this ...)

@@ +83,5 @@
> +    NS_ERROR("Unexpected PopupIPCTabContext type.  Aborting AllocPBrowserParent.");
> +    return false;
> +  }
> +
> +  TabParent* opener = static_cast<TabParent*>(popupContext.opener().get_PBrowserParent());

auto opener

Comment 69

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

Comment on attachment 8503929 [details] [diff] [review]
Patch 3: Use TabId to replace uint64_t of PBrowserOrId

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

::: netwerk/ipc/PNecko.ipdl
@@ +33,5 @@
>  
>  namespace mozilla {
>  namespace net {
>  
> +

nit, extraneous \n

::: netwerk/protocol/http/HttpChannelParent.h
@@ +176,5 @@
>    bool mDivertedOnStartRequest;
>  
>    bool mSuspendedForDiversion;
>  
> +  mozilla::dom::TabId mNestedFrameId;

just dom::TabId should be fine.

Comment 70

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

Comment on attachment 8503931 [details] [diff] [review]
Patch 4: Replace ManagedPBrowserParent in AppProcessChecker

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

::: dom/ipc/AppProcessChecker.cpp
@@ +144,5 @@
>                   AssertAppProcessType aType,
>                   const char* aCapability)
>  {
> +  nsAutoTArray<TabContext, 4> contextArray;
> +  static_cast<ContentParent*>(aActor)->GetManagedTabContext(contextArray);

If you switch to returning nsTArray&& like I believe I suggested then the nsAutoTArray is unnecessary (since we'll never use the built in storage in the auto part).

@@ +166,5 @@
>  AssertAppStatus(PContentParent* aActor,
>                  unsigned short aStatus)
>  {
> +  nsAutoTArray<TabContext, 4> contextArray;
> +  static_cast<ContentParent*>(aActor)->GetManagedTabContext(contextArray);

here too.

@@ +206,5 @@
>    bool inBrowserElement = aPrincipal->GetIsInBrowserElement();
>  
>    // Check if the permission's appId matches a child we manage.
> +  nsAutoTArray<TabContext, 4> contextArray;
> +  static_cast<ContentParent*>(aActor)->GetManagedTabContext(contextArray);

and here

Comment 71

[Kershaw Chang [:kershaw]]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #67)
> Comment on attachment 8503926 [details] [diff] [review]
> Patch 1: Replace process id with ContentParentId type - v3
> 
> Review of attachment 8503926 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/ipc/nsIContentParent.cpp
> @@ +111,4 @@
> >                                        const bool& aIsForApp,
> >                                        const bool& aIsForBrowser)
> >  {
> > +  unused << aCpId;
> 
> Why did you remove the line for aChromeFlags?

Because aChromeFlags is used below for creating a TabParent. 
Is aChromeFlags still a unused parameter?

Comment 72

[Kershaw Chang [:kershaw]]

> @@ +68,5 @@
> > +{
> > +  MOZ_ASSERT(NS_IsMainThread());
> > +  auto iter = mContentParentMap.find(aParentCpId);
> > +  if (iter == mContentParentMap.end()) {
> > +    MOZ_ASSERT(false, "Parent process should be already in map!");
> 
> If this is called with untrusted input (which it looks like this is) it
> really needs ASSERT_UNLESS_FUZZING.
> 
Could you please explain more why we need ASSERT_UNLESS_FUZZING?
I've searched ASSERT_UNLESS_FUZZING on DXR and found there is no common header file defining this. It's only defined in some cpp file. Should I do the same in ContentProcessManager.cpp also?

Comment 73

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

(In reply to Kershaw Chang [:kershaw] from comment #71)
> Because aChromeFlags is used below for creating a TabParent.

Ah, yes.  You are correct.

(In reply to Kershaw Chang [:kershaw] from comment #72)
> Could you please explain more why we need ASSERT_UNLESS_FUZZING?
> I've searched ASSERT_UNLESS_FUZZING on DXR and found there is no common
> header file defining this. It's only defined in some cpp file. Should I do
> the same in ContentProcessManager.cpp also?

Eventually we want to be able to fuzz IPDL protocols to ensure that a compromised child process cannot construct malformed messages/etc that cause the parent to behave incorrectly.  If we just assert then we won't test the handling of malformed messages in opt builds (which are what users will be using).  That's what we created ASSERT_UNLESS_FUZZING for.  Putting that somewhere common would be nice.

Comment 74

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

(That can be done in another bug).

Comment 75

[Kershaw Chang [:kershaw]]

Attachment: Patch 2: Manage TabParent in chrome process - v12

Comment 76

[Kershaw Chang [:kershaw]]

Attachment: Patch2 - interdiff of v11 and v12

Comment 77

[Kershaw Chang [:kershaw]]

Attachment: Patch 3: Use TabId to replace uint64_t of PBrowserOrId - v2

Comment 78

[Kershaw Chang [:kershaw]]

Attachment: Patch 4: Replace ManagedPBrowserParent in AppProcessChecker - v2

Comment 79

[Kershaw Chang [:kershaw]]

Hi Kyle,

Please help to review this v12 patch.

Thanks.

Comment 80

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

Comment on attachment 8507788 [details] [diff] [review]
Patch 2: Manage TabParent in chrome process - v12

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

r=me

::: dom/ipc/ContentParent.cpp
@@ +4171,5 @@
> +bool
> +ContentParent::RecvDeallocateTabId(const TabId& aTabId)
> +{
> +  DeallocateTabId(aTabId, this->ChildID());
> +  return true;

nit: 4 space indentation to match the rest of the file

Comment 81

[Kershaw Chang [:kershaw]]

Try server: https://tbpl.mozilla.org/?tree=Try&rev=ec8cbc785d11

There are some test failures on try server after adding assertions, I will fix this asap.

Comment 82

[Kershaw Chang [:kershaw]]

Attachment: Patch 1: Replace process id with ContentParentId type - v3, r=khuey

Rebase and carry reviewer's name.

Comment 83

[Kershaw Chang [:kershaw]]

Attachment: Patch 3: Use TabId to replace uint64_t of PBrowserOrId - v2, r=khuey

Rebase and carry reviewer's name.

Comment 84

[Kershaw Chang [:kershaw]]

Attachment: Patch 4: Replace ManagedPBrowserParent in AppProcessChecker - v2, r=khuey

Rebase and carry reviewer's name.

Comment 85

[Kershaw Chang [:kershaw]]

Attachment: Patch 2: Manage TabParent in chrome process - v13

Comment 86

[Kershaw Chang [:kershaw]]

Attachment: Patch2 - interdiff of v12 and v13

Hi Kyle,

The reason of failure on try server is because not every content process are managed. Some content processes are not added into the map.
So, I just move AddContentProcess() to the constructor of ContentParent. Would you please take a look?

Thanks.

Comment 87

[Kershaw Chang [:kershaw]]

Latest result on try server: https://tbpl.mozilla.org/?tree=Try&rev=f57133ef6351

Comment 88

[Kershaw Chang [:kershaw]]

Attachment: Patch 2: Manage TabParent in chrome process - v13, r=khuey

Carry r+ from khuey.

Comment 89

[Carsten Book [:Tomcat]]

Hi Kershaw, seems the patch(es) don't apply cleanly:

Hunk #3 FAILED at 403
1 out of 4 hunks FAILED -- saving rejects to file dom/ipc/ContentParent.h.rej
patching file dom/ipc/PContent.ipdl
Hunk #3 FAILED at 510
1 out of 3 hunks FAILED -- saving rejects to file dom/ipc/PContent.ipdl.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working dir
errors during apply, please fix and refresh 0001-Bug-1020172-Patch-1-Replace-process-id-with-ContentP.patch

could you take a look and maybe rebase against a current tree? Thanks!

Comment 90

[Kershaw Chang [:kershaw]]

Attachment: Patch 1: Replace process id with ContentParentId type - v3, r=khuey

rebase

Comment 91

[Kershaw Chang [:kershaw]]

Attachment: Patch 2: Manage TabParent in chrome process - v13, r=khuey

rebase

Comment 92

[Kershaw Chang [:kershaw]]

Attachment: Patch 3: Use TabId to replace uint64_t of PBrowserOrId - v2, r=khuey

rebase

Comment 93

[Kershaw Chang [:kershaw]]

Attachment: Patch 4: Replace ManagedPBrowserParent in AppProcessChecker - v2, r=khuey

rebase

Comment 94

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f895938013ff
https://hg.mozilla.org/integration/mozilla-inbound/rev/9d8c09e711f5
https://hg.mozilla.org/integration/mozilla-inbound/rev/90714be8e689
https://hg.mozilla.org/integration/mozilla-inbound/rev/176ca4363517

Comment 95

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

https://hg.mozilla.org/mozilla-central/rev/f895938013ff
https://hg.mozilla.org/mozilla-central/rev/9d8c09e711f5
https://hg.mozilla.org/mozilla-central/rev/90714be8e689
https://hg.mozilla.org/mozilla-central/rev/176ca4363517

Comment 96

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

I had to back these out in https://hg.mozilla.org/mozilla-central/rev/08ad036e00fe for causing bug 1089542.

Comment 97

[Kershaw Chang [:kershaw]]

Attachment: Patch 2: Manage TabParent in chrome process - v14

Hi Kyle,

I've found the root cause of bug 1089542 is that tab id is not allocated for every case. I already fix this. Please help to review the patch.

Thanks.

Comment 98

[Kershaw Chang [:kershaw]]

Attachment: Patch2 - interdiff of v14 and v13

Comment 99

[Kershaw Chang [:kershaw]]

I've found the root cause and provided a revised patch.

Comment 100

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

(In reply to Kershaw Chang [:kershaw] from comment #99)
> I've found the root cause and provided a revised patch.
Is it possible to make some test cases for it?

Comment 101

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

Comment on attachment 8512473 [details] [diff] [review]
Patch 2: Manage TabParent in chrome process - v14

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

Yeah, a regression test would be nice.

Comment 102

[Kershaw Chang [:kershaw]]

(In reply to Shih-Chiang Chien [:schien] (UTC+8) from comment #100)
> (In reply to Kershaw Chang [:kershaw] from comment #99)
> > I've found the root cause and provided a revised patch.
> Is it possible to make some test cases for it?

I think I can create a follow-up bug for creating a test case for this.
Since this bug is blocked many others, I'd like to land this first.

Comment 103

[Kershaw Chang [:kershaw]]

Attachment: Patch 2: Manage TabParent in chrome process - v14, r=khuey

Carry reviewer's name.

Comment 104

[Ryan VanderMeulen [:RyanVM]]

(In reply to Kershaw Chang [:kershaw] from comment #102)
> I think I can create a follow-up bug for creating a test case for this.

Was this done? If so, please mark the dep. If not, please do :)

Unfortunately, this just got bitrotted by bug 641685. Please rebase.

Comment 105

[Kershaw Chang [:kershaw]]

Attachment: Patch 2: Manage TabParent in chrome process - v14, r=khuey

Rebase

Comment 106

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fedce15e6ea2
https://hg.mozilla.org/integration/mozilla-inbound/rev/01b25915ca37
https://hg.mozilla.org/integration/mozilla-inbound/rev/f644da9988dc
https://hg.mozilla.org/integration/mozilla-inbound/rev/f31a5e752439

Comment 107

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/fedce15e6ea2
https://hg.mozilla.org/mozilla-central/rev/01b25915ca37
https://hg.mozilla.org/mozilla-central/rev/f644da9988dc
https://hg.mozilla.org/mozilla-central/rev/f31a5e752439