Let's look into making NPP_ClearSiteData run asynchronously so that we don't jank the main thread. +++ This bug was initially created as a clone of Bug #1091138 +++ One thing that might be responsible for bug 1088390 is our clearing of Flash plugin data. The code that does this is here: https://hg.mozilla.org/mozilla-central/annotate/c6989e473f97/browser/base/content/sanitize.js#l193 That "clearSiteData" might cause hangs on the main thread if there's a lot of plugin data to clear. I also wonder how often we fall into the "clear everything" case, which seems like it could be much worse. I think a few things are worth trying: - instrument a build to find out how much time is spent within "clearSiteData", and whether we hit the "plugin doesn't support time ranges, clear everything" code path. Give that build to the QA team to test in multiple different environments (different platforms, different versions of the flash plugin, etc.) and reports results - find some way to add a lot of data to the store that Flash is clearing here, and see what impact that has on overall Forget performance. I don't know how to do this exactly. Someone who knows more about how NPP_ClearSiteData works on the flash side could offer useful insight here, but I don't know who that might be.

Joel, I heard that you might want to assign somebody from your team to this one. It might require some IPC changes but I'm happy to mentor.

A bit more background... The are Telemetry probes measuring how long it takes to perform each of the "Forget history" sub-operations on the main thread (FX_SANITIZE_* probes). From reviewing this Telemetry in https://telemetry.mozilla.org, it's apparent there are 3 sub-operations causing main-thread jank: 1) closing windows (a median time of 600ms!) 2) clearing cookies (median time of 250ms) 3) clearing Places history (median time of 100ms) Window closing is likely very difficult to fix, Yoric is working on fixing clearing of Places, and this bug is for fixing the clearing of cookies. Unfortunately, the people who wrote the probes didn't report time to clear Flash cookies separately from time to clear Firefox cookies. However, I expect the cookie clearing operation to be very quick since the DB operations are performed off the main thread (http://telemetry.mozilla.org/slowsql/ backs this up). The first step may be to fix the Telemetry probe in sanitize.js to report the two cookie-clearing operations separately to Telemetry to confirm we're optimizing the right thing: http://hg.mozilla.org/mozilla-central/annotate/196d99aabc27/browser/base/content/sanitize.js#l182

Adding Chris as he might have some interest in this bug.

This adds a stopwatch that will be a segment of the old number in case we'd like to keep looking at that for any reason. I haven't worked with telemetry before, but this looks plausible.

Comment on attachment 8617674 [details] [diff] [review] Add a telemetry probe for how long it takes to clear plugin cookies during sanitize Review of attachment 8617674 [details] [diff] [review]: ----------------------------------------------------------------- Yeah let's keep FX_SANITIZE_COOKIES for now so we can do a direct before-and-after comparison ::: toolkit/components/telemetry/Histograms.json @@ +7765,5 @@ > "n_buckets": 20, > "extended_statistics_ok": true, > "description": "Sanitize: Time it takes to sanitize cookies (ms)" > }, > + "FX_SANITIZE_PLUGINS": { can you also add an FX_SANITIZE_COOKIES_2 which only measures Firefox cookie clearing time @@ +7766,5 @@ > "extended_statistics_ok": true, > "description": "Sanitize: Time it takes to sanitize cookies (ms)" > }, > + "FX_SANITIZE_PLUGINS": { > + "alert_emails": ["firefox-dev@mozilla.org", "gavin@mozilla.com"], We don't need to bug Gavin with these alerts @@ +7772,5 @@ > + "kind": "exponential", > + "high": "30000", > + "n_buckets": 20, > + "extended_statistics_ok": true, > + "description": "Sanitize: Time it takes to sanitize plugin cookies (ms)" Mention that this is a subset of FX_SANITIZE_COOKIES

Apparently billm might be making some changes around this for e10s. I added that bug to "See Also" for reference.

Note that bug 1089695 is about making sanitize.js async, which is necessary before we can use an OOP NPP_ClearSiteData.

Looking at the related bugs, maybe we should hold off on adding these probes until the changes in bug 1089695 land, and this seems like a duplicate of bug 1158561 in the sense that it establishes we need this operation to be async. If so, the only thing that remains is to implement that, so maybe there isn't much else to do in this bug.

I think that we'll still need to attack the synchronicity of NPP_ClearSiteData itself. Even if the JS stuff gets cleaned up to operate async in bug 1089695, the synchronous IPC call to NPP_ClearSiteData still needs to occur on the main thread, hence it will continue to jank.

(In reply to Aaron Klotz [:aklotz] (please use needinfo) from comment #10) > I think that we'll still need to attack the synchronicity of > NPP_ClearSiteData itself. Even if the JS stuff gets cleaned up to operate > async in bug 1089695, the synchronous IPC call to NPP_ClearSiteData still > needs to occur on the main thread, hence it will continue to jank. I understand, but it sounds a bit like that's already being worked on in bug 1158561.

There is no particular reason that the IPC call to NPP_ClearSiteData has to be synchronous. We don't need the return value of that function, for the most part.

As discussed on irc, here's a patch to make NPP_ClearSiteData async without regard for any return values. NS_ERROR_PLUGIN_TIME_RANGE_NOT_SUPPORTED is given special treatment in sanitize.js that will change behavior without it, so I don't know if we can get away with always ignoring the result.

Comment on attachment 8621361 [details] [diff] [review] Make NPP_ClearSiteData async Review of attachment 8621361 [details] [diff] [review]: ----------------------------------------------------------------- LGTM ::: dom/plugins/ipc/PluginModuleParent.cpp @@ +2606,5 @@ > { > if (!mClearSiteDataSupported) > return NS_ERROR_NOT_AVAILABLE; > > + if (!SendNPP_ClearSiteData(NullableString(site), flags, maxAge)) Nit: can you add curly braces around this if block? Current style calls for them so we usually just fix them as we come across them.

As discussed on irc, a patch showed up in bug 1158561 that achieves what I was aiming for here in a more thorough fashion, so I don't think it makes sense to pursue the patch in comment 13 at this time.

Looking in the patches in that bug, I'm going to mark this one as a dupe.

(In reply to Chris Manchester [:chmanchester] from comment #8) > Created attachment 8620724 [details] [diff] [review] > Add a telemetry probe for how long it takes to clear plugin cookies during > sanitize Telemetry dash shows that FX_SANITIZE_COOKIES ~= FX_SANITIZE_PLUGINS and FX_SANITIZE_COOKIES_2 ~= 0

Removing leave-open keyword from resolved bugs, per :sylvestre.