1107682 - Clean up the way we set breakpoints on newly introduced scripts

Status: RESOLVED FIXED

(DevTools :: Debugger, defect)

Description

[Eddy Bruel [:ejpbruel]]

The way we currently set breakpoints on newly introduced scripts could use some improvement.

Comment 1

[Eddy Bruel [:ejpbruel]]

Attachment: Clean up how we set breakpoints on newly introduced scripts

This patch removes the need for the aOnlyThisScript parameter in setBreakpoint, by checking if the BreakpointActor has already been set as a breakpoint handler for each script.

Comment 2

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

Comment on attachment 8532210 [details] [diff] [review]
Clean up how we set breakpoints on newly introduced scripts

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

::: toolkit/devtools/server/actors/script.js
@@ +1949,5 @@
>  
>      // |onNewScript| is only fired for top level scripts (AKA staticLevel == 0),
>      // so we have to make sure to call |_addScript| on every child script as
>      // well to restore breakpoints in those scripts.
> +    // well to restore breakpoints in those scripts.

remove accidental dupe line

@@ +2887,5 @@
>    },
>  
>    /**
> +   * Get or create a BreakpointActor for the given location, and set it as a
> +   * breakpoint handler on all scripts that match the given location for which 

nit: delete trailing white space.

@@ +2893,5 @@
> +   *
> +   * It is possible that no scripts match the given location, because they have
> +   * all been garbage collected. In that case, the BreakpointActor is not set as
> +   * a breakpoint handler for any script, but is still inserted in the
> +   * breakpoint cache as a pending breakpoint. Whenever a new script is

Nit: "breakpoint cache" -> "breakpoint store" or "breakpoint actor map" depending on whether those other patches have landed yet.

@@ +2902,5 @@
> +   * we check if any of these scripts has any entry points for the given
> +   * location. If not, we assume that the given location does not have any code
> +   * (this is not completely accurate, because there may still be child scripts
> +   * that match the given location and have entry point for the given line, but
> +   * have been garbage collected, but it is the best we can do).

The parenthesized is incorrect.

@@ +2935,5 @@
>      });
>  
> +    // Ignore scripts for which the BreakpointActor is already a breakpoint
> +    // handler.
> +    scripts = scripts.filter((script) => !actor.hasScript(script));

Filtering the set of scripts can mess up our our line sliding, you need to consider all scripts, but actually set the BP iff `!actor.hasScript(script)` in `_setBreakpointOnEntryPoints`.

Comment 3

[Eddy Bruel [:ejpbruel]]

Attachment: Clean up how we set breakpoints on newly introduced scripts (v2)

Comment 4

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 5

[Eddy Bruel [:ejpbruel]]

This patch actually never landed. The patch in comment 4 is for a different bug, and I messed up the bug number :-S

Comment 6

[Eddy Bruel [:ejpbruel]]

Try run for patch with comments addressed:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=47e45c72f248

Comment 7

[Eddy Bruel [:ejpbruel]]

Try run looks green, pushing to fx-team:
https://hg.mozilla.org/integration/fx-team/rev/29687660a627

Comment 8

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/29687660a627

Comment 9

[James Long (:jlongster)]

It's worth noting that I'm uplifting this along with bug 1122064 to aurora (and then to beta). We need this to fix issues with deeply nested scripts not getting breakpoints activated. It's a pretty serious bug that made its way out to beta, unfortunately.

This patch is part of it because it removes the constraint on `setBreakpoint` that only sets the breakpoint on the specific script passed in. We need it to set breakpoints on all scripts for a specific line.

(I'm combining this patch with bug 1122064 since they go together and uplifting over there)

Comment 10

[James Long (:jlongster)]

Attachment: 1107682-beta-uplift.patch

Comment 11

[James Long (:jlongster)]

Comment on attachment 8558579 [details] [diff] [review]
1107682-beta-uplift.patch

Approval Request Comment
[Feature/regressing bug #]: unknown
[User impact if declined]: User can't set breakpoint on script nested 2 levels deep or higher. This is a serious regression that breaks many attempts to set breakpoints.
[Describe test coverage new/current, TreeHerder]: m-c and aurora have new tests for this
[Risks and why]: We are touching the debugger server, but there is no risk for normal users, only developers. Still, this is a rebased patch so there is some risk.
[String/UUID change made/needed]:


The changes here were merged and rebased with bug 1122064 to uplift to Aurora, and that has already happened. We thought we also need to uplift those same changes to Beta, but we actually just need to uplift this part of it.

I'd like to work closely with a sheriff (RyanVM?) on this, as I had to rebase and fix some conflicts, and I'm scared of putting a bad patch on Beta. We should watch tests closely after this lands (though all are passing for me).

I'm going to go ahead and push out a try as well.

Comment 12

[Sylvestre Ledru [:Sylvestre]]

could you push it on try first ? thanks

Comment 13

[James Long (:jlongster)]

I was just working on that, it's here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d63b4120f18e

Comment 14

[James Long (:jlongster)]

Try is green except for the SM(GGC) test but I was told that beta doesn't have the GGC build turned on and that's why it's failing . We should be good to go.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/484b4f09fa9f