Comment on attachment 9039832 [details] [diff] [review] 0001-wip.patch Review of attachment 9039832 [details] [diff] [review]: ----------------------------------------------------------------- Thanks a lot for working on this. It is great to see that CDP could be implemented in Firefox somewhat easily! I will start with a high level feedback. 1) Unused/useless code. There is quite some cruft here and there that doesn't help reviewing this patch. I'll post another comment mentioning all the things that aren't used/useful. 2) HTTP vs WebSocket In the current patch, you are starting one HTTP server on the port specified in the preference/command line. And then, one WebSocket per target on random ports. This is a diverging point from CDP, which serves everything from the base HTTP server, typically opened on port 9222. I'm afraid we would break support from random tooling based on CDP with such difference. All targets on chrome's CDP are served from port 9222 via URL that look like: ws://localhost:9222/devtools/page/74B150DBA8D82B8275AED2180B64F3B4 Only the ID changes, the word "page" is used for all target kinds, even background page and workers. It would be great if we could open only one port and serve the websockets from it, like chrome. 3) Implementing the domain in Target's process. Then, the main comment I have is architectural. It will have a significant impact on this patch. In the current patch, and like the other puppeteer's patch you were starting from, you are implementing all the domains in the parent process. The domain implementation then interacts with message manager API in order to reach the content processes and interact with the DOM, listen to console message and do other typical interactions with the document to debug. It has the side effect of listening to all documents messages and piping more data through message manager communication channel than you really need. For domains like Console, DOM and Debugger, the vast majority of the requests and events require direct access/interation with the Target context (Page, Frame, Worker, ...). It means that you will manually setup Frame script messages to implement each method and event. Doing this won't scale. That's not what we do in existing RDP and CDP implementations. Both Firefox and Chromium implement the Domains inspecting the Target within the target's context. More concretely, it means that `remote/domain/Log.jsm` should be loaded in the page's process rather than in the parent process. Then `remote/ConsoleServiceObserver.jsm`, which is a frame script in the current patch, should now be merged into Log.jsm and used by it directly. Your job here, if you accept it, will be to setup the architecture to automatically load Log.jsm within the target's process on-demand, if this domain is queried. Now, not all Domain are specific to the target. Like "Browser" domain. This one can and probably should be implemented in the parent process. We also have this concept in Firefox. We have the "global actors", which are loaded in the parent process once, and the "target-scoped actors", which are loaded once for each target, in their process. Last but not least, in chromium, they support hybrid domains, which can be implemented in both. A first implementation in the parent process will be called and may optionally delegate to another one being run in the target's process(es) (very precise detail: it looks like it may be able to call all the targets). That may be very handy to support that, but I don't know if that's a must-have in the first iteration? I wrote a detailed description of CDP, which hopefully will help working on this patch: https://docs.google.com/document/d/1t06-C3pNp4cvfwGyXLFtMYXPegnky0iNBBK3D4kTtcA/edit# 4) Fission / site-isolation As you will see in the document I linked in the previous paragraph, remoted frames are debuggable through distinct target. You will first connect to the page target via websocket and then from its `Target` domain, be able to listen to new targets via `Target.targetCreated` and attach to it via `Target.attachTarget` method. Then you can attach to the new target, created for remote frame via the same websocket connection. This is all based on a `sessionId` attribute passed in the JSON objects. We should implement that at some point, but I think we can ignore this for now. Looking at how it is being implemented, it looks fairly trivial and require some "router" on both server and client side to redirect each request/event to the right target instance. It may actually align with the code dispatching the request from the parent process to the target's process. 5) Lazyness A first look, everything is pretty much loaded on startup or on first connection. A lesson learned from DevTools is that lazy loading is important. So it will be useful to ensure that we have a minimum level of lazy loading. For example: * load the domain implementation only if we use them, if a client query it, * avoid loading anything but the command line handler if the command line argument isn't passed. 6) Modules (and compartments) This comment is more about the future. We can probably move forward with JSM as a first iteration. 6.1) JSMs vs ES Modules You are using JSM, but there is a strong motivation to start using ES Modules in Firefox codebase. The only issue is that ES Modules aren't easily usable from chrome codebase. AFAIK, it still requires a document. But it may be worth pinging :jonco about that. It would be fantastic if we could be using ES Modules right away. We have plan to start using them in DevTools, but it is much harder on an existing and large codebase. 6.2) Compartments Using JSMs will later prevent us from debugging chrome targets. This is clearly out of scope of this project, but I'm already thinking about using CDP to debug Firefox, including browser.xul and all chrome privileged code. To do that, this codebase would have to be loaded in its own compartment, flagged with "invisibleToDebugger=true" so that the Debugger API know what is debuggee versus debugger frames. It is clearly not feasible with JSM as JSMs are global and there will be no way to say that everything under remote/ should be loaded slightly differently. It may be different if we are using ES Modules as we may be loading module from a dedicated Document/Context, which we should be able to flag accordingly. So I think it is best to ignore that for now, and see how things are going with ES Modules instead. 7) Testing chrome-remote-interface is supposed to work from a web page: https://github.com/cyrus-and/chrome-remote-interface#browser-usage So, I imagine we should be able to make it work from an xpshell test script and easily run demo.js, which would be awesome! You may need a document to make it work. Note the existence of this test helper, which helps load'n spawn a document in XPCShell: https://searchfox.org/mozilla-central/source/dom/ipc/tests/test_sharedMap.js#89-96 This is based on top of Services.appShell.createWindowlessBrowser, in case this helper isn't useful.

Comment on attachment 9039832 [details] [diff] [review] 0001-wip.patch Review of attachment 9039832 [details] [diff] [review]: ----------------------------------------------------------------- Here is some more low level random feedback, close to a code-review. I didn't went through everything, but I think we can simplify the current patch and start focusing on the point I raised in the previous comment. Also, we should find ways to collaborate here so that you aren't alone doing all the patches! ::: remote/Debugger.jsm @@ +18,5 @@ > + * and for each accepted connection a new Session is created. > + * There can be multiple sessions per target. > + * The session's lifetime is equal to the lifetime of the debugger connection. > + */ > +this.Debugger = class { Debugger isn't a great name for this class. Especially given that we are going to have a Debugger domain. We are having tons of poorly named classes in DevTools that no longer make any sense, so I would like to be careful about the names :) What about TargetServer or TargetWebSocket or ...? @@ +26,5 @@ > + this.sessions = new Map(); > + this.nextConnID = 0; > + } > + > + get connected() { This would better be named "listening". ::: remote/Handler.jsm @@ +17,5 @@ > + > +class Handler { > + register(server) { > + server.registerPathHandler(this.path, this.rawHandle); > + } This isn't used. ::: remote/RemoteAgent.js @@ +25,5 @@ > + > +const ENABLED = "remote.enabled"; > +const FORCE_LOCAL = "remote.force-local"; > +const HTTPD = "remote.httpd"; > +const SCHEME = `${HTTPD}.scheme`; This isn't really used. @@ +71,5 @@ > + this.consoleService = new ConsoleServiceObserver(); > + > + this.tabs = new TabObserver({registerExisting: true}); > + this.tabs.on("open", tab => this.targets.connect(tab.linkedBrowser)); > + this.tabs.on("close", tab => this.targets.disconnect(tab.linkedBrowser)); We might want to move all of that in listen or whenever we accept the first connection in order to do things lazily. @@ +73,5 @@ > + this.tabs = new TabObserver({registerExisting: true}); > + this.tabs.on("open", tab => this.targets.connect(tab.linkedBrowser)); > + this.tabs.on("close", tab => this.targets.disconnect(tab.linkedBrowser)); > + > + Services.ppmm.addMessageListener("RemoteAgent:IsRunning", this); This event isn't used. @@ +143,5 @@ > + get scheme() { > + if (!this.server) { > + return null; > + } > + return this.server.identity.primaryScheme; This may be better inlined into listen method as you will nullify all the pref if the server failed starting. Same applies to host and port. @@ +165,5 @@ > + > + async handle(cmdLine) { > + let flag; > + try { > + flag = cmdLine.handleFlagWithParam("debug", false); `--debug` already exists and spawn a C++ debugger. We should find another name. Should we align on the one used in chrome? Or align with existing command line used in devtools? @@ +231,5 @@ > + ]); > + } > +} > + > +class Targets { It may be easier to follow the code if each Class was in its own file? @@ +279,5 @@ > + } > + > + toJSON() { > + return [...this]; > + } TBH, this is really hard to follow *and* this is hurting you later in remote/Target.jsm's toJSON. The combination of already kind of magic "toJSON" + iterator is too much. You would benefit from being more explicit: ``` async toJSON() { const list = []; for (const target of this._targets.values()) { list.push(await target.toJSON()); } return list; } ``` I don't know if it is fine to use async/await when you explicitely call toJSON? Otherwise it is worth having custom method name for this. @@ +334,5 @@ > + } > + > + if (!this.instance_) { > + if (Services.appinfo.processType == Ci.nsIXULRuntime.PROCESS_TYPE_CONTENT) { > + this.instance_ = new ChildRemoteAgent(); This content process branch isn't used/useful. You can get rid of ChildRemoteAgent. If this code happens to be trigerred in the content (which should not and probably be investigated), we should throw Cr.NS_ERROR_NOT_IMPLEMENTED or some error. ::: remote/Session.jsm @@ +24,5 @@ > + this.connection.onmessage = null; > + this.domains.clear(); > + } > + > + async despatch({id, method, params}) { This method has a typo. @@ +101,5 @@ > + throw new Error("No such domain: " + name); > + } > + > + const inst = new Cls(this.session, this.session.target); > + inst.on("*", this.session); It would be great to stick to toolkit's EventEmitter, or the DevTools one, which still supports wildcard listener, but differently from you. https://searchfox.org/mozilla-central/source/devtools/shared/event-emitter.js#198-205 But I'm not sure we should do that. We should probably not listen to all the events. It is pretty error prone as *all* events will end up being piped on the transport layer. Whereas you may have internal events that can be emitted on the Domain, that shouldn't go on the wire. It would be better to either: * be more explicit on each domain, when we want to fire a network event to be sent to the client. So use another API. this.sendEvent("Log.entryAdded", {entry}) or this.emit("event", "Log.entryAdded", { entry }), or? * filter and only pipe the events mentioned in the schema. That's what we do in RDP. ::: remote/Target.jsm @@ +14,5 @@ > +XPCOMUtils.defineLazyServiceGetter(this, "Favicons", > + "@mozilla.org/browser/favicon-service;1", "nsIFaviconService"); > + > +/** A debugging target. */ > +this.Target = class { That may be a very personal opinion, but `class Target {` looks more readable to me. If that's to please JSM export symbols, may be putting later `this.Target = Target;` is a better tradeoff? @@ +19,5 @@ > + constructor(browser) { > + this.browser = browser; > + this.debugger = new Debugger(this); > + > + EventEmitter.decorate(this); DevTools's event emitter supports ES classes and allows extending from it. It may be worth using it, or tweaking toolkit's one to support that? @@ +25,5 @@ > + > + connect() { > + Services.obs.addObserver(this, "message-manager-disconnect"); > + this.debugger.listen(); > + this.emit("connect"); Noone listen for connect/disconnect. We should remove this code until there is a usage of it. ::: remote/nsIRemoteAgent.idl @@ +12,5 @@ > + * Remote agent is a low-level debugging interface > + * based on the CDP protocol. > + */ > +[scriptable, uuid(ccbd36a6-a8fc-4930-b747-8be79c7a04b2)] > +interface nsIRemoteAgent : nsISupports This nsIRemoteAgent isn't used, we should remove it. ::: remote/server/Socket.jsm @@ +4,5 @@ > + > +"use strict"; > + > +var EXPORTED_SYMBOLS = [ > + "ConnectionHandshake", This symbol isn't used externaly to this module. @@ +8,5 @@ > + "ConnectionHandshake", > + "SocketListener", > +]; > + > +// This is an XPCOM-service-ified copy of ../devtools/shared/security/socket.js. Hum. I'm wondering if we can make it so that devtools's server switches to ES Modules and have the CDP server share the files here... It may pull too many deps from shared... I don't know.

Pushed by ato@sny.no: https://hg.mozilla.org/integration/mozilla-inbound/rev/51a9ef380658 remote: initial cdp prototype; r=ochameau https://hg.mozilla.org/integration/mozilla-inbound/rev/e566701d45e0 remote: identify firefox-source-docs.m.o location absolutely; r=ochameau https://hg.mozilla.org/integration/mozilla-inbound/rev/4f3ebf4a8f5a remote: fix incorrect directory name for documentation; r=ochameau https://hg.mozilla.org/integration/mozilla-inbound/rev/63370914ed49 remote: remove unused index; r=ochameau https://hg.mozilla.org/integration/mozilla-inbound/rev/67c34144b072 remote: refer to correct bugzilla product in documentation; r=ochameau https://hg.mozilla.org/integration/mozilla-inbound/rev/3e150e389aaa remote: fix links to mailing list; r=ochameau https://hg.mozilla.org/integration/mozilla-inbound/rev/f7e7800294cb remote: rename Debugger to TargetListener; r=ochameau https://hg.mozilla.org/integration/mozilla-inbound/rev/9b980764075b remote: fix eslint error; r=ochameau https://hg.mozilla.org/integration/mozilla-inbound/rev/7b87252c4a31 remote: document what a Target is; r=ochameau https://hg.mozilla.org/integration/mozilla-inbound/rev/af7a76690b8a remote: remove unused ChildRemoteAgent; r=ochameau https://hg.mozilla.org/integration/mozilla-inbound/rev/f1d96bc26051 remote: s/despatch/dispatcher/g; r=ochameau https://hg.mozilla.org/integration/mozilla-inbound/rev/290a558818ef remote: fix TargetListener export; r=ato https://hg.mozilla.org/integration/mozilla-inbound/rev/294ef13003d0 remote: fix exporting formatError; r=ato https://hg.mozilla.org/integration/mozilla-inbound/rev/bea61357248a remote: stop binding JSM exported globals on this; r=ato https://hg.mozilla.org/integration/mozilla-inbound/rev/f73d7c3bced4 remote: execute the domains in the content process by piping all WebSocket request, response and events via message manager API; r=ato https://hg.mozilla.org/integration/mozilla-inbound/rev/ac7c2e018cb6 remote: update the demo script to use a local page; r=ato https://hg.mozilla.org/integration/mozilla-inbound/rev/d7e26a1b6ef7 remote: fix eslint by removing unecessary commas; r=ato https://hg.mozilla.org/integration/mozilla-inbound/rev/819bb544204d remote: remove unused modules; r=ato https://hg.mozilla.org/integration/mozilla-inbound/rev/8231251f03a4 remote: remove unused nsIRemoteAgent interface; r=ato https://hg.mozilla.org/integration/mozilla-inbound/rev/51927b210a12 remote: add missing Connection.close method called from TargetListener.close; r=ato https://hg.mozilla.org/integration/mozilla-inbound/rev/c54bbccb690b remote: add browser mochitest to test the agent via chrome-remote-interface; r=ato https://hg.mozilla.org/integration/mozilla-inbound/rev/ff4ff4765f3b remote: add a workaround in chrome-remote-interface to support server; r=ato https://hg.mozilla.org/integration/mozilla-inbound/rev/16812017daad remote: wait for browser element init before creating Target; r=ato https://hg.mozilla.org/integration/mozilla-inbound/rev/4abd3301e839 remote: fix build by removing non-existent browser_tabs.js test; r=ochameau https://hg.mozilla.org/integration/mozilla-inbound/rev/0c3648638c85 remote: remove head.js support file to make it possible to execute browser_cdp.js; r=ochameau https://hg.mozilla.org/integration/mozilla-inbound/rev/705542bc7f14 remote: update testing documentation; r=ochameau https://hg.mozilla.org/integration/mozilla-inbound/rev/275ad4598e75 remote: ignore lint errors in chrome-remote-interface.js; r=ochameau https://hg.mozilla.org/integration/mozilla-inbound/rev/705a560085d4 remote: fix lint problems related to new browser-chrome tests; r=ochameau https://hg.mozilla.org/integration/mozilla-inbound/rev/6ee3f621e6b8 remote: salvage some code style documentation from Marionette; r=me https://hg.mozilla.org/integration/mozilla-inbound/rev/d4ddf649862b remote: change flag from --debug to --remote-debugger; r=ochameau https://hg.mozilla.org/integration/mozilla-inbound/rev/f8726e16183f remote: mention which host it binds to by default in help text; r=ochameau https://hg.mozilla.org/integration/mozilla-inbound/rev/3f8fa17722f6 remote: contribute usage instructions; r=me a=docs https://hg.mozilla.org/integration/mozilla-inbound/rev/05d7e3b96382 remote: release test_Session.js under CC-0; r=ochameau https://hg.mozilla.org/integration/mozilla-inbound/rev/304bff20aa1f remote: move Targets class to separate module; r=ochameau https://hg.mozilla.org/integration/mozilla-inbound/rev/02050b4ee473 remote: fix IPv6 loopback hostname; r=ochameau https://hg.mozilla.org/integration/mozilla-inbound/rev/65f5581faf6a remote: use application/json mime type for responses; r=ochameau https://hg.mozilla.org/integration/mozilla-inbound/rev/947ad73d45af remote: respond with 500 Internal Server Error if unable to serialise JSON; r=ochameau https://hg.mozilla.org/integration/mozilla-inbound/rev/e3d8a66cdad1 remote: re-add browser_tabs.js test and push head.js to fix the mochitests; r=ato https://hg.mozilla.org/integration/mozilla-inbound/rev/59da827edf02 remote: drop protocol schema validation; r=ochameau https://hg.mozilla.org/integration/mozilla-inbound/rev/2f7767b51e69 remote: fix lint errors by removing unused variables; r=ochameau https://hg.mozilla.org/integration/mozilla-inbound/rev/b35c2a9a00d2 remote: make Session#receiveMessage slightly more succinct; r=ochameau https://hg.mozilla.org/integration/mozilla-inbound/rev/cce704488a62 remote: remove reference to Network domain in the test; r=ato https://hg.mozilla.org/integration/mozilla-inbound/rev/e3b2b601748c remote: display error message and a stack in mochitest; r=ato https://hg.mozilla.org/integration/mozilla-inbound/rev/c26cc4c7f021 remote: allow Domains to be implemented in either parent or content processes; r=ato https://hg.mozilla.org/integration/mozilla-inbound/rev/0d1d92205068 remote: introduce MessagePromise sync primitive; r=ochameau https://hg.mozilla.org/integration/mozilla-inbound/rev/b075e49fca67 remote: test that method own property on domain class is function; r=ochameau https://hg.mozilla.org/integration/mozilla-inbound/rev/9ffed583c0e4 remote: s/remote-protocol/remote/g; r=ochameau https://hg.mozilla.org/integration/mozilla-inbound/rev/5ce6f3fcc572 remote: destruct domain instances on closing session; r=ochameau https://hg.mozilla.org/integration/mozilla-inbound/rev/a1a5fdbf9bc8 remote: refactor Session#dispatch; r=ochameau https://hg.mozilla.org/integration/mozilla-inbound/rev/40a677ed5cc4 remote: refactor Domains to not extend Map and wean us off custom EventEmitter; r=ochameau https://hg.mozilla.org/integration/mozilla-inbound/rev/11a98da764d1 remote: improve error legibility somewhat; r=ochameau https://hg.mozilla.org/integration/mozilla-inbound/rev/8a8c0dcb9486 remote: propagate underlying cause to superclass; r=ochameau https://hg.mozilla.org/integration/mozilla-inbound/rev/2bb69a9505ba remote: rename TargetListener.connected to listening; r=ato https://hg.mozilla.org/integration/mozilla-inbound/rev/6a8bdb28bc82 remote: merge Handler into JSONHandler as we only have JSONHandlers; r=ato https://hg.mozilla.org/integration/mozilla-inbound/rev/9f03512232fb remote: remove unused informative remote.httpd.* preferences; r=ato https://hg.mozilla.org/integration/mozilla-inbound/rev/10dc88c2cacb remote: stop emitting unused connect/disconnect events; r=ato https://hg.mozilla.org/integration/mozilla-inbound/rev/298854093159 remote: rename Prefs.jsm to match its exported symbol; r=ato https://hg.mozilla.org/integration/mozilla-inbound/rev/0c54ddfc1b58 remote: remove unused listener argument passed to Connection; r=ato https://hg.mozilla.org/integration/mozilla-inbound/rev/7ab41ce7425a remote: remove unused EventEmitter interface from Target; r=ato https://hg.mozilla.org/integration/mozilla-inbound/rev/45eecaf3a611 remote: use toolkit's event emitter; r=ato https://hg.mozilla.org/integration/mozilla-inbound/rev/f9eb88415e06 remote: destroy the related Session when the WebSocket connection drops; r=ato https://hg.mozilla.org/integration/mozilla-inbound/rev/e43dbe94d1aa remote: correctly unregister accepted listener in TargetListener; r=ato https://hg.mozilla.org/integration/mozilla-inbound/rev/1244522a15c7 remote: remove unused Target symbol from RemoteAgent; r=ato https://hg.mozilla.org/integration/mozilla-inbound/rev/f298dfa8a1b1 remote: format error packets according to puppeteer expectations; r=ato https://hg.mozilla.org/integration/mozilla-inbound/rev/2b1bde33c72e remote: remove unsupported feature in demo; r=ochameau https://hg.mozilla.org/integration/mozilla-inbound/rev/06c54b40f312 remote: associate formatting with error prototype; r=ochameau https://hg.mozilla.org/integration/mozilla-inbound/rev/d9cce6c1e153 remote: add Log.verbose; r=ochameau https://hg.mozilla.org/integration/mozilla-inbound/rev/0fa4b12c2f35 remote: emit events when targets connect and disconnect; r=ochameau https://hg.mozilla.org/integration/mozilla-inbound/rev/932d90008fe2 remote: document Session class; r=ochameau https://hg.mozilla.org/integration/mozilla-inbound/rev/4696a88c5274 remote: use fatal error when unable to start HTTPD; r=ochameau https://hg.mozilla.org/integration/mozilla-inbound/rev/1f861b702507 remote: clarify error message when unable to start HTTPD; r=ochameau https://hg.mozilla.org/integration/mozilla-inbound/rev/c750fecfdf21 remote: self-assign id to connection; r=ochameau https://hg.mozilla.org/integration/mozilla-inbound/rev/e94205832ec1 remote: signal that transport is ready when connection is created; r=ochameau https://hg.mozilla.org/integration/mozilla-inbound/rev/2c703d235c4c remote: drop outdated todos; r=me https://hg.mozilla.org/integration/mozilla-inbound/rev/2dd67c30b7ae remote: add WebSocketServer.upgrade for upgrading existing httpd.js requests; r=ochameau https://hg.mozilla.org/integration/mozilla-inbound/rev/53947dc827dd remote: upgrade to WebSocket on existing HTTPD; r=ochameau https://hg.mozilla.org/integration/mozilla-inbound/rev/9e035690f943 remote: remove unused BrowserObserver; r=ochameau https://hg.mozilla.org/integration/mozilla-inbound/rev/de44fc903478 remote: remove unused WindowManager.isWindowIncluded; r=ochameau https://hg.mozilla.org/integration/mozilla-inbound/rev/ed31c156cec0 remote: drop unused module imports; r=ochameau https://hg.mozilla.org/integration/mozilla-inbound/rev/e32546a7d4cd remote: drop unused functions in Browser domain; r=ochameau https://hg.mozilla.org/integration/mozilla-inbound/rev/bf6f4ba40815 remote: sort imports; r=ochameau https://hg.mozilla.org/integration/mozilla-inbound/rev/64c382608b1c remote: format; r=me a=docs https://hg.mozilla.org/integration/mozilla-inbound/rev/56727ea2555c remote: remove unused variables; r=ochameau https://hg.mozilla.org/integration/mozilla-inbound/rev/25dd3f1e01c9 remote: fix WindowManager docs; r=me a=docs https://hg.mozilla.org/integration/mozilla-inbound/rev/2932c11d19a5 remote: disconnect from build system; r=me

Pushed by ato@sny.no: https://hg.mozilla.org/integration/mozilla-inbound/rev/820ed215c286 remote: hook new remote protocol up to build system; r?#build