901271 - Disabled breakpoints are lost on page reload

Status: RESOLVED FIXED

(DevTools :: Debugger, defect, P2)

Description

[Girish Sharma [:Optimizer]]

Using latest nightly ,

1) go to http://htmlpad.org/debugger/
2) Put breakpoints on line 23 and 33.
3) hit "Click Me" and ensure both breakpoints are hit.
4) Disable the breakpoint of line 23.
5) hit "Click me" again to see that breakpoint on line 23 is not being hit.
6) Reload the page.
7) The source list panel has only 1 breakpoint that was added on line 33.

expected : both breakpoints are preserved, lien 23 one being disabled.

Comment 1

[Victor Porof [:vporof][:vp]]

This is because disabled breakpoints are actually removed from the server, but preserved in the UI (in the sources list). When a page is refreshed, the sources list is emptied, and there are no breakpoints on the server, so, yeah...

Ideally, disabling breakpoints wouldn't be a synonym to removing them. It'd be nice if we had a disableBreakpoint packet that we could send to breakpoint actors.

What do you think, Nick?

Comment 2

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Sounds good to me. Jim, what do you think?

Comment 3

[Victor Porof [:vporof][:vp]]

Attachment: Part 1: Remove actor id dependencies from the debugger breakpoints view

Comment 4

[Victor Porof [:vporof][:vp]]

Attachment: Part 2: Preserve disabled breakpoints and re-add them in the views after a target navigation

This does the trick. I'd implement new tests if I were able to run them :(

Comment 5

[Victor Porof [:vporof][:vp]]

Attachment: Tests for part 1

Comment 6

[Victor Porof [:vporof][:vp]]

Attachment: Tests for part 2

Comment 7

[Victor Porof [:vporof][:vp]]

Attachment: Actual tests for part 2

Wrong patch lolol.

Comment 8

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 803941 [details] [diff] [review]
Part 1: Remove actor id dependencies from the debugger breakpoints view

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

Looks good to me. Please upload diffs with 8 lines of context in the future.

::: browser/devtools/debugger/debugger-panes.js
@@ +139,5 @@
>     * @param object aOptions [optional]
>     *        @see DebuggerController.Breakpoints.addBreakpoint
>     */
>    addBreakpoint: function(aBreakpointData, aOptions = {}) {
> +    let location = aBreakpointData.location;

let { location } = aBreakpointData;

is shorter, if you like ;)

Comment 9

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 803943 [details] [diff] [review]
Part 2: Preserve disabled breakpoints and re-add them in the views after a target navigation

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

::: browser/devtools/debugger/debugger-controller.js
@@ +1591,5 @@
>     * @param object aBreakpointData
>     *        Information about the breakpoint to be shown.
>     *        This object must have the following properties:
>     *          - location: the breakpoint's source location and line number
> +   *          - state: the breakpoint's state, either "enabled" or "disabled"

Do we expect to eventually have more states? If not, I'd prefer having an "enabled" property that is a boolean to a "state" property that is one of two strings. When you have a string property that is supposed to be one of two strings, you open up the gates for bugs where `state !== "enabled" && state !== "disabled"`.

I'm willing to be convinced otherwise, but at this point I'd ask you to change to a boolean "enabled" property before commiting.

@@ +1642,5 @@
> +   * Gets all Promises for the BreakpointActor client objects that are
> +   * either enabled (added to the server) or disabled (removed from the server,
> +   * but for which some details are preserved).
> +   */
> +  get _addedOrDisabled() {

This could be a generator, if you like.

::: browser/devtools/debugger/debugger-panes.js
@@ +134,5 @@
>     * @param object aBreakpointData
>     *        Information about the breakpoint to be shown.
>     *        This object must have the following properties:
>     *          - location: the breakpoint's source location and line number
> +   *          - state: the breakpoint's state, either "enabled" or "disabled"

ditto

Comment 10

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 804068 [details] [diff] [review]
Actual tests for part 2

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

::: browser/devtools/debugger/test/browser_dbg_breakpoints-disabled-reload.js
@@ +83,5 @@
> +        yield ensureSourceIs(aPanel, "-02.js");
> +        yield verifyView({ disabled: false, visible: false });
> +
> +        // Spin the event loop before causing the debuggee to pause, to allow
> +        // this function to yield first.

Redundant comment; you have a larger comment above these functions that says this.

@@ +100,5 @@
> +        yield ensureSourceIs(aPanel, "-02.js");
> +        yield verifyView({ disabled: true, visible: false });
> +
> +        // Spin the event loop before causing the debuggee to pause, to allow
> +        // this function to yield first.

Redundant comment.

Comment 11

[Victor Porof [:vporof][:vp]]

Try: https://tbpl.mozilla.org/?tree=Try&rev=9f30d1d35566

Comment 12

[Victor Porof [:vporof][:vp]]

(In reply to Nick Fitzgerald [:fitzgen] from comment #10)
> Comment on attachment 804068 [details] [diff] [review]

Thanks for the reviews. Agreed with all comments.

Comment 13

[Victor Porof [:vporof][:vp]]

Attachment: Part 2, addressed comments

Comment 14

[Victor Porof [:vporof][:vp]]

Attachment: Tests for part 2, addressed comments

Comment 15

[Victor Porof [:vporof][:vp]]

https://hg.mozilla.org/integration/fx-team/rev/c5c140cca0cf
https://hg.mozilla.org/integration/fx-team/rev/87860bf00124
https://hg.mozilla.org/integration/fx-team/rev/851e60b68784
https://hg.mozilla.org/integration/fx-team/rev/7ec66af65872

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c5c140cca0cf
https://hg.mozilla.org/mozilla-central/rev/87860bf00124
https://hg.mozilla.org/mozilla-central/rev/851e60b68784
https://hg.mozilla.org/mozilla-central/rev/7ec66af65872