1138975 - Refactor breakpoint sliding for non-source mapped sources

Status: RESOLVED FIXED

(DevTools :: Debugger, defect)

Description

[Eddy Bruel [:ejpbruel]]

Attachment: 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.

Comment 1

[Eddy Bruel [:ejpbruel]]

Note that this patch is based on top of the one in bug 1131646.

Comment 2

[James Long (:jlongster)]

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?

Comment 3

[Eddy Bruel [:ejpbruel]]

(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 :-)

Comment 4

[Eddy Bruel [:ejpbruel]]

Try push for this patch:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=999def0d4aa9

Comment 5

[Eddy Bruel [:ejpbruel]]

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.

Comment 6

[Eddy Bruel [:ejpbruel]]

Ok, so the failure is not intermittent, but consistent. Investigating.

Comment 7

[Eddy Bruel [:ejpbruel]]

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

Comment 8

[Eddy Bruel [:ejpbruel]]

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

Comment 9

[Carsten Book [:Tomcat]]

sorry had to back this out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=2336344&repo=fx-team

Comment 10

[Eddy Bruel [:ejpbruel]]

(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

Comment 11

[Eddy Bruel [:ejpbruel]]

(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.

Comment 12

[Eddy Bruel [:ejpbruel]]

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

Comment 13

[Eddy Bruel [:ejpbruel]]

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

Comment 14

[Eddy Bruel [:ejpbruel]]

Try push looks good, pushing to fx-team again:
https://hg.mozilla.org/integration/fx-team/rev/844a5d08948f

Comment 15

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

https://hg.mozilla.org/mozilla-central/rev/844a5d08948f