Closed Bug 1371888 Opened 3 years ago Closed 2 years ago

Cache plugin and plugin blocklist information so we can avoid loading the blocklist on startup in most cases

Categories

(Core :: Plug-ins, defect, P1)

defect

Tracking

()

VERIFIED FIXED
mozilla61
Tracking Status
firefox59 --- wontfix
firefox60 --- verified
firefox61 --- verified

People

(Reporter: mconley, Assigned: Gijs)

References

(Blocks 1 open bug)

Details

(Keywords: perf, Whiteboard: [measurement:client][qf:f61][qf:p1][fxperf:p1])

Attachments

(3 files, 1 obsolete file)

Here's a profile:

https://perfht.ml/2sdsDQN

Here's the code in question: http://searchfox.org/mozilla-central/source/toolkit/components/telemetry/TelemetryEnvironment.jsm#573

I feel like we can probably simplify this a bit, since I think the question is now "is Flash enabled or not". I suspect that question can either be answered on the main thread, or after first paint.
Benjamin, is collecting the plugin data after/on first paint acceptable for the crash ping purposes?

Alternatively, with everything but Flash being disabled now, is there a cheaper way to get the plugin information now?
Or could we improve the scanning?
Flags: needinfo?(benjamin)
I think we can probably use a eager-push/lazy-pull model. If a page loads plugins during startup, we push the plugin data to telemetry. Otherwise telemetry waits until after the startup critical path/idle.

That way if the user does end up interacting with Flash/click-to-play we know we'll have the data (and there is no additional perf penalty).
Flags: needinfo?(benjamin)
Thanks, that sounds good.

The profile in comment 0 shows an impact of 31ms on startup.
Priority: -- → P3
Summary: Telemetry causes nsPluginHost to instantiate in the parent process, which causes it to scan the disk on the main thread for plugins → Don't cause plugin scanning from Telemetry early in startup
Whiteboard: [measurement:client] [qf]
Priority: P3 → P2
No longer blocks: 1357749
Priority: P2 → P3
See Also: → 1358907
Kyle, do you mind having a look at this one please?
Flags: needinfo?(kyle)
Whiteboard: [measurement:client] [qf] → [measurement:client] [qf:p1]
Assignee: nobody → kyle
Flags: needinfo?(kyle)
So, even if we delay telemetry fetching of plugins, we're still going to incur the costs of plugin finding during the first startup of a content process (see bug 1337058), so afaict we're not getting much of a pre-first-paint win with this bug alone, unless telemetry is taking some sort of non-trivial amount of time that I'm missing. Do we need to look at trying to delay fetching more than that?
Flags: needinfo?(benjamin)
So my recollection of what we talked about at https://bugzilla.mozilla.org/show_bug.cgi?id=1337058#c5 was that we were going to:

* kick off plugin scanning fairly early in startup, hopefully on a background thread
* launch the first content process without plugin information
* send the plugin information to any content processes when the async scanning finishes, as well as update telemetry
* if a content process needs plugin information, it will block to wait for it (yuck but hopefully uncommon)

I know bug 1337058 got us part of the way there? But there may be more to do relative to this. Especially if we can move plugin scanning off the main thread that would help a bunch of things.

Also! On windows and mac, we don't need to actually load the plugin DLLs to scan for them: on mac we can use the plist information, and on Windows we can use LoadLibraryEx(LOAD_LIBRARY_AS_IMAGE_RESOURCE | LOAD_LIBRARY_AS_DATAFILE) to just load the resource fork of the DLL. That will skip DllMain, which would be huge.

feel free to put some time on vidyo if you want to talk this through, since I know it's rather complicated.
Flags: needinfo?(benjamin)
Ah, ok, reading back on that bug with what you just posted, there's more nuance to it than I realized or implemented. Let me take some time to go over what's laid out there and here again, and I'll get back to you on setting up a video conference probably sometime next week (meant to meet up at the work week, oops :/ ). 

It sounds like we could do the plist/resource DLL load to get ourselves an initial startup optimization, then implement the threading/async depending on time we've got left. Main reason I'm thinking about that ordering is that I'm worried that the async thread + content blocking while waiting for plugins might get hairy to test, so I'll take initial speedup by minimal loads, then work from there on what's left.
(In reply to Georg Fritzsche [:gfritzsche] from comment #3)
> Thanks, that sounds good.
> 
> The profile in comment 0 shows an impact of 31ms on startup.

_getActivePlugins is very expensive during startup, it now seems to be the last thing that makes us load the blocklist before first paint. In this startup profile https://perfht.ml/2uiNNhN on a slow netbook, we spend 970ms before first paint in _getActivePlugins, and almost 98% of that time is blocklist related.
Wow, are we really that slow at parsing XML? 

I'm working on taking FindPlugins off the main thread right now, though I'm not sure how much that's gonna help this problem, as blocklist loading happens after that (once we've fired the notification that we've updated the plugin list). 

Something we could maybe do is just cleanup the blocklist? Right now it contains a huge list of plugins and addons that are no longer even valid. I'm checking with people to see if that's something else we can do to cut down on loads here.
Ok, so first off, it's not the XML parsing (which is done via the dom loader), it's the node parsing after that. We just have a LOT of block list nodes.

Addons are the major portion of the blocklist, and having talked to the addon team, I don't think there is much we can do to the blocklist itself for 57. The same blockilst is presented to all versions of the browser, and addons in that list will be both legacy and web extension.

Plugins can at least be optimized some, that will be happening in bug 1391792.

Graphics drivers and certs obviously can't do anywhere.

It looks like loading the blocklists as JSON instead of XML is happening bug 1257565, which could drastically speed up this parsing step. I'm gonna mark this as dependent on that for now.
According to that profile, of the 900ms, we're spending ~300ms in XML parsing, ~300ms in addon node parsing, and ~300ms for plugins/graphics/certs.
Duplicate of this bug: 1398287
_getActivePlugins from resource://gre/modules/TelemetryEnvironment.jsm is 21% of the time spent before first paint on this startup profile: https://perfht.ml/2eKHRUw
This bug will be fixed for 57; moving it to QF:P2 for post 57 work.
I meant won't be fixed by 57.(In reply to Jean Gong from comment #14)
> This bug will be fixed for 57; moving it to QF:P2 for post 57 work.

I meant won't be fixed for 57!
Whiteboard: [measurement:client] [qf:p1] → [measurement:client] [qf:p2]
Keywords: perf
Whiteboard: [measurement:client] [qf:p2] → [measurement:client] [qf:p1][qf:i60]
Whiteboard: [measurement:client] [qf:p1][qf:i60] → [measurement:client][qf:f60][qf:p1]
My understanding is that currently, we load the blocklist on startup because telemetry asks for a list of plugins using a sync API that the plugin service provides, and that implicitly also requires the blocklist, which we then load sync using main-thread IO (iff there are any plugins at all, ie if the user has flash installed).

Furthermore, AFAICT the work in bug 1257565 is effectively blocked on making the blocklist APIs async.

To do this, AIUI these plugin APIs will need to be async as well, or they need to cache the plugin information such that we don't load the list on every startup just because telemetry wants a list of plugins.

Kyle, what's the status of this bug? Is there an easy way to cache the plugin blocklist data for installed plugins so that we can decouple plugin initialization from the blocklist?

If it's OK for telemetry, we could also potentially look at delaying telemetry fetching the list of plugins until after the blocklist completes async OMT-io loading... but I don't know if the plugin code itself won't then trigger similar blocklist fetching independently to determine whether a plugin is blocklisted (perhaps only if a webpage attempts to use a particular plugin?).

The blocklist service is pretty rubbish at the moment, and bug 1425602 tracks making this better in various ways. async-ifying the plugin stuff, or caching it so we don't need the sync IO, seems like the best short-term bet for improving things on the client. I'm looking at improving what the server sends us in bug 1434302.
Blocks: 1425602
Flags: needinfo?(kyle)
+kmag for comment #16 given he suggested it in https://bugzilla.mozilla.org/show_bug.cgi?id=1425600#c8
Here is a startup profile of Firefox 58 on a slow Ubuntu machine, where getPluginBlocklistState (called by _getActivePlugins from resource://gre/modules/TelemetryEnvironment.jsm) is 16% of the time spent before first paint: https://perfht.ml/2rY1iD2
I haven't touched this in months, because at the time it looks like the blocklist was going to move to a system that would parse faster and we were going to see how much of a speed up that gave us before poking at the rather brittle plugin code. I guess that initiative stalled (though I didn't realize that was partially due to plugin sync calls?), and I haven't taken a look on the plugin side since.

I guess I can look at caching the plugin list again, since it should be either 0 or 1 plugins these days anyways. The only weirdness around that is going to be flash upgrades happening in between browser sessions. It's been long enough since I've look at the system that I'm honestly not sure at what point that would be checked and when the blocklist load would happen in relation to that, so I'll have to do some research there.
Flags: needinfo?(kyle)
(In reply to :Gijs from comment #16)
> My understanding is that currently, we load the blocklist on startup because
> telemetry asks for a list of plugins using a sync API that the plugin
> service provides, and that implicitly also requires the blocklist, which we
> then load sync using main-thread IO (iff there are any plugins at all, ie if
> the user has flash installed).

That's one of the possible reasons we load it at startup. The graphics blocklist is another. We need to fix both of those problems, but the situation for plugins is uglier, so it's probably the one to start with.

> To do this, AIUI these plugin APIs will need to be async as well, or they
> need to cache the plugin information such that we don't load the list on
> every startup just because telemetry wants a list of plugins.

I think we need to cache the information (at least the blocklist state) whether or not we make the APIs async. The blocklistState property is particularly ugly, because we recompute it every time the property is accessed, it's absurdly expensive to compute, and there are places where we access it several times in a row:

https://searchfox.org/mozilla-central/rev/eeb7190f9ad6f1a846cd6df09986325b3f2c3117/toolkit/mozapps/extensions/internal/PluginProvider.jsm#334-337

(In reply to Kyle Machulis [:qdot] [:kmachulis] (if a patch has no decent commit message, automatic r-) from comment #19)
> I guess I can look at caching the plugin list again, since it should be
> either 0 or 1 plugins these days anyways. The only weirdness around that is
> going to be flash upgrades happening in between browser sessions.

We just need a way to detect when a plugin has changed from the last session. Timestamps might be a good enough place to start. That's more or less what we do for extensions, though in that case we have better version and ID metadata to go off of when the timestamps disagree.
(In reply to Kris Maglione [:kmag] (long backlog; ping on IRC if you're blocked) from comment #20)
> > To do this, AIUI these plugin APIs will need to be async as well, or they
> > need to cache the plugin information such that we don't load the list on
> > every startup just because telemetry wants a list of plugins.
> 
> I think we need to cache the information (at least the blocklist state)
> whether or not we make the APIs async. The blocklistState property is
> particularly ugly, because we recompute it every time the property is
> accessed, it's absurdly expensive to compute, and there are places where we
> access it several times in a row:
> 
> https://searchfox.org/mozilla-central/rev/
> eeb7190f9ad6f1a846cd6df09986325b3f2c3117/toolkit/mozapps/extensions/internal/
> PluginProvider.jsm#334-337

OK. Do you have concrete ideas about how to do the caching? I'm asking because in part, it's not clear to me where we could cache the information such that:
1) the relevant state is available as soon as telemetry needs it (on startup) and
2) getting that state doesn't require additional sync io.

Given that it's only flash, I've played with the idea of storing the information in prefs, even though I think we all know how storing more cruft in prefs is not an ideal solution... but I don't really have better ideas.

As for the blocklistState property, that seems fixable with changes to the runtime implementation of the plugins interfaces without changing how we read information at startup, right? That is, by avoiding the recomputation and caching and invalidating the information at runtime instead. Or am I missing something? In any case, my understanding is that doing that wouldn't solve the startup IO issue, though obviously it may solve other issues... perhaps it needs a separate bug?


The other solution here may be to postpone the plugin scanning telemetry does, which may be desirable *anyway*. Speaking to folks in #telemetry, it seems like this may also be acceptable, potentially if given some kind of alternative way of getting information into crash data (see most of https://mozilla.logbot.info/telemetry/20180131 ). I'm not sure if that will be sufficient to avoid any other accesses to the blocklist / plugin data, however...
Flags: needinfo?(kmaglione+bmo)
Whiteboard: [measurement:client][qf:f60][qf:p1] → [measurement:client][qf:f61][qf:p1]
Whiteboard: [measurement:client][qf:f61][qf:p1] → [measurement:client][qf:f61][qf:p1] [fxperf]
(In reply to Kyle Machulis [:qdot] [:kmachulis] (if a patch has no decent commit message, automatic r-) from comment #19)
> I guess I can look at caching the plugin list again, since it should be
> either 0 or 1 plugins these days anyways. The only weirdness around that is
> going to be flash upgrades happening in between browser sessions. It's been
> long enough since I've look at the system that I'm honestly not sure at what
> point that would be checked and when the blocklist load would happen in
> relation to that, so I'll have to do some research there.

This would be very helpful. I'm going to morph this bug accordingly. I'll file a separate bug on decoupling telemetry from plugins and work on that anyway. :qDot, will you be able to work on this in time for 60? Is there any information you need that would help with that?
Component: Telemetry → Plug-ins
Flags: needinfo?(kyle)
Product: Toolkit → Core
Summary: Don't cause plugin scanning from Telemetry early in startup → Cache plugin and plugin blocklist information so we can avoid loading the blocklist on startup in most cases
See Also: → 1439519
I ran into some issues with the telemetry work and as a result I finally found pluginreg.dat, so I'll just steal this for some of the work that will at least avoid the blocklist IO for the cases where we have a pluginreg that's not outdated (by storing the blocklist data in there). That doesn't fully fix the problem, but it mostly does, so that's something, and as we all know something is better than nothing...
Assignee: kyle → gijskruitbosch+bugs
Flags: needinfo?(kyle)
Flags: needinfo?(kmaglione+bmo)
Priority: P3 → P1
Comment on attachment 8952455 [details]
Bug 1371888 - cache plugin information in pluginreg.dat to avoid sync startup load,

https://reviewboard.mozilla.org/r/221686/#review227628

::: dom/plugins/base/nsPluginHost.cpp:3424
(Diff revision 1)
> +    if (blocklistAlteredPlugins) {
> -    // We update blocklists asynchronously by just sending a new plugin list to
> +      // We update blocklists asynchronously by just sending a new plugin list to
> -    // content.
> +      // content.
> -    if (XRE_IsParentProcess()) {
>        // We'll need to repack our tags and send them to content again.
>        IncrementChromeEpoch();
>        SendPluginsToContent();
>      }

I suspect that in this case we also need to rewrite the pluginreg.dat file if there's nothing that takes care of this already...
Comment on attachment 8952455 [details]
Bug 1371888 - cache plugin information in pluginreg.dat to avoid sync startup load,

https://reviewboard.mozilla.org/r/221686/#review227678

::: dom/plugins/base/nsPluginHost.cpp:3421
(Diff revision 1)
> +                                                       EmptyString(), &blocklistState);
> +      NS_ENSURE_SUCCESS(rv, rv);
> +      uint32_t oldBlocklistState;
> +      plugin->GetBlocklistState(oldBlocklistState);
> +      plugin->SetBlocklistState(blocklistState);
> +      blocklistAlteredPlugins = blocklistAlteredPlugins || (oldBlocklistState != blocklistState);

You can just use "|=" here can't you?

::: dom/plugins/base/nsPluginHost.cpp:3424
(Diff revision 1)
> +    if (blocklistAlteredPlugins) {
> -    // We update blocklists asynchronously by just sending a new plugin list to
> +      // We update blocklists asynchronously by just sending a new plugin list to
> -    // content.
> +      // content.
> -    if (XRE_IsParentProcess()) {
>        // We'll need to repack our tags and send them to content again.
>        IncrementChromeEpoch();
>        SendPluginsToContent();
>      }

Yep
Attachment #8952455 - Flags: review?(dtownsend) → review-
Comment on attachment 8952455 [details]
Bug 1371888 - cache plugin information in pluginreg.dat to avoid sync startup load,

https://reviewboard.mozilla.org/r/221686/#review227802

I don't feel competent to review the plugin changes, so I assume you were asking me to only look at the blocklist service change.

::: toolkit/mozapps/extensions/nsBlocklistService.js:817
(Diff revision 3)
>  
>      var appFile = FileUtils.getFile(KEY_APPDIR, [FILE_BLOCKLIST]);
>      try {
>        await this._preloadBlocklistFile(appFile.path);
> +      Services.tm.idleDispatchToMainThread(() => {
> +        this._loadBlocklist();

I think you need to add an if (!this._isBlocklistLoaded()) test here.

Actually, why are you triggering a load only in the case of having preloaded a blocklist file from the app dir and not from the profile dir?

::: toolkit/mozapps/extensions/nsBlocklistService.js:842
(Diff revision 3)
>  
>      let text = await OS.File.read(path, { encoding: "utf-8" });
>  
>      if (!this._addonEntries) {
>        // Store the content only if a sync load has not been performed in the meantime.
>        this._preloadedBlocklistContent = text;

I almost wonder if we could just replace this line with triggering a load from string from here, and remove the _preloadedBlocklistContent field completely.
Attachment #8952455 - Flags: review?(florian) → review-
Comment on attachment 8952455 [details]
Bug 1371888 - cache plugin information in pluginreg.dat to avoid sync startup load,

https://reviewboard.mozilla.org/r/221686/#review230312

r=me for the blocklist service changes.
Attachment #8952455 - Flags: review?(florian) → review+
Comment on attachment 8952455 [details]
Bug 1371888 - cache plugin information in pluginreg.dat to avoid sync startup load,

https://reviewboard.mozilla.org/r/221686/#review230772

::: commit-message-c0119:9
(Diff revision 4)
> +
> +There is now only the saved blocklist state in a plugin tag instance, rather than
> +looking it up from in there using the blocklist service, so it was renamed from
> +mCachedBlocklistState to mBlocklistState. We pass the 'right' state to the plugin
> +instance when the plugintag is constructed. If we don't have state, we mark it as
> +unblocked.

This doesn't appear to be the case. For newly detected plugins you call the blocklist to get its state. I happen to think that that might be the best choice but since it potentially causes a sync-load of the blocklist it might be an issue.
(In reply to Dave Townsend [:mossop] from comment #32)
> This doesn't appear to be the case. For newly detected plugins you call the
> blocklist to get its state.

Ugh, good catch.

> I happen to think that that might be the best
> choice but since it potentially causes a sync-load of the blocklist it might
> be an issue.

Yeah, I don't think we should do this. I've updated the patch to no longer do this. We now only call the 'get data out of the blocklist' APIs when we get the blocklist-loaded notification. This means that there is a tiny window until the blocklist loads where, in theory, we could be running a new plugin. In practice, given that there's a 24h window between each blocklist update, and for this to happen you'd need to actually hit a webpage that used the plugin immediately, that doesn't seem like a problem.

When investigating this I also noticed that we have a sync ipc message to get blocklist state in the content process. That's not necessary anymore in this setup, so I removed it in a separate cset.
Comment on attachment 8956811 [details]
Bug 1371888 - Stop having a sync IPC message to pass blocklist state,

https://reviewboard.mozilla.org/r/225784/#review231650

::: commit-message-f8c53:5
(Diff revision 1)
> +Bug 1371888 - Stop having a sync IPC message to pass blocklist state, r?florian
> +
> +Because plugin state in the content now contains blocklist state, and is updated
> +when the blocklist updates, we don't need to ask the parent if we're checking
> +blocklist state. All the consumers should now be asking the plugin code directly,

Are you sure https://searchfox.org/mozilla-central/rev/efce4ceddfbe5fe4e2d74f1aed93894bada9b273/dom/base/nsObjectLoadingContent.cpp#814 isn't needed?
Comment on attachment 8956811 [details]
Bug 1371888 - Stop having a sync IPC message to pass blocklist state,

https://reviewboard.mozilla.org/r/225784/#review231652

::: commit-message-f8c53:5
(Diff revision 1)
> +Bug 1371888 - Stop having a sync IPC message to pass blocklist state, r?florian
> +
> +Because plugin state in the content now contains blocklist state, and is updated
> +when the blocklist updates, we don't need to ask the parent if we're checking
> +blocklist state. All the consumers should now be asking the plugin code directly,

I've removed that callsite in the earlier commit. :-)
Comment on attachment 8956811 [details]
Bug 1371888 - Stop having a sync IPC message to pass blocklist state,

https://reviewboard.mozilla.org/r/225784/#review231670

Ok then :-). I agree that we should remove the content side of this service entirely!
Attachment #8956811 - Flags: review?(florian) → review+
Status: NEW → ASSIGNED
Whiteboard: [measurement:client][qf:f61][qf:p1] [fxperf] → [measurement:client][qf:f61][qf:p1][fxperf:p1]
Depends on: 1443870
Comment on attachment 8952455 [details]
Bug 1371888 - cache plugin information in pluginreg.dat to avoid sync startup load,

https://reviewboard.mozilla.org/r/221686/#review232208

::: dom/plugins/base/nsPluginHost.cpp:2104
(Diff revision 5)
> -      pluginTag = new nsPluginTag(&info, fileModTime, fromExtension);
> +      uint32_t state = nsIBlocklistService::STATE_NOT_BLOCKED;
> +      pluginTag = new nsPluginTag(&info, fileModTime, fromExtension, state);
> +      // If the blocklist is loaded, get the blocklist state now.
> +      // If it isn't loaded yet, we'll update it once it loads.
> +      bool isBlocklistLoaded = false;
> +      if (blocklist && NS_SUCCEEDED(blocklist->GetIsLoaded(&isBlocklistLoaded)) &&

The blocklist can't load while this file scan is running as it all happens on the main thread (:sadface:) so you may as well grab the value of isBlocklistLoaded before the start of the loop.
Attachment #8952455 - Flags: review?(dtownsend) → review+
(I intend to merge the 3rd cset into the 1st one when landing, but I wanted to split it out to ease reviewing)
Comment on attachment 8958291 [details]
Bug 1371888 - fix test_asyncBlocklistLoad.js and ensure we set addon/plugin/gfxEntries to empty arrays when starting to load the file,

https://reviewboard.mozilla.org/r/227220/#review233020

::: toolkit/mozapps/extensions/nsBlocklistService.js:836
(Diff revision 1)
>        return;
>      }
>  
>      let text = await OS.File.read(path, { encoding: "utf-8" });
>  
> +    await new Promise(resolve => {

I originally used `return` here but that upset eslint because the other `return` statements here don't return a value.
Comment on attachment 8958291 [details]
Bug 1371888 - fix test_asyncBlocklistLoad.js and ensure we set addon/plugin/gfxEntries to empty arrays when starting to load the file,

https://reviewboard.mozilla.org/r/227220/#review233226

Thanks for fixing the test! :-)

::: toolkit/mozapps/extensions/nsBlocklistService.js:840
(Diff revision 1)
>  
> +    await new Promise(resolve => {
> -    Services.tm.idleDispatchToMainThread(() => {
> +      Services.tm.idleDispatchToMainThread(() => {
> -      if (!this.isLoaded) {
> +        if (!this.isLoaded) {
> -        Services.telemetry.getHistogramById("BLOCKLIST_SYNC_FILE_LOAD").add(false);
> +          Services.telemetry.getHistogramById("BLOCKLIST_SYNC_FILE_LOAD").add(false);
> +          this._gfxEntries = [];

I'm under the impression that doing this at the beginning of the _loadBlocklistFromXML method would make more sense, but I don't feel strongly about it.
Attachment #8958291 - Flags: review?(florian) → review+
(Perhaps we should make the `isLoaded` flag just an explicitly set/unset bool property instead of the magic getter that doesn't make much sense... but that can be something for another bug...)
Annnnd I realized I misread your comment. I'll just do what you suggested instead. :-)
Attachment #8958291 - Attachment is obsolete: true
(In reply to :Gijs from comment #48)
> (Perhaps we should make the `isLoaded` flag just an explicitly set/unset
> bool property instead of the magic getter that doesn't make much sense...
> but that can be something for another bug...)

I almost suggested that, and making the 3 arrays already exist on the prototype instead of being null when not initialized, but  I figured it was a much bigger change than fixing this test is worth.
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/ce82f8fc8c80
cache plugin information in pluginreg.dat to avoid sync startup load, r=florian,mossop
https://hg.mozilla.org/integration/autoland/rev/d8b3ca1ff03c
Stop having a sync IPC message to pass blocklist state, r=florian
https://hg.mozilla.org/mozilla-central/rev/ce82f8fc8c80
https://hg.mozilla.org/mozilla-central/rev/d8b3ca1ff03c
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Depends on: 1445990
Attached patch Patches for betaSplinter Review
These are beta-grafted csets for this bug (bug 1371888), bug 1443870 and bug 1445990.

Approval Request Comment
[Feature/Bug causing the regression]: n/a
[User impact if declined]: slower startup for esr60, different plugin initialization, different pluginreg fileformat from all later versions
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: yes, see https://twitter.com/gijskruitbosch/status/974293602124910592 (ie telemetry verifies that this fix was effective)
[Needs manual test from QE? If yes, steps to reproduce]: It would be good to do a quick sanity check of flash-based (only plugin we support at this point) content, both that it generally works and that older versions of flash get blocked appropriately.
[List of other uplifts needed for the feature/fix]: this patch contains all the csets for the 3 relevant bugs.
[Is the change risky?]: not very
[Why is the change risky/not risky?]: this has baked on nightly for a bit, telemetry looks good, we still have ample beta runway, the patches are reasonably contained.
[String changes made/needed]: no
Attachment #8960152 - Flags: review+
Attachment #8960152 - Flags: approval-mozilla-beta?
See Also: → 1447680
Comment on attachment 8960152 [details] [diff] [review]
Patches for beta

Alright, let's give this a go.  beta60+, should be in 60.0b7.
Attachment #8960152 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
I talk with Gijs on slack and he suggested me to use 2 cases: "One where you have an older version of Flash (that we mark as having security vulnerabilities) installed, and checking that opening a site that uses flash shows the usual "this site wants to use a plugin and you can't use it because it's vulnerable" warnings, before + after the changes. The other would be where you have an up-to-date version of Flash and to check whether that works without issues. It looks like Flash player 26 should do the trick for being blocked".

I tested the 2 scenarios on Mac OS X 10.12, Windows 10 x64 and Ubuntu 16.04 with FF Nightly 61.0a1(2018-03-28) and FF beta 60.0b7 and everything works as expected. 
Please note that on Ubuntu I wasn't able to install the Flash player 26 to reproduce the warning, but I tested with the latest Flash version the fact that flash games are working.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.