Refactor breakpoint sliding for non-source mapped sources

RESOLVED FIXED in Firefox 39

Status

defect
RESOLVED FIXED
4 years ago
Last year

People

(Reporter: ejpbruel, Assigned: ejpbruel)

Tracking

unspecified
Firefox 39
x86
macOS

Firefox Tracking Flags

(firefox39 fixed)

Details

Attachments

(1 attachment)

Assignee

Description

4 years ago
Posted patch patchSplinter Review
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

4 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

4 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

4 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

4 years ago
Ok, so the failure is not intermittent, but consistent. Investigating.
Assignee

Comment 7

4 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

4 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

4 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

4 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

4 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

4 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

4 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
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 39

Updated

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