Breakpoints, source actors, and multiple concurrent sources

[Eddy Bruel]

Hi James,

I’ve been sending a lot of e-mails to you and Nick lately. Joe pointed out
to me that the devtools mailing list would probably be a better medium for
this, so here goes.

I’m currently trying to wrap my head around subtle problem involving
breakpoint sliding and source actors.

Could you go over my analysis of the problem below and tell me if it makes
sense to you? I currently have not figured out yet how the problem should
be solved, so if you have any suggestions in that direction I’d appreciate
that as well.

Currently, when a new script is introduced, we end up calling _addScript,
which performs the following steps:

1. Create a non-sourcemapped source actor for the newly introduced script.

2. For each BreakpointActor:

a. If the (generated) location of the BreakpointActor falls within range
of the newly introduced script:

I. Set the BreakpointActor as breakpoint handler for the newly
introduced script, using the generated location of the BreakpointActor.

3. Create one or more source mapped source actors for the newly introduced
script. Each newly created source actor causes a new source notification to
be sent.


In my mind, there are two problems with the above approach:

1. We assume that each BreakpointActor corresponds to a single generated
location. If the BreakpointActor represents a line breakpoint, a single
line in an original source could correspond to multiple lines in a
generated source, or even multiple generated sources.

2. If the BreakpointActor represents a pending breakpoint, its actual
location has not been determined yet. If the actual location ends up
changing due to breakpoint sliding, the set of generated locations
corresponding to the BreakpointActor will change as well. Therefore, the
check performed in step 2a can yield false negatives.


Here’s how I think step 2 should be performed instead:

2. For each BreakpointActor:

a. If the BreakpointActor represents a pending breakpoint:

I. Try to figure out the actual location of the BreakpointActor, using
breakpoint sliding.

b. If the BreakpointActor is no longer pending:

I. For each generated location corresponding to the actual location of
the BreakpointActor, set the BreakpointActor as breakpoint handler on the
newly introduced script, using the generated location.

Unfortunately, this approach doesn’t work. The problem occurs in
test_breakpoint-20.js, where we load a source, set a breakpoint in it, and
then load the same source again. The second load triggers a call to
_addScript, after which step 1 yields a new source actor that is distinct
from the one for the original script, but with the same actor ID. The
BreakpointStore compares source actors by their ID, so we find an existing
BreakpointActor for the source, and try to set it as a breakpoint handler
on the corresponding script.

However, the new source actor also has a different Debugger.Source instance
associated with it, which means that setBreakpoint will try to add the
BreakpointActor as handler to a different script depending on whether we
use the old or the new source actor. What’s more, when we obtain the
generated locations from the actual location of the BreakpointActor, they
still have the old source actor, so we end up setting the BreakpointActor
as breakpoint handler on the old script twice.


What I *think* is going on is this. We have two Debugger.Script instances,
each with a distinct Debugger.Source instance. However, both
Debugger.Source instances represent the same source. When the new script is
introduced, we try to get or create a source actor for its Debugger.Source
instance. We do so by checking a sourceActors Map, which is keyed by
Debugger.Source instances. Since the new Debugger.Source instance is
distinct from the old one, we don’t find an existing actor for it, so we
try to create a new one.

When creating a new source actor, we end up reusing the actor ID of the
source actor for the previous Debugger.Source instance. This source actor
then effectively replaces the old source actor. This feels wrong to me,
since both sources are still present, and the BreakpointActor will (or
should, at least) end up as a breakpoint handler for both corresponding
Debugger.Script instances.

In our current state, we end up only having an actor for the source that
was introduced last. On the other hand, the generated location stored in
the BreakpointActor is never updated, and will always point to the source
that was introduced first. The only reason this works is that we call
setBreakpointForActor on the new source actor, but pass it a generated
location that contains the old one. I probably don’t have to point out how
confusing and fragile this is.

I considered just updating the old source actor in place so that it
represents the new one, but it seems to me that that would have problems as
well. What happens if the user tries to set an additional breakpoint in the
first source, for instance? Since the actor ID now refers to the actor for
the new source, wouldn’t we end up setting the breakpoint there, instead of
on the second?

[James Long]

I can reply in more detail tomorrow, because there's a lot in here (might
be good to talk through some of it on IRC). Here are some notes after
reading through it though, in somewhat reverse order from how you typed it:

* Yeah, when I was doing Debugger.Source work I though about just changing
the Debugger.Source object (and any other properties) of a SourceActor when
the D.S instance changes. But this turns out to be a lot of work.
Unnecessary work, if you ask me. If you just key off of the source actor
ID, we can just make sure to use the same actor ID across D.S instances. I
don't think you should ever be keying on D.Source instance (unless it's
something specific to it, like source maps). You should always be keying
off actor ID.

* How do we ever have two Debugger.Source instances for the same script at
the same time? The only time this happens is when the page reloads. The old
Debugger.Source instance is useless then, we don't care about it at all.
But we *do* care about breakpoints set on it. So we use the same id (if we
can resolve it to one, usually based on URL), and initialize it with the
right things (like all the previous breakpoints).

* For _addScript, I thought we basically decided on this:

We have a top-level script (from `onNewScript`). This is guaranteed to be
the entire source of the Debugger.Source object. So just get the
non-sourcemapped source for it, get all the breakpoints for that source
actor ID, and activate each one. The `setBreakpoint` function is
responsible for setting a breakpoint on all child scripts to make sure all
the entry points have breakpoints. In `_addScript`, we just iterate over
the registered per-line breakpoints and give it to `setBreakpoint`.

If each BreakpointActor has multiple breakpoints, just iterate through all
of those as well, instead of just calling `setBreakpoint` once. Does that
make sense?

We can catch up on IRC tomorrow about this also. Thanks!

> _______________________________________________
> dev-developer-tools mailing list
> dev-devel...@lists.mozilla.org
> https://lists.mozilla.org/listinfo/dev-developer-tools
>

[Eddy Bruel]

---------- Forwarded message ----------
From: Eddy Bruel <ejpb...@mozilla.com>
Date: Thu, Feb 5, 2015 at 7:52 AM
Subject: Re: Breakpoints, source actors, and multiple concurrent sources
To: James Long <jl...@mozilla.com>


Take a look at test-breakpoint-20.js. What this test does is the following:

- Eval some test code.
This causes a Debugger.Script instance to be created, with a
corresponding Debugger.Source instance.
- Set a breakpoint.
We do this by finding the SourceActor corresponding to the
Debugger.Source instance we just created.
- Eval the same test code again.
Correct me if I'm wrong, but I believe this causes a new Debugger.Script
instance to be created, with a Debugger.Source instance that is separate
from the one we created earlier. At this point we replace the SourceActor
corresponding to the previous Debugger.Source instance with one
representing the new Debugger.Source instance, but the same actor ID.
- Check if the breakpoint we set earlier is hit in the new source as well.
This currently works because creating a new Debugger.Script causes
_addScript to be called. Since the actorID for the new and the old
SourceActor are the same, we find any BreakpointActors associated with the
old SourceActor, and then reset them as breakpoint handler on the newly
introduced script using the new Debugger.Source instance. So far so good.

The particular test code we are evalling pushes a function on the stack.
Now, what happens if somebody sets a second breakpoint, and then calls the
function pushed when the test code was evalled the first time?

The SourceActor refers to the Debugger.Source instance corresponding to the
second eval. Since the ScriptStore keys scripts by Debugger.Source
instance, won't we only find those scripts that where created with the
second call to eval? I suspect the new breakpoint will end up hitting in
the second function, but not in the first. Conversely, removing the
breakpoint on the second source will not cause it to be removed from the
first function.

[Eddy Bruel]

Hi James,

I've attached a patch that extends test_breakpoint-20.js to set a second
breakpoint, then call the function created by the first call to eval. As
expected, the breakpoint never hits. If I call the function created by the
second call instead, it does. This is because we set the breakpoint on the
Debugger.Source instance created by the second call to eval, which doesn't
know anything about the scripts introduced by the first call, which are
still accessible.

[James Long]

Ok, I see what you mean. It just seems like a lot of work to support that,
when that's potentially really confusing. We definitely do no support
multiple sources that have the exact same URL/actor. If code is eval'ed
twice, two difference sources show up in the source pane. If a code is
eval'ed with a sourceURL pragma twice, there will only be one source, but
that source is *always* the latest most recent eval. We should optimize for
this case. If there are internal scripts that somehow exist under the same
URL, and both should stay alive, they should have separate URLs. Because
they will only show up once in the source listing, but they are actually
different sources! We want those to show up as 2 difference sources.

If new code is eval'ed, and I set some breakpoints in it, if we set
breakpoint in an older version of the script, we have *no* idea that the
line numbers are still matching correctly because the newer script is
actually different code. What if I simply added some code above the
function? The line numbers would be all messed up, and breakpoints hit in
the older script would be at the wrong place, which would be wildly
confusing.

So I say if a script exists with the same URL/source actor, we should act
like we are replacing the old one. If we want to treat them both as
currently alive, we need to show them both as separate scripts in the
debugger.

[Nick Fitzgerald]

On Wed, Feb 4, 2015 at 11:19 PM, Eddy Bruel <ejpb...@mozilla.com> wrote:

> Hi James,
>
> I've attached a patch that extends test_breakpoint-20.js to set a second
> breakpoint, then call the function created by the first call to eval. As
> expected, the breakpoint never hits. If I call the function created by the
> second call instead, it does. This is because we set the breakpoint on the
> Debugger.Source instance created by the second call to eval, which doesn't
> know anything about the scripts introduced by the first call, which are
> still accessible.
>

​(a) Attachments get stripped on mailing lists, so we can't see it.​

(b) This is definitely a bug, if you set a BP in a source representing a
frame script, you should hit it in all D.Scripts from a D.Source "instance"
of that frame script.

Please file a bug.