Last Comment Bug 706647 - measure plugin shutdown with telemetry
: measure plugin shutdown with telemetry
Product: Core
Classification: Components
Component: Plug-ins (show other bugs)
: unspecified
: All All
-- normal (vote)
: mozilla13
Assigned To: Nathan Froyd [:froydnj]
: Benjamin Smedberg [:bsmedberg]
: 706650 (view as bug list)
Depends on: 90628
Blocks: 662444
  Show dependency treegraph
Reported: 2011-11-30 13:42 PST by Nathan Froyd [:froydnj]
Modified: 2012-03-01 06:08 PST (History)
13 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

patch (2.27 KB, patch)
2011-12-06 07:22 PST, Nathan Froyd [:froydnj]
jaas: feedback-
Details | Diff | Splinter Review
patch (2.50 KB, patch)
2012-02-28 12:33 PST, Nathan Froyd [:froydnj]
jaas: review+
Details | Diff | Splinter Review
patch (2.50 KB, patch)
2012-02-29 11:59 PST, Nathan Froyd [:froydnj]
nfroyd: review+
Details | Diff | Splinter Review

Description User image Nathan Froyd [:froydnj] 2011-11-30 13:42:45 PST
While discussing bug 662444, Vlad suggested that plugins might be a contributor to slow shutdowns.  We should record times for said shutdowns and--even better--correlate them with which plugins are taking that time, to identify problematic cases.
Comment 1 User image Andrew McCreight [:mccr8] 2011-12-01 12:28:39 PST
*** Bug 706650 has been marked as a duplicate of this bug. ***
Comment 2 User image Andrew McCreight [:mccr8] 2011-12-01 12:30:25 PST
From the hang detector, plugins seem like a fairly common source of hangs, so it would be good to be able to measure that.
Comment 3 User image (dormant account) 2011-12-02 16:38:13 PST
Nathan, if you think this a P1 please take care of this. If not, please de-assign and change to [Snappy]
Comment 4 User image Nathan Froyd [:froydnj] 2011-12-06 07:22:41 PST
Created attachment 579284 [details] [diff] [review]

Here's a patch that *looks* like it ought to DTRT, but I am very unfamiliar with this code.  Is this the right place to be looking, or are there other nooks and crannies to instrument due to OOPP?

Also, is this shutdown code going to be called, say, when you close a tab that's using Flash?  Or does the shutdown code only get called when all references to the plugin go away, i.e. when the browser closes?
Comment 5 User image Justin Lebar (not reading bugmail) 2011-12-06 15:10:47 PST
Comment on attachment 579284 [details] [diff] [review]

I'm not familiar with plugin code, but Josh should be able to help.
Comment 6 User image Josh Aas 2012-01-10 22:03:25 PST
I can help here but I want to wait for bug 90268 to land. Not a strict dependency, but it changes a lot of plugin code so it'll likely impact this, and I need to give it priority. Sorry for the review delay.
Comment 7 User image Josh Aas 2012-02-13 12:21:34 PST
Comment on attachment 579284 [details] [diff] [review]

Bug 90268 has landed (and stuck), we're ready for this now. Patch doesn't apply any more though.

Wouldn't this patch give a bad measurement if "PluginDestructionGuard::DelayDestroy" is true? You'd be measuring the time it takes us to decide to delay.
Comment 8 User image Nathan Froyd [:froydnj] 2012-02-28 12:33:20 PST
Created attachment 601371 [details] [diff] [review]

OK, if we don't want to measure time spent determining whether we're not or already shutting down, how about this patch instead?
Comment 9 User image Josh Matthews [:jdm] 2012-02-28 13:00:51 PST
Comment on attachment 601371 [details] [diff] [review]

I assume I was mistaken for Mr. Aas.
Comment 10 User image Nathan Froyd [:froydnj] 2012-02-29 11:59:57 PST
Created attachment 601694 [details] [diff] [review]

Nit: updating to proper reviewer.  Carrying over r+.
Comment 11 User image Ryan VanderMeulen [:RyanVM] 2012-02-29 16:44:38 PST
Comment 12 User image Marco Bonardo [::mak] 2012-03-01 06:08:56 PST

Note You need to log in before you can comment on or make changes to this bug.