Chromium regression when viewing a plugin document with plugins disabled

• Component: Plug-ins
• Status: RESOLVED WONTFIX

[Bernhard Bauer]

There's a renderer crash in Chromium when opening a plugin document with plugins disabled (http://crbug.com/50497).

I've bisected it to http://trac.webkit.org/changeset/61913 (ignore the commit message, it has nothing to do with the change).

[Bernhard Bauer]

The crash is a failed ASSERT(!m_embedElement) in PluginDocumentParser::appendBytes. It seems that this method is called more than once, but it sets m_embedElement, so the assertion fails the second time.

[Eric Seidel (no email)]

Thank you! I'll take a look later today.

[Bernhard Bauer]

Created attachment 63588 [details]
Remove assertion from PluginDocumentParser::appendBytes

I looked into it a bit, and I think that simply removing the failing ASSERT would be the right thing to do. appendBytes is called from FrameLoader::addData and from DocumentWriter::endIfNotLoadingMainResource (with a NULL string in the latter case), so the assumption that appendBytes is not called more than once is definitely not true. On the next line is a check that returns if m_embedElement already exists, so we also don't end up creating the document more than once.

[Adam Barth]

Comment on attachment 63588 [details]
Remove assertion from PluginDocumentParser::appendBytes

LGTM.  Eric might want to look too.  Do we not have a way of testing with plugins disabled?

[Eric Seidel (no email)]

Comment on attachment 63588 [details]
Remove assertion from PluginDocumentParser::appendBytes

If the "fix" is to remove the ASSERT, we should instead replace it with:

ASSERT_ARG(length, !m_embedElement || !length);

[Eric Seidel (no email)]

That ASSERT predates my touching this code.  It was wrong of me to cause this to be called twice, since that ASSERT was trying to guard against that.

I'm not sure if both calls make sense.

The reason why it's being called twice now is that before "raw" parsers had their own special call which was only made in one place.  Now all parsers get the raw bytes and only DecodedDataDocumentParsers bother to decode the raw bytes.

[Bernhard Bauer]

Created attachment 64022 [details]
Call DocumentWriter::addData only once.

Okay, how about we do the simplest thing that could work, and just call addData only once?

[Adam Barth]

Comment on attachment 64022 [details]
Call DocumentWriter::addData only once.

Please add a test and more descriptive ChangeLog entry.

That line you're removing is required for the old parser to function properly.  We're not quite ready to remove the old parser.  There are still a couple things left that depend on it.  Eric and I are working to remove all the dependencies so we can remove it.

[Eric Seidel (no email)]

The old parser is gone.

[Eric Seidel (no email)]

I don't think your patch still applies.  But I suspect this approach could be OK.

I'm not sure how we get the decoder to flush if we don't use this fake addData call.