measure plugin shutdown with telemetry

RESOLVED FIXED in mozilla13

Status

()

Core
Plug-ins
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: froydnj, Assigned: froydnj)

Tracking

(Blocks: 1 bug)

unspecified
mozilla13
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [Snappy:P1])

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

6 years ago
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.
Duplicate of this bug: 706650
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]

Comment 3

6 years ago
Nathan, if you think this a P1 please take care of this. If not, please de-assign and change to [Snappy]
Assignee: nobody → nfroyd
(Assignee)

Updated

6 years ago
Depends on: 707320
(Assignee)

Comment 4

6 years ago
Created attachment 579284 [details] [diff] [review]
patch

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)

Comment 6

6 years ago
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.
(Assignee)

Updated

6 years ago
Depends on: 90628
No longer depends on: 707320

Comment 7

6 years ago
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-
(Assignee)

Comment 8

6 years ago
Created attachment 601371 [details] [diff] [review]
patch

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 9

6 years ago
Comment on attachment 601371 [details] [diff] [review]
patch

I assume I was mistaken for Mr. Aas.
Attachment #601371 - Flags: review?(josh) → review?(joshmoz)

Updated

6 years ago
Attachment #601371 - Flags: review?(joshmoz) → review+
(Assignee)

Comment 10

6 years ago
Created attachment 601694 [details] [diff] [review]
patch

Nit: updating to proper reviewer.  Carrying over r+.
Attachment #601371 - Attachment is obsolete: true
Attachment #601694 - Flags: review+
(Assignee)

Updated

6 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/247780df6c28
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/247780df6c28
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla13
You need to log in before you can comment on or make changes to this bug.