Protect documents from deletion when their onload removes them

Protect documents from deletion when their onload removes them

When an XML document is the src of an iframe, and the onload method
changes the src to something else, the XML document may be garbage
collected before the original load is completed. Bad things result.

In this patch we protect the document in Document::finishedParsing.

R=abarth@chromium.org,eseidel@chromium.org,inferno@chromium.org
BUG=260428
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=154680

[abarth-chromium, 2013-07-22 18:06:24]

https://codereview.chromium.org/19962002/diff/1/Source/core/dom/Document.cpp
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/19962002/diff/1/Source/core/dom/Document.cpp#...
Source/core/dom/Document.cpp:4068: RefPtr<Document> protect(this);
Why doesn't this issue occur with HTML documents?  Presumably the
HTMLDocumentParser holds a reference to the Document.  Perhaps the
XMLDocumentParser should do the same?  Is the XMLDocumentParser prepared for its
|document| to be garbage after calling this function?

[Stephen Chennney, 2013-07-22 18:38:35]

On 2013/07/22 18:06:24, abarth wrote:
> https://codereview.chromium.org/19962002/diff/1/Source/core/dom/Document.cpp
> File Source/core/dom/Document.cpp (right):
> 
>
https://codereview.chromium.org/19962002/diff/1/Source/core/dom/Document.cpp#...
> Source/core/dom/Document.cpp:4068: RefPtr<Document> protect(this);
> Why doesn't this issue occur with HTML documents?  Presumably the
> HTMLDocumentParser holds a reference to the Document.  Perhaps the
> XMLDocumentParser should do the same?  Is the XMLDocumentParser prepared for
its
> |document| to be garbage after calling this function?

I did test a case with HTML instead of XML (or SVG, as was the original example)
and the HTML did not exhibit the bug. HTML documents invoke finishedParsing
through the HTMLTreeBuilder. I'm presuming one of the many classes involved in
HTML parsing and loading holds the necessary reference, but I don't know which
one.

The XMLDocumentParser cannot easily hold a reference because it would likely
introduce a cycle. As line 538 of Document.cpp in the destructor indicates,
"DocumentParser::detach() should ensure that even if the DocumentParser outlives
the Document it won't cause badness". If we have the parser hold the ref ptr
then we need to somehow figure out how to reliably detach the parser outside of
the Document destructor.

As it stands the parser is detached when the document is destroyed and will not
try to access it again. This is an invariant of DocumentParser, it seems. The
methods that XMLDocumentParser is using in this call stack do not access the
document as the stack unwinds.

[eseidel, 2013-07-22 19:13:36]

I'd be very interested to see stacks of both finishedParsing calls.

I'm not sure this change is wrong, but it seems suspect to make a change in
Document.cpp for XML documents and not HTML documents.  I'd like to better
understand how HTML documents are OK as-is.

[Stephen Chennney, 2013-07-22 19:27:37]

On 2013/07/22 19:13:36, eseidel wrote:
> I'd be very interested to see stacks of both finishedParsing calls.
> 
> I'm not sure this change is wrong, but it seems suspect to make a change in
> Document.cpp for XML documents and not HTML documents.  I'd like to better
> understand how HTML documents are OK as-is.

The crash report linked from the bug has the stack traces. Note that
Document::finishedParsing is the lowest common stack frame.

[eseidel, 2013-07-22 19:35:21]

So yes, if finishedParsing is being called by the TreeBuidler, then a
ParseSession is on the stack and the Document is being reffed.

I believe this is the relevant code in HTMLDocumentParser::prepareToStopParsing:

    // pumpTokenizer can cause this parser to be detached from the Document,
    // but we need to ensure it isn't deleted yet.
    RefPtr<HTMLDocumentParser> protect(this);

    // NOTE: This pump should only ever emit buffered character tokens,
    // so ForceSynchronous vs. AllowYield should be meaningless.
    if (m_tokenizer) {
        ASSERT(!m_haveBackgroundParser);
        pumpTokenizerIfPossible(ForceSynchronous);
    }

    if (isStopped())
        return;

[eseidel, 2013-07-22 19:36:23]

Actually, maybe it's:

    // Informs the the rest of WebCore that parsing is really finished (and
deletes this).
    m_treeBuilder->finished();

And I don't see where the Document is being held there.

[eseidel, 2013-07-22 19:41:38]

lgtm

Looking at this further, I think this is OK.

[commit-bot: I haz the power, 2013-07-22 19:41:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/schenney@chromium.org/19962002/1

[commit-bot: I haz the power, 2013-07-22 20:33:18]

Change committed as 154680

[Stephen Chennney, 2013-07-22 22:08:43]

I traced the code with an HTML document in the iframe. There the last ref is
removed through HTMLTreeBuilder, as I expected.

[eseidel, 2013-07-22 22:13:22]

So HTML has this same bug it just manages not to crash?

[Stephen Chennney, 2013-07-22 22:16:26]

On 2013/07/22 22:13:22, eseidel wrote:
> So HTML has this same bug it just manages not to crash?

Nope. The HTMLTreeBuilder is still keeping the document alive at the time
finishedParsing is called, while XMLDOcumentParser is not. It does suggest that
we could, if we chose, move the protecting ref count up to the
XMLDocumentParser. I do feel the current approach is safer.

[eseidel, 2013-07-22 22:18:27]

HTMLTreeBuilder doesn't have a RefPtr<Document> I believe it's kept alive by the
ParserSession, or?

[Stephen Chennney, 2013-07-22 22:35:20]

On 2013/07/22 22:18:27, eseidel wrote:
> HTMLTreeBuilder doesn't have a RefPtr<Document> I believe it's kept alive by
the
> ParserSession, or?

HTMLDocumentParser owns an HTMLTreeBuilder which owns/manages an
HTMLConstructionSite which owns some HTMLStackItems which combined own a bunch
of elements of which one is the Document which gets deleted in the process of
clearing the stack. So ultimately it is true to say that the HTMLDocumentParser
protects the document it is parsing/has-just-parsed.

[Stephen Chennney, 2013-07-22 22:36:14]

On 2013/07/22 22:35:20, Stephen Chenney wrote:
> On 2013/07/22 22:18:27, eseidel wrote:
> > HTMLTreeBuilder doesn't have a RefPtr<Document> I believe it's kept alive by
> the
> > ParserSession, or?
> 
> HTMLDocumentParser owns an HTMLTreeBuilder which owns/manages an
> HTMLConstructionSite which owns some HTMLStackItems which combined own a bunch
> of elements of which one is the Document which gets deleted in the process of
> clearing the stack. So ultimately it is true to say that the
HTMLDocumentParser
> protects the document it is parsing/has-just-parsed.

All that is inferred from the destructor call stack for the document that
corresponds to the xml doc that causes problems.