1137384 - Move ThreadSources into its own devtools-wide file

Status: RESOLVED FIXED

(DevTools :: Debugger, defect)

Description

[James Long (:jlongster)]

The idea of `ThreadSources` was to have a single places that sources are managed, and anything like sourcemapping can be done there, and eventually we could make this available to other tools. That time is now, since we need sourcemaps in the console.

I don't think it should be too hard to move `ThreadSources` into something per-tab. I'm not exactly where this should be yet, but I wanted to go ahead and file this bug because Eddy is working on the backend so I'll need to coordinate with him carefully so I don't conflict with this work.

Comment 1

[James Long (:jlongster)]

Eddy, this is not extremely urgent, so I'm happy to wait for a window that will work well for you. I don't know how much you are touching ThreadSources right now.

Comment 2

[James Long (:jlongster)]

Attachment: 1137384.patch

This actually wasn't that hard. This patch just moves ThreadSources up into the tab actors (and renamed TabSources) and makes everything work with that move. This doesn't add any new functionality, it just refactors the code and makes sure everything still works.

I need to wait for bug 1059308 to land before I finish the patch because that changes how the browser debugger works. The browser debugger will inherit from TabActor and get this for free, instead of now I'd have to manually patch the chrome debugger (and it's actually not clear to do it). That bug should land this week.

No need to review until I get that change in

Comment 3

[James Long (:jlongster)]

Attachment: 1137384.patch

All tests pass! Ready for review. I rebased this yesterday so it should be up-to-date. Hopefully it doesn't conflict much with any work you have in your repo.

Comment 4

[James Long (:jlongster)]

Nick was interested in this too, this is a big enough change that more reviewers would be good.

Comment 5

[James Long (:jlongster)]

Attachment: 1137384.patch

Oops, forgot to add some comments in TabSources

Comment 6

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 8575603 [details] [diff] [review]
1137384.patch

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

Overall, very nice. We've needed this for a while.

::: toolkit/devtools/server/actors/script.js
@@ +2026,5 @@
>      let endLine = aScript.startLine + aScript.lineCount - 1;
> +    for (let _actor of this.breakpointActorMap.findActors()) {
> +      // We do async work in here, so we need to create a fresh
> +      // binding because for/of does not yet do that in SpiderMonkey
> +      // (remove this when bug 1101653 is fixed)

Nit: file a follow up that depends on bug 1101653 (and gets an auto update when that bug is fixed) and link it here so that we don't forget.

    // XXX Bug #######: We do async work in here ...

::: toolkit/devtools/server/actors/utils/TabSources.js
@@ +153,5 @@
> +    if (id) {
> +      actor.actorID = id;
> +    }
> +
> +    this._thread.threadLifetimePool.addActor(actor);

The `SourceActor`s should be children of the `TabActor` in the actor hierarchy, that way non-ThreadActor actors can safely use them and not worry about lifetimes.

Comment 7

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 8575603 [details] [diff] [review]
1137384.patch

Stupid splinter

Comment 8

[Eddy Bruel [:ejpbruel]]

Comment on attachment 8575606 [details] [diff] [review]
1137384.patch

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

I only looked whether this patch would interfere with my work on breakpoint sliding. If I read the patch correctly, the API of ThreadSources didn't change, and is still available via threadActor.sources. I only need access to source map functions like getGeneratedLocation etc.

If correct, this should be fairly easy for me to rebase on top of, so feel free to land.

Comment 9

[James Long (:jlongster)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=eae4a82bb523

Comment 10

[James Long (:jlongster)]

Turns out ThreadSources had a bunch of dependencies on the synchronous `resolve` promise method, and making it async is showing a few race conditions. Bah.

Comment 11

[James Long (:jlongster)]

Still trying to track down the intermittent. I used to be able to reproduce it locally, but can't anymore, so I wonder if recent refactorings fixed it. Two try pushes:

TabSources using sync promises: https://treeherder.mozilla.org/#/jobs?repo=try&revision=87ae08be00ab
TabSources using async promises: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d6604c2b9703

I'm not going to use sync promises in the end, but its just a test case to see if the former succeeds and the latter fails.

Comment 12

[James Long (:jlongster)]

Above try looks green, but was only on linux. Here's a full try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8f382813f929

Comment 13

[James Long (:jlongster)]

Attachment: 1137384.patch

rebased

Comment 14

[James Long (:jlongster)]

Hm, looks like there might be some leaks... let's see if this fixes it: https://treeherder.mozilla.org/#/jobs?repo=try&revision=69694a28eba4

Comment 15

[James Long (:jlongster)]

Attachment: 1137384.patch

rebased

Comment 16

[James Long (:jlongster)]

Windows debug failure doesn't look like mine, and OSX seems to have trouble building, I just retriggered it though to see if works.

Comment 17

[James Long (:jlongster)]

Attachment: 1137384.patch

rebased

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/977b4598da47

Comment 19

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

https://hg.mozilla.org/mozilla-central/rev/977b4598da47