Closed
Bug 1107682
Opened 6 years ago
Closed 6 years ago
Clean up the way we set breakpoints on newly introduced scripts
Categories
(DevTools :: Debugger, defect)
Tracking
(firefox36 fixed, firefox37 fixed, firefox38 fixed)
RESOLVED
FIXED
Firefox 38
People
(Reporter: ejpbruel, Assigned: ejpbruel)
References
Details
Attachments
(2 files, 1 obsolete file)
7.15 KB,
patch
|
fitzgen
:
review+
|
Details | Diff | Splinter Review |
5.67 KB,
patch
|
Sylvestre
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
The way we currently set breakpoints on newly introduced scripts could use some improvement.
Assignee | ||
Updated•6 years ago
|
Assignee: nobody → ejpbruel
Assignee | ||
Comment 1•6 years ago
|
||
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 2•6 years ago
|
||
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-
Assignee | ||
Comment 3•6 years ago
|
||
Attachment #8532210 -
Attachment is obsolete: true
Attachment #8532320 -
Flags: review?(nfitzgerald)
Updated•6 years ago
|
Attachment #8532320 -
Flags: review?(nfitzgerald) → review+
https://hg.mozilla.org/mozilla-central/rev/e3cc96211c46
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 37
Assignee | ||
Comment 5•6 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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 6•6 years ago
|
||
Try run for patch with comments addressed: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=47e45c72f248
Assignee | ||
Comment 7•6 years ago
|
||
Try run looks green, pushing to fx-team: https://hg.mozilla.org/integration/fx-team/rev/29687660a627
Comment 8•6 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/29687660a627
Status: REOPENED → RESOLVED
Closed: 6 years ago → 6 years ago
Resolution: --- → FIXED
Comment 9•6 years ago
|
||
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)
Updated•6 years ago
|
status-firefox36:
--- → affected
status-firefox37:
--- → fixed
status-firefox38:
--- → fixed
Target Milestone: Firefox 37 → Firefox 38
Comment 10•6 years ago
|
||
Comment 11•6 years ago
|
||
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?
Comment 12•6 years ago
|
||
could you push it on try first ? thanks
Comment 13•6 years ago
|
||
I was just working on that, it's here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d63b4120f18e
Comment 14•6 years ago
|
||
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.
Updated•6 years ago
|
Attachment #8558579 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Updated•3 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•