Disabled breakpoints are lost on page reload

RESOLVED FIXED in Firefox 26

Status

()

Firefox
Developer Tools: Debugger
P2
normal
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: Optimizer, Assigned: vporof)

Tracking

unspecified
Firefox 26
x86_64
Windows 7
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(6 attachments, 1 obsolete attachment)

(Reporter)

Description

4 years ago
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.
Assignee: nobody → nfitzgerald
Priority: -- → P2
(Assignee)

Comment 1

4 years ago
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?
Sounds good to me. Jim, what do you think?
Flags: needinfo?(jimb)
(Assignee)

Updated

4 years ago
Assignee: nfitzgerald → vporof
Status: NEW → ASSIGNED
Flags: needinfo?(jimb)
(Assignee)

Comment 3

4 years ago
Created attachment 803941 [details] [diff] [review]
Part 1: Remove actor id dependencies from the debugger breakpoints view
(Assignee)

Comment 4

4 years ago
Created attachment 803943 [details] [diff] [review]
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 :(
(Assignee)

Comment 5

4 years ago
Created attachment 804063 [details] [diff] [review]
Tests for part 1
Attachment #804063 - Flags: review?(nfitzgerald)
(Assignee)

Comment 6

4 years ago
Created attachment 804064 [details] [diff] [review]
Tests for part 2
Attachment #804064 - Flags: review?(nfitzgerald)
(Assignee)

Updated

4 years ago
Attachment #803941 - Flags: review?(nfitzgerald)
(Assignee)

Updated

4 years ago
Attachment #803943 - Flags: review?(nfitzgerald)
(Assignee)

Comment 7

4 years ago
Created attachment 804068 [details] [diff] [review]
Actual tests for part 2

Wrong patch lolol.
Attachment #804064 - Attachment is obsolete: true
Attachment #804064 - Flags: review?(nfitzgerald)
Attachment #804068 - Flags: review?(nfitzgerald)
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 ;)
Attachment #803941 - Flags: review?(nfitzgerald) → review+
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
Attachment #803943 - Flags: review?(nfitzgerald) → review+
Attachment #804063 - Flags: review?(nfitzgerald) → review+
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.
Attachment #804068 - Flags: review?(nfitzgerald) → review+
(Assignee)

Comment 11

4 years ago
Try: https://tbpl.mozilla.org/?tree=Try&rev=9f30d1d35566
(Assignee)

Comment 12

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

Thanks for the reviews. Agreed with all comments.
(Assignee)

Comment 13

4 years ago
Created attachment 804274 [details] [diff] [review]
Part 2, addressed comments
(Assignee)

Comment 14

4 years ago
Created attachment 804275 [details] [diff] [review]
Tests for part 2, addressed comments
(Assignee)

Comment 15

4 years ago
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
Whiteboard: [fixed-in-fx-team]
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
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 26
You need to log in before you can comment on or make changes to this bug.