1370832 - PluginProvider.jsm sometimes forces NSS initialization on the main thread before first paint

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect, P2)

Description

[Florian Quèze [:florian]]

The code at http://searchfox.org/mozilla-central/rev/20963d7269b1b14d455f47bc0260d0653015bf84/toolkit/mozapps/extensions/internal/PluginProvider.jsm#31 uses NSS to compute MD5 hashes.

See this profile: https://perfht.ml/2sf5TQ3

The JS call stack is:
getIDHashForString
getPluginList
buildPluginList
getAddonsByTypes
callProviderAsync
promiseCallProvider/<
Promise
promiseCallProvider
getAddonsByTypes/<
next

Comment 1

[Andrew Swan [:aswan]]

This sounds like perhaps bug 1358907?

Comment 2

[(no longer active)]

(In reply to Andrew Swan [:aswan] from comment #1)
> This sounds like perhaps bug 1358907?

That's what triggers this now but the bug itself would arise with any other code invoking this.

The real issue dates back to bug 617289.  The code here is trying to compute a persisted identifier for the plugins that is preserved across restarts.

Now that we only support Flash and a handful of other known plugins, can we have a whitelist of known plugin types here?  Needinfoing qdot as he has worked on related stuff recently.

Comment 3

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

Er, I'm not sure what you mean by whitelist here. This hashes the plugin by its name and description, and the description will contain the version number, so we'll still need some id for differentiating plugins (since Flash can be updated in the background while Firefox is running). I don't think it needs to be an md5 though. We've got few enough plugins that we could possibly just use the name/description string itself?

Comment 4

[Benjamin Smedberg]

Yeah, I don't see any reason to use a hash here versus the actual strings.

Comment 5

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

Attachment: Bug 1370832 - Remove PluginProvider Hash calculation for plugin name/desc;

PluginProvider used NSS to create an MD5 hash for plugin
identification. This would bring NSS up on startup, causing a
noticable slowdown. Since we're down to only allowing one plugin, we
can concatenate the description and name and call that good enough.

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

Comment 6

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

Comment on attachment 8877317 [details]
Bug 1370832 - Remove PluginProvider Hash calculation for plugin name/desc;

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

Comment 7

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

This passed try, after I realized the plugin test failures were already intermittents that weren't related to this: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a7cf9532e0f0f89356ad6a991e0ce96c8ea92aa0

Comment 8

[Benjamin Smedberg]

Comment on attachment 8877317 [details]
Bug 1370832 - Remove PluginProvider Hash calculation for plugin name/desc;

https://reviewboard.mozilla.org/r/148652/#review153618

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7f2e9866b29b
Remove PluginProvider Hash calculation for plugin name/desc; r=bsmedberg

Comment 10

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

Since this is a fairly benign change that gets us a startup speed boost, is it worth uplifting?

Comment 11

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/7f2e9866b29b

Comment 12

[Benjamin Smedberg]

I don't think an uplift is necessary. Keep focus on 57!