Do not try to use an HTML embed instead of a Flash one inside a PluginDocument.

Do not try to use an HTML embed instead of a Flash one inside a PluginDocument.

The PluginDocument is written in a way that expects to get a plugin
inside the created <embed>. If we want to handle this case, we would
have to either make this relationship less strict or prevent the
creation of the document when we know a rewrite will happen.

BUG=636226
TEST=cluster-fuzz

Committed: https://crrev.com/8ec7d278c86fbf5194eb5acabbe6e3dab88777f1
Cr-Commit-Position: refs/heads/master@{#411598}

[mlamouri (slow - plz ping), 2016-08-11 16:55:07]

mlamouri@chromium.org changed reviewers:
+ pdr@chromium.org

[mlamouri (slow - plz ping), 2016-08-11 16:55:08]

pdr@, PTAL

[mlamouri (slow - plz ping), 2016-08-11 16:55:11]

The CQ bit was checked by mlamouri@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-11 16:55:36]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-11 19:41:33]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-11 19:41:34]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[pdr., 2016-08-11 23:21:53]

Overall this looks reasonable

https://codereview.chromium.org/2238983002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/html/HTMLEmbedElement.cpp (right):

https://codereview.chromium.org/2238983002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/HTMLEmbedElement.cpp:150: // Overwrites the
URL and MIME type of a Flash embed to use an HTML5 embed
I see on the bug that this will be very difficult to test in the public repo due
to the dependency on flash. In lieu of a test, WDYT of some asserts in this
area? Possibly turn the ASSERT into a CHECK in PluginDocument::pluginWidget?

https://codereview.chromium.org/2238983002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/HTMLEmbedElement.cpp:152: if
(!document().isPluginDocument()) {
Instead of a blacklist, can this be a whitelist? I.e., isHTMLDocument()

[mlamouri (slow - plz ping), 2016-08-12 10:27:04]

Description was changed from

==========
Do not try to use an HTML embed instead of a Flash one inside a PluginDocument.

The PluginDocument is written in a way that expects to get a plugin
inside the created <embed>. If we want to handle this case, we would
have to either make this relationship less strict or prevent the
creation of the document when we know a rewrite will happen.

BUG=636226
==========

to

==========
Do not try to use an HTML embed instead of a Flash one inside a PluginDocument.

The PluginDocument is written in a way that expects to get a plugin
inside the created <embed>. If we want to handle this case, we would
have to either make this relationship less strict or prevent the
creation of the document when we know a rewrite will happen.

BUG=636226
TEST=cluster-fuzz
==========

[mlamouri (slow - plz ping), 2016-08-12 10:27:31]

mlamouri@chromium.org changed reviewers:
+ foolip@chromium.org

[mlamouri (slow - plz ping), 2016-08-12 10:27:32]

foolip@, can you stamp this? It needs to land to fix a crash.

https://codereview.chromium.org/2238983002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/html/HTMLEmbedElement.cpp (right):

https://codereview.chromium.org/2238983002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/HTMLEmbedElement.cpp:150: // Overwrites the
URL and MIME type of a Flash embed to use an HTML5 embed
On 2016/08/11 at 23:21:53, pdr. wrote:
> I see on the bug that this will be very difficult to test in the public repo
due to the dependency on flash. In lieu of a test, WDYT of some asserts in this
area? Possibly turn the ASSERT into a CHECK in PluginDocument::pluginWidget?

I've changed the ASSERT() to a CHECK().

FWIW, a repro was added to the fuzzer so the fuzzer will keep the regression
test around. IMO, this is the best way to test the bug.

https://codereview.chromium.org/2238983002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/HTMLEmbedElement.cpp:152: if
(!document().isPluginDocument()) {
On 2016/08/11 at 23:21:53, pdr. wrote:
> Instead of a blacklist, can this be a whitelist? I.e., isHTMLDocument()

The issue is really that the PluginDocument is making assumptions it shouldn't
make. I'm not a big fan of adding checks that would be hard to understand in the
future. Also, ideally, we should find a way for the PluginDocument issue to be
fixed.

[foolip, 2016-08-12 10:29:38]

lgtm

[mlamouri (slow - plz ping), 2016-08-12 10:30:16]

The CQ bit was checked by mlamouri@chromium.org

[commit-bot: I haz the power, 2016-08-12 10:30:32]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-12 11:47:29]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-08-12 11:49:23]

Description was changed from

==========
Do not try to use an HTML embed instead of a Flash one inside a PluginDocument.

The PluginDocument is written in a way that expects to get a plugin
inside the created <embed>. If we want to handle this case, we would
have to either make this relationship less strict or prevent the
creation of the document when we know a rewrite will happen.

BUG=636226
TEST=cluster-fuzz
==========

to

==========
Do not try to use an HTML embed instead of a Flash one inside a PluginDocument.

The PluginDocument is written in a way that expects to get a plugin
inside the created <embed>. If we want to handle this case, we would
have to either make this relationship less strict or prevent the
creation of the document when we know a rewrite will happen.

BUG=636226
TEST=cluster-fuzz

Committed: https://crrev.com/8ec7d278c86fbf5194eb5acabbe6e3dab88777f1
Cr-Commit-Position: refs/heads/master@{#411598}
==========

[commit-bot: I haz the power, 2016-08-12 11:49:24]

Patchset 2 (id:??) landed as
https://crrev.com/8ec7d278c86fbf5194eb5acabbe6e3dab88777f1
Cr-Commit-Position: refs/heads/master@{#411598}