Closed Bug 882339 Opened 8 years ago Closed 8 years ago

Ts and Tp regression from bug 875454/bug 880675


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




Tracking Status
firefox23 --- unaffected
firefox24 + fixed


(Reporter: benjamin, Assigned: benjamin)



(Keywords: perf, regression)


(1 file)

There were Ts/Tp regressions from bug 875454/bug880675. I believe that the problem is that calling into the blocklist service is rather expensive, and we do it a fair bit when walking through enabled plugin tags and whatnot.

My first plan is to cache the blocklist state on the plugin tag, but not persist it anywhere. This should completely fix Tp. I'm a little worried about Ts because we may be now loading the blocklist service prior to first-paint and if so I'm not sure what we can do about that. But first patch first.
Attachment #762119 - Flags: review?(jschoenick)
Oops the change in test_bug391728.html is a leftover, it shouldn't be necessary.
Attachment #762119 - Flags: review?(jschoenick) → review+
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
But it didn't work. Talos did not budge at all with this patch.

I'm a little worried now:
* Perhaps this caching code is not being effective because blocklist->GetPluginBlocklistState is throwing an exception in the talos environment? I'd like to understand whether the blocklist is enabled or disabled in Talos and what other non-default configs it uses

* Perhaps the blocklist code is not the problem, and it's actually the permission manager which is causing the slowdown. I'll know more with some profiling on today's nightly. But given that information, we'd basically have to fix the permission manager, since using the permission manager is fundamentally necessary to making plugins click-to-play by default.

Is it possible to figure out if specific Tp pages are responsible for this slowdown, or whether it's spread evenly across a lot of pages?
Flags: needinfo?(jmaher)
Resolution: FIXED → ---
Closed: 8 years ago8 years ago
Resolution: --- → FIXED
Resolution: FIXED → ---
I looked at this with bsmedberg via irc.  The extension blocklist is set to, basically a 404.  We added this about a month ago to talos as we found there was network access requests getting sent out.  We don't need the entire list of dummy server, but we did need the extensions.update.background.url to prevent nss crashes on android:

I have looked at a before/after of the test run in more details (per page, not single value for the entire run) and compiled a spreadsheet:

There are some large increases, worth looking into.
Flags: needinfo?(jmaher)
I'm now very confused about this. The highest regressions appear to be in and I loaded those pages in a local browser and I see:

* Neither of these pages loads any plugins
* when loading these pages, I don't trip breakpoints on any of the following functions:
** nsObjectLoadingContent::ShouldPlay
** nsObjectLoadingContent::LoadObject
** nsContentDLF::CreateInstance at the point where we check for plugin MIME types
** nsPluginTag::GetBlocklistState
** nsPluginHost::PluginExistsForType

The two regression changesets are:

It appears that those functions are the only entry points for changes in these bugs, which leaves me quite stumped as to what is different. johns, do you have any other suggestions before I back out one or the other of these patches?
Flags: needinfo?(jschoenick)
johns helped me figure it out! These pages are enumerating navigator.plugins (more than once in some cases), *and* there was a bug in my caching where nsPluginTag::IsBlocklisted wasn't using the cache.

Got r=johns over IRC for the obvious fix. I now owe lots of people excellent libations.
Flags: needinfo?(jschoenick)
Closed: 8 years ago8 years ago
Resolution: --- → FIXED
Blocks: 889484
You need to log in before you can comment on or make changes to this bug.