Use ContentProcessManager::AllocateTabId to allocate tabId for each frameloader

RESOLVED WONTFIX

Status

()

Core
DOM
RESOLVED WONTFIX
3 years ago
10 months ago

People

(Reporter: kanru, Unassigned)

Tracking

(Blocks: 2 bugs)

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

3 years ago
Currently TabParent/TabChild pair has unique identifier TabId so we could track them cross-processes. It will be nice if we could have unique id for each frameLoader no matter if they own in-process or remote frame.

We could reuse the ContentProcessManager::AllocateTabId() mechanism to manage the id allocation and bookkeeping.
(Reporter)

Updated

3 years ago
Blocks: 1020135

Comment 1

3 years ago
Created attachment 8698398 [details] [diff] [review]
[WIP] 1226535-wip-20151215.patch

Split from https://bugzilla.mozilla.org/show_bug.cgi?id=1187757#c14

[TODO]
1. Add test.
2. Fix that Firefox crashes when new page is opened.

Comment 2

2 years ago
Created attachment 8715770 [details] [diff] [review]
[WIP] 1226535-wip-20160204.patch

Fix crash issue.

[TODO]
1. Add test.
Attachment #8698398 - Attachment is obsolete: true
Comment on attachment 8715770 [details] [diff] [review]
[WIP] 1226535-wip-20160204.patch

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

::: dom/base/nsFrameLoader.cpp
@@ +166,5 @@
> +  UnsafeIPCTabContext unsafeTabContext;
> +  IPCTabContext *ipcContext;
> +  ipcContext = new mozilla::dom::IPCTabContext(unsafeTabContext);
> +  ContentParentId temp3;
> +  uint64_t tabId = ContentParent::AllocateTabId(temp1, *ipcContext, temp3);

These code doesn't look correctly to me. The creation of TabContext should be different for b2g process and content process.

This might be the root cause of the mismatching TabId issue in bug 1187757.

Comment 4

2 years ago
Created attachment 8733826 [details] [diff] [review]
[WIP] 1226535-wip-20160323.patch

Now bug 1187757 works on oop <iframe> in b2g process.
[TODO]
1. Add test.
2. Remove temporal code.
Attachment #8715770 - Attachment is obsolete: true

Comment 5

2 years ago
Created attachment 8735797 [details] [diff] [review]
[WIP] 1226535-wip-20160329.patch

Remove temporal code.
[TODO]
1. Add test.
Attachment #8733826 - Attachment is obsolete: true
Attachment #8735797 - Flags: feedback?(schien)
Comment on attachment 8735797 [details] [diff] [review]
[WIP] 1226535-wip-20160329.patch

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

After this patch, tabId for nsFrameLoader in chrome process will have no corresponding remote browser. We might need to re-evaluate assertions in ContentProcessManager. Need @kanru to double check the tabId allocation code in nsFrameLoader.

::: dom/base/nsFrameLoader.cpp
@@ +3190,5 @@
> +      mTabId = tabId;
> +    } else {
> +      ContentProcessManager* cpm = ContentProcessManager::GetSingleton();
> +      mTabId = cpm->AllocateTabId();
> +    }

nsFrameLoader can still exist in child process and you'll need add corresponding TabId allocation code here. You can use the original "AllocateTabId" method to get TabId by get openerTabId like in [1], tab context from nsFrameLoader::GetNewTabContext [2], and cpId from ContentChild::GetId().

[1] https://dxr.mozilla.org/mozilla-central/source/dom/ipc/ContentParent.cpp#1138 
[2] https://dxr.mozilla.org/mozilla-central/source/dom/base/nsFrameLoader.cpp#2414

::: dom/base/nsIFrameLoader.idl
@@ +208,4 @@
>     */
>    readonly attribute boolean ownerIsWidget;
>  
> +  readonly attribute unsigned long long tabId;

can we use |unint64_t| instead of |unsigned long long| here?

::: dom/ipc/ContentProcessManager.cpp
@@ +138,5 @@
>  TabId
> +ContentProcessManager::AllocateTabId()
> +{
> +  mUniqueId = ++gTabId;
> +  return mUniqueId;

add |MOZ_ASSERT(XRE_IsParentProcess());|
Attachment #8735797 - Flags: feedback?(schien) → feedback?(kchen)

Comment 7

2 years ago
Created attachment 8740213 [details] [diff] [review]
[WIP] 1226535-wip-20160412.patch

> nsFrameLoader can still exist in child process and you'll need add corresponding TabId allocation code here. You can use the original "AllocateTabId" method to get TabId by get openerTabId like in [1], tab context from nsFrameLoader::GetNewTabContext [2], and cpId from ContentChild::GetId().
Actually I could not find what value set to cpId and why we need to allocate new openerTabId and tab context. Would you mind giving me more information about this?
Attachment #8735797 - Attachment is obsolete: true
Attachment #8735797 - Flags: feedback?(kchen)
Flags: needinfo?(schien)
Status: NEW → RESOLVED
Last Resolved: 10 months ago
Flags: needinfo?(schien)
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.