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

RESOLVED FIXED in Firefox 56

Status

()

Core
Plug-ins
P2
normal
RESOLVED FIXED
2 months ago
2 months ago

People

(Reporter: florian, Assigned: qdot)

Tracking

unspecified
mozilla56
Points:
---

Firefox Tracking Flags

(firefox56 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Reporter)

Description

2 months ago
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

2 months ago
This sounds like perhaps bug 1358907?
(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.
Flags: needinfo?(kyle)
Blocks: 617289
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?
Flags: needinfo?(kyle)

Updated

2 months ago
Component: Add-ons Manager → Plug-ins
Product: Toolkit → Core

Comment 4

2 months ago
Yeah, I don't see any reason to use a hash here versus the actual strings.
Comment hidden (mozreview-request)

Updated

2 months ago
Priority: -- → P2
Comment hidden (mozreview-request)
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

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

https://reviewboard.mozilla.org/r/148652/#review153618
Attachment #8877317 - Flags: review?(benjamin) → review+

Comment 9

2 months ago
Pushed by kmachulis@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7f2e9866b29b
Remove PluginProvider Hash calculation for plugin name/desc; r=bsmedberg
Assignee: nobody → kyle
Since this is a fairly benign change that gets us a startup speed boost, is it worth uplifting?
Flags: needinfo?(benjamin)
https://hg.mozilla.org/mozilla-central/rev/7f2e9866b29b
Status: NEW → RESOLVED
Last Resolved: 2 months ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56

Comment 12

2 months ago
I don't think an uplift is necessary. Keep focus on 57!
Flags: needinfo?(benjamin)
You need to log in before you can comment on or make changes to this bug.