1425600 - Stop using nsIPluginTag.filename and .name thousands of times because it's clownshoes

Status: RESOLVED FIXED

(Toolkit :: Blocklist Implementation, enhancement, P1)

Description

[:Gijs (he/him)]

Blocklist stuff does this:

https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/nsBlocklistService.js#1134-1154

    for (var blockEntry of pluginEntries) {
      var matchFailed = false;
      for (var name in blockEntry.matches) {
        if (!(name in plugin) ||
            typeof(plugin[name]) != "string" ||
            !blockEntry.matches[name].test(plugin[name])) {
          matchFailed = true;
          break;
        }
      }

      if (matchFailed)
        continue;

      for (let blockEntryVersion of blockEntry.versions) {
        if (blockEntryVersion.includesItem(plugin.version, appVersion,
                                           toolkitVersion)) {
          return {entry: blockEntry, version: blockEntryVersion};
        }
      }
    }

here, `pluginEntries` is a bunch of blocklist entries that are completely unsorted. `plugin` is the actual real nsIPluginTag implementation.

As you can see, we iterate over the blocklist entries' "matches" list/thing and get every string in it.

Then we check if the nsIPluginTag object has that property.
Then we check if it's a string.
Then we compare it.

Once that's done, we also explicitly check the version.

This fails pretty badly. For one, this stuff should be in a more efficient data structure, for another, we should only be fetching the plugin information once instead of several times every time we go through a part of `matches`.

Comment 1

[:Gijs (he/him)]

(In reply to :Gijs from comment #0)
> we should only be fetching the plugin
> information once instead of several times every time we go through a part of
> `matches`.

To be 100% clear, I meant we can just extract all the fields out of the plugin object at the top of this method (or at callsites?) 1 (one) time, and create a plain JS object to compare.

Even that would likely measurably improve the running time of this method *just* because of how often we cross the JS/C++ xpconnect boundary/threshold/whatever, and maybe enabling more JIT optimizations or whatever -- though obviously making it more efficient generally would be a Good Idea. I'll file another metabug for that...

Comment 2

[Chris Peterson [:cpeterson]]

Adding [qf] whiteboard tag to put this on Quantum Flow triage's radar.

Comment 3

[:Gijs (he/him)]

(In reply to :Gijs (lower availability until Jan 27) from comment #1)
> Obviously making it more efficient generally would be a
> Good Idea. I'll file another metabug for that...

FWIW, there's a summary of the variety of issues with the blocklist implementation in bug 1425602.

Comment 4

[Chris Peterson [:cpeterson]]

Attachment: Bug 1425600 - Cache nsIPluginTag properties accessed in the plugin blocklist loop.

On my MacBook Pro, this change reduces this loop's time from ~4 ms to ~3 ms. This loop is not hot, but moving these xpconnect calls out of the loop is still a good idea.

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

Comment 5

[Chris Peterson [:cpeterson]]

I also experimented with skipping non-Flash pluginItems in the blocklist. That reduces the final plugin blocklist from 115 entries to just 6 and reduces the _getPluginBlocklistEntry loop time from ~3 ms to ~0-1 ms. 

The change assumed that Flash plugin entries, current and future, would continue to specify the same adobe.com infoURL. This is a fragile assumption and we also would need to add fake Flash infoURLs to the plugin blocklist tests' blocklists so their test plugin entries are not skipped.

Because the loop is not hot and a ~dozen tests would need to be fixed, I abandoned this change. Ideally, we would instead remove the non-Flash plugin entries (bug 1433627) but Firefox ESR 52 still supports non-Flash plugins and needs to know about all blocked plugins.

Comment 6

[:Gijs (he/him)]

Comment on attachment 8946315 [details]
Bug 1425600 - Cache nsIPluginTag properties accessed in the plugin blocklist loop.

https://reviewboard.mozilla.org/r/216284/#review222184

r=me because it's a good idea.

More generally, I expect we need to move away from the monolithic, repetitive format we have now, and have incremental updates and so on, but we don't need to do that here.

I think more dramatic changes here would still be a good idea. We control the blocklist format, so presumably we could add a flag for flash-relevant things? Looks like the client side will just ignore bits it doesn't know about, so we could introduce that on the server side and then filter for flash on the client using that rather than the adobe URL. We could also filter the toolkitVersion when reading the file, instead of every time we hit `_getPluginBlocklistEntry` (assuming there are no consumers that care about whether we block something in the past/future - seems unlikely!). So we could version-limit the non-flash item to toolkitVersion < 57, and then ensure toolkit version filtering just avoids having these items in the `pluginEntries` list at all.

Other low-hanging fruit includes that a lot of the plugin blocklist items implicitly are limited to an OS (because they specify filename="foo.so" or filename="foo.dll") but don't have an os="WINNT" attribute or similar. Those attributes allow items to get filtered out immediately when processing the file, so that's another simple step we could take server-side that would reduce client-side processing time.

It also seems that if we parse the list synchronously (which from code inspection I *believe* will (almost?) always happen), we first fetch it via XHR and get a responseXML object, and check it (which implies getting it + parsing it) and then re-parse the string as XML again separately. Off-hand that seems daft too, in that case (ie when we're not using the asynchronously-read text stuff) we should just reuse responseXML...

Comment 7

[Chris Peterson [:cpeterson]]

Yes, there are a lot of improvements that could be made to this blocklist code. This patch is small because I started to make more changes and then realized there was more work to do here than I was ready to commit to. :)

We could probably add `<versionRange maxVersion="57.0"> to all the legacy add-on entries so Firefox 57+ can skip all those entries while leaving them for ESR 52 to continue using.

Comment 8

[Kris Maglione [:kmag]]

Yes, the blocklist code is a disaster. We already have nominal support for incremental updates, in the Kinto backend, but still have a lot of work to do to finish migrating/asyncifying. In particular, see bug 1257565 (which is apparently blocked on a needinfo from me...)

That said, I think it would be more useful at this point to spend the effort caching the blocklist states for plugins and graphics drivers, like we now do for extensions, than to spend it optimizing the blocklist matching code. Having that code only run when when we update the blocklist or detect a plugin is much more efficient than having it run relatively quickly every time we check its blocklist state, especially when that check involves a sync load of the blocklist DB.

Comment 9

[Narcis Beleuzu [:NarcisB]]

Backed out for xpcshell failures on /test_pluginInfoURL.js

Push: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=0210af232008cb3f2be29900792d02d88b0dcbf6
Backout: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=47bb6da4b0d1800b1496910660b1a2155350b155
Log: https://treeherder.mozilla.org/logviewer.html#?job_id=159250402&repo=autoland&lineNumber=2731

Comment 10

[:Gijs (he/him)]

(In reply to Narcis Beleuzu [:NarcisB] from comment #9)
> Backed out for xpcshell failures on /test_pluginInfoURL.js
> 
> Push:
> https://treeherder.mozilla.org/#/
> jobs?repo=autoland&revision=0210af232008cb3f2be29900792d02d88b0dcbf6
> Backout:
> https://treeherder.mozilla.org/#/
> jobs?repo=autoland&revision=47bb6da4b0d1800b1496910660b1a2155350b155
> Log:
> https://treeherder.mozilla.org/logviewer.
> html#?job_id=159250402&repo=autoland&lineNumber=2731

This is caused by that test using a test file which has <match> elements for name="version". I don't think the blocklist we currently use ships that...

Adding `version: plugin.version` to the definition of `pluginProperties` fixes things. Then you can omit defining `pluginVersion` and just use `pluginProperties.version` in the loop at the bottom.

In fact, I'll just push that to inbound, I guess...

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3f2f569d74cc
Cache nsIPluginTag properties accessed in the plugin blocklist loop. r=Gijs

Comment 12

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/3f2f569d74cc