Closed Bug 1137384 Opened 5 years ago Closed 5 years ago

Move ThreadSources into its own devtools-wide file


(DevTools :: Debugger, defect)

Not set


(firefox39 fixed)

Firefox 39
Tracking Status
firefox39 --- fixed


(Reporter: jlong, Assigned: jlong)


(Blocks 1 open bug)



(1 file, 5 obsolete files)

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.
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.
Blocks: 1132501
Depends on: 1059308
Attached patch 1137384.patch (obsolete) — Splinter Review
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: nobody → jlong
Attached patch 1137384.patch (obsolete) — Splinter Review
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)
Nick was interested in this too, this is a big enough change that more reviewers would be good.
Flags: needinfo?(nfitzgerald)
Attached patch 1137384.patch (obsolete) — Splinter Review
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]

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]

Stupid splinter
Attachment #8575603 - Attachment is obsolete: true
Flags: needinfo?(nfitzgerald)
Comment on attachment 8575606 [details] [diff] [review]

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+
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.
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:
TabSources using async promises:

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.
Above try looks green, but was only on linux. Here's a full try:
Attached patch 1137384.patch (obsolete) — Splinter Review
Attachment #8575606 - Attachment is obsolete: true
Hm, looks like there might be some leaks... let's see if this fixes it:
Attached patch 1137384.patch (obsolete) — Splinter Review
Attachment #8580186 - Attachment is obsolete: true
Keywords: checkin-needed
Windows debug failure doesn't look like mine, and OSX seems to have trouble building, I just retriggered it though to see if works.
Attached patch 1137384.patchSplinter Review
Attachment #8580717 - Attachment is obsolete: true
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
Closed: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 39
Depends on: 1149910
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.