New issue
Advanced search Search tips

Issue 594677 link

Starred by 1 user

Issue metadata

Status: Available
Owner: ----
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

media documents run finish parsing logic twice

Project Member Reported by bmcquade@chromium.org, Mar 14 2016

Issue description

To repro:
navigate the browser to 'data:video/webm,'

Observation:
Document::finishedParsing gets called twice for this document
Document::finishedParsing is responsible for firing domcontentloaded event among other things, and it doesn't protect against being called multiple times

The first time finishedParsing gets called, the call stack looks like:

#2 0x7f7d95c5fe59 blink::Document::finishedParsing()
#3 0x7f7d95f3508b blink::RawDataDocumentParser::finish()
#4 0x7f7d95f3711b blink::MediaDocumentParser::appendBytes()
#5 0x7f7d96638e89 blink::DocumentWriter::addData()
#6 0x7f7d9662776a blink::DocumentLoader::commitData()
#7 0x7f7d966273e1 blink::DocumentLoader::finishedLoading()

the second time, the call stack looks like:

#2 0x7f7d95c5fe59 blink::Document::finishedParsing()
#3 0x7f7d95f3508b blink::RawDataDocumentParser::finish()
#4 0x7f7d96639012 blink::DocumentWriter::end()
#5 0x7f7d96627818 blink::DocumentLoader::endWriting()
#6 0x7f7d9662741c blink::DocumentLoader::finishedLoading()

It looks like MediaDocumentParser::appendBytes calls RawDataDocumentParser::finish:

void MediaDocumentParser::appendBytes(const char*, size_t)
{
    ...
    finish();
}

but DocumentWriter::end() also calls RawDataDocumentParser::finish():

void DocumentWriter::end()
{
    ...
    m_parser->finish();
    ...
}

which results in Document::finishedParsing getting called twice.
 
MediaDocumentParser::appendBytes implements DocumentParser::appendBytes. This method is called in two places: DocumentWriter::addData, as in this trace, and in XMLHttpRequest::parseDocumentChunk.

A simple fix would be to no longer call RawDataDocumentParser::finish() from MediaDocumentParser::appendBytes, but it's unclear if that could break the XHR side. Can an XHR ever instantiate a MediaDocumentParser?

Comment 2 by japhet@chromium.org, Mar 14 2016

I don't think so. XMLHttpRequest.cpp appears to only create XMLDocument and HTMLDocument.
Excellent, thank you!

BTW is this a loader bug? I suppose it's more of a media document parser bug, but I didn't see a category for that. How would you categorize this?

Comment 4 by japhet@chromium.org, Mar 14 2016

I don't see a great match, so Loader should suffice.
I tried removing the call and some layout tests broke, so this seems to be a bit more complex than expected.
Status: Available (was: Untriaged)
Marking as available. Bryan, would you mind sharing the CL that removes this so we can see the failed tests?
Labels: -Pri-2 Pri-3
Here's what I tried:
https://codereview.chromium.org/1804733002

I had thought this was going to block me from making progress, but that turns out to not be the case, so I dropped this. This is a corner case, so unless it's blocking us from making progress, I don't think it's worth investing in.
Labels: Hotlist-CodeHealth
Project Member

Comment 9 by sheriffbot@chromium.org, Mar 22 2017

Labels: Hotlist-Recharge-Cold
Status: Untriaged (was: Available)
This issue has been available for more than 365 days, and should be re-evaluated. Please re-triage this issue.
The Hotlist-Recharge-Cold label is applied for tracking purposes, and should not be removed after re-triaging the issue.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Status: Available (was: Untriaged)
I think this is still p3
Project Member

Comment 11 by sheriffbot@chromium.org, Apr 16 2018

Status: Untriaged (was: Available)
This issue has been Available for over a year. If it's no longer important or seems unlikely to be fixed, please consider closing it out. If it is important, please re-triage the issue.

Sorry for the inconvenience if the bug really should have been left as Available.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Cc: kouhei@chromium.org
Status: Available (was: Untriaged)

Sign in to add a comment