Closed
Bug 1371888
Opened 7 years ago
Closed 6 years ago
Cache plugin and plugin blocklist information so we can avoid loading the blocklist on startup in most cases
Categories
(Core Graveyard :: Plug-ins, defect, P1)
Core Graveyard
Plug-ins
Tracking
(Performance Impact:high, firefox59 wontfix, firefox60 verified, firefox61 verified)
People
(Reporter: mconley, Assigned: Gijs)
References
(Blocks 1 open bug)
Details
(Keywords: perf, Whiteboard: [measurement:client][fxperf:p1])
Attachments
(3 files, 1 obsolete file)
59 bytes,
text/x-review-board-request
|
mossop
:
review+
florian
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
florian
:
review+
|
Details |
68.73 KB,
patch
|
Gijs
:
review+
jcristau
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
Here's a profile: https://perfht.ml/2sdsDQN Here's the code in question: http://searchfox.org/mozilla-central/source/toolkit/components/telemetry/TelemetryEnvironment.jsm#573 I feel like we can probably simplify this a bit, since I think the question is now "is Flash enabled or not". I suspect that question can either be answered on the main thread, or after first paint.
Comment 1•7 years ago
|
||
Benjamin, is collecting the plugin data after/on first paint acceptable for the crash ping purposes? Alternatively, with everything but Flash being disabled now, is there a cheaper way to get the plugin information now? Or could we improve the scanning?
Flags: needinfo?(benjamin)
Comment 2•7 years ago
|
||
I think we can probably use a eager-push/lazy-pull model. If a page loads plugins during startup, we push the plugin data to telemetry. Otherwise telemetry waits until after the startup critical path/idle. That way if the user does end up interacting with Flash/click-to-play we know we'll have the data (and there is no additional perf penalty).
Flags: needinfo?(benjamin)
Comment 3•7 years ago
|
||
Thanks, that sounds good. The profile in comment 0 shows an impact of 31ms on startup.
Priority: -- → P3
Summary: Telemetry causes nsPluginHost to instantiate in the parent process, which causes it to scan the disk on the main thread for plugins → Don't cause plugin scanning from Telemetry early in startup
Whiteboard: [measurement:client] [qf]
Updated•7 years ago
|
Priority: P3 → P2
Comment 4•7 years ago
|
||
Kyle, do you mind having a look at this one please?
Flags: needinfo?(kyle)
Whiteboard: [measurement:client] [qf] → [measurement:client] [qf:p1]
Updated•7 years ago
|
Assignee: nobody → kyle
Flags: needinfo?(kyle)
Comment 5•7 years ago
|
||
So, even if we delay telemetry fetching of plugins, we're still going to incur the costs of plugin finding during the first startup of a content process (see bug 1337058), so afaict we're not getting much of a pre-first-paint win with this bug alone, unless telemetry is taking some sort of non-trivial amount of time that I'm missing. Do we need to look at trying to delay fetching more than that?
Flags: needinfo?(benjamin)
Comment 6•7 years ago
|
||
So my recollection of what we talked about at https://bugzilla.mozilla.org/show_bug.cgi?id=1337058#c5 was that we were going to: * kick off plugin scanning fairly early in startup, hopefully on a background thread * launch the first content process without plugin information * send the plugin information to any content processes when the async scanning finishes, as well as update telemetry * if a content process needs plugin information, it will block to wait for it (yuck but hopefully uncommon) I know bug 1337058 got us part of the way there? But there may be more to do relative to this. Especially if we can move plugin scanning off the main thread that would help a bunch of things. Also! On windows and mac, we don't need to actually load the plugin DLLs to scan for them: on mac we can use the plist information, and on Windows we can use LoadLibraryEx(LOAD_LIBRARY_AS_IMAGE_RESOURCE | LOAD_LIBRARY_AS_DATAFILE) to just load the resource fork of the DLL. That will skip DllMain, which would be huge. feel free to put some time on vidyo if you want to talk this through, since I know it's rather complicated.
Flags: needinfo?(benjamin)
Comment 7•7 years ago
|
||
Ah, ok, reading back on that bug with what you just posted, there's more nuance to it than I realized or implemented. Let me take some time to go over what's laid out there and here again, and I'll get back to you on setting up a video conference probably sometime next week (meant to meet up at the work week, oops :/ ). It sounds like we could do the plist/resource DLL load to get ourselves an initial startup optimization, then implement the threading/async depending on time we've got left. Main reason I'm thinking about that ordering is that I'm worried that the async thread + content blocking while waiting for plugins might get hairy to test, so I'll take initial speedup by minimal loads, then work from there on what's left.
Comment 8•7 years ago
|
||
(In reply to Georg Fritzsche [:gfritzsche] from comment #3) > Thanks, that sounds good. > > The profile in comment 0 shows an impact of 31ms on startup. _getActivePlugins is very expensive during startup, it now seems to be the last thing that makes us load the blocklist before first paint. In this startup profile https://perfht.ml/2uiNNhN on a slow netbook, we spend 970ms before first paint in _getActivePlugins, and almost 98% of that time is blocklist related.
Comment 9•7 years ago
|
||
Wow, are we really that slow at parsing XML? I'm working on taking FindPlugins off the main thread right now, though I'm not sure how much that's gonna help this problem, as blocklist loading happens after that (once we've fired the notification that we've updated the plugin list). Something we could maybe do is just cleanup the blocklist? Right now it contains a huge list of plugins and addons that are no longer even valid. I'm checking with people to see if that's something else we can do to cut down on loads here.
Comment 10•7 years ago
|
||
Ok, so first off, it's not the XML parsing (which is done via the dom loader), it's the node parsing after that. We just have a LOT of block list nodes. Addons are the major portion of the blocklist, and having talked to the addon team, I don't think there is much we can do to the blocklist itself for 57. The same blockilst is presented to all versions of the browser, and addons in that list will be both legacy and web extension. Plugins can at least be optimized some, that will be happening in bug 1391792. Graphics drivers and certs obviously can't do anywhere. It looks like loading the blocklists as JSON instead of XML is happening bug 1257565, which could drastically speed up this parsing step. I'm gonna mark this as dependent on that for now.
Comment 11•7 years ago
|
||
According to that profile, of the 900ms, we're spending ~300ms in XML parsing, ~300ms in addon node parsing, and ~300ms for plugins/graphics/certs.
Comment 13•7 years ago
|
||
_getActivePlugins from resource://gre/modules/TelemetryEnvironment.jsm is 21% of the time spent before first paint on this startup profile: https://perfht.ml/2eKHRUw
Comment 14•7 years ago
|
||
This bug will be fixed for 57; moving it to QF:P2 for post 57 work.
Comment 15•7 years ago
|
||
I meant won't be fixed by 57.(In reply to Jean Gong from comment #14) > This bug will be fixed for 57; moving it to QF:P2 for post 57 work. I meant won't be fixed for 57!
Whiteboard: [measurement:client] [qf:p1] → [measurement:client] [qf:p2]
Updated•6 years ago
|
Whiteboard: [measurement:client] [qf:p2] → [measurement:client] [qf:p1][qf:i60]
Updated•6 years ago
|
Whiteboard: [measurement:client] [qf:p1][qf:i60] → [measurement:client][qf:f60][qf:p1]
Assignee | ||
Comment 16•6 years ago
|
||
My understanding is that currently, we load the blocklist on startup because telemetry asks for a list of plugins using a sync API that the plugin service provides, and that implicitly also requires the blocklist, which we then load sync using main-thread IO (iff there are any plugins at all, ie if the user has flash installed). Furthermore, AFAICT the work in bug 1257565 is effectively blocked on making the blocklist APIs async. To do this, AIUI these plugin APIs will need to be async as well, or they need to cache the plugin information such that we don't load the list on every startup just because telemetry wants a list of plugins. Kyle, what's the status of this bug? Is there an easy way to cache the plugin blocklist data for installed plugins so that we can decouple plugin initialization from the blocklist? If it's OK for telemetry, we could also potentially look at delaying telemetry fetching the list of plugins until after the blocklist completes async OMT-io loading... but I don't know if the plugin code itself won't then trigger similar blocklist fetching independently to determine whether a plugin is blocklisted (perhaps only if a webpage attempts to use a particular plugin?). The blocklist service is pretty rubbish at the moment, and bug 1425602 tracks making this better in various ways. async-ifying the plugin stuff, or caching it so we don't need the sync IO, seems like the best short-term bet for improving things on the client. I'm looking at improving what the server sends us in bug 1434302.
Blocks: 1425602
Flags: needinfo?(kyle)
Assignee | ||
Comment 17•6 years ago
|
||
+kmag for comment #16 given he suggested it in https://bugzilla.mozilla.org/show_bug.cgi?id=1425600#c8
Comment 18•6 years ago
|
||
Here is a startup profile of Firefox 58 on a slow Ubuntu machine, where getPluginBlocklistState (called by _getActivePlugins from resource://gre/modules/TelemetryEnvironment.jsm) is 16% of the time spent before first paint: https://perfht.ml/2rY1iD2
Comment 19•6 years ago
|
||
I haven't touched this in months, because at the time it looks like the blocklist was going to move to a system that would parse faster and we were going to see how much of a speed up that gave us before poking at the rather brittle plugin code. I guess that initiative stalled (though I didn't realize that was partially due to plugin sync calls?), and I haven't taken a look on the plugin side since. I guess I can look at caching the plugin list again, since it should be either 0 or 1 plugins these days anyways. The only weirdness around that is going to be flash upgrades happening in between browser sessions. It's been long enough since I've look at the system that I'm honestly not sure at what point that would be checked and when the blocklist load would happen in relation to that, so I'll have to do some research there.
Flags: needinfo?(kyle)
Comment 20•6 years ago
|
||
(In reply to :Gijs from comment #16) > My understanding is that currently, we load the blocklist on startup because > telemetry asks for a list of plugins using a sync API that the plugin > service provides, and that implicitly also requires the blocklist, which we > then load sync using main-thread IO (iff there are any plugins at all, ie if > the user has flash installed). That's one of the possible reasons we load it at startup. The graphics blocklist is another. We need to fix both of those problems, but the situation for plugins is uglier, so it's probably the one to start with. > To do this, AIUI these plugin APIs will need to be async as well, or they > need to cache the plugin information such that we don't load the list on > every startup just because telemetry wants a list of plugins. I think we need to cache the information (at least the blocklist state) whether or not we make the APIs async. The blocklistState property is particularly ugly, because we recompute it every time the property is accessed, it's absurdly expensive to compute, and there are places where we access it several times in a row: https://searchfox.org/mozilla-central/rev/eeb7190f9ad6f1a846cd6df09986325b3f2c3117/toolkit/mozapps/extensions/internal/PluginProvider.jsm#334-337 (In reply to Kyle Machulis [:qdot] [:kmachulis] (if a patch has no decent commit message, automatic r-) from comment #19) > I guess I can look at caching the plugin list again, since it should be > either 0 or 1 plugins these days anyways. The only weirdness around that is > going to be flash upgrades happening in between browser sessions. We just need a way to detect when a plugin has changed from the last session. Timestamps might be a good enough place to start. That's more or less what we do for extensions, though in that case we have better version and ID metadata to go off of when the timestamps disagree.
Assignee | ||
Comment 21•6 years ago
|
||
(In reply to Kris Maglione [:kmag] (long backlog; ping on IRC if you're blocked) from comment #20) > > To do this, AIUI these plugin APIs will need to be async as well, or they > > need to cache the plugin information such that we don't load the list on > > every startup just because telemetry wants a list of plugins. > > I think we need to cache the information (at least the blocklist state) > whether or not we make the APIs async. The blocklistState property is > particularly ugly, because we recompute it every time the property is > accessed, it's absurdly expensive to compute, and there are places where we > access it several times in a row: > > https://searchfox.org/mozilla-central/rev/ > eeb7190f9ad6f1a846cd6df09986325b3f2c3117/toolkit/mozapps/extensions/internal/ > PluginProvider.jsm#334-337 OK. Do you have concrete ideas about how to do the caching? I'm asking because in part, it's not clear to me where we could cache the information such that: 1) the relevant state is available as soon as telemetry needs it (on startup) and 2) getting that state doesn't require additional sync io. Given that it's only flash, I've played with the idea of storing the information in prefs, even though I think we all know how storing more cruft in prefs is not an ideal solution... but I don't really have better ideas. As for the blocklistState property, that seems fixable with changes to the runtime implementation of the plugins interfaces without changing how we read information at startup, right? That is, by avoiding the recomputation and caching and invalidating the information at runtime instead. Or am I missing something? In any case, my understanding is that doing that wouldn't solve the startup IO issue, though obviously it may solve other issues... perhaps it needs a separate bug? The other solution here may be to postpone the plugin scanning telemetry does, which may be desirable *anyway*. Speaking to folks in #telemetry, it seems like this may also be acceptable, potentially if given some kind of alternative way of getting information into crash data (see most of https://mozilla.logbot.info/telemetry/20180131 ). I'm not sure if that will be sufficient to avoid any other accesses to the blocklist / plugin data, however...
Flags: needinfo?(kmaglione+bmo)
Updated•6 years ago
|
Whiteboard: [measurement:client][qf:f60][qf:p1] → [measurement:client][qf:f61][qf:p1]
Updated•6 years ago
|
Whiteboard: [measurement:client][qf:f61][qf:p1] → [measurement:client][qf:f61][qf:p1] [fxperf]
Assignee | ||
Comment 22•6 years ago
|
||
(In reply to Kyle Machulis [:qdot] [:kmachulis] (if a patch has no decent commit message, automatic r-) from comment #19) > I guess I can look at caching the plugin list again, since it should be > either 0 or 1 plugins these days anyways. The only weirdness around that is > going to be flash upgrades happening in between browser sessions. It's been > long enough since I've look at the system that I'm honestly not sure at what > point that would be checked and when the blocklist load would happen in > relation to that, so I'll have to do some research there. This would be very helpful. I'm going to morph this bug accordingly. I'll file a separate bug on decoupling telemetry from plugins and work on that anyway. :qDot, will you be able to work on this in time for 60? Is there any information you need that would help with that?
Component: Telemetry → Plug-ins
Flags: needinfo?(kyle)
Product: Toolkit → Core
Summary: Don't cause plugin scanning from Telemetry early in startup → Cache plugin and plugin blocklist information so we can avoid loading the blocklist on startup in most cases
Assignee | ||
Comment 23•6 years ago
|
||
I ran into some issues with the telemetry work and as a result I finally found pluginreg.dat, so I'll just steal this for some of the work that will at least avoid the blocklist IO for the cases where we have a pluginreg that's not outdated (by storing the blocklist data in there). That doesn't fully fix the problem, but it mostly does, so that's something, and as we all know something is better than nothing...
Assignee: kyle → gijskruitbosch+bugs
Flags: needinfo?(kyle)
Flags: needinfo?(kmaglione+bmo)
Priority: P3 → P1
Comment hidden (mozreview-request) |
Assignee | ||
Comment 25•6 years ago
|
||
mozreview-review |
Comment on attachment 8952455 [details] Bug 1371888 - cache plugin information in pluginreg.dat to avoid sync startup load, https://reviewboard.mozilla.org/r/221686/#review227628 ::: dom/plugins/base/nsPluginHost.cpp:3424 (Diff revision 1) > + if (blocklistAlteredPlugins) { > - // We update blocklists asynchronously by just sending a new plugin list to > + // We update blocklists asynchronously by just sending a new plugin list to > - // content. > + // content. > - if (XRE_IsParentProcess()) { > // We'll need to repack our tags and send them to content again. > IncrementChromeEpoch(); > SendPluginsToContent(); > } I suspect that in this case we also need to rewrite the pluginreg.dat file if there's nothing that takes care of this already...
Comment hidden (mozreview-request) |
Comment 27•6 years ago
|
||
mozreview-review |
Comment on attachment 8952455 [details] Bug 1371888 - cache plugin information in pluginreg.dat to avoid sync startup load, https://reviewboard.mozilla.org/r/221686/#review227678 ::: dom/plugins/base/nsPluginHost.cpp:3421 (Diff revision 1) > + EmptyString(), &blocklistState); > + NS_ENSURE_SUCCESS(rv, rv); > + uint32_t oldBlocklistState; > + plugin->GetBlocklistState(oldBlocklistState); > + plugin->SetBlocklistState(blocklistState); > + blocklistAlteredPlugins = blocklistAlteredPlugins || (oldBlocklistState != blocklistState); You can just use "|=" here can't you? ::: dom/plugins/base/nsPluginHost.cpp:3424 (Diff revision 1) > + if (blocklistAlteredPlugins) { > - // We update blocklists asynchronously by just sending a new plugin list to > + // We update blocklists asynchronously by just sending a new plugin list to > - // content. > + // content. > - if (XRE_IsParentProcess()) { > // We'll need to repack our tags and send them to content again. > IncrementChromeEpoch(); > SendPluginsToContent(); > } Yep
Attachment #8952455 -
Flags: review?(dtownsend) → review-
Comment hidden (mozreview-request) |
Comment 29•6 years ago
|
||
mozreview-review |
Comment on attachment 8952455 [details] Bug 1371888 - cache plugin information in pluginreg.dat to avoid sync startup load, https://reviewboard.mozilla.org/r/221686/#review227802 I don't feel competent to review the plugin changes, so I assume you were asking me to only look at the blocklist service change. ::: toolkit/mozapps/extensions/nsBlocklistService.js:817 (Diff revision 3) > > var appFile = FileUtils.getFile(KEY_APPDIR, [FILE_BLOCKLIST]); > try { > await this._preloadBlocklistFile(appFile.path); > + Services.tm.idleDispatchToMainThread(() => { > + this._loadBlocklist(); I think you need to add an if (!this._isBlocklistLoaded()) test here. Actually, why are you triggering a load only in the case of having preloaded a blocklist file from the app dir and not from the profile dir? ::: toolkit/mozapps/extensions/nsBlocklistService.js:842 (Diff revision 3) > > let text = await OS.File.read(path, { encoding: "utf-8" }); > > if (!this._addonEntries) { > // Store the content only if a sync load has not been performed in the meantime. > this._preloadedBlocklistContent = text; I almost wonder if we could just replace this line with triggering a load from string from here, and remove the _preloadedBlocklistContent field completely.
Attachment #8952455 -
Flags: review?(florian) → review-
Comment hidden (mozreview-request) |
Comment 31•6 years ago
|
||
mozreview-review |
Comment on attachment 8952455 [details] Bug 1371888 - cache plugin information in pluginreg.dat to avoid sync startup load, https://reviewboard.mozilla.org/r/221686/#review230312 r=me for the blocklist service changes.
Attachment #8952455 -
Flags: review?(florian) → review+
Comment 32•6 years ago
|
||
mozreview-review |
Comment on attachment 8952455 [details] Bug 1371888 - cache plugin information in pluginreg.dat to avoid sync startup load, https://reviewboard.mozilla.org/r/221686/#review230772 ::: commit-message-c0119:9 (Diff revision 4) > + > +There is now only the saved blocklist state in a plugin tag instance, rather than > +looking it up from in there using the blocklist service, so it was renamed from > +mCachedBlocklistState to mBlocklistState. We pass the 'right' state to the plugin > +instance when the plugintag is constructed. If we don't have state, we mark it as > +unblocked. This doesn't appear to be the case. For newly detected plugins you call the blocklist to get its state. I happen to think that that might be the best choice but since it potentially causes a sync-load of the blocklist it might be an issue.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 35•6 years ago
|
||
(In reply to Dave Townsend [:mossop] from comment #32) > This doesn't appear to be the case. For newly detected plugins you call the > blocklist to get its state. Ugh, good catch. > I happen to think that that might be the best > choice but since it potentially causes a sync-load of the blocklist it might > be an issue. Yeah, I don't think we should do this. I've updated the patch to no longer do this. We now only call the 'get data out of the blocklist' APIs when we get the blocklist-loaded notification. This means that there is a tiny window until the blocklist loads where, in theory, we could be running a new plugin. In practice, given that there's a 24h window between each blocklist update, and for this to happen you'd need to actually hit a webpage that used the plugin immediately, that doesn't seem like a problem. When investigating this I also noticed that we have a sync ipc message to get blocklist state in the content process. That's not necessary anymore in this setup, so I removed it in a separate cset.
Comment 36•6 years ago
|
||
mozreview-review |
Comment on attachment 8956811 [details] Bug 1371888 - Stop having a sync IPC message to pass blocklist state, https://reviewboard.mozilla.org/r/225784/#review231650 ::: commit-message-f8c53:5 (Diff revision 1) > +Bug 1371888 - Stop having a sync IPC message to pass blocklist state, r?florian > + > +Because plugin state in the content now contains blocklist state, and is updated > +when the blocklist updates, we don't need to ask the parent if we're checking > +blocklist state. All the consumers should now be asking the plugin code directly, Are you sure https://searchfox.org/mozilla-central/rev/efce4ceddfbe5fe4e2d74f1aed93894bada9b273/dom/base/nsObjectLoadingContent.cpp#814 isn't needed?
Assignee | ||
Comment 37•6 years ago
|
||
mozreview-review |
Comment on attachment 8956811 [details] Bug 1371888 - Stop having a sync IPC message to pass blocklist state, https://reviewboard.mozilla.org/r/225784/#review231652 ::: commit-message-f8c53:5 (Diff revision 1) > +Bug 1371888 - Stop having a sync IPC message to pass blocklist state, r?florian > + > +Because plugin state in the content now contains blocklist state, and is updated > +when the blocklist updates, we don't need to ask the parent if we're checking > +blocklist state. All the consumers should now be asking the plugin code directly, I've removed that callsite in the earlier commit. :-)
Comment 38•6 years ago
|
||
mozreview-review |
Comment on attachment 8956811 [details] Bug 1371888 - Stop having a sync IPC message to pass blocklist state, https://reviewboard.mozilla.org/r/225784/#review231670 Ok then :-). I agree that we should remove the content side of this service entirely!
Attachment #8956811 -
Flags: review?(florian) → review+
Updated•6 years ago
|
Status: NEW → ASSIGNED
Whiteboard: [measurement:client][qf:f61][qf:p1] [fxperf] → [measurement:client][qf:f61][qf:p1][fxperf:p1]
Comment 39•6 years ago
|
||
mozreview-review |
Comment on attachment 8952455 [details] Bug 1371888 - cache plugin information in pluginreg.dat to avoid sync startup load, https://reviewboard.mozilla.org/r/221686/#review232208 ::: dom/plugins/base/nsPluginHost.cpp:2104 (Diff revision 5) > - pluginTag = new nsPluginTag(&info, fileModTime, fromExtension); > + uint32_t state = nsIBlocklistService::STATE_NOT_BLOCKED; > + pluginTag = new nsPluginTag(&info, fileModTime, fromExtension, state); > + // If the blocklist is loaded, get the blocklist state now. > + // If it isn't loaded yet, we'll update it once it loads. > + bool isBlocklistLoaded = false; > + if (blocklist && NS_SUCCEEDED(blocklist->GetIsLoaded(&isBlocklistLoaded)) && The blocklist can't load while this file scan is running as it all happens on the main thread (:sadface:) so you may as well grab the value of isBlocklistLoaded before the start of the loop.
Attachment #8952455 -
Flags: review?(dtownsend) → review+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 42•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d2854effa97e3520dbfd98ac8457f13fa68f3ae8
Comment hidden (mozreview-request) |
Assignee | ||
Comment 44•6 years ago
|
||
(I intend to merge the 3rd cset into the 1st one when landing, but I wanted to split it out to ease reviewing)
Assignee | ||
Comment 45•6 years ago
|
||
mozreview-review |
Comment on attachment 8958291 [details] Bug 1371888 - fix test_asyncBlocklistLoad.js and ensure we set addon/plugin/gfxEntries to empty arrays when starting to load the file, https://reviewboard.mozilla.org/r/227220/#review233020 ::: toolkit/mozapps/extensions/nsBlocklistService.js:836 (Diff revision 1) > return; > } > > let text = await OS.File.read(path, { encoding: "utf-8" }); > > + await new Promise(resolve => { I originally used `return` here but that upset eslint because the other `return` statements here don't return a value.
Comment 46•6 years ago
|
||
mozreview-review |
Comment on attachment 8958291 [details] Bug 1371888 - fix test_asyncBlocklistLoad.js and ensure we set addon/plugin/gfxEntries to empty arrays when starting to load the file, https://reviewboard.mozilla.org/r/227220/#review233226 Thanks for fixing the test! :-) ::: toolkit/mozapps/extensions/nsBlocklistService.js:840 (Diff revision 1) > > + await new Promise(resolve => { > - Services.tm.idleDispatchToMainThread(() => { > + Services.tm.idleDispatchToMainThread(() => { > - if (!this.isLoaded) { > + if (!this.isLoaded) { > - Services.telemetry.getHistogramById("BLOCKLIST_SYNC_FILE_LOAD").add(false); > + Services.telemetry.getHistogramById("BLOCKLIST_SYNC_FILE_LOAD").add(false); > + this._gfxEntries = []; I'm under the impression that doing this at the beginning of the _loadBlocklistFromXML method would make more sense, but I don't feel strongly about it.
Attachment #8958291 -
Flags: review?(florian) → review+
Comment hidden (obsolete) |
Assignee | ||
Comment 48•6 years ago
|
||
(Perhaps we should make the `isLoaded` flag just an explicitly set/unset bool property instead of the magic getter that doesn't make much sense... but that can be something for another bug...)
Assignee | ||
Comment 49•6 years ago
|
||
Annnnd I realized I misread your comment. I'll just do what you suggested instead. :-)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•6 years ago
|
Attachment #8958291 -
Attachment is obsolete: true
Comment 52•6 years ago
|
||
(In reply to :Gijs from comment #48) > (Perhaps we should make the `isLoaded` flag just an explicitly set/unset > bool property instead of the magic getter that doesn't make much sense... > but that can be something for another bug...) I almost suggested that, and making the 3 arrays already exist on the prototype instead of being null when not initialized, but I figured it was a much bigger change than fixing this test is worth.
Comment 53•6 years ago
|
||
Pushed by gijskruitbosch@gmail.com: https://hg.mozilla.org/integration/autoland/rev/ce82f8fc8c80 cache plugin information in pluginreg.dat to avoid sync startup load, r=florian,mossop https://hg.mozilla.org/integration/autoland/rev/d8b3ca1ff03c Stop having a sync IPC message to pass blocklist state, r=florian
Comment 54•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/ce82f8fc8c80 https://hg.mozilla.org/mozilla-central/rev/d8b3ca1ff03c
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox61:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Assignee | ||
Comment 55•6 years ago
|
||
These are beta-grafted csets for this bug (bug 1371888), bug 1443870 and bug 1445990. Approval Request Comment [Feature/Bug causing the regression]: n/a [User impact if declined]: slower startup for esr60, different plugin initialization, different pluginreg fileformat from all later versions [Is this code covered by automated tests?]: yes [Has the fix been verified in Nightly?]: yes, see https://twitter.com/gijskruitbosch/status/974293602124910592 (ie telemetry verifies that this fix was effective) [Needs manual test from QE? If yes, steps to reproduce]: It would be good to do a quick sanity check of flash-based (only plugin we support at this point) content, both that it generally works and that older versions of flash get blocked appropriately. [List of other uplifts needed for the feature/fix]: this patch contains all the csets for the 3 relevant bugs. [Is the change risky?]: not very [Why is the change risky/not risky?]: this has baked on nightly for a bit, telemetry looks good, we still have ample beta runway, the patches are reasonably contained. [String changes made/needed]: no
Attachment #8960152 -
Flags: review+
Attachment #8960152 -
Flags: approval-mozilla-beta?
Assignee | ||
Updated•6 years ago
|
status-firefox59:
--- → wontfix
status-firefox60:
--- → affected
Comment 56•6 years ago
|
||
Comment on attachment 8960152 [details] [diff] [review] Patches for beta Alright, let's give this a go. beta60+, should be in 60.0b7.
Attachment #8960152 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment 57•6 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/a04c414e17ea https://hg.mozilla.org/releases/mozilla-beta/rev/6ab477572d7f
Flags: in-testsuite+
Updated•6 years ago
|
Flags: qe-verify+
Comment 58•6 years ago
|
||
I talk with Gijs on slack and he suggested me to use 2 cases: "One where you have an older version of Flash (that we mark as having security vulnerabilities) installed, and checking that opening a site that uses flash shows the usual "this site wants to use a plugin and you can't use it because it's vulnerable" warnings, before + after the changes. The other would be where you have an up-to-date version of Flash and to check whether that works without issues. It looks like Flash player 26 should do the trick for being blocked". I tested the 2 scenarios on Mac OS X 10.12, Windows 10 x64 and Ubuntu 16.04 with FF Nightly 61.0a1(2018-03-28) and FF beta 60.0b7 and everything works as expected. Please note that on Ubuntu I wasn't able to install the Flash player 26 to reproduce the warning, but I tested with the latest Flash version the fact that flash games are working.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Updated•2 years ago
|
Performance Impact: --- → P1
Whiteboard: [measurement:client][qf:f61][qf:p1][fxperf:p1] → [measurement:client][fxperf:p1]
Updated•2 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•