1574190 - Add server support for setting watchpoints

Status: RESOLVED FIXED

(DevTools :: Debugger, enhancement, P1)

Description

[Jason Laster [:jlast]]

The server should have endpoint for adding and removing (get|set) watchpoints

Comment 1

[Miriam]

Attachment: Add server support for setting watchpoints.

Add server support for watchpoints

Comment 2

[Miriam]

Attachment: Add server support for watchpoints.

Lint and add test.

Add server support for watchpoints.

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8e22b66d70f9
Add server support for watchpoints. r=jlast

Comment 4

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/8e22b66d70f9

Comment 5

[Miriam]

Attachment: Add watchpoint as a trait in the server, remove watchpoints when object actor is released, add watchpoint to property descriptor object

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b0a5b50962e7
Add watchpoint as a trait in the server, remove watchpoints when object actor is released, add watchpoint to property descriptor object r=jlast

Comment 7

[Natalia Csoregi [:nataliaCs]]

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

Comment 8

[Miriam]

Attachment: Add watchpoints trait to browsingContextTargetPrototype and remove from RootActor

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/cc3c6d62c9e6
Add watchpoints trait to browsingContextTargetPrototype and remove from RootActor r=jlast

Comment 10

[Raul Gurzau (:RaulG)]

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

Comment 11

[Jason Laster [:jlast]]

Could we uplift the first two commits?

1. https://hg.mozilla.org/mozilla-central/rev/8e22b66d70f9
2. https://hg.mozilla.org/mozilla-central/rev/b0a5b50962e7

Comment 12

[Jason Laster [:jlast]]

Attachment: commits to uplift

Beta/Release Uplift Approval Request

• User impact if declined: the vscode debugger will not have support for watchpoints for another cycle
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): the server code is not executed unless a watchpoint is added
• String changes made/needed:

Comment 13

[Liz Henry (:lizzard) (relman/hg->git project)]

Do we have a reason to rush (part of) this to release? Conversely would letting it stay in nightly for a while help uncover potential bugs? Is there any testing/QA we can do around this feature?

Comment 14

[Liz Henry (:lizzard) (relman/hg->git project)]

Also noticing that some of this landed in 70 nightly (so it's already marked as "fixed" for 70, which is likely not correct)

Comment 15

[Jason Laster [:jlast]]

We hope that this feature will be helpful for users who would like to debug firefox 70 from vs code.

Comment 16

[:Harald Kirschner :digitarald]

Do we have a reason to rush (part of) this to release?

VSCode's Firefox debugger is an extension developed off the train for VSCode, to which we added Watchpoint support. Given that some of the patches landed in 70, it seemed low-risk to uplift the remaining work.

Conversely would letting it stay in nightly for a while help uncover potential bugs?

Maybe, as we just landed UI for it in DevTools.

Is there any testing/QA we can do around this feature?

We planned to let Watchpoints be Nightly to collect feedback from internal users.

Comment 17

[Liz Henry (:lizzard) (relman/hg->git project)]

OK, I just found the planning info in the browser engineering board.
Why don't we go ahead and uplift for beta 7 (right at the end of "early beta") since this missed today's build.

Comment 18

[:Harald Kirschner :digitarald]

Thanks Liz, that sounds good! Is there anything else we need to update in this bug?

Comment 19

[BugBot [:suhaib / :marco/ :calixte]]

Changing the priority to p1 as the bug is tracked by a release manager for the current beta.
See What Do You Triage for more information

Comment 20

[Pascal Chevrel:pascalc]

Comment on attachment 9092119 [details]
commits to uplift

Baked 2 weeks in Nightly without reported regressions, covered by tests  and we are now past the "Early Beta" phase, uplift approved for 79 beta 9, thanks.

Comment 21

[Pascal Chevrel:pascalc]

Note to sheriffs for the beta uplift. The accepted uplift patch contains the message with the 2 revisions to uplift:

It'd be great to uplift these two commits: https://hg.mozilla.org/mozilla-central/rev/8e22b66d70f9 https://hg.mozilla.org/mozilla-central/rev/b0a5b50962e7

Comment 22

[Alexandru Michis [:malexandru]]

Jason, I'm getting the error "note: graft of 562003:8e22b66d70f9 created no changes to commit" when using graft to uplift the patch to beta and conflicts when I tried importing the patches with moz-phab.

Comment 23

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-beta/rev/87c8abc9206b

Comment 24

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

The first revision landed for Gecko 70 and didn't need uplift.