In nested oop case, nsFrameLoader::ResetPermissionManagerStatus only releases the app id in [B]. We have to also release the app id [C] that created in content process. Chrome Process Content Processes +- PContentParent[A] ----- PContentChild | `-- PBrowesParent[B] ---- PBrowserChild | PContentBridgeParent | `--- PBrowserParent[C] --+ | | `- PContentParent --- PContentChild | PContentBridgeChild | `--- PBrowserChild-------+

Chrome process Content Process Content Process +-----+ +-------+ +-------+ | |-------------| |----------- | Grand | | B2G | | Child | | child | +-----+ +-------+ +-------+ The problem is caused by losing track of AppId. As far as I know, the grandchild process will be called by nsFrameLoader::TryRemoteBrowser[1] in child process. However, the following nsFrameLoader::ResetPermissionManagerStatus() called in the child process can't add the refcount of this grandchild process because it is called in child process instead of B2G[2,3]. That is also why [4] doesn't work(nsFrameLoader::ResetPermissionManagerStatus will early return if it is not called in B2G[2]). To keep track the appId, 1. (1) Add the refcount of content process by permissionManager.addrefAppId(request.principal.appId) before checking the permission[5] if this appId has never been add to permissionManager before. (2) Release all of the grandchild processes's appId at the same time when the child process is killed 2. (1) Add the refcount of the grandchild process' appId after adding grandchild process (2) Release all of the grandchild processes's appId at the same time when the child process is killed 3. Ask B2G to handle the addrefAppId and releaseAppId of grandchild process if nsFrameLoader::ResetPermissionManagerStatus() is not called in B2G The above is what I could think of off the top of my head now. Any ideas? [1] https://dxr.mozilla.org/mozilla-central/source/dom/base/nsFrameLoader.cpp#2074 [2] https://dxr.mozilla.org/mozilla-central/source/dom/base/nsFrameLoader.cpp#2611 [3] https://dxr.mozilla.org/mozilla-central/source/dom/base/nsFrameLoader.cpp#2663 [4] https://dxr.mozilla.org/mozilla-central/source/dom/base/nsFrameLoader.cpp#2656 [5] https://dxr.mozilla.org/mozilla-central/source/b2g/components/ContentPermissionPrompt.js#346 [6] https://dxr.mozilla.org/mozilla-central/source/dom/ipc/ContentParent.cpp#928

Hi, Monica Does the appId need to be keeping tracked once the app starts, no matter who open the app? Or permission can start tracking the appId only when the app require some permissions? Could you give me some suggestions? I will appreciate your help.

Hey Chun-Min, Thanks for the diagnosis. Comment 1 seems correct in that the appid is getting lost, but I am not super-familiar with this code so am not sure if refcounting is the right fix. e10s has a regular triage meeting where they go over bugs marked "tracking-e10s", which is now set on this bug, and also you may be able to find assistance in irc channel #e10s before the triage meeting happens. Thanks, Monica

This doesn't sound like an e10s-team bug, tbh. smaug - do you know which team this bug would fall under? b2g platform?

Yeah, b2g.

Since ContentParent already use the PermissionManager[1], I think the simplest approach is to add the refcount of the appId in nested-oop app after grandchild process is created[2]. [1] https://dxr.mozilla.org/mozilla-central/source/dom/ipc/ContentParent.cpp#162 [2] https://dxr.mozilla.org/mozilla-central/source/dom/ipc/ContentParent.cpp#932

The content process need to remove all the refcount of its descendants' appId before it is killed

Comment on attachment 8558343 [details] [diff] [review] [Bug 1114507 v2] part1 - Remove add/release appid from nsFrameLoader Review of attachment 8558343 [details] [diff] [review]: ----------------------------------------------------------------- Some feedback below: 1. Why there are a lot of redundant changes in your patch? 2. I don't think we can just remove nsFrameLoader::ResetPermissionManagerStatus, otherwise there will be a problem when swapping with another loader. 3. We have to also consider the case for in-process app. AllocateTabId is only for oop case.

As far as I’m concerned, it's better to manage the app's permission by nsFrameLoader itself. If it's called in main process, then its app permission can be added/released by permissionManager directly. Otherwise, it should ask the main process to do this job via IPC because the authority of app permission's setting should be restricted to main process only. The demonstration can be portrayed as following: ************ * Figure 1 * ************ main process process 1 process 2 +-----------------------+ IPC +-----------------------+ IPC +-----------------------+ | chrome |-----------| Child A |----------| Child B | | <iframe remote=true | PContent | <iframe remote=true | PContent | <iframe remote=false | | mozapp=A mozbrowser> | | mozapp=B mozbrowser> | Bridge | mozapp=C mozbrowser> | | ▲ | | ▲ | | ▲ | | nsFrameLoader will be | | nsFrameLoader will be | | nsFrameLoader will be | | called here to load | | called here to load | | called here to load | | Child A | | GrandChild B. | | Child C. | | A's permnission will | | B's permission will | | C's permission will | | be set directly | | be set via IPC request| | be set via IPC request| +-----------------------+ +-----------------------+ | | | | +---------+ | | | | Child C | | | IPC : PContent | +---------+ | +------------------------------------------------------------+-----------------------+ However, the main process will construct the oop relation tree after the nsFrameLoader::TryRemoteBrowser() is called, so personally I think the app's permission should be set after calling nsFrameLoader::TryRemoteBrowser()(At that time, the ContentParent can check the ownership of the app needing to set its permission). To summarize the discussion, there are four possible cases need to be dealt with 1) app's iframe attribute remote ≠ true && setting of app permission is called in main process 2) app's iframe attribute remote ≠ true && setting of app permission is called out of main process 3) app's iframe attribute remote = true && setting of app permission is called in main process 4) app's iframe attribute remote = true && setting of app permission is called out of main process In original nsFrameLoader[1], the setting of app's permission is done by ResetPermissionManagerStatus(). It will be called only when nsFrameLoader's constructor, deconstructror and frame content need to be swapped. Attachment 8561315 [details] [diff] and attachment 8561317 [details] [diff] [review] is a simple approach to handle the case 1,3,4 by adding a ResetPermissionManagerStatus() calling after TryRemoteBrowser() and some conditions in ResetPermissionManagerStatus(). The code flow can be depicted as following: case 1: nsFrameLoader::nsFrameLoader -----x----> | Reset permission here case 3: nsFrameLoader::nsFrameLoader -----x----> nsFrameLoader::TryRemoteBrowser -----x----> | | Reset permission here do nothing case 4: nsFrameLoader::nsFrameLoader -----x----> nsFrameLoader::TryRemoteBrowser -----x----> | | do nothing Reset permission by IPC P.S. TryRemoteBrowser() will be called only when attribute remote = true However, the case 2 will happen when the Child C in figure 1 embed another child D by adding <iframe remote=false mozapp=D mozbrowser>. I am not sure how to handle this case. The IPC between Child B and chrome is the only contact with the main process of the process 2. Thus, it's need to find a way that can send a message from child c to chrome: Child C ---> Child B ---> chrome. Any idea? [1](https://dxr.mozilla.org/mozilla-central/source/dom/base/nsFrameLoader.cpp)

(In reply to Chun-Min Chang[:chunmin] from comment #13) > However, the case 2 will happen when the Child C in figure 1 embed another > child D by adding <iframe remote=false mozapp=D mozbrowser>. I am not sure > how to handle this case. The IPC between Child B and chrome is the only > contact with the main process of the process 2. Thus, it's need to find a > way that can send a message from child c to chrome: Child C ---> Child B > ---> chrome. Any idea? > The case 2 you mentioned is not allowed now. Please see bug 1097479. You can only open an in-process browser iframe in content process. About your question, child C can send message to chrome process directly via PContent.

(In reply to Kershaw Chang [:kershaw] from comment #14) > The case 2 you mentioned is not allowed now. Please see bug 1097479. You can > only open an in-process browser iframe in content process. Thanks for your reply. Despite the disallowance of embedding app in content process, the iframe's permission still need to be tracked if the iframe embedded is a browser, right? To handle such case and prevent the app can be embedded someday, the possible approach is as follows: As the mention in comment #13, the ResetPermissionManagerStatus() will be called after nsFrameLoader's constructor, deconstructror and when frame content need to be swapped. I reckon ResetPermissionManagerStatus() can be called again after nsFrameLoader::ReallyStartLoadingInternal(). In nsFrameLoader::ReallyStartLoadingInternal(), if the app's iframe attribute remote = true, it will trigger TryRemoteBrowser() and return early. On the other hand, the remaining part of nsFrameLoader::ReallyStartLoadingInternal() will be executed. The member variable mDocShell will be set in the beginning of nsFrameLoader::ReallyStartLoadingInternal(), so it can be used to check the ResetPermissionManagerStatus() is triggered from nsFrameLoader's constructor or not. Moreover, if the app's iframe attribute remote = true, then nsFrameLoader::TryRemoteBrowser() will be called and the member variable mRemoteBrowser will be set. It can be also used to find out where the ResetPermissionManagerStatus() is triggered. Attachment 8563178 [details] [diff] and attachment 8563179 [details] [diff] [review] implement this idea, and it can be roughly portrayed as: ::ResetPermissionManagerStatus() ◥ set mRemoteBrowser / ◤ / == initial == \ / mRemoteBrowser = nullptr ::TryRemoteBrowser() mDocShell = nullptr ◥ ◥ / / Yes / / / ::nsFrameLoader ---> ::ResetPermissionManagerStatus() ---> ::ReallyStartLoadingInternal() ---> remote = true? / / / No / ◣ / set mDocShell ◣ remaining part of ::ReallyStartLoadingInternal() / / ◣ ::ResetPermissionManagerStatus() In this case, the code flow of four possible cases need to be dealt with in comment #13 are: case 1: ::nsFrameLoader -----x----> ::ReallyStartLoadingInternal() ---> remaining part of it -----x----> | | Reset permission here do nothing case 2: ::nsFrameLoader -----x----> ::ReallyStartLoadingInternal() ---> remaining part of it -----x----> | | do nothing Reset permission by IPC case 3: ::nsFrameLoader -----x----> ::ReallyStartLoadingInternal() ---> ::TryRemoteBrowser -----x----> | | Reset permission here do nothing case 4: ::nsFrameLoader -----x----> ::ReallyStartLoadingInternal() ---> ::TryRemoteBrowser -----x----> | | do nothing Reset permission by IPC

Add more comments

Hi, Kanru, Could you give these patches a review when you are available?

(In reply to Chun-Min Chang[:chunmin] from comment #17) > (In reply to Kershaw Chang [:kershaw] from comment #14) > > The case 2 you mentioned is not allowed now. Please see bug 1097479. You can > > only open an in-process browser iframe in content process. > Thanks for your reply. Despite the disallowance of embedding app in content > process, the iframe's permission still need to be tracked if the iframe > embedded is a browser, right? To handle such case and prevent the app can be > embedded someday, the possible approach is as follows: > > As the mention in comment #13, the ResetPermissionManagerStatus() will be > called after nsFrameLoader's constructor, deconstructror and when frame > content need to be swapped. I reckon ResetPermissionManagerStatus() can be > called again after nsFrameLoader::ReallyStartLoadingInternal(). In > nsFrameLoader::ReallyStartLoadingInternal(), if the app's iframe attribute > remote = true, it will trigger TryRemoteBrowser() and return early. On the > other hand, the remaining part of > nsFrameLoader::ReallyStartLoadingInternal() will be executed. The member > variable mDocShell will be set in the beginning of > nsFrameLoader::ReallyStartLoadingInternal(), so it can be used to check the > ResetPermissionManagerStatus() is triggered from nsFrameLoader's constructor > or not. Moreover, if the app's iframe attribute remote = true, then > nsFrameLoader::TryRemoteBrowser() will be called and the member variable > mRemoteBrowser will be set. It can be also used to find out where the > ResetPermissionManagerStatus() is triggered. Attachment 8563178 [details] [diff] > [diff] and attachment 8563179 [details] [diff] [review] implement this idea, > and it can be roughly portrayed as: > > > ::ResetPermissionManagerStatus() > > ◥ > > set mRemoteBrowser / > > ◤ / > == initial == > \ / > mRemoteBrowser = nullptr > ::TryRemoteBrowser() > mDocShell = nullptr > ◥ > ◥ > / > / > Yes / > / > / > ::nsFrameLoader ---> ::ResetPermissionManagerStatus() ---> > ::ReallyStartLoadingInternal() ---> remote = true? > / > / > / > No / > ◣ > / > set mDocShell > ◣ > > remaining part of ::ReallyStartLoadingInternal() > > / > > / > > ◣ > > ::ResetPermissionManagerStatus() > > > > In this case, the code flow of four possible cases need to be dealt with in > comment #13 are: > > case 1: > ::nsFrameLoader -----x----> ::ReallyStartLoadingInternal() ---> remaining > part of it -----x----> > | > | > Reset permission here > do nothing > > case 2: > ::nsFrameLoader -----x----> ::ReallyStartLoadingInternal() ---> remaining > part of it -----x----> > | > | > do nothing > Reset permission by IPC > > case 3: > ::nsFrameLoader -----x----> ::ReallyStartLoadingInternal() ---> > ::TryRemoteBrowser -----x----> > | > | > Reset permission here > do nothing > > case 4: > ::nsFrameLoader -----x----> ::ReallyStartLoadingInternal() ---> > ::TryRemoteBrowser -----x----> > | > | > do nothing > Reset permission by IPC After looking your patch, I have some comments below: 1. You didn't remove the ResetPermissionManagerStatus() in nsFrameLoader::nsFrameLoader(). We don't have to call ResetPermissionManagerStatus() multiple times. 2. Maybe you can put ResetPermissionManagerStatus() at the start of ReallyStartLoadingInternal(). 3. We can use mRemoteFrame to identify the embedded iframe is in-process or oop. Using mDocShell is not clear. 4. I don't know if it's ok to put a lot of comment in the code. I think it's not a good idea to do this.

Remove ResetPermissionManagerStatus() from nsFrameLoader::nsFrameLoader()

> After looking your patch, I have some comments below: > 1. You didn't remove the ResetPermissionManagerStatus() in > nsFrameLoader::nsFrameLoader(). We don't have to call > ResetPermissionManagerStatus() multiple times. > 2. Maybe you can put ResetPermissionManagerStatus() at the start of > ReallyStartLoadingInternal(). > 3. We can use mRemoteFrame to identify the embedded iframe is in-process or > oop. Using mDocShell is not clear. Yes, I think you point out the problems. Removing the ResetPermissionManagerStatus() from nsFrameLoader::nsFrameLoader() can avoid calling ResetPermissionManagerStatus() multiple times. However, if we use mRemoteFrame to identify the embedded iframe is in-process or oop, the ResetPermissionManagerStatus() can't be put at the start of ReallyStartLoadingInternal() because the mRemoteFrame is set after TryRemoteBrowser() in ReallyStartLoadingInternal(). Thus, ResetPermissionManagerStatus() is still need to be called after TryRemoteBrowser(). After removing the ResetPermissionManagerStatus() from nsFrameLoader::nsFrameLoader(), mDocShell is no longer need to be used to identify where the ResetPermissionManagerStatus() is called. Attachment 8568330 [details] [diff], that is the revised patch after adding your opinion, can show all the ideas mentioned above. In summary, the code flow now are: 1) app's iframe attribute remote ≠ true && setting of app permission is called in main process ::ReallyStartLoadingInternal() ---> remaining part of it -----x----> | Reset permission directly 2) app's iframe attribute remote ≠ true && setting of app permission is called out of main process ::ReallyStartLoadingInternal() ---> remaining part of it -----x----> | Reset permission by IPC 3) app's iframe attribute remote = true && setting of app permission is called in main process ::ReallyStartLoadingInternal() ---> ::TryRemoteBrowser -----x----> | Reset permission directly 4) app's iframe attribute remote = true && setting of app permission is called out of main process ::ReallyStartLoadingInternal() ---> ::TryRemoteBrowser -----x----> | Reset permission by IPC > 4. I don't know if it's ok to put a lot of comment in the code. I think it's > not a good idea to do this. Alright, I can try to use less comments to explain this modification.

Comment on attachment 8563178 [details] [diff] [review] part1 - Add/Release the app permission in ContentParent Review of attachment 8563178 [details] [diff] [review]: ----------------------------------------------------------------- In your current patches I think point (2), release all of the grandchild processes's appId at the same time when the child process is killed, is not handled. What happens when a child process crashes? ContentParent automatically kills all grandchild processes but doesn't release the permission reference counts. You might also want :baku's feedback as he implemented this originally. ::: dom/ipc/PContent.ipdl @@ +590,5 @@ > + * Track the app's permission by adding/releasing the appId > + * that can be accessed by given ContentParentId and TabId > + */ > + sync AddrefAppId(ContentParentId cpId, TabId tabId); > + sync ReleaseAppId(ContentParentId cpId, TabId tabId); These could have better names. Maybe |PermissionManagerAddref| and |PermissionManagerRelease| Do they have to be synchronous?

Hi, baku, Could you give me some information how the permission reference counts be released when a child process crashes in current design?

(In reply to Kan-Ru Chen [:kanru] from comment #23) > In your current patches I think point (2), release all of the grandchild > processes's appId at the same time when the child process is killed, is not > handled. What happens when a child process crashes? ContentParent > automatically kills all grandchild processes but doesn't release the > permission reference counts. I thought the nsFrameLoader::Destroy() would be called no matter whether it's terminated normally or not. Nevertheless, if a child process crashes, then only its nsFrameLoader::Destroy() will be called, right? (ContentParent will still kills all the grandchild processes created by this child process.). Function ContentParent::ActorDestroy will be triggered when the child process crashes. In this function, it will destroy any processes created by this ContentParent and those processes can be access through ContentProcessManager. Thus, one simple method to fix this problem is to add some code before those processes were destroyed(for-loop at [1]): nsCOMPtr<nsIPermissionManager> permMgr = services::GetPermissionManager(); if(permMgr){ nsTArray<TabContext> tabCtxs = cpm->GetTabContextByContentProcess(childIDArray[i]); for(uint32_t j = 0 ; j < tabCtxs.Length() ; j++){ if(tabCtxs[j].OwnOrContainingAppId()!= nsIScriptSecurityManager::NO_APP_ID){ permMgr->ReleaseAppId(tabCtxs[j].OwnOrContainingAppId()); } } } However, I still prefer finding a way that nsFrameLoader can manage its permission reference count by itself(Trying to call nsFrameLoader::Destroy() of those grandchild processes created by crashed child process?). One main reason is that the releasing/adding of permission can be put in one place and same file rather than many different files. It might be easier to maintain the code. > ::: dom/ipc/PContent.ipdl > @@ +590,5 @@ > > + * Track the app's permission by adding/releasing the appId > > + * that can be accessed by given ContentParentId and TabId > > + */ > > + sync AddrefAppId(ContentParentId cpId, TabId tabId); > > + sync ReleaseAppId(ContentParentId cpId, TabId tabId); > > These could have better names. Maybe |PermissionManagerAddref| and > |PermissionManagerRelease| > > Do they have to be synchronous? Oh, you're right. It's not necessary to be synchronous currently. I will rename these two IPC functions and replace sync with async in next patch. [1] https://dxr.mozilla.org/mozilla-central/source/dom/ipc/ContentParent.cpp#1963

(In reply to Chun-Min Chang[:chunmin] from comment #25) > (In reply to Kan-Ru Chen [:kanru] from comment #23) > > In your current patches I think point (2), release all of the grandchild > > processes's appId at the same time when the child process is killed, is not > > handled. What happens when a child process crashes? ContentParent > > automatically kills all grandchild processes but doesn't release the > > permission reference counts. > I thought the nsFrameLoader::Destroy() would be called no matter whether > it's terminated normally or not. Only when this nsFrameLoader's owning process is still alive. > Nevertheless, if a child process crashes, > then only its nsFrameLoader::Destroy() will be called, right? (ContentParent > will still kills all the grandchild processes created by this child > process.). Function ContentParent::ActorDestroy will be triggered when the > child process crashes. In this function, it will destroy any processes > created by this ContentParent and those processes can be access through > ContentProcessManager. Thus, one simple method to fix this problem is to add > some code before those processes were destroyed(for-loop at [1]): > > nsCOMPtr<nsIPermissionManager> permMgr = services::GetPermissionManager(); > if(permMgr){ > nsTArray<TabContext> tabCtxs = > cpm->GetTabContextByContentProcess(childIDArray[i]); > for(uint32_t j = 0 ; j < tabCtxs.Length() ; j++){ > if(tabCtxs[j].OwnOrContainingAppId()!= > nsIScriptSecurityManager::NO_APP_ID){ > permMgr->ReleaseAppId(tabCtxs[j].OwnOrContainingAppId()); > } > } > } Yes, this looks right. > However, I still prefer finding a way that nsFrameLoader can manage its > permission reference count by itself(Trying to call nsFrameLoader::Destroy() > of those grandchild processes created by crashed child process?). nsFrameLoaders live in the crashed child process so it's impossible to call nsFrameLoader::Destory() for them > One main > reason is that the releasing/adding of permission can be put in one place > and same file rather than many different files. It might be easier to > maintain the code. I think what we could do here is to release the grandchild processes' appId in the child processes' nsFrameLoader::Destroy(), i.e. run above code after releasing its own appId. > > ::: dom/ipc/PContent.ipdl > > @@ +590,5 @@ > > > + * Track the app's permission by adding/releasing the appId > > > + * that can be accessed by given ContentParentId and TabId > > > + */ > > > + sync AddrefAppId(ContentParentId cpId, TabId tabId); > > > + sync ReleaseAppId(ContentParentId cpId, TabId tabId); > > > > These could have better names. Maybe |PermissionManagerAddref| and > > |PermissionManagerRelease| > > > > Do they have to be synchronous? > Oh, you're right. It's not necessary to be synchronous currently. I will > rename these two IPC functions and replace sync with async in next patch. > > [1] > https://dxr.mozilla.org/mozilla-central/source/dom/ipc/ContentParent.cpp#1963

I think I may misunderstand something previously. 1) app's iframe attribute remote ≠ true && setting of app permission is called in main process (in-process) 2) app's iframe attribute remote ≠ true && setting of app permission is called out of main process (in-process) 3) app's iframe attribute remote = true && setting of app permission is called in main process (oop) 4) app's iframe attribute remote = true && setting of app permission is called out of main process (oop) case 2: +--------+ +-----------+ | chrome |------| app A | +--------+ | +-------+ | | | app B | | | +-------+ | +-----------+ In case 2, the reference count of A's appId should stay constant when app A open a in-process iframe(app B). No matter how many in-process apps opened by app A, for chrome process, the app A is still one, so there is no need to handle this case.

oop case: +--------+ | chrome | +--------+ / \ +-------+ +-------+ | app A | | app C | +-------+ +-------+ / \ +-------+ +-------+ | app B | | app C | +-------+ +-------+ \ +-------+ | app D | +-------+ If we want to handle all the appId's reference counting in one file, I reckon nsFrameLoader is better choice than ContentParent(It seems that ContentParent can't know the opening/closing of a in-process app/browser, thus case 1 can't be handled in ContentParent). To handle the crash case, if app A's nsFrameLoader::Destroy() is called, all the appId of the grandchild processes opend by app A should be release after releasing its own appId. However, if the process is terminated normally, all the nsFrameLoader::Destroy() of app A's grandchild processes will be called. It might release same appId several times. For example, in the figure above, FrameLoader::Destroy() of app A, B, C, D will be triggered if app A is terminated. Then, app C's FrameLoader::Destroy() will be called again when app C is terminated. In such case, it might cause unexpected result: ori : refcount of c = 2 after A is closed : refcount of c = 1 (release the grandchild processes' appId(B,C,D) in the child process A's nsFrameLoader::Destroy()) after C is closed : refcount of c = 0 (C's nsFrameLoader::Destroy()) ==> refcount of c is expected to be 1 if app A is closed normally How to prevent such case mentioned? 1) The content process's appId will be released by the process who manage it only when the managing process crashes => In nsFrameLoader, it seems not easy to know FrameLoader::Destroy() is called in crash case. 2) Check whether its own reference count is cleared by other process already or not => a bit weird..? 3) ?? On the other hand, according to the differenct cases, tracking the appId's reference in different files might be easier to implement. Attachment 8571823 [details] [diff] + attachment 8571825 [details] [diff] [review] is an example. There are another approaches to track appId's reference count in different files. For instance, the case 3,4 mention previously can be handled in ContentParent. We can add appId's reference count after allocating TabId and release appId's reference count in ContentParent::ActorDestroy. The case 1 is still handled by nsFrameLoader. By taking this approach, there is no need to add/release appId's reference count via IPC. The execution to track appId's reference will be faster but the code will be harder to read. One different thing is that the appId's reference count won't be reset after nsFrameLoader::SwapWithOtherRemoteLoader or nsFrameLoader::SwapWithOtherLoader. However, I have no idea why we trigger nsFrameLoader::ResetPermissionManagerStatus() after nsFrameLoader::SwapWith*.

Hi, baku, I was wondering if you could tell me why we trigger nsFrameLoader::ResetPermissionManagerStatus() after nsFrameLoader::SwapWithOtherRemoteLoader/SwapWithOtherLoader? What's its usage? Does it will cause any problems if we do nothing to reference count(especially in oop case) when nsFrameLoader::SwapWith* is called? I would appreciate it if you could give me some suggestions.

Actually I'm not expert of this part of the code. Moving the NI to Bill McCloskey.

Hi, Bill, Would you mind giving me some information about the problems mentioned in comment #31?

The frame loader swapping code is never called for <iframe mozbrowser/mozapp>, so I don't think you need to worry about it. It's only used by Firefox desktop for <xul:browser> elements. It looks like ResetPermissionManagerStatus is called from SetOwnerContent, which is called from the swapping functions. However, you could probably move ResetPermissionManagerStatus to nsFrameLoader::Destroy instead, since the SetOwnerContent(null) call there should be the only place where it matters. Hope this helps.

(In reply to Chun-Min Chang[:chunmin] from comment #30) > By taking this approach, there is no need to add/release appId's reference count via IPC. This is incorrect! I originally thought that we can simply call permissionManager to add the appId's reference count in ContentParent::AllocateTabId[1] and to release the appId's reference count in ContentParent::DeallocateTabId[2] if their ProcessType is chrome. However, if one TabParent is not in the chrome process, it won't pass its manager's ContentParentId as the parameter of ContentParent::DeallocateTabId. Instead, it will pass ContentParentId(0)[3]. Thus, if we use the parameters passed in ContentParent::DeallocateTabId(const ContentParentId& aChildCpId, const TabId& aChildTabId) to get the appId, it will be incorrect in this case. P.S. In this case, we can get the ContentParentId of this TabParent's manager by calling "Manager()->ChildID()" because its Manager() is ContentBridgeParent, not ContentParent(Manager()->AsContentParent()->ChildID() will be correct only if the TabParent's manager is ContentParent). [1] https://dxr.mozilla.org/mozilla-central/source/dom/ipc/ContentParent.cpp#4615 [2] https://dxr.mozilla.org/mozilla-central/source/dom/ipc/ContentParent.cpp#4632 [3] https://dxr.mozilla.org/mozilla-central/source/dom/ipc/TabParent.cpp#395

Comment on attachment 8573048 [details] [diff] [review] [v5.1]part1: add/release the appId's refcnt in oop case Review of attachment 8573048 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/ipc/ContentParent.cpp @@ +932,5 @@ > aContext, > cp->ChildID()); > + // bug 1114507: Add appId's reference count in oop-case > + if (*aTabId != 0) { > + PermissionManagerAddref(cp->ChildID(), *aTabId); Can we add this function in AllocateTabId? So, we don't have to invoke PermissionManagerAddref everywhere. Note that you have to make sure the IPCTabContext is not a PopupIPCTabContext before adding the reference count. @@ +4838,5 @@ > + } > + > + return descendantsAppIdList; > +} > + Only find the 1st level of child process should be enough. ::: dom/ipc/ContentParent.h @@ +205,5 @@ > + * if the given tab is not an app. > + */ > + static uint32_t > + GetAppId(const ContentParentId& aCpId, const TabId& aTabId); > + Better to move this function to ContentProcessManager and give this function a more specific name. @@ +212,5 @@ > + * Descendants are frames opened in other process by the given ContentParent > + */ > + static nsTArray<uint32_t> > + GetAllDescendantsAppId(const ContentParentId& aCpId); > + ditto @@ +231,5 @@ > + * ContentParentId > + */ > + static bool > + PermissionManagerReleaseAllDescendants(const ContentParentId& aCpId); > + It seems this function doesn't need to be static. @@ +238,5 @@ > + * ContentParentId and the given ContentParent ifself > + */ > + static bool > + PermissionManagerReleaseAllRelated(const ContentParentId& aCpId); > + ditto ::: dom/ipc/TabParent.cpp @@ +372,5 @@ > if (XRE_GetProcessType() == GeckoProcessType_Default) { > Manager()->AsContentParent()->NotifyTabDestroyed(this, mMarkedDestroying); > + // bug 1114507: Release appId's reference count > + ContentParent::PermissionManagerRelease(Manager()->AsContentParent()->ChildID(), > + mTabId); I think we can do this in ContentParent::DeallocateTabId. @@ +379,5 @@ > } > else { > + // bug 1114507: Release appId's reference count > + ContentParent::PermissionManagerRelease(Manager()->ChildID(), mTabId); > + ditto. We can also save one time IPC call.

Summary of comment #35 and comment#40 1. I suspect the current code in TabParent::Recv__delete__() and ContentParent::RecvDeallocateTabId may be wrong. +----------------+ +-------------------+ +-------------------+ | chrome process | | content process A | | content process B | | | | | | | | +-----------+ | IPC | +----------+ | | +----------+ | | | TabParent |--+-------------+-| TabChild | | +-----+-| TabChild | | | +-----------+ | PBowser:1 | +----------+ | | | +----------+ | | | | | | | | | +---------+ | | +-----------+ | IPC | | +---------+ | | | Content |----+--+ | | TabParent |-----+---------+ | | Content | | | | Parent | | | | +-----------+ | PBowser:2 +--+-| Bridge | | | +---------+ | | | | | | | Child | | | | | IPC | +---------+ | | | +---------+ | | +---------+ | +------------| Content | | | | | | | Content | | PContent:1 | | Child | | | | +---------+ | | | Parent | | | +---------+ | | | | Content | | | +---+-----+ | | | | | | Child | | | | | | +---------+ | | | +---------+ | +-----+----------| | | Content | | IPC | | | | | | | Bridge |-------+------------+ +------+------------+ | | | Parent | | PContent | | | +---------+ | Bridge | | | | | | +-------------------+ | | IPC | +-------------------------------------------------------------------+ PContent:2 The TabParent::Recv__delete__() in process A will be called when process B is closed and it will call ContentParent::DeallocateTabId(TabId=2, ContentParentId=0)[1]. Then, ContentParent::DeallocateTabId will call ContentChild::GetSingleton()->SendDeallocateTabId(TabId=2)[2]. This IPC message will be sent via PBowser:1. Next, ContentParent::RecvDeallocateTabId(TabId=2)[3] will be called in chrome process. Finally, DeallocateTabId(TabId=2, this->ChildID()) will be executed. Here, the parameter "this->ChildID()" is 1. However, we expect 2 here. After modifying ContentParent::DeallocateTabId(mTabId, ContentParentId(0)) to ContentParent::DeallocateTabId(mTabId, Manager()->ChildID()) in TabParent::Recv__delete__(), we can release the appId's reference count in ContentParent::DeallocateTabId directly. 2. I'd love to check the IPCTabContext is not a PopupIPCTabContext before adding the appId's reference count in ContentParent::AllocateTabId, but I have no idea how to check this condition again in ContentParent::DeallocateTabId. Is there other way to check it? [1] https://dxr.mozilla.org/mozilla-central/source/dom/ipc/TabParent.cpp#395 [2] https://dxr.mozilla.org/mozilla-central/source/dom/ipc/ContentParent.cpp#4637 [3] https://dxr.mozilla.org/mozilla-central/source/dom/ipc/ContentParent.cpp#4657

Hi, Kanru, Could you give these patches a review when you are free?

Comment on attachment 8578574 [details] [diff] [review] [v6.2]part1: add/release the appId's refcnt if frame is in chrome process Review of attachment 8578574 [details] [diff] [review]: ----------------------------------------------------------------- Good. I'd like to take a second look of the comments. ::: dom/base/nsFrameLoader.cpp @@ +451,5 @@ > mURIToLoad = nullptr; > NS_ENSURE_SUCCESS(rv, rv); > > + // bug 1114507: Track the appId's reference count if this frame is in-process > + ResetPermissionManagerStatus(); nit: remove the bug number reference @@ +2619,2 @@ > // The resetting of the permissions status can run only > + // in the main process and this frame must be a in-process frame. nit: Simplify the comment above. State that only in-main-process && in-process frame is handled here and all other cases are handled by ContentParent.

Comment on attachment 8578575 [details] [diff] [review] [v6.2]part2: add/release the appId's refcnt in oop case Review of attachment 8578575 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/ipc/ContentParent.cpp @@ +1957,5 @@ > NS_DispatchToCurrentThread(new DelayedDeleteContentParentTask(this)); > > + // Bug 1114507: Release the appId's reference count of any processes > + // created by this ContentParent and the frame opened by this ContentParent > + // if this ContentParent crashes. nit: Remove the bug number reference. @@ +1968,5 @@ > + // Release the appId's reference count of its child-processes > + for (uint32_t i = 0; i < childIDArray.Length(); i++) { > + nsTArray<TabContext> tabCtxs = cpm->GetTabContextByContentProcess(childIDArray[i]); > + for (uint32_t j = 0 ; j < tabCtxs.Length() ; j++) { > + if (tabCtxs[j].OwnOrContainingAppId() != nsIScriptSecurityManager::NO_APP_ID ) { nit: extra space before ) @@ +4646,5 @@ > TabId tabId; > if (XRE_GetProcessType() == GeckoProcessType_Default) { > ContentProcessManager *cpm = ContentProcessManager::GetSingleton(); > tabId = cpm->AllocateTabId(aOpenerTabId, aContext, aCpId); > + // bug 1114507: Add appId's reference count in oop case nit: Remove the bug number @@ +4665,5 @@ > ContentParent::DeallocateTabId(const TabId& aTabId, > const ContentParentId& aCpId) > { > if (XRE_GetProcessType() == GeckoProcessType_Default) { > + // bug 1114507: Release appId's reference count in oop case nit: Remove the bug number @@ +4768,5 @@ > + const TabId& aTabId) > +{ > + NS_ASSERTION(XRE_GetProcessType() == GeckoProcessType_Default, > + "Call PermissionManagerAddref in content process!"); > + ContentProcessManager *cpm = ContentProcessManager::GetSingleton(); nit: T* cpm @@ +4784,5 @@ > + const TabId& aTabId) > +{ > + NS_ASSERTION(XRE_GetProcessType() == GeckoProcessType_Default, > + "Call PermissionManagerRelease in content process!"); > + ContentProcessManager *cpm = ContentProcessManager::GetSingleton(); nit: T* cpm ::: dom/ipc/ContentProcessManager.cpp @@ +19,5 @@ > #else > #define ASSERT_UNLESS_FUZZING(...) MOZ_ASSERT(false, __VA_ARGS__) > #endif > +// Constant duplicated from nsIScriptSecurityManager > +#define CPM_NO_APP_ID 0 Why?

(In reply to Kan-Ru Chen [:kanru] from comment #53) > ::: dom/ipc/ContentProcessManager.cpp > @@ +19,5 @@ > > #else > > #define ASSERT_UNLESS_FUZZING(...) MOZ_ASSERT(false, __VA_ARGS__) > > #endif > > +// Constant duplicated from nsIScriptSecurityManager > > +#define CPM_NO_APP_ID 0 > > Why? It's a constant duplicated from nsIScriptSecurityManager::NO_APP_ID. I am not sure if ContentProcessManager is need to know the nsIScriptSecurityManager, so I use this macro to define like [1]. [1] https://dxr.mozilla.org/mozilla-central/source/netwerk/base/nsNetUtil.h#1720

(In reply to Chun-Min Chang[:chunmin] from comment #54) > (In reply to Kan-Ru Chen [:kanru] from comment #53) > > ::: dom/ipc/ContentProcessManager.cpp > > @@ +19,5 @@ > > > #else > > > #define ASSERT_UNLESS_FUZZING(...) MOZ_ASSERT(false, __VA_ARGS__) > > > #endif > > > +// Constant duplicated from nsIScriptSecurityManager > > > +#define CPM_NO_APP_ID 0 > > > > Why? > > It's a constant duplicated from nsIScriptSecurityManager::NO_APP_ID. I am > not sure if ContentProcessManager is need to know the > nsIScriptSecurityManager, so I use this macro to define like [1]. > > [1] > https://dxr.mozilla.org/mozilla-central/source/netwerk/base/nsNetUtil.h#1720 Necko is more isolated. But I don't think this justifies the duplicated definition. It's only a source of confusion. If you need it, just include nsIScriptSecurityManager.h

Comment on attachment 8586673 [details] [diff] [review] [v6.3]part2: add/release the appId's refcnt in oop case Review of attachment 8586673 [details] [diff] [review]: ----------------------------------------------------------------- Remember to rebase and change MOZ_OVERRIDE to override.

Comment on attachment 8586675 [details] [diff] [review] [v6.3]test cases Review of attachment 8586675 [details] [diff] [review]: ----------------------------------------------------------------- I didn't finish the test_bug1114507.js file. Overall it looks good but it has too many commented out codes and unclear comments. Please refine it and ask for review again. ::: dom/ipc/tests/test_bug1114507.js @@ +29,5 @@ > +// the execution of ContentParent::DeallocateTabId, > +// so we need to delay the permission check. > +var TIMEOUT_ID; > +var DELAY_SECOND = 3; > +SimpleTest.requestFlakyTimeout("untriaged"); setTimeout is prone to intermittent failures. I think we could listen the "ipc:content-shutdown" observer instead. @@ +67,5 @@ > + > + // Add permission to app > + function() { > + addPermissionToApp(APP_URL, APP_MANIFEST); > + }, Why add permission again? @@ +199,5 @@ > + appId: appId, > + isInBrowserElement: false })) { > + // Error! > + errorHandler('App should have permission: ' + PERMISSION_TYPE); > + } Is this check redundant if we call runNextIfAppHasNoPermission? @@ +422,5 @@ > + var ifr = document.createElement('iframe'); > + ifr.setAttribute('id', APP_IFRAME_ID); > + ifr.setAttribute('remote', useRemote? "true" : "false"); > + //ifr.setAttribute('mozbrowser', 'true'); > + SpecialPowers.wrap(ifr).mozbrowser = true; Give this test the permission to embed mozbrowser && mozapp to avoid the wrapper. @@ +441,5 @@ > + { url: appURL, > + appId: appId, > + isInBrowserElement: false }, > + permManager.EXPIRE_SESSION, > + now + SESSION_PERSIST_MINUTES*60*1000); I think it's better to use pushPermissions instead rely on timeouts to cleanup the permissions. ::: dom/ipc/tests/test_bug1114507_embed.html @@ +18,5 @@ > + // The permission check should be called after > + // the execution of ContentParent::DeallocateTabId, > + // so we need to delay the permission check. > + var TIMEOUT_ID; > + var DELAY_SECOND = 3; setTimeout is prone to intermittent failures. You could listen to the "mozbrowserloadend" event on the iframe. @@ +22,5 @@ > + var DELAY_SECOND = 3; > + > + function allocateAppFrame(source = APP_URL, > + manifestURL = APP_MANIFEST, > + useRemote = false) { nit: Unused default parameters. Copied from somewhere? @@ +44,5 @@ > + } > + > + // This will be trigger after body has been loaded > + document.addEventListener('DOMContentLoaded', function(){ > + allocateAppFrame(APP_URL, APP_MANIFEST, true); allocateAppFrame could be inlined. We could even avoid listening to the DOMContentLoaded event by putting this <script> block in <body>

(In reply to Kan-Ru Chen [:kanru] from comment #60) > Comment on attachment 8586675 [details] [diff] [review] > [v6.3]test cases > > Review of attachment 8586675 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/ipc/tests/test_bug1114507.js > @@ +29,5 @@ > > +// the execution of ContentParent::DeallocateTabId, > > +// so we need to delay the permission check. > > +var TIMEOUT_ID; > > +var DELAY_SECOND = 3; > > +SimpleTest.requestFlakyTimeout("untriaged"); > > setTimeout is prone to intermittent failures. I think we could listen the > "ipc:content-shutdown" observer instead. Sure. I don't know how to deal with it so I use settimeout. I think I should ask you earlier, haha. There's always something I can learn from Gecko. > @@ +67,5 @@ > > + > > + // Add permission to app > > + function() { > > + addPermissionToApp(APP_URL, APP_MANIFEST); > > + }, > > Why add permission again? In previous round, permission is given to a test-target-app X and X is embedded in a iframe Y. After the iframe Y is removed, the app X's permission will be released. If the app X is used to be a test target in next round, the the permission need to be added to app X again. (We expect the permission will be released after iframe is removed) > @@ +199,5 @@ > > + appId: appId, > > + isInBrowserElement: false })) { > > + // Error! > > + errorHandler('App should have permission: ' + PERMISSION_TYPE); > > + } > > Is this check redundant if we call runNextIfAppHasNoPermission? runNextIfAppHasNoPermission will be called after removing the iframe Y containing a test-target-app X. If app X has no permission originally, then we can't know whether the permission is released or not after removing iframe Y. (app X has no permission -- test --> app X has no permission. expect: app X has permission --> app X has no permission) > @@ +422,5 @@ > > + var ifr = document.createElement('iframe'); > > + ifr.setAttribute('id', APP_IFRAME_ID); > > + ifr.setAttribute('remote', useRemote? "true" : "false"); > > + //ifr.setAttribute('mozbrowser', 'true'); > > + SpecialPowers.wrap(ifr).mozbrowser = true; > > Give this test the permission to embed mozbrowser && mozapp to avoid the > wrapper. Sure. > @@ +441,5 @@ > > + { url: appURL, > > + appId: appId, > > + isInBrowserElement: false }, > > + permManager.EXPIRE_SESSION, > > + now + SESSION_PERSIST_MINUTES*60*1000); > > I think it's better to use pushPermissions instead rely on timeouts to > cleanup the permissions. OK. > ::: dom/ipc/tests/test_bug1114507_embed.html > @@ +18,5 @@ > > + // The permission check should be called after > > + // the execution of ContentParent::DeallocateTabId, > > + // so we need to delay the permission check. > > + var TIMEOUT_ID; > > + var DELAY_SECOND = 3; > > setTimeout is prone to intermittent failures. You could listen to the > "mozbrowserloadend" event on the iframe. Sure. > @@ +22,5 @@ > > + var DELAY_SECOND = 3; > > + > > + function allocateAppFrame(source = APP_URL, > > + manifestURL = APP_MANIFEST, > > + useRemote = false) { > > nit: Unused default parameters. Copied from somewhere? I copy from test_bug1114507.js but forget to remove it > @@ +44,5 @@ > > + } > > + > > + // This will be trigger after body has been loaded > > + document.addEventListener('DOMContentLoaded', function(){ > > + allocateAppFrame(APP_URL, APP_MANIFEST, true); > > allocateAppFrame could be inlined. We could even avoid listening to the > DOMContentLoaded event by putting this <script> block in <body> OK.

Comment on attachment 8591479 [details] [diff] [review] [v6.3.1]test cases Review of attachment 8591479 [details] [diff] [review]: ----------------------------------------------------------------- Great! Last round :) In the meantime you could try the patches on the try server. ::: dom/ipc/tests/test_bug1114507.js @@ +25,5 @@ > +var embedApp; > + > +// Used to add listener for ipc:content-shutdown that > +// will be triggered after ContentParent::DeallocateTabId > +var gScript = SpecialPowers.loadChromeScript(SimpleTest.getTestFileURL('bug1114507-chrome-script.js')); You could use SpecialPowers.Services.obs.addObserver here @@ +101,5 @@ > + // 3 child process <-- app is here > + > + // Add permission to app > + function() { > + //addPermissionToApp(embedApp.origin, embedApp.manifestURL); nit: commented code @@ +288,5 @@ > + > + var appId = gAppsService.getAppLocalIdByManifestURL(APP_MANIFEST); > + > + if (!SpecialPowers.hasPermission(PERMISSION_TYPE, > + { url: APP_URL,/*embedApp.origin,*/ nit: commented code @@ +349,5 @@ > + { "type": "embed-apps", "allow": true, "context": document }, > + { "type": "webapps-manage", "allow": true, "context": document }], > + function() { > + SpecialPowers.setAllAppsLaunchable(true); > + SpecialPowers.setBoolPref("dom.mozBrowserFramesEnabled", true); nit: Already set via pushPrefEnv @@ +390,5 @@ > +} > + > +function removeAppFrame(parentNode, id) { > + var ifr = document.getElementById(id); > + parentNode.removeChild(ifr); ifr.remove() @@ +466,5 @@ > + SimpleTest.expectChildProcessCrash(); > + // Inject a frame script that crashes the content process > + var child = document.getElementById(frameId); > + var mm = SpecialPowers.getBrowserFrameMessageManager(child); > + mm.loadFrameScript('data:,new ' + function ContentScriptScope() { Use strings instead of Function.prototype.toString(). The latter is not guaranteed to return the original source. ::: dom/ipc/tests/test_bug1114507_embed.html @@ +15,5 @@ > + var APP_IFRAME_ID = "test-target"; > + > + function removeAppFrame(){ > + var ifr = document.getElementById(APP_IFRAME_ID); > + document.body.removeChild(ifr); ifr.remove();

(In reply to Kan-Ru Chen [:kanru] from comment #63) > Comment on attachment 8591479 [details] [diff] [review] > [v6.3.1]test cases > > Review of attachment 8591479 [details] [diff] [review]: > ----------------------------------------------------------------- > > Great! Last round :) > In the meantime you could try the patches on the try server. > > ::: dom/ipc/tests/test_bug1114507.js > @@ +25,5 @@ > > +var embedApp; > > + > > +// Used to add listener for ipc:content-shutdown that > > +// will be triggered after ContentParent::DeallocateTabId > > +var gScript = SpecialPowers.loadChromeScript(SimpleTest.getTestFileURL('bug1114507-chrome-script.js')); > > You could use SpecialPowers.Services.obs.addObserver here The Error: Permission denied for <http://mochi.test:8888> to create wrapper for object of class UnnamedClass will occur if I use SpecialPowers.Services.obs.addObserver(observer, "ipc:content-shutdown", false); I guess this message will only be reported to their ContentParent and their ContentParent is chrome.

Comment on attachment 8592044 [details] [diff] [review] [v6.3.2]test cases Review of attachment 8592044 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/ipc/tests/test_bug1114507_embed.html @@ +33,5 @@ > + sendMessgeToParent('crash'); > + } else { > + removeAppFrame(); > + // Notify its parent process > + sendMessgeToParent('removed'); I think I make a mistake here. The iframe itself embedding "test_bug1114507_embed.html" may be closed earlier than the iframe embedding "http://example.org" is closed. 1) The removeAppFrame() will trigger TabChild::RecvDestroy() --> DelayedDeleteRunnable() --> PBrowserChild::Send__delete__(mTabChild) --> TabParent::Recv__delete__() --> ContentParent::DeallocateTabId 2) sendMessgeToParent('removed') will trigger another removeAppFrame() in its opener iframe (2) may finish execution earlier than (1), and it's not what we want. The (2) is expected to be executed after finishing (1). Add a delay after removeAppFrame() might solve the problem: removeAppFrame(); setTimeout(function(){ // Notify its parent process sendMessgeToParent('removed'); }, 3000); However, I wonder, is there a better solution? In "test_bug1114507_embed.html", we can't get SpecialPowers, so we can't add an observer to listen ipc:content-shutdown. What else can we do?

Comment on attachment 8592044 [details] [diff] [review] [v6.3.2]test cases Review of attachment 8592044 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/ipc/tests/test_bug1114507_embed.html @@ +33,5 @@ > + sendMessgeToParent('crash'); > + } else { > + removeAppFrame(); > + // Notify its parent process > + sendMessgeToParent('removed'); You could wait "removed" message in the opener iframe and also listen to ipc:content-shutdown in the opener iframe. I think something like function receiveMessage(message) { if (message.name == "content-removed") { afterContentShutdown(1, callback); } } would work.

(In reply to Chun-Min Chang[:chunmin] from comment #69) > Created attachment 8599182 [details] [diff] [review] > [v7]part3: Remove PContetBridge channel when grandchild-process is killed (In reply to Chun-Min Chang[:chunmin] from comment #68) > Created attachment 8599181 [details] [diff] [review] > [v7]part2: add/release the appId's refcnt in oop case Hi, Kanru, Could you take a look at these? I am not sure if I am on the right track. I would be thankful if you can point me in the right direction. :)

Comment on attachment 8599181 [details] [diff] [review] [v7]part2: add/release the appId's refcnt in oop case Review of attachment 8599181 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/ipc/ContentParent.cpp @@ +4872,5 @@ > > /*static*/ void > ContentParent::DeallocateTabId(const TabId& aTabId, > + const ContentParentId& aCpId, > + bool mMarkedDestroying) aMarkedDestroying @@ +4889,5 @@ > } > else { > + ContentChild::GetSingleton()->SendDeallocateTabId(aTabId, > + aCpId, > + mMarkedDestroying); The number of numLiveTabs in ContentParent::NotifyTabDestroying should also use the tabIds.Length() and you should also send NotifyTabDestroying to ContentParent @@ +4909,5 @@ > > bool > +ContentParent::RecvDeallocateTabId(const TabId& aTabId, > + const ContentParentId& aCpId, > + const bool& mMarkedDestroying) aMarkedDestroying

Comment on attachment 8610385 [details] [diff] [review] [WIP]part2: add/release the appId's refcnt in oop case Review of attachment 8610385 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/ipc/ContentParent.cpp @@ +2103,5 @@ > + ContentProcessManager *cpm = ContentProcessManager::GetSingleton(); > + ContentParent* cp = cpm->GetContentProcessById(aCpId); > + ++cp->mNumDestroyingTabs; > + nsTArray<TabId> tabIds = cpm->GetTabParentsByProcessId(aCpId); > + if (cp->mNumDestroyingTabs != (int32_t)tabIds.Length()) { case 1: main process --- child process --- grandchild process < grandchild crashes! In case 1, if numLiveTabs is replaced by tabIds.Length(), ContentParent* cp = cpm->GetContentProcessById(aCpId) called in child process will get stuck at [1] because the ContentParent::ActorDestroy called in main process will remove the process information from mContentParentMap[2] before GetContentProcessById(aCpId) is executed. [1] https://dxr.mozilla.org/mozilla-central/source/dom/ipc/ContentProcessManager.cpp#110 [2] https://dxr.mozilla.org/mozilla-central/source/dom/ipc/ContentParent.cpp#2065 I don't know how to deal with it, so I temporarily use the flag mIsDestroyed to prevent executing ContentParent::NotifyTabDestroying in TabParent::Destroy().

(In reply to Chun-Min Chang[:chunmin] from comment #76) > I don't know how to deal with it, so I temporarily use the flag mIsDestroyed > to prevent executing ContentParent::NotifyTabDestroying in > TabParent::Destroy(). When tab crashes, I think there is no need to call ContentParent::NotifyTabDestroying because the jobs in ContentParent::NotifyTabDestroying will be done by ContentParent::ActorDestroy. The next patch will be uploaded after bug 1141415 is finished.

Comment on attachment 8610386 [details] [diff] [review] [v7]part3: Remove PContetBridge channel when grandchild-process is killed This patch doesn't be needed after bug 1170939 is finished

Comment on attachment 8610387 [details] [diff] [review] [v8]test cases cancel review until part 2 is finished.

rebase and carry r+

Comment on attachment 8643612 [details] [diff] [review] [v9]part2: add/release the appId's refcnt in oop case Review of attachment 8643612 [details] [diff] [review]: ----------------------------------------------------------------- Hey, we are getting close :) ::: dom/ipc/ContentParent.cpp @@ +1447,5 @@ > } > Preferences::AddStrongObserver(this, ""); > if (obs) { > + nsAutoString cpId; > + cpId.AppendInt(static_cast<int32_t>(this->ChildID())); Should be uint64_t @@ +2041,5 @@ > } > #endif > } > + nsAutoString cpId; > + cpId.AppendInt(static_cast<int32_t>(this->ChildID())); uint64_t @@ +2089,5 @@ > + } > + } > + } > + // Release the appId's reference count belong to itself > + nsTArray<TabContext> tCtxs = cpm->GetTabContextByContentProcess(mChildID); Consistent naming tabCtxs here @@ +2116,2 @@ > { > + if (XRE_GetProcessType() == GeckoProcessType_Default) { Use XRE_IsParentProcess() @@ +2123,5 @@ > + ContentProcessManager *cpm = ContentProcessManager::GetSingleton(); > + ContentParent* cp = cpm->GetContentProcessById(aCpId); > + ++cp->mNumDestroyingTabs; > + nsTArray<TabId> tabIds = cpm->GetTabParentsByProcessId(aCpId); > + if (cp->mNumDestroyingTabs != (int32_t)tabIds.Length()) { Why do we need this casting? @@ +4935,5 @@ > + if (aTabId) { > + PermissionManagerRelease(aCpId, aTabId); > + } > + > + ContentProcessManager *cpm = ContentProcessManager::GetSingleton(); Snuggle pointer stars with the type @@ +5152,5 @@ > +ContentParent::PermissionManagerAddref(const ContentParentId& aCpId, > + const TabId& aTabId) > +{ > + NS_ASSERTION(XRE_GetProcessType() == GeckoProcessType_Default, > + "Call PermissionManagerAddref in content process!"); Use MOZ_ASSERT and XRE_IsParentProcess() @@ +5168,5 @@ > +ContentParent::PermissionManagerRelease(const ContentParentId& aCpId, > + const TabId& aTabId) > +{ > + NS_ASSERTION(XRE_GetProcessType() == GeckoProcessType_Default, > + "Call PermissionManagerRelease in content process!"); Use MOZ_ASSERT and XRE_IsParentProcess() ::: dom/ipc/TabParent.cpp @@ +500,5 @@ > + if (XRE_GetProcessType() == GeckoProcessType_Content && > + why == AbnormalShutdown && > + !mIsDestroyed) { > + mIsDestroyed = true; > + } Use XRE_IsContentProcess() here This skips a lots of things in TabParent::Destroy(), that is not good. Could we be smarter in ContentParent::NotifyTabDestroying that just skips disappeared TabParent? @@ +1376,5 @@ > if (widget) { > + // When we mouseenter the tab, the tab's cursor should become the current > + // cursor. When we mouseexit, we stop. > + if (event.message == NS_MOUSE_ENTER_WIDGET || > + event.message == NS_MOUSE_OVER) { Rebase error? And rest of the patch for this file.

(In reply to Kan-Ru Chen [:kanru] from comment #88) > @@ +2123,5 @@ > > + ContentProcessManager *cpm = ContentProcessManager::GetSingleton(); > > + ContentParent* cp = cpm->GetContentProcessById(aCpId); > > + ++cp->mNumDestroyingTabs; > > + nsTArray<TabId> tabIds = cpm->GetTabParentsByProcessId(aCpId); > > + if (cp->mNumDestroyingTabs != (int32_t)tabIds.Length()) { > > Why do we need this casting? Thx for your review! I'll get an error: comparison between signed and unsigned integer expressions if I don't cast the type. | Variable | Type | |--------------------|------------------| | mNumDestroyingTabs | int32_t | | nsTArray.Length() | (unsigned)size_t | | Cast | Risk | From | To | |---------|---------|----------------------------------------------------- | int32_t | size_t | negative int overflow into an incorrect huge size_t |---------|---------|------------------------------------------------------ | size_t | int32_t | huge size_t overflow into an incorrect negative int I cast (unsigned)size_t to signed int because I think tabIds.Length() won't be huge. If mNumDestroyingTabs is always positive, I'm ok to change it.

Correct the file name

(In reply to Kan-Ru Chen [:kanru] from comment #88) > ::: dom/ipc/TabParent.cpp > @@ +500,5 @@ > > + if (XRE_GetProcessType() == GeckoProcessType_Content && > > + why == AbnormalShutdown && > > + !mIsDestroyed) { > > + mIsDestroyed = true; > > + } > > Use XRE_IsContentProcess() here > > This skips a lots of things in TabParent::Destroy(), that is not good. > Could we be smarter in ContentParent::NotifyTabDestroying that just skips > disappeared TabParent? If we use GetTabParentsByProcessId to get inexistent ContentParent, the program will get stuck at [1]. Is there other way to check whether or not the ContentParent/TabParent exists? [1] https://dxr.mozilla.org/mozilla-central/source/dom/ipc/ContentProcessManager.cpp?offset=200#288

rename all files

Format to hg patch

Move some parts of TabParent::Destroy() into TabParent::DestroyInternal() and call TabParent::DestroyInternal() when tab crashed in content process

Comment on attachment 8646830 [details] [diff] [review] [v10]part3: Remove PContetBridge channel when grandchild-process is killed Review of attachment 8646830 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/ipc/ContentBridgeParent.cpp @@ +150,5 @@ > { > + int32_t numLiveTabs = ManagedPBrowserParent().Length(); > + if (!numLiveTabs) { > + this->Close(); > + } At this point the PBrowserParent is not yet removed from the managed PBrowserParent array. You probably want to check if numLiveTabs == 1. And please post a message to the same thread to Close the ContentBridgeParent. Close too early might end up triggering bad actors assertions.

(In reply to Kan-Ru Chen [:kanru] from comment #102) > Comment on attachment 8646830 [details] [diff] [review] > [v10]part3: Remove PContetBridge channel when grandchild-process is killed > > Review of attachment 8646830 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/ipc/ContentBridgeParent.cpp > @@ +150,5 @@ > > { > > + int32_t numLiveTabs = ManagedPBrowserParent().Length(); > > + if (!numLiveTabs) { > > + this->Close(); > > + } > > At this point the PBrowserParent is not yet removed from the managed > PBrowserParent array. You probably want to check if numLiveTabs == 1. And > please post a message to the same thread to Close the ContentBridgeParent. > Close too early might end up triggering bad actors assertions. Thx for your review! OK, instead of calling this->Close() directly, I will post a message to the same thread to close the channel. On the other hand, I think numLiveTabs will be 0 at this time point. If I understand correctly, the code flow when we close a tab is: 1) TabParent::Destroy() 2) TabParent::DestroyInternal() --> call SendDestroy() 3) case PBrowser::Msg___delete____ID in PBrowserParent::OnMessageReceived[1] 4) TabParent::Recv__delete__() 5) [IPC] ContentParent::DeallocateTabId 6) case PBrowserMsgStart in PContentBridgeParent::RemoveManagee[2] (called by [3]) 7) (mManagedPBrowserParent).RemoveElementSorted(actor)[4] 8) ContentBridgeParent::DeallocPBrowserParent (called by [5]) When the last tab is closed, the variable numLiveTabs = ManagedPBrowserParent().Length()[6] called in step(8) will be 0 because step(7) will remove the last element in mManagedPBrowserParent array. [1] https://dxr.mozilla.org/mozilla-central/source/obj-x86_64-unknown-linux-gnu/ipc/ipdl/PBrowserParent.cpp#2796 [2] https://dxr.mozilla.org/mozilla-central/source/obj-x86_64-unknown-linux-gnu/ipc/ipdl/PContentBridgeParent.cpp#370 [3] https://dxr.mozilla.org/mozilla-central/source/obj-x86_64-unknown-linux-gnu/ipc/ipdl/PBrowserParent.cpp#2822 [4] https://dxr.mozilla.org/mozilla-central/source/obj-x86_64-unknown-linux-gnu/ipc/ipdl/PContentBridgeParent.cpp#375 [5] https://dxr.mozilla.org/mozilla-central/source/obj-x86_64-unknown-linux-gnu/ipc/ipdl/PContentBridgeParent.cpp#376 [6] https://dxr.mozilla.org/mozilla-central/source/obj-x86_64-unknown-linux-gnu/ipc/ipdl/PContentBridgeParent.cpp#138

(In reply to Chun-Min Chang[:chunmin] from comment #103) > (In reply to Kan-Ru Chen [:kanru] from comment #102) > > Comment on attachment 8646830 [details] [diff] [review] > > [v10]part3: Remove PContetBridge channel when grandchild-process is killed > > > > Review of attachment 8646830 [details] [diff] [review]: > > ----------------------------------------------------------------- > > > > ::: dom/ipc/ContentBridgeParent.cpp > > @@ +150,5 @@ > > > { > > > + int32_t numLiveTabs = ManagedPBrowserParent().Length(); > > > + if (!numLiveTabs) { > > > + this->Close(); > > > + } > > > > At this point the PBrowserParent is not yet removed from the managed > > PBrowserParent array. You probably want to check if numLiveTabs == 1. And > > please post a message to the same thread to Close the ContentBridgeParent. > > Close too early might end up triggering bad actors assertions. > Thx for your review! > OK, instead of calling this->Close() directly, I will post a message to the > same thread to close the channel. > > On the other hand, I think numLiveTabs will be 0 at this time point. > If I understand correctly, the code flow when we close a tab is: > 1) TabParent::Destroy() > 2) TabParent::DestroyInternal() --> call SendDestroy() > 3) case PBrowser::Msg___delete____ID in PBrowserParent::OnMessageReceived[1] > 4) TabParent::Recv__delete__() > 5) [IPC] ContentParent::DeallocateTabId > 6) case PBrowserMsgStart in PContentBridgeParent::RemoveManagee[2] (called > by [3]) > 7) (mManagedPBrowserParent).RemoveElementSorted(actor)[4] > 8) ContentBridgeParent::DeallocPBrowserParent (called by [5]) > > When the last tab is closed, the variable numLiveTabs = > ManagedPBrowserParent().Length()[6] called in step(8) will be 0 because > step(7) will remove the last element in mManagedPBrowserParent array. > > [1] > https://dxr.mozilla.org/mozilla-central/source/obj-x86_64-unknown-linux-gnu/ > ipc/ipdl/PBrowserParent.cpp#2796 > [2] > https://dxr.mozilla.org/mozilla-central/source/obj-x86_64-unknown-linux-gnu/ > ipc/ipdl/PContentBridgeParent.cpp#370 > [3] > https://dxr.mozilla.org/mozilla-central/source/obj-x86_64-unknown-linux-gnu/ > ipc/ipdl/PBrowserParent.cpp#2822 > [4] > https://dxr.mozilla.org/mozilla-central/source/obj-x86_64-unknown-linux-gnu/ > ipc/ipdl/PContentBridgeParent.cpp#375 > [5] > https://dxr.mozilla.org/mozilla-central/source/obj-x86_64-unknown-linux-gnu/ > ipc/ipdl/PContentBridgeParent.cpp#376 > [6] > https://dxr.mozilla.org/mozilla-central/source/obj-x86_64-unknown-linux-gnu/ > ipc/ipdl/PContentBridgeParent.cpp#138 Thanks, r+!

Rebase and carry r+

rebase and carry r+

rebase and carry r+

add r+ to patch file

Hi, Kanru, After pushing previous patches on Treeherder-jamin(for nested-oop tests), I found some work-queue errors happened. To avoid that, I adjust the order to close the PContentBridge channel a little bit from previous patch. Mainly, I add a new function 'ContentBridgeParent::NotifyTabDestroyed' to close the PContentBridge channel and call it in TabParent::Recv__delete__() if the process isn't chrome-process.

(In reply to Chun-Min Chang[:chunmin] from comment #112) > Created attachment 8653848 [details] [diff] [review] > [v10.2]part3: Remove PContetBridge channel when grandchild-process is killed > Hi, Kanru, I add one line in TabParent::Recv__delete__() to meet the need of part3 patch and carry r+, is that ok?

correct the file name

Comment on attachment 8653848 [details] [diff] [review] [v10.2]part3: Remove PContetBridge channel when grandchild-process is killed Review of attachment 8653848 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/ipc/ContentBridgeParent.h @@ +25,5 @@ > NS_DECL_ISUPPORTS > NS_DECL_NSIOBSERVER > virtual void ActorDestroy(ActorDestroyReason aWhy) override; > void DeferredDestroy(); > + virtual bool IsContentBridgeParent() override { return true; } Another patch? ::: dom/ipc/nsIContentParent.cpp @@ +49,5 @@ > +nsIContentParent::AsContentBridgeParent() > +{ > + MOZ_ASSERT(IsContentBridgeParent()); > + return static_cast<ContentBridgeParent*>(this); > +} Another patch? ::: dom/ipc/nsIContentParent.h @@ +77,5 @@ > const bool& aIsForBrowser) = 0; > virtual bool IsContentParent() { return false; } > ContentParent* AsContentParent(); > + virtual bool IsContentBridgeParent() { return false; } > + ContentBridgeParent* AsContentBridgeParent(); Another patch?

(In reply to Chun-Min Chang[:chunmin] from comment #114) > Created attachment 8653991 [details] [diff] [review] > 0002-bug-1114507-part2-part2-add-release-the-appId-s-refc.patch > > (In reply to Chun-Min Chang[:chunmin] from comment #112) > > Created attachment 8653848 [details] [diff] [review] > > [v10.2]part3: Remove PContetBridge channel when grandchild-process is killed > > > Hi, Kanru, > I add one line in TabParent::Recv__delete__() to meet the need of part3 > patch and carry r+, is that ok? I think this is fine for now. Ideally we should use DeallocPBrowserParent. After you land this patch series could you write a wiki about how tab destruction works at https://wiki.mozilla.org/Nested_Content_Processes ?

(In reply to Kan-Ru Chen [:kanru] from comment #116) > Another patch? No, it's part of this patch. To call ContentBridgeParent::NotifyTabDestroyed in TabParent::Recv__delete__(), I add this method(nsIContentParent::AsContentBridgeParent()) for casting TabParent's Manager() in content process.

(In reply to Kan-Ru Chen [:kanru] from comment #117) OK, I'll update wiki after landing it.

update title and carry r+

https://hg.mozilla.org/integration/mozilla-inbound/rev/2beac00f41a1 https://hg.mozilla.org/integration/mozilla-inbound/rev/b08257bf7dfe https://hg.mozilla.org/integration/mozilla-inbound/rev/d3a6c5aa62b2 https://hg.mozilla.org/integration/mozilla-inbound/rev/b44e68ea022f

https://hg.mozilla.org/mozilla-central/rev/2beac00f41a1 https://hg.mozilla.org/mozilla-central/rev/b08257bf7dfe https://hg.mozilla.org/mozilla-central/rev/d3a6c5aa62b2 https://hg.mozilla.org/mozilla-central/rev/b44e68ea022f