Bug 43117 - Chromium regression when viewing a plugin document with plugins disabled
Summary: Chromium regression when viewing a plugin document with plugins disabled
Status: RESOLVED WONTFIX
Alias: None
Product: WebKit
Classification: Unclassified
Component: Plug-ins (show other bugs)
Version: 528+ (Nightly build)
Hardware: All OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 43797
  Show dependency treegraph
 
Reported: 2010-07-28 06:26 PDT by Bernhard Bauer
Modified: 2013-04-11 12:35 PDT (History)
5 users (show)

See Also:


Attachments
Remove assertion from PluginDocumentParser::appendBytes (970 bytes, patch)
2010-08-05 07:23 PDT, Bernhard Bauer
eric: review-
Details | Formatted Diff | Diff
Call DocumentWriter::addData only once. (1.54 KB, patch)
2010-08-10 10:05 PDT, Bernhard Bauer
abarth: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Bernhard Bauer 2010-07-28 06:26:30 PDT
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).
Comment 1 Bernhard Bauer 2010-07-28 06:41:11 PDT
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.
Comment 2 Eric Seidel (no email) 2010-07-28 07:16:34 PDT
Thank you! I'll take a look later today.
Comment 3 Bernhard Bauer 2010-08-05 07:23:53 PDT
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.
Comment 4 Adam Barth 2010-08-05 09:45:51 PDT
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?
Comment 5 Eric Seidel (no email) 2010-08-05 10:37:47 PDT
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);
Comment 6 Eric Seidel (no email) 2010-08-05 10:39:01 PDT
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.
Comment 7 Bernhard Bauer 2010-08-10 10:05:30 PDT
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?
Comment 8 Adam Barth 2010-08-10 11:09:17 PDT
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.
Comment 9 Eric Seidel (no email) 2010-11-03 14:34:17 PDT
The old parser is gone.
Comment 10 Eric Seidel (no email) 2010-11-03 14:39:52 PDT
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.