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.

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

Target at v2.1 sprint 3.

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

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

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

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

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 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.

(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.

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

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

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

Hi SC, Would you like to take a look at the v3 patch? Thanks.

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.

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

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.

(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 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.

(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.

(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 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.

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

(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

(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. :)

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.

Hi Kan-Ru, Would you please also take a look at the part2 patch? Thanks.

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

(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.

(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 on attachment 8491435 [details] [diff] [review] Part2 - Replace ManagedPBrowserParent in AppProcessChecker-v1 Review of attachment 8491435 [details] [diff] [review]: ----------------------------------------------------------------- Looks good.

Hi Kan-Ru, Would you please take a look at these patches? Your last feedback is addressed. Thanks.

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.

(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?

(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.

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 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.

(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 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>>

Hi Kyle, Could you please take a look at this? Thanks.

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

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

Hi Kyle, May I know your initial thoughts on it? Thanks.

Rebase and fix error on TrySever

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

Rebase

Rebase

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 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 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 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 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

(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) 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?

> @@ +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?

(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.

(That can be done in another bug).

Hi Kyle, Please help to review this v12 patch. Thanks.

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

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.

Rebase and carry reviewer's name.

Rebase and carry reviewer's name.

Rebase and carry reviewer's name.

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.

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

Carry r+ from khuey.

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!

rebase

rebase

rebase

rebase

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

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

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

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.

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

(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 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.

(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.

Carry reviewer's name.

(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.

Rebase

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

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