Closed
Bug 1163664
Opened 6 years ago
Closed 6 years ago
Don't check for plugin blocklist state on Android
Categories
(Core :: Plug-ins, defect)
Tracking
()
RESOLVED
FIXED
mozilla41
Tracking | Status | |
---|---|---|
firefox41 | --- | fixed |
People
(Reporter: jchen, Assigned: rbarker)
Details
Attachments
(1 file)
2.60 KB,
patch
|
jimm
:
review+
jchen
:
review+
|
Details | Diff | Splinter Review |
Fennec doesn't support plugins, but nsIBlocklistService::GetPluginBlocklistState still gets called through analytics scripts, etc. This causes us to load the blocklist, which can take ~1 second on an Android device, affecting our pageload performance.
Comment 1•6 years ago
|
||
Do you have a stack of this being triggered? We should still be loading the blocklist to do android addon blocklisting though, right?
Updated•6 years ago
|
Flags: needinfo?(nchen)
Reporter | ||
Comment 2•6 years ago
|
||
(In reply to Benjamin Smedberg [:bsmedberg] from comment #1) > Do you have a stack of this being triggered? Looks like this is only triggered if Flash is installed on the device, which makes more sense and is not as bad as I had thought (even though currently the blocklist is not applicable to Flash on Android). In summary, blocklist loading could slow down pageload by 1s or more, but it only affects those with Flash installed. So this bug is potentially WONTFIX. The stack is something like, > navigator.javaEnabled (or navigator.plugins) > ... > nsPluginArray::EnsurePlugins > nsPluginHost::GetPlugins > nsPluginHost::LoadPlugins > ... > nsPluginHost::AddPluginTag > nsPluginTag::IsActive > nsPluginTag::IsBlocklisted > nsPluginTag::GetBlocklistState > nsBlocklistService.getPluginBlocklistState > nsBlocklistService._loadBlocklist > We should still be loading the blocklist to do android addon blocklisting > though, right? Right, the blocklist should be loaded when the addons manager needs it (e.g. when installing a new addon).
Flags: needinfo?(nchen)
Assignee: nobody → rbarker
Assignee | ||
Comment 3•6 years ago
|
||
Assignee | ||
Comment 4•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1333d8b732b6
Assignee | ||
Updated•6 years ago
|
Attachment #8613805 -
Flags: review?(nchen)
Assignee | ||
Updated•6 years ago
|
Attachment #8613805 -
Flags: review?(joshmoz)
Reporter | ||
Updated•6 years ago
|
Attachment #8613805 -
Flags: review?(nchen) → review+
Assignee | ||
Updated•6 years ago
|
Attachment #8613805 -
Flags: review?(joshmoz) → review?(jmathies)
![]() |
||
Comment 5•6 years ago
|
||
You've hardcoded disabling the blocklist for plugins on android. Are you sure you want to do this?
![]() |
||
Comment 6•6 years ago
|
||
Comment on attachment 8613805 [details] [diff] [review] 0001-Bug-1163664-Don-t-check-for-plugin-blocklist-state-on-Android-15060117-6c135d9.patch patch looks fine.
Attachment #8613805 -
Flags: review?(jmathies) → review+
Assignee | ||
Comment 7•6 years ago
|
||
(In reply to Jim Mathies [:jimm] from comment #5) > You've hardcoded disabling the blocklist for plugins on android. Are you > sure you want to do this? :snorp said we do since we only support Flash on Android and will never block flash.
Assignee | ||
Updated•6 years ago
|
Keywords: checkin-needed
![]() |
||
Comment 9•6 years ago
|
||
(In reply to Randall Barker [:rbarker] from comment #7) > (In reply to Jim Mathies [:jimm] from comment #5) > > You've hardcoded disabling the blocklist for plugins on android. Are you > > sure you want to do this? > > :snorp said we do since we only support Flash on Android and will never > block flash. Why would we never want to block flash? That's the one plugin we block the most on desktop.
(In reply to Jim Mathies [:jimm] from comment #9) > (In reply to Randall Barker [:rbarker] from comment #7) > > (In reply to Jim Mathies [:jimm] from comment #5) > > > You've hardcoded disabling the blocklist for plugins on android. Are you > > > sure you want to do this? > > > > :snorp said we do since we only support Flash on Android and will never > > block flash. > > Why would we never want to block flash? That's the one plugin we block the > most on desktop. It's unmaintained. If we block it, nobody would ever be able to use it. And that would make those people very angry. We accidentally blocked in the past because 'linux' matches Android as well, and it was not a good thing.
I should add, though, that the real reason we want this patch is because loading the stupid blocklist takes OVER ONE SECOND on the first page that queries for plugins.
![]() |
||
Comment 12•6 years ago
|
||
(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) from comment #11) > I should add, though, that the real reason we want this patch is because > loading the stupid blocklist takes OVER ONE SECOND on the first page that > queries for plugins. That sounds like a bug worth filing, a 163kb xml file shouldn't take a second to parse.
Comment 13•6 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/82b993933c92
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox41:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
You need to log in
before you can comment on or make changes to this bug.
Description
•