Refactor breakpoint sliding for non-source mapped sources

RESOLVED FIXED in Firefox 39

Status

DevTools
Debugger
RESOLVED FIXED
3 years ago
a month ago

People

(Reporter: ejpbruel, Assigned: ejpbruel)

Tracking

unspecified
Firefox 39
x86
Mac OS X

Firefox Tracking Flags

(firefox39 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

3 years ago
Created attachment 8571999 [details] [diff] [review]
patch

This patch refactors the breakpoint sliding algorithm for non-source mapped sources. I tried to heavily comment what I'm doing and why, so the patch should be fairly self-explanatory.

The end goal here is to get rid of setBreakpointForActorAtLocationSliding (which used to be setBreakpointForActor). Once this patch lands, the only thing depending on that function will be breakpoint sliding for source mapped sources, which I plan to tackle next.
Attachment #8571999 - Flags: review?(jlong)
(Assignee)

Comment 1

3 years ago
Note that this patch is based on top of the one in bug 1131646.
Comment on attachment 8571999 [details] [diff] [review]
patch

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

A few nits, but this looks sane. (edit: oops I never actually published this)

::: toolkit/devtools/server/actors/script.js
@@ +2844,5 @@
> +        let lineToEntryPointsMap = [];
> +
> +        // Iterate over all scripts that correspond to this source actor.
> +        let scripts = this.scripts.getScriptsBySourceActor(this);
> +        for (let script of scripts) {

What do you think about putting this into its own helper function, like _scriptEntryPointsByLine?

Also, it seems like we could cache this, right? Not sure setting a breakpoint really needs to be that optimized though. Although it might speed up refreshing a page with breakpoint. Don't worry about caching for now.

@@ +2952,5 @@
> +      this,
> +      generatedLine
> +    ).filter((script) => !actor.hasScript(script));
> +
> +    // Find all entry points that correspond to the given location.

Seems like it'd be nice to make a few different "findEntryPoints" helper methods that would discover entry points in a few different ways. It would make it a little easier to read through the algorithm, and provide opportunities for caching later on.

@@ +3137,5 @@
>        }
>        actor.addScript(script, this.threadActor);
>      }
>  
> +    return;

What's the point of this return?
Attachment #8571999 - Flags: review?(jlong) → review+
(Assignee)

Comment 3

3 years ago
(In reply to James Long (:jlongster) from comment #2)
> Comment on attachment 8571999 [details] [diff] [review]
> patch
> 
> Review of attachment 8571999 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> A few nits, but this looks sane. (edit: oops I never actually published this)
> 
> ::: toolkit/devtools/server/actors/script.js
> @@ +2844,5 @@
> > +        let lineToEntryPointsMap = [];
> > +
> > +        // Iterate over all scripts that correspond to this source actor.
> > +        let scripts = this.scripts.getScriptsBySourceActor(this);
> > +        for (let script of scripts) {
> 
> What do you think about putting this into its own helper function, like
> _scriptEntryPointsByLine?
> 

It's not yet clear to me to what extent breakpoint code can/should be refactored. For that reason, I'd like to hold off from abstracting until the factor is complete.

> Also, it seems like we could cache this, right? Not sure setting a
> breakpoint really needs to be that optimized though. Although it might speed
> up refreshing a page with breakpoint. Don't worry about caching for now.
> 

I doubt that would be worthwhile. Since setting a breakpoint is something that involves a user interaction 99% of the time, as long as the total interaction takes < 30ms to complete, the user most likely wouldn't even notice further improvements.

> @@ +2952,5 @@
> > +      this,
> > +      generatedLine
> > +    ).filter((script) => !actor.hasScript(script));
> > +
> > +    // Find all entry points that correspond to the given location.
> 
> Seems like it'd be nice to make a few different "findEntryPoints" helper
> methods that would discover entry points in a few different ways. It would
> make it a little easier to read through the algorithm, and provide
> opportunities for caching later on.
> 

I will probably end up doing something like that, but see above.

> @@ +3137,5 @@
> >        }
> >        actor.addScript(script, this.threadActor);
> >      }
> >  
> > +    return;
> 
> What's the point of this return?

Oops :-)
(Assignee)

Comment 5

3 years ago
I'm seeing a test failure in browser_dbg_source-maps-04.js that I cannot reproduce locally. What's strange is that the test seems to pass on all Linux builds except Linux debug. What's more, the nature of the bug (setting a breakpoint returns an error) is such that I'd expect it to fail on all platforms, or not at all.

I've retriggered the offending test suite. If I cannot reproduce it, I'm going to assume that this is an intermittent test failure due to a subtle race condition that was either caused by my changes, or simply made visible, and will file a followup bug for that.
(Assignee)

Comment 6

3 years ago
Ok, so the failure is not intermittent, but consistent. Investigating.
(Assignee)

Comment 7

3 years ago
The test failures were most likely caused by a patch that landed before this one, and should now be resolved.

New try push:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=dbdca3003850
(Assignee)

Comment 8

3 years ago
New try push has some failures on Windows, but those look like infra issues. Looks fine otherwise, so pushing to fx-team:
https://hg.mozilla.org/integration/fx-team/rev/de24b63c6966
sorry had to back this out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=2336344&repo=fx-team
Flags: needinfo?(ejpbruel)
(Assignee)

Comment 10

3 years ago
(In reply to Carsten Book [:Tomcat] from comment #9)
> sorry had to back this out for test failures like
> https://treeherder.mozilla.org/logviewer.html#?job_id=2336344&repo=fx-team

That's strange, since those failures didn't show up on try. Are you sure this isn't just an intermittent?

In any case, I've made a new try push for this patch:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=540316168441
Flags: needinfo?(ejpbruel) → needinfo?(cbook)
(Assignee)

Comment 11

3 years ago
(In reply to Eddy Bruel [:ejpbruel] from comment #10)
> (In reply to Carsten Book [:Tomcat] from comment #9)
> > sorry had to back this out for test failures like
> > https://treeherder.mozilla.org/logviewer.html#?job_id=2336344&repo=fx-team
> 
> That's strange, since those failures didn't show up on try. Are you sure
> this isn't just an intermittent?
> 
> In any case, I've made a new try push for this patch:
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=540316168441

After studying the test for a bit, I think the test is at fault here, because it is gc sensitive. That would explain why it only fails on Linux debug, and only some of the time. I'm going to refactor that test as part of the patch, then try to land again.
Flags: needinfo?(cbook)
(Assignee)

Comment 12

3 years ago
New try push with browser_dbg_source-maps-04.js refactored. Hopefully, it should now no longer fail on Linux:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=087e6453c45d
(Assignee)

Comment 13

3 years ago
Nope, that did not quite work. Cancelled that try run and started a new one:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=750bb16ee6c5
(Assignee)

Comment 14

3 years ago
Try push looks good, pushing to fx-team again:
https://hg.mozilla.org/integration/fx-team/rev/844a5d08948f
https://hg.mozilla.org/mozilla-central/rev/844a5d08948f
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox39: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 39

Updated

a month ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.