Clean up the way we set breakpoints on newly introduced scripts

RESOLVED FIXED in Firefox 36



Developer Tools: Debugger
3 years ago
a year ago


(Reporter: ejpbruel, Assigned: ejpbruel)


Firefox 38
Mac OS X

Firefox Tracking Flags

(firefox36 fixed, firefox37 fixed, firefox38 fixed)



(2 attachments, 1 obsolete attachment)



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


3 years ago
Assignee: nobody → ejpbruel

Comment 1

3 years ago
Created attachment 8532210 [details] [diff] [review]
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.
Attachment #8532210 - Flags: review?(nfitzgerald)
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`.
Attachment #8532210 - Flags: review?(nfitzgerald) → review-

Comment 3

3 years ago
Created attachment 8532320 [details] [diff] [review]
Clean up how we set breakpoints on newly introduced scripts (v2)
Attachment #8532210 - Attachment is obsolete: true
Attachment #8532320 - Flags: review?(nfitzgerald)
Attachment #8532320 - Flags: review?(nfitzgerald) → review+
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 37

Comment 5

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

Comment 6

3 years ago
Try run for patch with comments addressed:

Comment 7

3 years ago
Try run looks green, pushing to fx-team:
Last Resolved: 3 years ago3 years ago
Resolution: --- → FIXED
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)
status-firefox36: --- → affected
status-firefox37: --- → fixed
status-firefox38: --- → fixed
Target Milestone: Firefox 37 → Firefox 38
Created attachment 8558579 [details] [diff] [review]
Comment on attachment 8558579 [details] [diff] [review]

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.
Attachment #8558579 - Flags: approval-mozilla-beta?
could you push it on try first ? thanks
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.
Attachment #8558579 - Flags: approval-mozilla-beta? → approval-mozilla-beta+


a year ago
See Also: → bug 1327974
You need to log in before you can comment on or make changes to this bug.