Closed Bug 1226535 Opened 9 years ago Closed 7 years ago

Use ContentProcessManager::AllocateTabId to allocate tabId for each frameloader

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set
normal

Tracking

()

RESOLVED WONTFIX

People

(Reporter: kanru, Unassigned)

References

Details

Attachments

(1 file, 4 obsolete files)

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.
Blocks: nested-oop
Attached patch [WIP] 1226535-wip-20151215.patch (obsolete) — Splinter Review
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.
Attached patch [WIP] 1226535-wip-20160204.patch (obsolete) — Splinter Review
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.
Attached patch [WIP] 1226535-wip-20160323.patch (obsolete) — Splinter Review
Now bug 1187757 works on oop <iframe> in b2g process.
[TODO]
1. Add test.
2. Remove temporal code.
Attachment #8715770 - Attachment is obsolete: true
Attached patch [WIP] 1226535-wip-20160329.patch (obsolete) — Splinter Review
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)
> 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
Closed: 7 years ago
Flags: needinfo?(schien)
Resolution: --- → WONTFIX
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: