Closed Bug 997119 Opened 6 years ago Closed 1 year ago

Decouple ThreadActor from BrowsingContextTargetActor

Categories

(DevTools :: Framework, task, P3)

task

Tracking

(Fission Milestone:M4, firefox70 fixed)

RESOLVED FIXED
Firefox 70
Fission Milestone M4
Tracking Status
firefox70 --- fixed

People

(Reporter: ochameau, Assigned: jarilvalenciano)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

bug 977043 followup.
Ideally, DebuggerServer should only be about implementing the server and doing the network and actor machinery.
But it contains some code specific to debugger actor, especially some code specific to the ThreadActor. So that if you want a very good comprehension of ThreadActor you have to read the ThreadActor class, but also a bunch of DebuggerServer methods.
Bug 977043 introduced new will-navigate/navigate events that should help moving thread actor code back to the ThreadActor class.
Depends on: 977043
I believe when you write DebuggerServer above you are referring to the TabActor in webbrowser.js (DebuggerServer is in main.js).
Component: Developer Tools → Developer Tools: Debugger
OS: Windows 7 → All
Priority: -- → P3
Hardware: x86_64 → All
Summary: Uncouple ThreadActor from DebuggerServer → Uncouple ThreadActor from TabActor
Version: unspecified → Trunk
Hi Alex. Could you clarify exactly how you would like to decouple ThreadActor from TabActor?
Flags: needinfo?(poirot.alex)
Move (if possible) all code related to ThreadActor out of main.js (most likely script.js?)

Code like this can be easily moved to script.js:
http://mxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/webbrowser.js#1293

TabActor should be a generic abstraction and shouldn't contain anything of the child actors it manages.
For historical reason, many code related to the debugger have been entangled within core code for the debugger server/protocol. Now we have some usage of the DebuggerServer where we don't care about the debugger (script.js, js code debugging, breakpoint, ...). And we should be able to load a barebone DebuggerServer instance without loading anything regarding the debugger. This is related to my work to strip down memory usage of the server.
Flags: needinfo?(poirot.alex)
(In reply to Alexandre Poirot [:ochameau] from comment #3)
> Move (if possible) all code related to ThreadActor out of main.js (most
> likely script.js?)
> 
> Code like this can be easily moved to script.js:
> http://mxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/
> webbrowser.js#1293
> 
> TabActor should be a generic abstraction and shouldn't contain anything of
> the child actors it manages.
> For historical reason, many code related to the debugger have been entangled
> within core code for the debugger server/protocol. Now we have some usage of
> the DebuggerServer where we don't care about the debugger (script.js, js
> code debugging, breakpoint, ...). And we should be able to load a barebone
> DebuggerServer instance without loading anything regarding the debugger.
> This is related to my work to strip down memory usage of the server.

I agree. Ideally TabActor should be completely unaware of the internals of ThreadActor.

Are you aware that we've recently added some dependencies for ThreadActor on TabActor (so in the other direction?) I'm talking about stuff like makeDebugger in webbrowser.js. Are you ok with those dependencies? I assume you are only talking about dependencies of TabActor on ThreadActor.
Yes deps on the other direction are fine.
Summary: Uncouple ThreadActor from TabActor → Decouple ThreadActor from TabActor
Depends on: 1172897
Summary: Decouple ThreadActor from TabActor → Decouple ThreadActor from BrowsingContextTargetActor
Product: Firefox → DevTools
Component: Debugger → General
Type: defect → task
Component: General → Framework

failed test: devtools/client/debugger/test/mochitest/browser_dbg-browser-content-toolbox.js
failed test: devtools/client/debugger/test/mochitest/browser_dbg-chrome-debugging.js
failed test: devtools/client/debugger/test/mochitest/browser_dbg-windowless-workers-early-breakpoint.js

diff --git a/devtools/server/actors/targets/browsing-context.js b/devtools/server/actors/targets/browsing-context.js
index 5e71b1320a5e..d2a7dac2d85c 100644
--- a/devtools/server/actors/targets/browsing-context.js
+++ b/devtools/server/actors/targets/browsing-context.js
@@ -1341,16 +1341,6 @@ const browsingContextTargetPrototype = {
       return;
     }
 
-    // Proceed normally only if the debuggee is not paused.
-    // TODO bug 997119: move that code to ThreadActor by listening to
-    // will-navigate
-    const threadActor = this.threadActor;
-    if (threadActor.state == "paused") {
-      threadActor.unsafeSynchronize(Promise.resolve(threadActor.doResume()));
-      threadActor.dbg.enabled = false;
-    }
-    threadActor.disableAllBreakpoints();
-
     this.emit("tabNavigated", {
       url: newURI,
       nativeConsoleAPI: true,
diff --git a/devtools/server/actors/thread.js b/devtools/server/actors/thread.js
index 182bf6d46736..cfb93b7693d5 100644
--- a/devtools/server/actors/thread.js
+++ b/devtools/server/actors/thread.js
@@ -99,6 +99,7 @@ const ThreadActor = ActorClassWithSpec(threadSpec, {
     this.objectGrip = this.objectGrip.bind(this);
     this.pauseObjectGrip = this.pauseObjectGrip.bind(this);
     this._onOpeningRequest = this._onOpeningRequest.bind(this);
+    this.onWillNavigate = this.onWillNavigate.bind(this)
 
     if (Services.obs) {
       // Set a wrappedJSObject property so |this| can be sent via the observer svc
@@ -279,6 +280,8 @@ const ThreadActor = ActorClassWithSpec(threadSpec, {
     this.sources.setOptions(this._options);
     this.sources.on("newSource", this.onNewSourceEvent);
 
+    this._parent.on("will-navigate", this.onWillNavigate);
+
     // Initialize an event loop stack. This can't be done in the constructor,
     // because this.conn is not yet initialized by the actor pool at that time.
     this._nestedEventLoops = new EventLoopStack({
@@ -348,19 +351,29 @@ const ThreadActor = ActorClassWithSpec(threadSpec, {
       return this._pauseOverlay;
     }

 
+  onWillNavigate: function () {
+    if (this.state == "paused") {
+      this.unsafeSynchronize(Promise.resolve(this.doResume()));
+      // this.doResume();
+      this.dbg.enabled = false;
+    }
+    this.disableAllBreakpoints();
+  },
Pushed by ystartsev@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f285ee7fedd7
Move ThreadActor logic from BrowsingContext. r=yulia
Status: NEW → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 70
Assignee: nobody → jarilvalenciano

Retroactively moving fixed bugs whose summaries mention "Fission" (or other Fission-related keywords) but are not assigned to a Fission Milestone to an appropriate Fission Milestone.

This will generate a lot of bugmail, so you can filter your bugmail for the following UUID and delete them en masse:

0ee3c76a-bc79-4eb2-8d12-05dc0b68e732

Fission Milestone: --- → M4
You need to log in before you can comment on or make changes to this bug.