Move ThreadSources into its own devtools-wide file

RESOLVED FIXED in Firefox 39

Status

DevTools
Debugger
RESOLVED FIXED
3 years ago
6 days ago

People

(Reporter: jlongster, Assigned: jlongster)

Tracking

(Blocks: 1 bug)

unspecified
Firefox 39
x86
Mac OS X
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox39 fixed)

Details

Attachments

(1 attachment, 5 obsolete attachments)

(Assignee)

Description

3 years ago
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.
(Assignee)

Comment 1

3 years ago
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.
(Assignee)

Updated

3 years ago
Blocks: 1132501
(Assignee)

Updated

3 years ago
Depends on: 1059308
(Assignee)

Comment 2

3 years ago
Created attachment 8572208 [details] [diff] [review]
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
(Assignee)

Updated

3 years ago
Assignee: nobody → jlong
(Assignee)

Comment 3

3 years ago
Created attachment 8575603 [details] [diff] [review]
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.
Attachment #8572208 - Attachment is obsolete: true
Attachment #8575603 - Flags: review?(ejpbruel)
(Assignee)

Comment 4

3 years ago
Nick was interested in this too, this is a big enough change that more reviewers would be good.
Flags: needinfo?(nfitzgerald)
(Assignee)

Comment 5

3 years ago
Created attachment 8575606 [details] [diff] [review]
1137384.patch

Oops, forgot to add some comments in TabSources
Attachment #8575603 - Attachment is obsolete: true
Attachment #8575603 - Flags: review?(ejpbruel)
Attachment #8575606 - Flags: review?(ejpbruel)
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.
Attachment #8575603 - Attachment is obsolete: false
Comment on attachment 8575603 [details] [diff] [review]
1137384.patch

Stupid splinter
Attachment #8575603 - Attachment is obsolete: true
Flags: needinfo?(nfitzgerald)
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.
Attachment #8575606 - Flags: review?(ejpbruel) → review+
(Assignee)

Comment 10

3 years ago
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.
(Assignee)

Comment 11

3 years ago
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.
(Assignee)

Comment 12

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

Comment 13

3 years ago
Created attachment 8580186 [details] [diff] [review]
1137384.patch

rebased
Attachment #8575606 - Attachment is obsolete: true
(Assignee)

Comment 14

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

Comment 15

3 years ago
Created attachment 8580717 [details] [diff] [review]
1137384.patch

rebased
Attachment #8580186 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Keywords: checkin-needed
(Assignee)

Comment 16

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

Comment 17

3 years ago
Created attachment 8581775 [details] [diff] [review]
1137384.patch

rebased
Attachment #8580717 - Attachment is obsolete: true
https://hg.mozilla.org/integration/fx-team/rev/977b4598da47
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/977b4598da47
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox39: --- → fixed
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 39

Updated

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