Closed Bug 1521149 Opened 1 year ago Closed 1 year ago

Keep track of all BrowsingContext objects in a BrowsingContextGroup

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla67
Fission Milestone M1
Tracking Status
firefox67 --- fixed

People

(Reporter: Nika, Assigned: farre)

References

Details

Attachments

(2 files)

No description provided.

Generally I'm all in favour of this patch with the exception of BrowsingContextGroup::mContexts, which I don't understand. Why do we need to keep track of all BrowsingContext in BrowsingContextGroup more than we already do?

Checking if a BrowsingContext is a member of a particular BrowsingContextGroup doesn't need a hash table, but can just be:

bool BrowsingContextGroup::Contains(BrowsingContext* aBC) { return this == aBC->Group(); }

Flags: needinfo?(nika)
Attachment #9037620 - Attachment description: Bug 1521149 - Keep track of all BrowsingContext object in a BrowsingContextGroup, → Bug 1521149 - Keep track of all BrowsingContext object in a BrowsingContextGroup
Assignee: nika → afarre
Priority: -- → P2

Try says that I need to put some more work into this: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6dde50aa544826a1bd832353e26fafa0a6c53f2e&selectedJob=223735159

Also, there are reasons for mContext. Will be used for validation in BC syncing.

Flags: needinfo?(nika)

:neha asked me to look into a fix for the leaking issue. This patch seems to work OK locally for fixing it.

I think the root of the problem is that there was a cycle keeping nsDocShell alive after it had been destroyed. This patch changes the code to null out the BrowsingContext -> nsDocShell reference when the nsDocShell is destroyed.

There were a few other needed changes to prevent the cycle collector from destroying the BrowsingContextGroup object too early which should ideally be unnecessary after the syncing work is done, and the ContentParent is keeping the BrowsingContextGroup alive for us.

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8a1fb6c3211843bcda8dd241920419d47eef1218

Flags: needinfo?(afarre)

So just that I understand, there was a cycle keeping nsDocShell alive, right? As in the direct cycle created by BrowsingContext::mDocShell and nsDocShell::mBrowsingContext isn't that cycle? Because there are some subtle ownership changes here. The list of root browsing contexts used to be the thing keeping browsing contexts alive in the content parent, but now I guess that that is done by WindowGlobalParent instead, because the static list of BCG that replaces the static list of root BC only keep pointers.

Flags: needinfo?(afarre) → needinfo?(nika)

(In reply to Andreas Farre [:farre] from comment #6)

So just that I understand, there was a cycle keeping nsDocShell alive, right? As in the direct cycle created by BrowsingContext::mDocShell and nsDocShell::mBrowsingContext isn't that cycle? Because there are some subtle ownership changes here. The list of root browsing contexts used to be the thing keeping browsing contexts alive in the content parent, but now I guess that that is done by WindowGlobalParent instead, because the static list of BCG that replaces the static list of root BC only keep pointers.

Yeah, the ownership keeping things alive isn't great right now. What's keeping it alive in the content parent right now is the hacky part which doesn't traverse one CC edge, which isn't great... You might want to clean that up.

The mDocShell <-> mBrowsingContext cycle was kinda keeping nsDocShell alive. nsDocShell used to not be cycle collected at all, and has some super weird lifetime semantics because of it. In the past, I tried to make the nsDocShell <-> nsGlobalWindowOuter relationship not be cleared by nsDocShell::Destroy, but it caused massive leaks like this one. I'm guessing that the issue here is probably similar -- there are some edges which I couldn't find which are keeping the whole tree alive, and are being kept alive themselves from the nsDocShell, but aren't being traversed.

Flags: needinfo?(nika)
Fission Milestone: --- → M1
Pushed by afarre@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/db3c21efe3c0
Keep track of all BrowsingContext object in a BrowsingContextGroup r=nika
Status: NEW → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla67
Depends on: 1524188
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.