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)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
WONTFIX
People
(Reporter: kanru, Unassigned)
References
Details
Attachments
(1 file, 4 obsolete files)
(deleted),
patch
|
Details | Diff | Splinter Review |
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•9 years ago
|
Blocks: nested-oop
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.
Fix crash issue.
[TODO]
1. Add test.
Attachment #8698398 -
Attachment is obsolete: true
Comment 3•9 years ago
|
||
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.
Now bug 1187757 works on oop <iframe> in b2g process.
[TODO]
1. Add test.
2. Remove temporal code.
Attachment #8715770 -
Attachment is obsolete: true
Remove temporal code.
[TODO]
1. Add test.
Attachment #8733826 -
Attachment is obsolete: true
Attachment #8735797 -
Flags: feedback?(schien)
Comment 6•9 years ago
|
||
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)
Updated•7 years ago
|
Status: NEW → RESOLVED
Closed: 7 years ago
Flags: needinfo?(schien)
Resolution: --- → WONTFIX
Assignee | ||
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•