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

RESOLVED FIXED in Firefox 55

Status

()

Core
Plug-ins
RESOLVED FIXED
a month ago
15 days ago

People

(Reporter: qdot, Assigned: qdot)

Tracking

(Blocks: 2 bugs)

unspecified
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: [qf:p1])

Attachments

(3 attachments, 1 obsolete attachment)

(Assignee)

Description

a month ago
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 hidden (mozreview-request)

Comment 2

a month ago
mozreview-review
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?
Attachment #8852248 - Flags: review?(benjamin) → review+
(Assignee)

Comment 3

a month ago
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?
Flags: needinfo?(benjamin)
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...
Flags: needinfo?(benjamin)
(Assignee)

Comment 5

a month ago
(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.
Flags: needinfo?(benjamin)
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!
Flags: needinfo?(benjamin)
(Assignee)

Comment 7

a month ago
(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.
(Assignee)

Updated

a month ago
Blocks: 1351885
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 11

a month ago
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.
(Assignee)

Updated

a month ago
Attachment #8852248 - Flags: review+
(Assignee)

Comment 12

a month ago
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.
Attachment #8852248 - Flags: review?(benjamin)
(Assignee)

Updated

28 days ago
Attachment #8852248 - Flags: review?(benjamin)
Comment hidden (mozreview-request)
(Assignee)

Updated

28 days ago
Attachment #8852248 - Flags: review?(benjamin)
(Assignee)

Comment 14

28 days ago
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!
Attachment #8852248 - Attachment is obsolete: true
Attachment #8852248 - Flags: review?(benjamin)

Comment 15

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

https://reviewboard.mozilla.org/r/124470/#review128238
Attachment #8852248 - Flags: review+
(Assignee)

Comment 16

28 days ago
Created attachment 8853578 [details] [diff] [review]
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.
Attachment #8853578 - Flags: review?(benjamin)
Attachment #8853578 - Flags: review?(benjamin) → review+
(Assignee)

Comment 17

28 days ago
Ugh. Still failing browser tests.

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

Will fix this up next week.
Flags: needinfo?(kyle)
Whiteboard: [qf]
(Assignee)

Comment 18

23 days ago
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.
Flags: needinfo?(kyle) → needinfo?(benjamin)
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.
Flags: needinfo?(benjamin)
(Assignee)

Comment 20

23 days ago
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.
(Assignee)

Comment 21

22 days ago
Created attachment 8855500 [details] [diff] [review]
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
Attachment #8855500 - Flags: review?(benjamin)
Whiteboard: [qf] → [qf:p1]
(Assignee)

Comment 22

22 days ago
Created attachment 8855530 [details] [diff] [review]
Patch 3 (v1) - Turn on Bug 1165981 test for all platforms
Attachment #8855530 - Flags: review?(benjamin)
Attachment #8855500 - Flags: review?(benjamin) → review+
Attachment #8855530 - Flags: review?(benjamin) → review+

Comment 23

17 days ago
Pushed by kmachulis@mozilla.com:
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

16 days ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/907269666ac6
https://hg.mozilla.org/mozilla-central/rev/45b6a6fa3f31
https://hg.mozilla.org/mozilla-central/rev/adac0e45871e
Status: NEW → RESOLVED
Last Resolved: 16 days ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.