1351490 - Only run plugin finding/init on flash and pdf MIME types

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, enhancement)

Description

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

We only support flash and PDF plugins now, drastically reducing the number of MIME types we care about in order to trigger plugin finding. We should only start initializing plugins if we find that we have a MIME type that matches one of those types.

Comment 1

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Bug 1351490 - Only run plugin finding on flash mime types/extensions;

Now that we only support pdf and flash plugins, we should only run
plugin finding if either of these two mime types is requested.

Review commit: https://reviewboard.mozilla.org/r/124470/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/124470/

Comment 2

[Benjamin Smedberg]

Comment on attachment 8852248 [details]
Bug 1351490 - Only run plugin finding on flash mime types/extensions;

https://reviewboard.mozilla.org/r/124470/#review127264

::: docshell/base/nsWebNavigationInfo.cpp:59
(Diff revision 1)
>  
>    if (*aIsTypeSupported) {
>      return rv;
>    }
>  
> +  // We only support flash or PDF as plugins, so if the mime types don't match

Do we need to worry about the test plugin here?

Comment 3

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Ok, yeah, I was a few levels too far down in the stack. The function we're interested in is nsContentUtils::HtmlObjectContentTypeForMIMEType. If I put the check there, things start failing all over the place. I'll see what I can do about fixing this.

That said, do we want to make this the bug where we kill the java test plugin? Or at least change its mime type to something crazy?

Comment 4

[Benjamin Smedberg]

Feel free to kill the Java one right now, with fire.

My longer-term plan was to only have the Flash test plugin and kill the others. Then have a testing mode where we load the Flash test plugin in place of real Flash. And as a result we only have one active plugin at a time and can make a bunch of optimizations around there being only one.

Feel free not to do that in this bug, but if it helps...

Comment 5

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

(In reply to Benjamin Smedberg [:bsmedberg] from comment #4)
> Feel free to kill the Java one right now, with fire.
> 
> My longer-term plan was to only have the Flash test plugin and kill the
> others. Then have a testing mode where we load the Flash test plugin in
> place of real Flash. And as a result we only have one active plugin at a
> time and can make a bunch of optimizations around there being only one.
> 
> Feel free not to do that in this bug, but if it helps...

Ok, I'm on board for all of this, but I'll break it into multiple bugs. The plan for right now is to just land this bug with test and java mime types enabled, so I don't bloat the scope of this bug too much. I'll create a new bug for removing everything that's not the test flash plugin, and convert all of our remaining tests over to that. Will also create a bug blocked on that, for removing the rest of the java stuff around NPAPI/nsObjectLoadingContent/etc, since we won't be getting oranges/reds anymore after dropping the tests. 

Do we have a bug for the single plugin optimization work, or should I create that too? I don't think getting ourselves down to one plugin fixes the original problem of the sync FindPlugins message, since we'll still need to be able to detect the plugin on each content process startup, right? Just want to make sure I'm not missing something and that the work happens in the right order.

Comment 6

[Benjamin Smedberg]

Correct, a single plugin reduces complexity but doesn't solve the root problem. So just solving the problem at hand and ignoring the other changes is fine too!

Comment 7

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

(In reply to Benjamin Smedberg [:bsmedberg] from comment #6)
> Correct, a single plugin reduces complexity but doesn't solve the root
> problem. So just solving the problem at hand and ignoring the other changes
> is fine too!

Yup, just wanted to make sure I wasn't getting to excited with the prospect of chainsaws. I'll file the other bugs, get the sync message stuff done, then move on to those.

Comment 8

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Comment on attachment 8852248 [details]
Bug 1351490 - Only run plugin finding on flash mime types/extensions;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/124470/diff/1-2/

Comment 9

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Comment on attachment 8852248 [details]
Bug 1351490 - Only run plugin finding on flash mime types/extensions;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/124470/diff/2-3/

Comment 10

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Comment on attachment 8852248 [details]
Bug 1351490 - Only run plugin finding on flash mime types/extensions;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/124470/diff/3-4/

Comment 11

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Updated patch to deal with test mime types, and also removed check for the pdf type. We don't actually use the acrobat plugin, but rather pdf.js (via the stream converter), which is taken care of before the plugin check happens.

Comment 12

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Comment on attachment 8852248 [details]
Bug 1351490 - Only run plugin finding on flash mime types/extensions;

Rerequesting review since I changed enough to warrant it.

Comment 13

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Comment on attachment 8852248 [details]
Bug 1351490 - Only run plugin finding on flash mime types/extensions;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/124470/diff/4-5/

Comment 14

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Comment on attachment 8852248 [details]
Bug 1351490 - Only run plugin finding on flash mime types/extensions;

Kicking review out of mozreview since it can't seem to handle clearing reviews on wrong patches. Back to splinter!

Comment 15

[Benjamin Smedberg]

Comment on attachment 8852248 [details]
Bug 1351490 - Only run plugin finding on flash mime types/extensions;

https://reviewboard.mozilla.org/r/124470/#review128238

Comment 16

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 1 (v3) - Only run plugin finding on flash mime type/extensions

New version of patch that covers test plugin mime types, as well as extensions. Also pushes checks to lowest level possible where available.

Comment 17

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Ugh. Still failing browser tests.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6f0c4e25a98feb60f5701d34b8b9ff091e86f5d5

Will fix this up next week.

Comment 18

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Hmm. Running into something really weird here. 

I thought we made it so that if a plugin declares support for MIME types we don't care about, we don't keep it in the list? I just found out this patch was failing on linux try due to libtotem-mully-plugin.so showing up in the list.

https://treeherder.mozilla.org/logviewer.html#?job_id=89072997&repo=try&lineNumber=7619 (you may have to scroll up a bit to see where the errors happen)

Things trip up when it tries to get the permission name for the mime type, but can't resolve it (because it's outside of the mime types I allow). I'm not sure why it's in the plugin list in the first place though.

I can get this cleaned up, just wondering if this was missed in another bug or something.

Comment 19

[Benjamin Smedberg]

The load_flash_only pref is set to false for mochitests currently so that we can load the test plugin. I bet that's causing your issue.

Comment 20

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Ah hah. That would do it.

While I'd like to just fix this by doing bug 1351885, I need to get this landed then spend a couple of days on U2F stuff before coming back for plugin cleanup. I'll just make a new pref that will allow test plugins similar to load_flash_only, and default it on for mochitests.

Comment 21

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 2 (v1) - Only load either flash and/or test plugins. Period.

Fixed the browser-chrome test failures, and simplifies plugin checking to one function. 

Fixed try:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1f18297a842c15ec2577487fdbcc41e95d80e8c1&selectedJob=89281208

Comment 22

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 3 (v1) - Turn on Bug 1165981 test for all platforms

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/907269666ac6
Only run plugin finding on flash mime types/extensions; r=bsmedberg
https://hg.mozilla.org/integration/mozilla-inbound/rev/45b6a6fa3f31
Only load either flash and/or test plugins. Period. r=bsmedberg
https://hg.mozilla.org/integration/mozilla-inbound/rev/adac0e45871e
Turn on Bug 1165981 test for all platforms; r=bsmedberg

Comment 24

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/907269666ac6
https://hg.mozilla.org/mozilla-central/rev/45b6a6fa3f31
https://hg.mozilla.org/mozilla-central/rev/adac0e45871e