21187 - [dogfood] webshells in text controls leaked

Status: VERIFIED FIXED

(Core :: Layout, defect, P1)

Description

[buster]

from mScott's original note:

I've been spending some off time the last couple days trying to track down our
remaining webshell leaks. I've been using the leak detection
tools the memory team developed a while ago. Part of this work has led me down
the path of tracking down why each gfx text widget
leaks a webshell.

I've discovered that for every nsGFXTextControlFrame that we create, we leak an
html parser instance, a nsHTMLDocument, and
nsHTMLContentSink and a webshell!!! *yikes* I suspect the leaks must be pretty
high for windows like the compose window or web
pages with lots of text form fields.

I've been able to figure out why we leak a webshell per widget but not how to
fix it =). I'm hoping someone who understands the
relationships between the gfx text widget, the parser, the html document and the
html content sink can help me out.

It all comes down to a circular reference count between the html document which
has a ref on the parser which has a ref on the html
content sink which has a reference on the html document.

How did we get in this spot?
We create an nsGfxTextControlFrame.  In nsGfxTextControlFrame::CreateSubDoc, we
create a webshell for the widget and we create a
nsHTMLDocument which we want to then set on the webshell by calling
nsWebShell::SetDocument. SetDocument attempts to bypass the
normal url load process, instead embedding a document directly in the webshell.
As part of this work, SetDocument ends up firing a
OnStartDocumentLoad on the html document. This triggers a chain of events where
the html document creates a parser and an html sink
setting up the ownership model I described above. So now we have a circular
reference from the document to the parser to the sink back
to the document.

What's special about calling SetDocument?
If we we weren't going through nsWebShell::SetDocument, but instead were loading
a url like about:blank in order to set the document  in
the webshell, then we would be feeding the html parser data through calls to
OnDataAvailable, followed by another call: OnStopRequest.
This is how the circular reference was broken before (I think). When the parser
gets an OnStopRequest, it eventually informs the html
content sink that we are done by calling HTMLContentSink::DidBuildModel.
DidBuildModel removed  the circular ref between the parser
and the sink. It then released its reference on the html document. This in turn
allowed the html document to go away which then released
the last reference on the html parser, allowing it to go away.

With nsWebShell::SetDocument we aren't actually feeding async data into the
parser. We've created a parser when we called
nsHTMLDocument::OnStartDocumentLoad but it is never given any data because we
already have a document! As a result, the
OnStopRequest for the parser is never issued and this prevents the circular
reference chain from getting broken. And that's how we get this
big leak right now with using nsGFXTextControlFrames.

I'm pretty sure this leak is relatively new. It looks like
nsGfxTextControlFrame::CreateSubDoc used to call nsWebShell->LoadURL and
this code path shouldn't have leaked for reasons I described above. It now calls
this new SetDocument method which causes the leak
because we aren't really loading content into the parser. I'm adding travis to
the cc list because I think he wrote SetDocument.

Solutions:
I tried simulating a OnStopRequest call on the parser in nsWebShell::SetDocument
just to see if it would fix the problem but it doesn't
because the parser ends up writing some fake data if it gets an OnStopRequest
without receiving any data. And this trickles down to the
content sink and conflicts with the document we've already created without going
through the parser. (I got turns of asserts and errors when
I did this). So that's not the right approach to take.

I'm willing to do the leg work to fix this problem as I'm currently taking all
the webshell leaks and stuff too much to heart. But I'm not sure
how to solve this problem with the html parser and SetDocument. So any
suggestions is much appreciated!

Thanks,
-Scott

Comment 1

[buster]

I have a patch for this ready to go.  The idea is to use the "aCommand" param of
OnStartDocumentLoad to pass a hint to the nsHTMLDocument.  This hint is used to
avoid doing a lot of unnecessary work, including creating a parser.

Comment 2

[leger]

Putting on the PDT+ radar.

Comment 3

[buster]

does this mean I have permission to check in? or do I have to ask for that
separately?

Comment 4

[buster]

fixed.  The fix is to use the "aCommand" param or
nsHTMLDocument::StartDocumentLoad to pass a hint
to the nsHTMLDocument, telling it there's no need to involve a parser.

Comment 5

[Chris Petersen]

Marking Verified per last comments.