Closed Bug 706647 Opened 12 years ago Closed 11 years ago

measure plugin shutdown with telemetry

Categories

(Core Graveyard :: Plug-ins, defect)

Product:
Core Graveyard
Component:
Plug-ins
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
mozilla13

People

(Reporter: froydnj, Assigned: froydnj)

References

Details

(Whiteboard: [Snappy:P1])

Attachments

(1 file, 2 obsolete files)

patch
2.50 KB, patch
froydnj
: review+
Details | Diff | Splinter Review 
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.
From the hang detector, plugins seem like a fairly common source of hangs, so it would be good to be able to measure that.
Whiteboard: [Snappy] → [Snappy:P1]
Nathan, if you think this a P1 please take care of this. If not, please de-assign and change to [Snappy]
Assignee: nobody → nfroyd
Depends on: 707320
Attached patch patch (obsolete) — — Splinter 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?
Attachment #579284 - Flags: feedback?(justin.lebar+bug)
Comment on attachment 579284 [details] [diff] [review]
patch

I'm not familiar with plugin code, but Josh should be able to help.
Attachment #579284 - Flags: feedback?(justin.lebar+bug) → feedback?(joshmoz)
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.
Depends on: 90628
No longer depends on: 707320
Comment on attachment 579284 [details] [diff] [review]
patch

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.
Attachment #579284 - Flags: feedback?(joshmoz) → feedback-
Attached patch patch (obsolete) — — Splinter 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?
Attachment #579284 - Attachment is obsolete: true
Attachment #601371 - Flags: review?(josh)
Comment on attachment 601371 [details] [diff] [review]
patch

I assume I was mistaken for Mr. Aas.
Attachment #601371 - Flags: review?(josh) → review?(joshmoz)
Attachment #601371 - Flags: review?(joshmoz) → review+
Attached patch patch — — Splinter Review 
Nit: updating to proper reviewer.  Carrying over r+.
Attachment #601371 - Attachment is obsolete: true
Attachment #601694 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/247780df6c28
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla13
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.