1521149 - Keep track of all BrowsingContext objects in a BrowsingContextGroup

Status: RESOLVED FIXED

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

Comment 1

[Nika Layzell [:nika] (ni? for response)]

Attachment: Bug 1521149 - Keep track of all BrowsingContext object in a BrowsingContextGroup

Comment 2

[Andreas Farre [:farre]]

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(); }

Comment 3

[Andreas Farre [:farre]]

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.

Comment 4

[Nika Layzell [:nika] (ni? for response)]

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

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(); }

We'll talk more about this at our meeting later today. The idea here is to

Comment 5

[Nika Layzell [:nika] (ni? for response)]

Attachment: Potential Leak Fix

: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

Comment 6

[Andreas Farre [:farre]]

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.

Comment 7

[Nika Layzell [:nika] (ni? for response)]

(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.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/db3c21efe3c0
Keep track of all BrowsingContext object in a BrowsingContextGroup r=nika

Comment 9

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/db3c21efe3c0