Clean up the way we set breakpoints on newly introduced scripts

RESOLVED FIXED in Firefox 36

Status

defect
RESOLVED FIXED
5 years ago
Last year

People

(Reporter: ejpbruel, Assigned: ejpbruel)

Tracking

unspecified
Firefox 38
x86
macOS
Points:
---

Firefox Tracking Flags

(firefox36 fixed, firefox37 fixed, firefox38 fixed)

Details

Attachments

(2 attachments, 1 obsolete attachment)

The way we currently set breakpoints on newly introduced scripts could use some improvement.
Assignee: nobody → ejpbruel
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-
Attachment #8532210 - Attachment is obsolete: true
Attachment #8532320 - Flags: review?(nfitzgerald)
Attachment #8532320 - Flags: review?(nfitzgerald) → review+
https://hg.mozilla.org/mozilla-central/rev/e3cc96211c46
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 37
This patch actually never landed. The patch in comment 4 is for a different bug, and I messed up the bug number :-S
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Try run for patch with comments addressed:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=47e45c72f248
Try run looks green, pushing to fx-team:
https://hg.mozilla.org/integration/fx-team/rev/29687660a627
https://hg.mozilla.org/mozilla-central/rev/29687660a627
Status: REOPENED → RESOLVED
Closed: 5 years ago5 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)
Target Milestone: Firefox 37 → Firefox 38
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.
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+
See Also: → 1327974
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.