Closed
Bug 878292
Opened 11 years ago
Closed 11 years ago
Profiler actor leaves the profiler running in some cases
Categories
(DevTools :: Performance Tools (Profiler/Timeline), defect)
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 24
People
(Reporter: dcamp, Assigned: dcamp)
Details
Attachments
(1 file, 2 obsolete files)
5.56 KB,
patch
|
rcampbell
:
review+
|
Details | Diff | Splinter Review |
... because it counts connections rather than active profilers, and doesn't stop if the last connection didn't start.
Attachment #756777 -
Flags: review?(anton)
Assignee | ||
Comment 1•11 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=44f0f69b4217
Comment 2•11 years ago
|
||
Comment on attachment 756777 [details] [diff] [review] v1 Review of attachment 756777 [details] [diff] [review]: ----------------------------------------------------------------- LGTM. I guess since it's not a regression fix we don't need a test case to cover your scenario (a starts profiler, disconnects, b doesn't start, disconnects)? Other than that, if try is clean: r+.
Attachment #756777 -
Flags: review?(anton) → review+
Assignee | ||
Comment 3•11 years ago
|
||
No, I should probably write a test...
Comment 4•11 years ago
|
||
Ok. I'd write a test then—shouldn't be too time consuming.
Assignee | ||
Comment 5•11 years ago
|
||
Attachment #756777 -
Attachment is obsolete: true
Attachment #756862 -
Flags: review?(anton)
Comment 6•11 years ago
|
||
Comment on attachment 756862 [details] [diff] [review] with tests Review of attachment 756862 [details] [diff] [review]: ----------------------------------------------------------------- what's up with all the Profiler.IsActive()s? ::: toolkit/devtools/server/tests/unit/test_profiler_activation.js @@ +16,5 @@ > + > +function run_test() > +{ > + // Ensure the profiler is not running when the test starts (it could > + // happen if the MOZ_PROFILER_STARTUP environment variable is set) shouldn't ever happen in our generic mochitest environment should it? Guess it's good to be safe. @@ +18,5 @@ > +{ > + // Ensure the profiler is not running when the test starts (it could > + // happen if the MOZ_PROFILER_STARTUP environment variable is set) > + Profiler.StopProfiler(); > + Profiler.IsActive(); do you need IsActive() here? Should you do a do_check_false() on it or something? @@ +34,5 @@ > +} > + > +function activate_first(client1, actor1, client2, actor2) { > + let Profiler = Cc["@mozilla.org/tools/profiler;1"].getService(Ci.nsIProfiler); > + Profiler.IsActive(); and here? @@ +39,5 @@ > + > + // Start the profiler on the first connection.... > + client1.request({ to: actor1, type: "startProfiler", features: ['js']}, startResponse => { > + dump("GOT RESPONSE HERE: " + Profiler + "\n"); > + Profiler.IsActive(); what about this one?
Attachment #756862 -
Flags: review?(anton)
Assignee | ||
Comment 7•11 years ago
|
||
Attachment #756862 -
Attachment is obsolete: true
Attachment #756982 -
Flags: review?(rcampbell)
Updated•11 years ago
|
Attachment #756982 -
Flags: review?(rcampbell) → review+
Updated•11 years ago
|
Whiteboard: [land-in-fx-team]
Assignee | ||
Comment 8•11 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/994dee02848d
Assignee: nobody → dcamp
Whiteboard: [land-in-fx-team] → [fixed-in-fx-team]
Comment 9•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/994dee02848d
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 24
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•