1169884 - Pressing 'Clear' button when recording via call to console.profile leaves little indication that a recording is still ongoing

Status: RESOLVED FIXED

(DevTools :: Performance Tools (Profiler/Timeline), defect)

Description

[Sami Jaktholm]

STR:
1. Open console and run: console.profile("a")
2. Go to performance tab.
3. Press the 'Clear' button.

What happens: All clues that a recording is running are removed from the toolbox. The only thing left is the green performance tab if you change to another tool.

What should happen: It should do something more sensible such as:
a) The pending recording is stopped as happens to those started from the tool itself
b) The pending recording is not removed from the tool leaving the information about the recording to the UI.
c) Indicate in a clear way that there's a recording running and how to stop it even though it's not presented in the UI.

Comment 1

[Jordan Santell [:jsantell] [@jsantell]]

Hm, "Clear" should probably not clear out console recordings, as devtools shouldn't really be in charge of calling `console.profileEnd` for several reasons, and this would still reflect the current state of the recordings

Comment 2

[Andrew Krawchyk]

(In reply to Jordan Santell [:jsantell] [@jsantell] from comment #1)
> Hm, "Clear" should probably not clear out console recordings, as devtools
> shouldn't really be in charge of calling `console.profileEnd` for several
> reasons, and this would still reflect the current state of the recordings

I can try to take this one, where should I get started?

Comment 3

[Jordan Santell [:jsantell] [@jsantell]]

In devtools/client/performance/peformance-controller.js, there's a function, `clearRecordings` that empties out all recordings, and stops any in progress manual recordings. We should only clear out manual recordings, and console recordings that are done -- in progress console recordings, we should not remove from the internal `this._recordings` array. Let me know if you have any questions!

Comment 4

[Jomy Emmanuel [:je_007]]

(In reply to Jordan Santell [:jsantell] [@jsantell] from comment #3)
> In devtools/client/performance/peformance-controller.js, there's a function,
> `clearRecordings` that empties out all recordings, and stops any in progress
> manual recordings. We should only clear out manual recordings, and console
> recordings that are done -- in progress console recordings, we should not
> remove from the internal `this._recordings` array. Let me know if you have
> any questions!

Can I also work on this bug? As of now, I have made a minor change in code wherein the 'this._recordings' array is cleared only if the console recording is completed else that array is left untouched. However, I feel, the UI part also needs to be modified as  Sami Jaktholm had mentioned. What do you suggest?

Comment 5

[Andrew Krawchyk]

(In reply to Jomy [:je_007] from comment #4)
> (In reply to Jordan Santell [:jsantell] [@jsantell] from comment #3)
> > In devtools/client/performance/peformance-controller.js, there's a function,
> > `clearRecordings` that empties out all recordings, and stops any in progress
> > manual recordings. We should only clear out manual recordings, and console
> > recordings that are done -- in progress console recordings, we should not
> > remove from the internal `this._recordings` array. Let me know if you have
> > any questions!
> 
> Can I also work on this bug? As of now, I have made a minor change in code
> wherein the 'this._recordings' array is cleared only if the console
> recording is completed else that array is left untouched. However, I feel,
> the UI part also needs to be modified as  Sami Jaktholm had mentioned. What
> do you suggest?

Yes, I've been distracted with another bug, please feel free to take it.

Comment 6

[Jordan Santell [:jsantell] [@jsantell]]

Thanks Jomy and Andrew!

Jomy, the recordings view (devtools/client/performance/views/recording.js) listens to the RECORDINGS_CLEARED event and handles in onRecordingsCleared and empties itself out. In this case, clearing all recordings is no longer clearing everything, but a select few. Solutions:

1) onRecordingsCleared searches through remaining recordings and does not remove in progress console recordings. This duplicates logic with what was deleted, and seems fragile.

2) Pass the recordings that are remaining in PerformanceController's clearRecordings to the event that's emitted, so the recording view can remove all UI components for recordings that are no longer remaining. Or maybe pass in the recordings that are being removed, which makes more sense based off of the event/function names.

There are a handful of tests for clearing recordings, so be sure to run the tests when changing things!

Comment 7

[Jomy Emmanuel [:je_007]]

Thanks Andrew and Jordan. Since this is the first bug I am working on, it might take me a while to get it fixed.

Comment 8

[Jomy Emmanuel [:je_007]]

Jordan, I have modified the code as you had mentioned in the second step of comment #6. The completed recordings get removed from the UI. But I have one doubt, Should the in progress recordings be stopped on clicking the clear button or should it just continue recording?

Comment 9

[Jomy Emmanuel [:je_007]]

(In reply to Jomy [:je_007] from comment #8)
> Jordan, I have modified the code as you had mentioned in the second step of
> comment #6. The completed recordings get removed from the UI. But I have one
> doubt, Should the in progress recordings be stopped on clicking the clear
> button or should it just continue recording?

In both cases, as mentioned above, there is a little issue. Suppose, the UI contains a completed recording and an in progress recording. On clicking the clear button, the completed recording gets removed, leaving only the in progress recording in the UI. In such a case, the graph of the in progress recording on the right side disappears and can be viewed if we switch between recordings, i.e, start a new recording manually and switch back to the old one.

Comment 10

[Jomy Emmanuel [:je_007]]

I have managed to fix the issue that I had discussed in comment #9. Right now, on clicking the clear button, completed recordings get removed, and in progress recordings which get stopped, remains in the UI. Only on a second click of the clear button will those (stopped) in progress recordings get removed from the UI. But there is an issue. If there are 2 recordings, one completed (Recording #1) and one in progress recording (Recording #2), on clicking the clear button, the completed recording (Recording #1) gets removed and the in progress recording (Recording #2) remains in the UI. However, if I were to start another recording manually via the console, that new recording would also be assigned Recording #2. Could you please tell me a basic approach on how to fix this issue. I think it has something to do with itemCount of addEmptyRecording function in recording.js.

Comment 11

[Jordan Santell [:jsantell] [@jsantell]]

Hey Jomy, sorry it's taken me a few to respond -- please needinfo me in the future so I'm sure to see!

Currently, in progress recordings are stopped if hitting "clear" -- the part that this bug should handle is it should leave in progress console recordings (triggered via console.profile()). I'm not sure what the issue is with a new recording overwriting a previously deleted recording without code -- can you attach what you have so far?

Comment 12

[Jomy Emmanuel [:je_007]]

Attachment: On clicking the clear button, completed recordings get removed and in progress recordings remain in the console.

Yes, the in progress console recordings (triggered via console.profile()) remain in the UI; in progress console recordings (triggered manually via the console)  also remain in the UI, get stopped and completed recordings are removed from the UI. The issue I had discussed in comment #10 was a numbering issue.

Comment 13

[Jomy Emmanuel [:je_007]]

sorry, this is what I really meant to say:
On clicking the 'clear' button:
1. In progress console recordings (triggered via console.profile()) remain in the UI (they do not get stopped).
2.  In progress console recordings (triggered via performance tab) remain in the UI (they get stopped).
3. Completed recordings get removed from the UI.

Comment 14

[Jomy Emmanuel [:je_007]]

Attachment: bug_1169884_clearbuttonfixchange2.diff

The 1st attachment does what I mentioned in comment #13. I think this is attachent that you were looking for. It does the following:
On clicking the 'clear' button:
1. In progress console recordings (triggered via console.profile()) remain in the UI (they do not get stopped).
2. In progress console recordings (triggered via performance tab) after getting stopped, are removed from the UI.
3. Completed recordings get removed from the UI.

Comment 15

[Jordan Santell [:jsantell] [@jsantell]]

(In reply to Jomy [:je_007] from comment #13)
> sorry, this is what I really meant to say:
> On clicking the 'clear' button:
> 1. In progress console recordings (triggered via console.profile()) remain
> in the UI (they do not get stopped).
> 2.  In progress console recordings (triggered via performance tab) remain in
> the UI (they get stopped).
> 3. Completed recordings get removed from the UI.

1 is the desired behavior -- we shouldn't stop in progress console recordings.
2 (i assume is normal recording, not "console", as it's not triggered via console.profile()) should stop when clearing -- this is what the current behavior is as well.
3 All completed recordings (console or not) should be removed, correct

Sorry it's taken me a bit to check out the patch -- can you combine future changes into the same patch? I had to apply several to get the correct state :) This looks good though! Would you like to write tests for this, or I can check it out?

Comment 16

[Jomy Emmanuel [:je_007]]

Attachment: bug_1169884_clearbuttonfixfinal.diff

Sorry to mess up with those patches. I have attached a new patch that combines changes contained within those two patches. Please look through it. The previous two attachments have been marked obsolete and need not be considered.

I am interested in writing tests. How do I proceed?

Comment 17

[Jordan Santell [:jsantell] [@jsantell]]

Comment on attachment 8687518 [details] [diff] [review]
bug_1169884_clearbuttonfixfinal.diff

Review of attachment 8687518 [details] [diff] [review]:
-----------------------------------------------------------------

Some comments below. For the tests, there are some that probably need updated:

browser_perf-clear-01.js
browser_perf-clear-02.js
browser_perf-console-record-09.js

And then we should add another test that ensures in progress console recordings are not removed

::: devtools/client/performance/performance-controller.js
@@ +311,4 @@
>     * Emits `EVENTS.RECORDINGS_CLEARED` when complete so other components can clean up.
>     */
>    clearRecordings: Task.async(function* () {
> +    for(let i = this._recordings.length - 1; i>=0; i--) {

spacing, and rewriting:
`for (let model of this._recordings) {`

@@ +312,5 @@
>     */
>    clearRecordings: Task.async(function* () {
> +    for(let i = this._recordings.length - 1; i>=0; i--) {
> +      let model = this._recordings[i];
> +      if (!model.isConsole() && !model.isImported()) {

We can make this more straight forward;

if isRecording and !isConsole:
  stop recording

if !isRecording and !isCompleted:
  wait for recording to finish finalizing and then clear it, both console and non console

if isCompleted
  clear it

@@ +325,5 @@
> +      }
> +      // If recording is completed,
> +      // clean it up from UI and remove it from the _recordings array.
> +      if (model && model.isCompleted()) {
> +        this.setCurrentRecording(null);

We should set current recordings outside of this loop; if our current recording is no longer available, then set it to the first available recording. If there are no recordings available, then we can set to `null`.

@@ +326,5 @@
> +      // If recording is completed,
> +      // clean it up from UI and remove it from the _recordings array.
> +      if (model && model.isCompleted()) {
> +        this.setCurrentRecording(null);
> +        this.emit(EVENTS.RECORDINGS_CLEARED, model);

Now that this event only handles one model at a time, we should change this to RECORDING_DELETED

::: devtools/client/performance/views/recordings.js
@@ +147,2 @@
>     */
> +  _onRecordingsCleared: function (_, recording) {

This should now listen to a RECORDING_DELETED event, and update the handler name as well

Comment 18

[Jomy Emmanuel [:je_007]]

Attachment: Clear Button Fix With Tests

1. I am not sure what you intended by 'for (let model of this._recordings) {` because 'this._recordings.splice(i, 1)' method removes an item from the array, and modifies the array. So the array length decreases by one.
2. Of these tests,
   browser_perf-clear-01.js
   browser_perf-clear-02.js
   browser_perf-console-record-09.js
--> I have modified 'browser_perf-clear-02.js' so that it listens to the event RECORDING_DELETED.
--> 'browser_perf-console-record-09.js' has also been modified and checks for length of array and current selection after console.profile() has been called and before console.profileEnd() has been called.
Please look through the attachment. Please also tell me if there are other kinds of tests we could do.

Comment 19

[Jomy Emmanuel [:je_007]]

I have also changed the function name 'onRecordingsCleared' to 'onRecordingDeleted' in the attachment #8688640 [details] [diff] [review] I had attached in comment #18 though I am not sure about this change. Please refer comment #18.

Comment 20

[Jordan Santell [:jsantell] [@jsantell]]

Comment on attachment 8688640 [details] [diff] [review]
Clear Button Fix With Tests

Review of attachment 8688640 [details] [diff] [review]:
-----------------------------------------------------------------

Thank you for taking the time to write this and the tests as well, this is looking great! Just some small comments on the tests to cover a few more cases and I think we're good to land this

::: devtools/client/performance/performance-controller.js
@@ +332,3 @@
>      }
> +    if (this._recordings.length > 0) {
> +      if (!(this.getCurrentRecording() in this._recordings)) {

The `in` operator checks the existence of a property of an object, so this won't do what we want -- we can write this as `if (!this._recordings.includes(this.getCurrentRecording())) {`

::: devtools/client/performance/test/browser_perf-clear-02.js
@@ +16,5 @@
>    yield startRecording(panel);
>  
>    let stopped = Promise.all([
>      once(PerformanceController, EVENTS.RECORDING_STOPPED),
> +    once(PerformanceController, EVENTS.RECORDING_DELETED)

We should ensure that this is called twice since this will be fired for both recordings:

let deleted = Promise.defer();
let deleteCount = 0;
function onDeleted () {
  if (++deleteCount === 2) {
    deleted.resolve();
    PerformanceController.off(EVENTS.RECORDING_DELETED, onDeleted);
  }
}

PerformanceController.on(EVENTS.RECORDING_DELETED, onDeleted);

let stopped = Promise.all([
  once(PerformanceController, EVENTS.RECORDING_STOPPED),
  deleted.promise
]);

::: devtools/client/performance/test/browser_perf-console-record-09.js
@@ +5,2 @@
>   * Tests that an error is not thrown when clearing out the recordings if there's
>   * an in-progress console profile.

Also add: "and that console profiles are not cleared if in progress"

@@ +14,1 @@
>  

We should record a non profile recording here for later:

yield startRecording(panel);
yield stopRecording(panel);

@@ +17,5 @@
>    yield PerformanceController.clearRecordings();
> +  let recordings = PerformanceController.getRecordings();
> +  is(recordings.length, 1, "1 recording found");
> +  is(RecordingsView.selectedItem.attachment, recordings[0],
> +    "The first console recording should be selected.");

So this will now confirm that the in-progress console recording persists, AND that the non-console profile was removed correctly

@@ +23,5 @@
>    consoleMethod("profileEnd");
>    // Wait for the front to receive the stopped event
>    yield once(gFront, "recording-stopped");
>  
>    // Wait an extra tick or two since the above promise will be resolved

At this point, we should clear recordings again and ensure that we have 0 recordings, which will cover all the scenarios for this bug

Comment 21

[Jordan Santell [:jsantell] [@jsantell]]

Comment on attachment 8688640 [details] [diff] [review]
Clear Button Fix With Tests

Review of attachment 8688640 [details] [diff] [review]:
-----------------------------------------------------------------

::: devtools/client/performance/performance-controller.js
@@ +326,5 @@
> +      // If recording is completed,
> +      // clean it up from UI and remove it from the _recordings array.
> +      if (model.isCompleted()) {
> +        this.emit(EVENTS.RECORDING_DELETED, model);
> +        this._recordings.splice(i, 1);

You're right -- removing the element while iterating over the array will mean we skip some elements -- your while loop solution from before will be better here!

::: devtools/client/performance/views/recordings.js
@@ +16,5 @@
>      this._onSelect = this._onSelect.bind(this);
>      this._onRecordingStateChange = this._onRecordingStateChange.bind(this);
>      this._onNewRecording = this._onNewRecording.bind(this);
>      this._onSaveButtonClick = this._onSaveButtonClick.bind(this);
> +    this._onRecordingDeleted = this._onRecordingDeleted.bind(this);

This is a fine method name now that the event name changed. Thanks!

Comment 22

[Jomy Emmanuel [:je_007]]

Attachment: bug_1169884_Clear_Button_Fix_With_Tests.diff

Thanks for the suggestions. I ran a browser chrome test which successfully passed :)

Comment 23

[Jordan Santell [:jsantell] [@jsantell]]

Comment on attachment 8690246 [details] [diff] [review]
bug_1169884_Clear_Button_Fix_With_Tests.diff

Review of attachment 8690246 [details] [diff] [review]:
-----------------------------------------------------------------

Looks great! Last thing is, we need the bug number in the commit message, as well as the reviewer, so something like: "Bug 1169884 - Clearing recordings no longer clears in progress console recordings. r=jsantell"

I'll send this to our CI so we can run the tests on all the platforms, and go from there. Thanks so much!

::: devtools/client/performance/test/browser_perf-console-record-09.js
@@ +17,4 @@
>  
>    info("Starting console.profile()...");
>    yield consoleProfile(win);
> +  

nit: white space

@@ +21,5 @@
>    yield PerformanceController.clearRecordings();
> +  let recordings = PerformanceController.getRecordings();
> +  is(recordings.length, 1, "1 recording found");
> +  is(recordings[0].isConsole(), true, "recording from console.profile is not cleared.");
> +  

nit: white space

Comment 24

[Jomy Emmanuel [:je_007]]

Attachment: bug 1169884 Clear Button Fix With Tests

I have removed the white spaces and added the commit message. Thanks for having mentored me :) Though it took me a while, I have learned a lot. This was my first attempt in fixing a bug and I am a lot more excited to contribute even more to Mozilla.

Comment 25

[Jomy Emmanuel [:je_007]]

Attachment: bug 1169884 Clear Button Fix With Tests

I have removed 'RecordingsView' from  'let { gFront, PerformanceController, RecordingsView } = win;' (devtools/client/performance/test/browser_perf-console-record-09.js), the only change from my previous attachment, since it is no longer necessary. Once again, thanks!

Comment 26

[Jordan Santell [:jsantell] [@jsantell]]

Comment on attachment 8690456 [details] [diff] [review]
bug 1169884 Clear Button Fix  With Tests

Review of attachment 8690456 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for all the work, Jomy, and always glad to help! Running tests on our CI now, if everything is looking good, we'll get this landed -- thanks again!

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6df5a0925a72

Comment 27

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/e145a1dcd0b3

Comment 28

[Jordan Santell [:jsantell] [@jsantell]]

Sorry it's taken so long to land this, Jomy, busy week! Thank you so much for the patch -- your work should be available in Firefox 45 nightly in a day or two

Comment 29

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e145a1dcd0b3

Comment 30

[Robert Helmer [:rhelmer]]

Attachment: MozReview Request: Bug 1169884 - Clearing recordings no longer clears in progress console recordings. r=jsantell

Bug 1169884 - Clearing recordings no longer clears in progress console recordings. r=jsantell

Comment 31

[Robert Helmer [:rhelmer]]

Attachment: MozReview Request: bug 1228792 - remove use of array comprehensions r?mossop

bug 1228792 - remove use of array comprehensions r?mossop

Comment 32

[Robert Helmer [:rhelmer]]

Attachment: MozReview Request: bug 1228792 - remove addons manager from eslint ignore r?mossop

bug 1228792 - remove addons manager from eslint ignore r?mossop

Comment 33

[Robert Helmer [:rhelmer]]

Attachment: MozReview Request: bug 1228792 - use standard version of catch r?mossop

bug 1228792 - use standard version of catch r?mossop

Comment 34

[Robert Helmer [:rhelmer]]

Attachment: MozReview Request: bug 1228792 - use function* for generators r?mossop

bug 1228792 - use function* for generators r?mossop

Comment 35

[Robert Helmer [:rhelmer]]

Attachment: MozReview Request: bug 1228792 - remove leading 0, be explicit about octals r?mossop

bug 1228792 - remove leading 0, be explicit about octals r?mossop

Comment 36

[Jordan Santell [:jsantell] [@jsantell]]

I'm not sure what these reviews are doing here

Comment 37

[Robert Helmer [:rhelmer]]

(In reply to Jordan Santell [:jsantell] [@jsantell] (Please needinfo) from comment #36)
> I'm not sure what these reviews are doing here

My fault - I accidentally got one of the commits from this bug into my set of changes for bug 1228792. Sorry about that!