1568686 - Should PresShell be cycle-collectable?

Status: NEW

(Core :: Layout, defect, P4)

Description

[Emilio Cobos Álvarez [:emilio]]

So in bug 1567237 I got backed out because a test of mine perma-leaked.

Upon further thinking, I came up with bug 1567237 comment 13, which fixes it. The test was grabbing a reference to an nsISelectionController (the PresShell), and not releasing it. PresShell is not cycle-collectable and holds references to a document, which means we leak.

In hindsight, all other tests I cargo-culted from were using the controller inside a function, which is presumably why they don't leak.

I think we don't have many references to pres shells from JS other than from a couple tests, but it seems a bit sketchy/unexpected to leak because of holding one alive...

Should PresShell be properly cycle-collectable? It seems hard-ish, and I wonder there may be a bit of history for this. Cc'ing a few people that may be familiar with the cycle collection setup here, and with nsISelectionController.

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

Well, we don't in general cycle collect all the internal classes.

For normal cases PresShell::Destroy should cut most of the edges.

Comment 2

[Boris Zbarsky [:bzbarsky]]

PresShell::Destroy does not clear out mDocument.

In general, it seems like either Destroy needs to drop refs or we should CC PresShell.

Note that nsPresContext is CCed, and if we still want to merge that with presshell, we would want to therefore make presshell CCed.

What seems hard about making PresShell CCed? It's mainthread-only, so what would be the issue?

Comment 3

[Emilio Cobos Álvarez [:emilio]]

Well, I meant that it's hard to get to all the relevant edges. PresShell keeps a lot of references to DOM objects transitively, for example styles contain style structs which contain image requests, which contain a docgroup pointer (that one in particular we may be able to remove).

Comment 4

[Boris Zbarsky [:bzbarsky]]

Well, I meant that it's hard to get to all the relevant edges.

You really only need to get to the edges that are not dropped in Destroy, right?

Comment 5

[Emilio Cobos Álvarez [:emilio]]

Well, from the leak in bug 1567237, it looks that we're at least leaking some style structs, even though at that point PresShell::Destroy should've been called. But I guess I should dig more and figure out where they come from and why there are style structs around after Destroy() has been called...

Comment 6

[Mats Palmgren (inactive)]

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #2)

PresShell::Destroy does not clear out mDocument.

Are there any fundamental reasons why we couldn't do so?
We would have to fix a few places that use mDocument unconditionally after a possible Destroy(), like DoFlushPendingNotifications(), but that could be fixed by replacing its uses of
mDocument with a local strong ref on the stack. Other than that, I don't see any issues.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Right, we'd need to find all places that use mDocument on presshell and null-check them. Probably including all callers of PresShell::GetDocument: https://searchfox.org/mozilla-central/search?q=symbol:_ZNK7mozilla9PresShell11GetDocumentEv&redirect=false

Comment 8

[BugBot [:suhaib / :marco/ :calixte]]

The priority flag is not set for this bug.
:jwatt, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 9

[Mats Palmgren (inactive)]

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #7)

Right, we'd need to find all places that use mDocument on presshell and null-check them. Probably including all callers of PresShell::GetDocument

Fwiw, I investigated that a bit and the consumers seem reasonable to me, so we probably shouldn't null out mDocument in Destroy() after all. Instead, I think the correct solution is to merge PresContext/PresShell, perhaps by embedding a PresContext member in PresShell like we did with the FrameConstructor. Then we can remove PresShell::mDocument and make GetDocument() return mPresContext.mDocument. Easier said than done though...