Closed Bug 1444132 Opened 6 years ago Closed 6 years ago

Try to simplify the netmonitor actor codebase running in parent process

Categories

(DevTools :: Netmonitor, enhancement, P1)

Product:
DevTools
Component:
Netmonitor
Type:
enhancement

Tracking

(firefox63 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 63
Tracking Flags:
Tracking Status
firefox63 --- fixed

People

(Reporter: ochameau, Assigned: ochameau)

References

(Blocks 3 open bugs)

Details

(Whiteboard: dt-fission)

Attachments

(12 files, 1 obsolete file)

Bug 1444132 - Spawn a NetworkMonitorActor directly in the parent process.
(deleted), text/x-review-board-request
jryans
: review+
Details
Network Communication Sequence Diagram
(deleted), image/png
Details
Bug 1444132 - Fix stacktrace support with the new setup.
(deleted), text/x-review-board-request
jryans
: review+
Details
Bug 1444132 - HSTS redirections now have stack traces.
(deleted), text/x-review-board-request
jryans
: review+
Details
Bug 1444132 - Fix stylesheet retrieval from network monitor with the new setup.
(deleted), text/x-review-board-request
jryans
: review+
Details
Bug 1444132 - Remove the old proxy code.
(deleted), text/x-review-board-request
jryans
: review+
Details
Bug 1444132 - Fix netmonitor preferences with the new setup.
(deleted), text/x-review-board-request
jryans
: review+
Details
Bug 1444132 - Fix sendHTTPRequest with the new setup.
(deleted), text/x-review-board-request
jryans
: review+
Details
Bug 1444132 - Fetch stacks from content process only on-demand.
(deleted), text/x-review-board-request
jryans
: review+
Details
Bug 1444132 - Prevent race in browser_net_view_source-debugger.js if the stack button is lazily created.
(deleted), text/x-review-board-request
jryans
: review+
Details
Bug 1444132 - Always wait for full Connector setup in getHarExportConnector.
(deleted), text/x-review-board-request
jdescottes
: review+
Details
Bug 1444132 - Fix the order of requests done by html_cause-test-page.html test.
(deleted), text/x-review-board-request
jdescottes
: review+
Details 
In prevision of site-isolation project and in an attempt to improve performance,
it would be great to simplify the actor codebase related to the network monitor.

A significant part of it is run in the parent process and is piped to the child process, before being piped back to the parent process to be handed over to the frontend.
This creates 3 copies of network data/metadata instead of just one.
Attachment #8957225 - Flags: feedback?(jryans)
Assignee: nobody → poirot.alex
Depends on: 1449162
Comment on attachment 8957225 [details]
Bug 1444132 - Spawn a NetworkMonitorActor directly in the parent process.

https://reviewboard.mozilla.org/r/226158/#review238224

Apologies for the slow feedback here...  I'll try to do better!

As I mentioned during our meeting, unlike most actor-related work, this change is all server-side, so that gives us a bit more freedom to experiment without having to worry about future compatibility.

I'd like to see documentation changes for this new approach when it's ready for review.

::: devtools/server/actors/network-monitor.js:39
(Diff revision 2)
> +    // but from the parent process!
> +    this.webConsoleActorID = webConsoleActorID;
> +  },
> +
> +  // New code, to receive the stack from the content process
> +  // ideally, we would transfer the stacktraces lazily, whenever NetworkEventActor.getStacktrace is called

Hmm, it does seem something lazy would be better here since the client is already trying to be lazy today...

I'd like to have more to say about this topic, but I just tried testing stock Nightly stack traces, and I don't seem to get them anymore...?

::: devtools/server/actors/network-monitor.js:99
(Diff revision 2)
> +
> +    this._networkEventActorsByURL.set(actor._request.url, actor);
> +
> +    // /!\ here is the trick /!\
> +    // we use the web console actor ID in `from`, so that we keep the compatiblity.
> +    // It is as if the web console sent this message.

Haha, this is quite wacky! :P

You are quite lucky that this is a "one way" actor emitting events, I suppose...  If the client tried to talk to you based on this ID, it would of course not arrive here.

Please add even more detail to this comment to clarify that this actor is running in the parent process but reusing the target context's webconsle actor ID (so most likely in the content process) purely for compatibility reasons.  If compat was a not a concern, we would just let this actor speak with it's own ID.

::: devtools/server/actors/webconsole.js:625
(Diff revision 2)
>              break;
>            }
>            if (!this.networkMonitor) {
> +            let actor = this.conn.spawnActorInParentProcess({
> +              module: "devtools/server/actors/network-monitor",
> +              actorSymbol: "NetworkMonitorActor"

What about `constructor` instead?  That would match the `registerModule` API.

::: devtools/server/actors/webconsole.js:626
(Diff revision 2)
>            }
>            if (!this.networkMonitor) {
> +            let actor = this.conn.spawnActorInParentProcess({
> +              module: "devtools/server/actors/network-monitor",
> +              actorSymbol: "NetworkMonitorActor"
> +              // We may drop actorSymbol if we say that actor's module should export

Maybe so, but such a change feels quite unrelated to this work.  Perhaps a different bug might try to do that for all actors?  

With the current module structure, it seems useful to specify the constructor.

::: devtools/server/actors/webconsole.js:630
(Diff revision 2)
> +              actorSymbol: "NetworkMonitorActor"
> +              // We may drop actorSymbol if we say that actor's module should export
> +              // the Actor instance on module.exports?
> +            });
> +            if (actor) {
> +              actor.then(actorId => {

Maybe use `await` here?

::: devtools/server/actors/webconsole.js:637
(Diff revision 2)
> +
> +                // These two lines could be done within `spawnActorInParentProcess` by passing
> +                // "frontModule" and "frontSymbol" arguments, like for actor spawn in parent...
> +                // Then `spawnActorInParentProcess` would return a Front and we would not have
> +                // to know about actor IDs.
> +                let { NetworkMonitorFront } = require("devtools/shared/fronts/network-monitor");

I think this "manual" version is okay for now.  We can refine this part later on if needed.  The server already does way too many things...

::: devtools/server/actors/webconsole.js:640
(Diff revision 2)
> +                // Then `spawnActorInParentProcess` would return a Front and we would not have
> +                // to know about actor IDs.
> +                let { NetworkMonitorFront } = require("devtools/shared/fronts/network-monitor");
> +                let front = new NetworkMonitorFront(this.conn, { actor: actorId });
> +
> +                front.start(this.parentActor.outerWindowID, this.actorID);

If we make some kind of standard `spawnedByActorID` system as I mention elsewhere, you could drop the actor ID here, since it should be the same as that.

::: devtools/server/main.js:1066
(Diff revision 2)
>            return false;
>          }
>        };
>  
> +      let onSpawnActorInParent = function (msg) {
> +        // We may have multiple connectToChild instance running for the same tab

Recently renamed to `connectToFrame`; please update this comment.

::: devtools/server/main.js:1087
(Diff revision 2)
> +
> +          let constructor = m[actorSymbol];
> +          let instance = new constructor(connection, null);
> +          instance.conn = connection;
> +          instance.parentID = null;
> +          connection.addActor(instance);

So, this will give the actor a parent ID like:

server0.conn0.network-monitor0

We currently also have child IDs of the form:

server0.conn0.child0/bob0

Since this actor is running in the parent by design, we don't want any forwarding in the way.  At the same time though, I think it would be useful to somehow note that this actor was spawn from the child, which makes it different from others.

Should we try to pack that info into the actorID on the wire?  Should we at least record a new `spawnedByActorID` field on the actor?  Some standard way to track who spawn you sounds useful.

::: devtools/server/main.js:1112
(Diff revision 2)
>          // Pipe Debugger message from/to parent/child via the message manager
>          childTransport = new ChildDebuggerTransport(mm, prefix);
>          childTransport.hooks = {
> -          onPacket: connection.send.bind(connection),
> +          onPacket(packet) {
> +            dump("connecToChild.onPacket(to:"+packet.to+", from:"+packet.from+" type:"+packet.type+")\n");
> +            if (packet.to) {

A comment about the case you're handling would probably be helpful for future readers.

::: devtools/server/main.js:2003
(Diff revision 2)
> +      let listener = msg => {
> +        if (msg.json.prefix != this.prefix) {
> +          return;
> +        }
> +        removeMessageListener("debug:spawn-actor-in-parent:actor", listener);
> +        done(msg.json.actorID);

Nit: I think we typically use `resolve`...?

::: devtools/shared/fronts/network-monitor.js:10
(Diff revision 2)
> +"use strict";
> +
> +const { FrontClassWithSpec } = require("devtools/shared/protocol");
> +const { networkMonitorSpec } = require("devtools/shared/specs/network-monitor");
> +
> +const NetworkMonitorFront = FrontClassWithSpec(networkMonitorSpec, {});

Hmm, does an "empty" front like this get cleaned up correctly?  Typically we've had to "self-manage" fronts that aren't a child of some other p.js thing via `this.manage(this)`[1].

[1]: https://searchfox.org/mozilla-central/source/devtools/shared/fronts/emulation.js#16

::: devtools/shared/specs/network-monitor.js:18
(Diff revision 2)
> +    start: {
> +      request: {
> +        outerWindowID: Arg(0, "number"),
> +        webConsoleActorID: Arg(1, "string")
> +      },
> +      response: {

You can leave out keys like `response` if they are empty...  Some files do, some file don't... up to you.

::: devtools/shared/specs/network-monitor.js:24
(Diff revision 2)
> +      }
> +    },
> +    setStackTrace: {
> +      request: {
> +        channelID: Arg(0, "number"),
> +        stacktrace: Arg(1, "json")

Nit: "stack trace" is typically two words, so maybe `stackTrace` here?

::: devtools/shared/webconsole/network-monitor.js:201
(Diff revision 2)
>  
>    _saveStackTrace(channel, stacktrace) {
>      this.stacktracesById.set(channel.channelId, stacktrace);
> +
> +    // Communicate to the actor in parent process the stack trace:
> +    this.parentFront.setStackTrace(channel.channelId, stacktrace);

Hmm, should you instead consume the stack trace via `getStackTrace` instead?

Or is this just a temporary thing so that the existing code still works for comparison?
Comment on attachment 8957225 [details]
Bug 1444132 - Spawn a NetworkMonitorActor directly in the parent process.

https://reviewboard.mozilla.org/r/226158/#review238224

> Hmm, it does seem something lazy would be better here since the client is already trying to be lazy today...
> 
> I'd like to have more to say about this topic, but I just tried testing stock Nightly stack traces, and I don't seem to get them anymore...?

Oh, I guess I didn't realize that the stack traces moved to a sidebar tab. :S

What if instead we "do the right thing" and make the client directly ask an actor in the content process for the stack trace lazily when it's needed?  Sure, that we would be a protocol change, and we should discuss the compat impact... but that seems much closer to what we actually want to happen, so that we avoid worrying about stack traces here.

(My initial feeling on the compat impact is that we could make breaking change here...  Or if we want to keep compat, use a trait so the client knows who to ask for the stack.)

Might be best to make a separate commit for this issue.
Comment on attachment 8957225 [details]
Bug 1444132 - Spawn a NetworkMonitorActor directly in the parent process.

https://reviewboard.mozilla.org/r/226158/#review240006

::: devtools/server/main.js:1093
(Diff revision 3)
> +          instance.actorID = connection.allocID(contentPrefix + "/" + instance.typeName);
> +          connection.addActor(instance);
> +
> +          // Forward responses sent by this actor back to the content process
> +          let forwardPrefix = connection.prefix + contentPrefix
> +          connection.setActorToContentForwarding(forwardPrefix, childTransport);

I started addressing various of your comments (but not all of them yet).
But here is the main things I added.
Here where I request to pipe Actor messages back to content process.

::: devtools/server/main.js:1566
(Diff revision 3)
> +          dump("Connection.send(from:" + packet.from+" to:"+packet.to+" >>forwarded\n");
> +          return;
> +        }
> +        separator = from.lastIndexOf("/");
> +      }
> +    }

Here, where we actually pipe the message to content.

::: devtools/server/main.js:1852
(Diff revision 3)
> +      let front = this.getActor(packet.from);
> +      if (front) {
> +        dump("Connection.onPacket(from:" + packet.from+" to:"+packet.to+" actor:"+actor+" >> sent to front\n");
> +        front.onPacket(packet);
> +        return;
> +      }

On finally, here, where we hand responses over to Front in the content process.

As we discussed in 1:1.
I think I can implement all that without hacking things in DebuggerServerConnection.
If we uses distinct Connection objects, implemented just for this usecase in both side, we should be able to put this extra logic in these two Connection objects.
It will:
* keep existing code as-is without putting extra hacks just for this:
* these connection objects will only contain what we need for this spawnActorInParentProcess feature to work
* only add extra code still in connectToFrame
* still keep Client/Actor logic - RDP API as-is

I still think it will be a good iteration over existing code.
Now may be get rid of this concept of actors communicating with another code in another process, but it isn't clear how to do that from the client.
Attached image Network Communication Sequence Diagram (deleted) — 
I definitely agree that this patch is getting quite complex, like you mentioned in 1:1.  No matter what we end up doing here, I agree that extracting a Connection object would be valuable work regardless, and it should give us more flexibility in the future.

As for my own thinking, I sketched out a sequence diagram how all the parts here might work together.  In particular, I tried to imagine a version with only client <-> parent actor and client <-> content actor communication (so nothing between parent and content).

I am hopeful something similar can also be done for Storage as well, but I need more time to read through everything happening there.
Looking at Storage, I believe the approach I drew above would also work there.

Similar to Network actors, the Storage actors have a mix of content and parent process data:

# Content

* Local Storage
* Session Storage
* Cache API

# Parent

* IndexedDB
* Cookies

For the ones running in the parent, they mainly need to know the set of hosts the target cares about.  This could be passed as part of the `context` during a one-time startup message to launch the parent actor, similar to window ID in my Network diagram.

I suppose one tricky part is that the set of valid hosts can change while the tools are open if frames are created or destroyed.  Currently the main Storage actor watches for this case and recomputes things to update the UI to match.  So, we might need some some way to notify the parent process actors of host changes...  Do we really a full RDP connection between content and parent actors for just that, though...?  I am not sure it's worth it.  Once platform adds "abstract browsing trees" (bug 1437994), all processes will know some metadata about frames, so we could likely remove whatever messaging sends host updates for Storage at that point.

So after having looked at Network and Storage, I am thinking we want:

* Mechanism for content to launch a parent actor with some static startup data
* The parent actor splices directly into the RDP connection, talking to the client without content process involvement
* For Storage, we craft something light to emit host updates to the parent (that is hopefully slated for removal once we have bug 1437994)

How does that sound?
Flags: needinfo?(poirot.alex)
Product: Firefox → DevTools
I'm starting to get back to this now that NetworkEventActor refactoring has landed.

I would like to avoid submitting a complex patch doing both things at once and so have two independent steps:
* move actor from content to parent -and/or- stop piping all data from parent to content,
* change RDP API.

I think it would be better to change RDP API first by pulling NetMonitor class into an independent actor.
But this isn't that easy because of the duplicated network monitor instances necessary for service workers:
  https://searchfox.org/mozilla-central/rev/d0a41d2e7770fc00df7844d5f840067cc35ba26f/devtools/server/actors/webconsole.js#630-638
  // Start a network monitor in the parent process to listen to most requests that happen in parent
  this.networkMonitor = new NetworkMonitorChild(this.parentActor.outerWindowID, messageManager, this.conn, this);
  this.networkMonitor.init();
  // Spawn also one in the child to listen to service workers
  this.networkMonitorChild = new NetworkMonitor({ window }, this);
  this.networkMonitorChild.init();

It would mean that the client would have to maintain/communicate with two network-monitor actors.
Also note that the two NetworkMonitor instance here have to run in two distinct process...

Also, I saw that emulation actor is calling console actor methods directly, I'm not sure how to address that. I imagine the client should call the actor instead of doing cross actor calls like this...
  https://searchfox.org/mozilla-central/rev/d0a41d2e7770fc00df7844d5f840067cc35ba26f/devtools/server/actors/emulation.js#125-136

Otherwise, an incremental step could be to start from my first patch, that do not try to instanciate fronts in the server.
* instanciate NetworkMonitor via spawnActorInParentProcess
* no longer try to send events with console actor id (client doesn't care from which actor it is sent from)
* let NetworkMonitor be a real actor and send events with its id
* no longer pipe any data from parent to content (just the actor id, but it won't be used)
* no longer pipe any data from content to parent *except* setPreferences data (throttleData, saveRequestAndResponseBodies)
  We should be able to get rid of that by changing RDP API, but it is unclear yet how. setPreferences would be a method of NetworkMonitorActor.
* Fix stack traces retrieval.
  Today, we fetch stacks on-demand via NetworkEventActor.getStackTrace():
    https://searchfox.org/mozilla-central/rev/d0a41d2e7770fc00df7844d5f840067cc35ba26f/devtools/server/actors/network-event.js#254-258
  But as NetworkEventActor would now run in the parent process, it won't have access to the stacks.
  The stacks are available in StackCollector, which run in the content process and is currently hosted on the console actor.
  I see two options here:
  * start changing RDP API. Have a new getStackForNetworkEvent API. It would be on WebConsole actor, until we figure out how to change RDP API more seriously around network observation.
  * still use message manager to keep NetworkEventActor.getStackTrace working until we figure out the RDP API change. TBH I'm ok with that option. In such patch I would remove most of the message manager work, especially the one that have a significant impact on performance. We could then followup to get rid of all message manager usage by doing a significant RDP API change.
Flags: needinfo?(poirot.alex) → needinfo?(jryans)
(In reply to Alexandre Poirot [:ochameau] from comment #11)
> I would like to avoid submitting a complex patch doing both things at once
> and so have two independent steps:
> * move actor from content to parent -and/or- stop piping all data from
> parent to content,
> * change RDP API.

Breaking the work up into steps sounds great!

> I think it would be better to change RDP API first by pulling NetMonitor
> class into an independent actor.
> But this isn't that easy because of the duplicated network monitor instances
> necessary for service workers:
>  
> https://searchfox.org/mozilla-central/rev/
> d0a41d2e7770fc00df7844d5f840067cc35ba26f/devtools/server/actors/webconsole.
> js#630-638
>   // Start a network monitor in the parent process to listen to most
> requests that happen in parent
>   this.networkMonitor = new
> NetworkMonitorChild(this.parentActor.outerWindowID, messageManager,
> this.conn, this);
>   this.networkMonitor.init();
>   // Spawn also one in the child to listen to service workers
>   this.networkMonitorChild = new NetworkMonitor({ window }, this);
>   this.networkMonitorChild.init();
> 
> It would mean that the client would have to maintain/communicate with two
> network-monitor actors.
> Also note that the two NetworkMonitor instance here have to run in two
> distinct process...

I think we may want to make this leap though?  It's possible we'll find even more places to gather network data from in the future, and it would nice if we don't have to force their data to some single place because solely because client can only handle talking to one actor.

How hard does it seem to make the client listen to several actors?  It seems like maybe just a few tweaks to https://searchfox.org/mozilla-central/source/devtools/client/netmonitor/src/connector/firefox-connector.js.  How should the several actors be discovered?  Would each one be listed as a target-scoped actor for the tab, or something else?  It's possible the resource actor would could assist here, but of course it's not available yet.

> Also, I saw that emulation actor is calling console actor methods directly,
> I'm not sure how to address that. I imagine the client should call the actor
> instead of doing cross actor calls like this...
>  
> https://searchfox.org/mozilla-central/rev/
> d0a41d2e7770fc00df7844d5f840067cc35ba26f/devtools/server/actors/emulation.
> js#125-136

Yes, it's really only in the emulation actor for convenience of grouping all RDM functions.  If we need to change to the client messaging the network monitor instead, that seems fine.

> Otherwise, an incremental step could be to start from my first patch, that
> do not try to instanciate fronts in the server.
> * instanciate NetworkMonitor via spawnActorInParentProcess
> * no longer try to send events with console actor id (client doesn't care
> from which actor it is sent from)
> * let NetworkMonitor be a real actor and send events with its id
> * no longer pipe any data from parent to content (just the actor id, but it
> won't be used)
> * no longer pipe any data from content to parent *except* setPreferences
> data (throttleData, saveRequestAndResponseBodies)
>   We should be able to get rid of that by changing RDP API, but it is
> unclear yet how. setPreferences would be a method of NetworkMonitorActor.
> * Fix stack traces retrieval.
>   Today, we fetch stacks on-demand via NetworkEventActor.getStackTrace():
>    
> https://searchfox.org/mozilla-central/rev/
> d0a41d2e7770fc00df7844d5f840067cc35ba26f/devtools/server/actors/network-
> event.js#254-258
>   But as NetworkEventActor would now run in the parent process, it won't
> have access to the stacks.
>   The stacks are available in StackCollector, which run in the content
> process and is currently hosted on the console actor.
>   I see two options here:
>   * start changing RDP API. Have a new getStackForNetworkEvent API. It would
> be on WebConsole actor, until we figure out how to change RDP API more
> seriously around network observation.
>   * still use message manager to keep NetworkEventActor.getStackTrace
> working until we figure out the RDP API change. TBH I'm ok with that option.
> In such patch I would remove most of the message manager work, especially
> the one that have a significant impact on performance. We could then
> followup to get rid of all message manager usage by doing a significant RDP
> API change.

It seems fine to keep using message manager for the stacks as a temporary step that we can clean up later.  As an end state, maybe we'll have a new NetworkStackTraceActor on the target that can look up a stack trace by channel ID or something.  With Fission, there might be many such actors for frames.
Flags: needinfo?(jryans)
This new patch queue should be (almost) green and is already proving significant performance improvement, especially on exportHar: -61%!!

https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=try&originalRevision=d4ff17875c4d9b9005dd5fe8aaa20029b187b72d&newProject=try&newRevision=bf1658e39ffadb4808c659c6a5e7dceed29b5360&originalSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&newSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&showOnlyImportant=1&framework=1

This is still using message manager to keep the same RDP API, but also avoid refactoring too much code at once.
I would like you feedback, as I wish I could proceed as-is and open followup for every single message manager usage in order to get rid of them. Some may depend on the resource actors, some will be harder than the others (like sendHTTPRequest that has to do the request on the content process while returning the actor form the parent process...).
I was confinced I ni? you on comment 29, but did not...

Now, I spent some more time looking at the followups.
The first followup is about how replacing `webConsole.startListeners(["NetworkActivity"])` by something else, more specific to the netmonitor, allowing to retrieve multiple NetworkMonitorActor instances. This is a preliminary step in order to adress the message manager usages. For example, after that, it will be easy to call setPreferences directly on the NetworkMonitorActor.

I would move it to BrowsingContextActor as-is, but we may want to use the resource actor. It seems to match pretty well its abstraction. We want the list of netmonitor actor for the current target. And the list will be different, depending on the target. If the target is in the parent process or not.

I'm quite hesitant to make an RDP change if we are planning to re-change that once the resource actor lands...
Flags: needinfo?(jryans)
Comment on attachment 8993342 [details]
Bug 1444132 - Fix stacktrace support with the new setup.

https://reviewboard.mozilla.org/r/257584/#review265882

::: devtools/server/actors/network-monitor.js:66
(Diff revision 1)
>        this.netMonitor = null;
>      }
> +
> +    if (this.messageManager) {
> +      this.stackTraces.clear();
> +      this.messageManager.removeMessageListener("debug-request-stack-available",

I think we typically use "debug:" prefix instead of "debug-" for DevTools MM messages.
Comment on attachment 8993343 [details]
Bug 1444132 - HSTS redirections now have stack traces.

https://reviewboard.mozilla.org/r/257586/#review265884

Cool, a feature improvement! :D
Comment on attachment 8993344 [details]
Bug 1444132 - Fix stylesheet retrieval from network monitor with the new setup.

https://reviewboard.mozilla.org/r/257588/#review265886

::: devtools/server/actors/network-monitor.js:51
(Diff revision 1)
>        this.stackTraces = new Map();
>        this.onStackTraceAvailable = this.onStackTraceAvailable.bind(this);
>        this.messageManager.addMessageListener("debug-request-stack-available",
>          this.onStackTraceAvailable);
> +      this.onRequestContent = this.onRequestContent.bind(this);
> +      this.messageManager.addMessageListener("debug-request-content",

Another message prefix to maybe change to "debug:".
Cool! :) I think the patches you have here seem like a reasonable incremental step that gets us a lot of the perf and security benefits we were hoping for.

It does seem like the resource actor could help you to aggregate the network monitor instances like you mentioned, so maybe you'd want to wait before doing that follow up to see how the resource actor comes together.
Flags: needinfo?(jryans)
Depends on: 1477988
Blocks: 1478683
Blocks: 1478688
Attachment #8993341 - Attachment is obsolete: true
Almost green try before addressing eslint nits:
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=2782f1bd0b1de1e79e653b59fb34e075a7eb2b6c&selectedJob=190316815
I still have one issue with sendHttpRequest.
Comment on attachment 8957225 [details]
Bug 1444132 - Spawn a NetworkMonitorActor directly in the parent process.

https://reviewboard.mozilla.org/r/226158/#review266728


Code analysis found 1 defect in this patch:
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: devtools/server/actors/webconsole.js:633
(Diff revision 5)
>                // most requests than happen in parent
> -              this.networkMonitor =
> -                new NetworkMonitorChild(this.parentActor.outerWindowID,
> -                                        messageManager, this.conn, this);
> -              this.networkMonitor.init();
> +              this.networkMonitorActorId = await this.conn.spawnActorInParentProcess(
> +                this.actorID, {
> +                  module: "devtools/server/actors/network-monitor",
> +                  constructor: "NetworkMonitorActor",
> +                  args: [ { outerWindowID: this.parentActor.outerWindowID }, this.actorID ],

Error: Line 633 exceeds the maximum line length of 90. [eslint: max-len]
Comment on attachment 8957225 [details]
Bug 1444132 - Spawn a NetworkMonitorActor directly in the parent process.

https://reviewboard.mozilla.org/r/226158/#review266742

::: devtools/server/actors/webconsole.js:629
(Diff revision 5)
>              this.stackTraceCollector.init();
>  
>              if (messageManager && processBoundary) {
>                // Start a network monitor in the parent process to listen to
>                // most requests than happen in parent
> -              this.networkMonitor =
> +              this.networkMonitorActorId = await this.conn.spawnActorInParentProcess(

Where do you use `networkMonitorActorId`? 

Also, it's a bit confusing to see both `networkMonitorActorId` and `networkMonitorActor` properties... I know they have different purposes, but it's a bit hard to follow.
Attachment #8957225 - Flags: review?(jryans) → review+
Comment on attachment 8993342 [details]
Bug 1444132 - Fix stacktrace support with the new setup.

https://reviewboard.mozilla.org/r/257584/#review266744

::: devtools/server/actors/network-monitor.js:28
(Diff revision 2)
>    // `filters` argument either contains an `outerWindowID` attribute when this is used
>    // cross processes. Or a `window` attribute when instanciated in the same process.
>    // `parentID` is an optional argument, to be removed, that specify the ID of the Web
>    // console actor. This is used to fake emitting an event from it to prevent changing
>    // RDP behavior.
> -  initialize(conn, filters, parentID) {
> +  // `messageManager` is another optional argument, passed only when it is instanciated

What's the case where it's same process?  Maybe browser toolbox?

Wouldn't it be easier to always use the message manager path?  After all, that's typically how message manager is used: we always send messages, even if things are same process because it greatly simplifies code paths.

::: devtools/server/actors/network-monitor.js:33
(Diff revision 2)
> -  initialize(conn, filters, parentID) {
> +  // `messageManager` is another optional argument, passed only when it is instanciated
> +  // across processes. This is the manager to use to communicate with the other process.
> +  // `stackTraceCollector` when the actor run in the same process than the requests
> +  // we are inspecting, the web console actor hands over a shared instance to the stack
> +  // trace collector.
> +  initialize(conn, filters, parentID, messageManager, stackTraceCollector) {

This whole comment block could use some reworking in to the use `/** */` style for functions...
Attachment #8993342 - Flags: review?(jryans) → review+
Comment on attachment 8993343 [details]
Bug 1444132 - HSTS redirections now have stack traces.

https://reviewboard.mozilla.org/r/257586/#review266746
Attachment #8993343 - Flags: review?(jryans) → review+
Comment on attachment 8993344 [details]
Bug 1444132 - Fix stylesheet retrieval from network monitor with the new setup.

https://reviewboard.mozilla.org/r/257588/#review266750
Attachment #8993344 - Flags: review?(jryans) → review+
Comment on attachment 8993345 [details]
Bug 1444132 - Remove the old proxy code.

https://reviewboard.mozilla.org/r/257590/#review266752

Looks great, please find more crufty things to delete! :)
Attachment #8993345 - Flags: review?(jryans) → review+
Comment on attachment 8993346 [details]
Bug 1444132 - Fix netmonitor preferences with the new setup.

https://reviewboard.mozilla.org/r/257592/#review266756
Attachment #8993346 - Flags: review?(jryans) → review+
Comment on attachment 8993347 [details]
Bug 1444132 - Fix sendHTTPRequest with the new setup.

https://reviewboard.mozilla.org/r/257594/#review266760
Attachment #8993347 - Flags: review?(jryans) → review+
Comment on attachment 8993348 [details]
Bug 1444132 - Fetch stacks from content process only on-demand.

https://reviewboard.mozilla.org/r/257596/#review266762

::: devtools/shared/webconsole/network-monitor.js:192
(Diff revision 2)
>    init() {
>      Services.obs.addObserver(this, "http-on-opening-request");
>      ChannelEventSinkFactory.getService().registerCollector(this);
> +    if (this.messageManager) {
> +      this.onGetStack = this.onGetStack.bind(this);
> +      this.messageManager.addMessageListener("debug-request-stack", this.onGetStack);

Use `debug:` prefix.
Attachment #8993348 - Flags: review?(jryans) → review+
Comment on attachment 8993349 [details]
Bug 1444132 - Prevent race in browser_net_view_source-debugger.js if the stack button is lazily created.

https://reviewboard.mozilla.org/r/257598/#review266764
Attachment #8993349 - Flags: review?(jryans) → review+
(In reply to J. Ryan Stinnett [:jryans] from comment #49)
> Comment on attachment 8957225 [details]
> Bug 1444132 - Spawn a NetworkMonitorActor directly in the parent process.
> 
> https://reviewboard.mozilla.org/r/226158/#review266742
> 
> ::: devtools/server/actors/webconsole.js:629
> (Diff revision 5)
> >              this.stackTraceCollector.init();
> >  
> >              if (messageManager && processBoundary) {
> >                // Start a network monitor in the parent process to listen to
> >                // most requests than happen in parent
> > -              this.networkMonitor =
> > +              this.networkMonitorActorId = await this.conn.spawnActorInParentProcess(
> 
> Where do you use `networkMonitorActorId`? 
> 
> Also, it's a bit confusing to see both `networkMonitorActorId` and
> `networkMonitorActor` properties... I know they have different purposes, but
> it's a bit hard to follow.

In another patch to send a message to destroy it, or simply to differentiate in-parent-process from in-content-process.
I'll try to document all these fields.
I hope to be able to streamline this in bug 1478683, so that we have a netmonitor/requests scanner. The scanner will focus on managing them, so it should be easier to follow. Here is is quite hard to follow as it is in middle of the Gigantic web console actor...

(In reply to J. Ryan Stinnett [:jryans] from comment #50)
> Comment on attachment 8993342 [details]
> Bug 1444132 - Fix stacktrace support with the new setup.
> 
> https://reviewboard.mozilla.org/r/257584/#review266744
> 
> ::: devtools/server/actors/network-monitor.js:28
> (Diff revision 2)
> >    // `filters` argument either contains an `outerWindowID` attribute when this is used
> >    // cross processes. Or a `window` attribute when instanciated in the same process.
> >    // `parentID` is an optional argument, to be removed, that specify the ID of the Web
> >    // console actor. This is used to fake emitting an event from it to prevent changing
> >    // RDP behavior.
> > -  initialize(conn, filters, parentID) {
> > +  // `messageManager` is another optional argument, passed only when it is instanciated
> 
> What's the case where it's same process?  Maybe browser toolbox?

Yes, exactly. When the parent actor of the console one is the root actor.

> Wouldn't it be easier to always use the message manager path?  After all,
> that's typically how message manager is used: we always send messages, even
> if things are same process because it greatly simplifies code paths.

It sure would! I think this idea crossed my mind at some point.
I looked into this again but this is not trivial:
* the root actor is not associated to any window/message manager
* the console uses the browser console window:
  https://searchfox.org/mozilla-central/source/devtools/server/actors/webconsole.js#181-184
* but for the network monitor usecase, uses no window. That's to not filter by window and retrieve all the windows:
  https://searchfox.org/mozilla-central/source/devtools/server/actors/webconsole.js#
So there is no message manager to use that really makes sense, choosing one would be arbitrary.

We could choose the one for the topmost browser window, but I imagine it will break if we close it.

Also, we would have to evaluate the netmonitor actor in a frame script in order for it to have a reference to the sibling message manager that will work with the one we use in the console actor.
The following code doesn't log anything:
  let mm = gBrowser.selectedBrowser.messageManager;
  mm.addMessageListener("foo", console.log.bind(console, "bar"))
  mm.sendAsyncMessage("foo", {})
We would need something like spawnActorInParentProcess, but in the same process and may be with slightly different logic around the `connection` object.

At the end I think it would be a trade. It would simplfy this code but introduce this new spawnActorInSameProcess helper.
Do you think that a valuable trade? (assuming we find a useful message manager to use? Or may be we can use a mock, and do not need this spawnActorInSameProcess thing?)

(In reply to J. Ryan Stinnett [:jryans] from comment #53)
> Comment on attachment 8993345 [details]
> Bug 1444132 - Remove the old proxy code.
> 
> https://reviewboard.mozilla.org/r/257590/#review266752
> 
> Looks great, please find more crufty things to delete! :)

I'm pretty confident there is more to delete around netmonitor, but I missed it!
(In reply to Alexandre Poirot [:ochameau] from comment #58)
> (In reply to J. Ryan Stinnett [:jryans] from comment #49)
> > Wouldn't it be easier to always use the message manager path?  After all,
> > that's typically how message manager is used: we always send messages, even
> > if things are same process because it greatly simplifies code paths.
> 
> It sure would! I think this idea crossed my mind at some point.
> I looked into this again but this is not trivial:
> * the root actor is not associated to any window/message manager
> * the console uses the browser console window:
>  
> https://searchfox.org/mozilla-central/source/devtools/server/actors/
> webconsole.js#181-184
> * but for the network monitor usecase, uses no window. That's to not filter
> by window and retrieve all the windows:
>  
> https://searchfox.org/mozilla-central/source/devtools/server/actors/
> webconsole.js#
> So there is no message manager to use that really makes sense, choosing one
> would be arbitrary.
> 
> We could choose the one for the topmost browser window, but I imagine it
> will break if we close it.
> 
> Also, we would have to evaluate the netmonitor actor in a frame script in
> order for it to have a reference to the sibling message manager that will
> work with the one we use in the console actor.
> The following code doesn't log anything:
>   let mm = gBrowser.selectedBrowser.messageManager;
>   mm.addMessageListener("foo", console.log.bind(console, "bar"))
>   mm.sendAsyncMessage("foo", {})
> We would need something like spawnActorInParentProcess, but in the same
> process and may be with slightly different logic around the `connection`
> object.
> 
> At the end I think it would be a trade. It would simplfy this code but
> introduce this new spawnActorInSameProcess helper.
> Do you think that a valuable trade? (assuming we find a useful message
> manager to use? Or may be we can use a mock, and do not need this
> spawnActorInSameProcess thing?)

Hmm... maybe the mock idea is a good way to go? A fake message manager for same process activity seems like it would be easy to understand and is should also simplify this code greatly by always having a MM around.
Depends on: 1479524
(In reply to J. Ryan Stinnett [:jryans] from comment #59)
> Hmm... maybe the mock idea is a good way to go? A fake message manager for
> same process activity seems like it would be easy to understand and is
> should also simplify this code greatly by always having a MM around.

I started looking into that, but I'll do that in a followup bug 1479524.


I'll try to land this tomorrow to let some days between landing debugger's bug 1434305 and this one.
I'm expecting to see some impact on BHR for both.

DAMP still reports massive win on HAR export (63%):
https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=try&originalRevision=03e1f8759a9d7cfe3de45a450d4513917ac866db&newProject=try&newRevision=525382df29b2e30abc9cb62afd645abc4f9e538d&originalSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&newSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&showOnlyConfident=1&framework=1
Only 3.5% on requestsFinished (I was expecting more, message manager usages may regress the overall possible win here.
Various simple.* tests are slower but I'll assume that's because of the massive win on HAR that has an impact on all the subsequent tests.
Pushed by apoirot@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5fe879d4e4cb
Spawn a NetworkMonitorActor directly in the parent process. r=jryans
https://hg.mozilla.org/integration/autoland/rev/09b0af4bda95
Fix stacktrace support with the new setup. r=jryans
https://hg.mozilla.org/integration/autoland/rev/526c4eaaac80
HSTS redirections now have stack traces. r=jryans
https://hg.mozilla.org/integration/autoland/rev/c33d134af968
Fix stylesheet retrieval from network monitor with the new setup. r=jryans
https://hg.mozilla.org/integration/autoland/rev/862a9a773767
Remove the old proxy code. r=jryans
https://hg.mozilla.org/integration/autoland/rev/6afaf8f9db5e
Fix netmonitor preferences with the new setup. r=jryans
https://hg.mozilla.org/integration/autoland/rev/793019c3fd49
Fix sendHTTPRequest with the new setup. r=jryans
https://hg.mozilla.org/integration/autoland/rev/22470174dfa0
Fetch stacks from content process only on-demand. r=jryans
https://hg.mozilla.org/integration/autoland/rev/e702d9a79546
Prevent race in browser_net_view_source-debugger.js if the stack button is lazily created. r=jryans
browser_ext_devtools_network.js failure highlighted a pre-existing failure in har codebase, calling Connector.getTabTarget with null `this`.
And there was also an error on test verify because html_cause-test-page.html was dispatching requests in random orders.
Flags: needinfo?(poirot.alex)
Comment on attachment 8957225 [details]
Bug 1444132 - Spawn a NetworkMonitorActor directly in the parent process.

https://reviewboard.mozilla.org/r/226158/#review267694

::: devtools/server/actors/network-event.js:75
(Diff revision 7)
> -    if (!this.webConsoleActor) {
> +    if (!this.netMonitorActor) {
>        return;
>      }

Should we remove this?

This was a valid check for the previous actor because we used to nullify this.webConsoleActor, but it doesn't seem relevant here. netMonitorActor is never nullified and must be not null for initialize() to complete without crashing.

::: devtools/server/actors/network-monitor.js:14
(Diff revision 7)
> +
> +loader.lazyRequireGetter(this, "NetworkMonitor", "devtools/shared/webconsole/network-monitor", true);
> +loader.lazyRequireGetter(this, "NetworkEventActor", "devtools/server/actors/network-event", true);
> +
> +const NetworkMonitorActor = ActorClassWithSpec(networkMonitorSpec, {
> +  // Imported from WebConsole actor

Is this comment still useful in the final implementation?

::: devtools/server/actors/network-monitor.js:19
(Diff revision 7)
> +  // Imported from WebConsole actor
> +  _netEvents: new Map(),
> +  _networkEventActorsByURL: new Map(),
> +
> +  // NetworkMonitorActor is instanciated from WebConsoleActor.startListeners
> +  // Either in the same process, for debugging service worker requests or when debugger

nit: debugger -> debugging
Comment on attachment 8996325 [details]
Bug 1444132 - Always wait for full Connector setup in getHarExportConnector.

https://reviewboard.mozilla.org/r/260466/#review267696

The test still fails intermittently for me with the patch. 
All the methods from connector/index are normally bound to the instance, so calling `getTabTarget()` or `connector.getTabTarget()` should be the same.

I added a few logs and it seems a connector gets a getTabTarget call before this.connector is created.

The stack that leads to the getTabTarget call is:

getTabTarget@resource://devtools/shared/base-loader.js -> resource://devtools/client/netmonitor/src/connector/index.js:105:19
buildHarData@resource://devtools/shared/base-loader.js -> resource://devtools/client/netmonitor/src/har/har-exporter.js:195:9
async*fetchHarData@resource://devtools/shared/base-loader.js -> resource://devtools/client/netmonitor/src/har/har-exporter.js:159:12
getHar@resource://devtools/shared/base-loader.js -> resource://devtools/client/netmonitor/src/har/har-exporter.js:129:12
onRequestAdded@resource://devtools/shared/base-loader.js -> resource://devtools/client/netmonitor/src/api.js:137:23
Async*emit@resource://devtools/shared/base-loader.js -> resource://devtools/shared/event-emitter.js:178:37
emit@resource://devtools/shared/base-loader.js -> resource://devtools/shared/event-emitter.js:255:29
emit@resource://devtools/shared/base-loader.js -> resource://devtools/client/netmonitor/src/connector/firefox-data-provider.js:656:7
addRequest@resource://devtools/shared/base-loader.js -> resource://devtools/client/netmonitor/src/connector/firefox-data-provider.js:95:5
async*onNetworkEvent@resource://devtools/shared/base-loader.js -> resource://devtools/client/netmonitor/src/connector/firefox-data-provider.js:328:11
Async*displayCachedEvents@resource://devtools/shared/base-loader.js -> resource://devtools/client/netmonitor/src/connector/firefox-connector.js:225:7
connect@resource://devtools/shared/base-loader.js -> resource://devtools/client/netmonitor/src/connector/firefox-connector.js:89:7
Async*connectFirefox@resource://devtools/shared/base-loader.js -> resource://devtools/client/netmonitor/src/connector/index.js:76:12
connectBackend@resource://devtools/shared/base-loader.js -> resource://devtools/client/netmonitor/src/api.js:93:12
async*connect@resource://devtools/shared/base-loader.js -> resource://devtools/client/netmonitor/src/api.js:66:11
async*getNetMonitorAPI@resource://devtools/shared/base-loader.js -> resource://devtools/client/framework/toolbox.js:3266:11
async*waitForRequestAdded/<@chrome://mochitests/content/browser/browser/components/extensions/test/browser/test-oop-extensions/browser_ext_devtools_network.js:109:26
async*waitForRequestAdded@chrome://mochitests/content/browser/browser/components/extensions/test/browser/test-oop-extensions/browser_ext_devtools_network.js:108:10
navigateToolboxTarget@chrome://mochitests/content/browser/browser/components/extensions/test/browser/test-oop-extensions/browser_ext_devtools_network.js:123:5
async*test_devtools_network_on_request_finished@chrome://mochitests/content/browser/browser/components/extensions/test/browser/test-oop-extensions/browser_ext_devtools_network.js:236:9

So the network monitor api file has two connectors: this.connector and this.harExportConnector. And it looks like the connection of this.connector triggers a network event that the second one is not ready to handle.
I believe a fix for that is in async getHarExportConnector(). We need to make sure the connector is connected before returning, and this is not done if we early return (if (this.harExportConnector) { return this.harExportConnector; })
Attachment #8996325 - Flags: review?(jdescottes)
The suggested fix works for me locally but I have no idea why your changes trigger this issue?
Comment on attachment 8996326 [details]
Bug 1444132 - Fix the order of requests done by html_cause-test-page.html test.

https://reviewboard.mozilla.org/r/260468/#review267712

Looks good to me thanks!
The test was not failing with --verify before this patch series however? Why is the random order an issue now?
Attachment #8996326 - Flags: review?(jdescottes) → review+
(In reply to Julian Descottes [:jdescottes][:julian] from comment #87)
> Comment on attachment 8996325 [details]
> Bug 1444132 - Call Connector.getTabTarget with right `this` value.
> 
> https://reviewboard.mozilla.org/r/260466/#review267696
> 
> The test still fails intermittently for me with the patch. 

Oh right, I didn't realized it was intermittent, especially given that both local and try run were green.
But with a couple more runs, it starts failing:
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=a7be8bc631d25bfddbc33ea971450a6122ab4e4f&selectedJob=191315452

(In reply to Julian Descottes [:jdescottes][:julian] from comment #88)
> The suggested fix works for me locally but I have no idea why your changes
> trigger this issue?

It does not work for me?

I tried this:
@@ -180,17 +181,21 @@ NetMonitorAPI.prototype = {
     if (this.harExportConnector) {
       return this.harExportConnector;
     }
-
-    const connection = {
-      tabConnection: {
-        tabTarget: this.toolbox.target,
-      },
-      toolbox: this.toolbox,
-    };
-
-    this.harExportConnector = new Connector();
-    await this.connectBackend(this.harExportConnector, connection);
-    return this.harExportConnector;
+    // Save a promise in order to ensure only resolving the connector
+    // after connectBackend completes.
+    this.harExportConnector = (async () => {
+      const connection = {
+        tabConnection: {
+          tabTarget: this.toolbox.target,
+        },
+        toolbox: this.toolbox,
+      };
+
+      const connector = new Connector();
+      await this.connectBackend(connector, connection);
+      return connector;
+    })();
+    return this.harExportConnected;
   },
 };
(In reply to Julian Descottes [:jdescottes][:julian] from comment #88)
> The suggested fix works for me locally but I have no idea why your changes
> trigger this issue?

Ok, I finally found a difference!
I think it is a simple race where the new code is faster.

On current tip:
  INFO Developer toolbox opened.
  >> getNetworkEvents(0)
  onNetworkEvent(http://example.com/)
  >>> getHarExportConnector new
  INFO Wait for an onRequestFinished event
  >> getTabTarget

With the patch, but without your fix:
  INFO Developer toolbox opened.
  onNetworkEvent(http://example.com/) <=== here the actor notifies about the request *before* the call to getNetworkEvents()
  >> getNetworkEvents(1) <<==== here we process one request on startup, whereas before, we were not.
  >>> getHarExportConnector new
  >>> getHarExportConnector cached
  >> getTabTarget
  => getTabTarget throws on this.connector is null

With the patch and your fix (without the race):
  INFO Developer toolbox opened.
  >> getNetworkEvents(0) <<== this is a race, so sometimes the requests is received just after, like before
  onNetworkEvent(http://example.com/)
  >>> getHarExportConnector new
  INFO Wait for an onRequestFinished event
  >> getTabTarget

With the patch and your fix (with the race):
  INFO Developer toolbox opened.
  onNetworkEvent(http://example.com/)
  >> getNetworkEvents(1)
  >>> getHarExportConnector new
  >>> getHarExportConnector cached
  INFO Wait for an onRequestFinished event
  >> getTabTarget
  => getTabTarget NO LONGER throws on this.connector is null

getNetworkEvents refers to consoleClient.getNetworkEvents, that is being called from here:
  https://searchfox.org/mozilla-central/source/devtools/client/netmonitor/src/connector/firefox-connector.js#223
  It is used to process requests recorded before opening the netmonitor panel (and after the toolbox opening).
  It will ultimately introduce an early new call to getHarExportConnector from:
    https://searchfox.org/mozilla-central/source/devtools/client/netmonitor/src/api.js#124
  Thus, introducing the additional call to getHarExportConnector, in middle of its initialization...


(In reply to Julian Descottes [:jdescottes][:julian] from comment #89)
> The test was not failing with --verify before this patch series however? Why
> is the random order an issue now?

I've not looked into that, but given my investigation on connector, the new code is significantly faster to notify about the incoming request. So given that the test itself doesn't guarantee that the requests are done in a precise order, I'm not surprised that this test fails.
(In reply to Julian Descottes [:jdescottes][:julian] from comment #86)
> Comment on attachment 8957225 [details]
> Bug 1444132 - Spawn a NetworkMonitorActor directly in the parent process.
> 
> https://reviewboard.mozilla.org/r/226158/#review267694
> 
> ::: devtools/server/actors/network-event.js:75
> (Diff revision 7)
> > -    if (!this.webConsoleActor) {
> > +    if (!this.netMonitorActor) {
> >        return;
> >      }
> 
> Should we remove this?
> 
> This was a valid check for the previous actor because we used to nullify
> this.webConsoleActor, but it doesn't seem relevant here. netMonitorActor is
> never nullified and must be not null for initialize() to complete without
> crashing.

I meant to remove the useless call to `actor.releaseActor(this);`,
but not really the nullification of its parent actor.
I think it is still valuable to nullify it, so I restored it.

> ::: devtools/server/actors/network-monitor.js:14
> (Diff revision 7)
> > +
> > +loader.lazyRequireGetter(this, "NetworkMonitor", "devtools/shared/webconsole/network-monitor", true);
> > +loader.lazyRequireGetter(this, "NetworkEventActor", "devtools/server/actors/network-event", true);
> > +
> > +const NetworkMonitorActor = ActorClassWithSpec(networkMonitorSpec, {
> > +  // Imported from WebConsole actor
> 
> Is this comment still useful in the final implementation?

No, thanks for noticing it!

> ::: devtools/server/actors/network-monitor.js:19
> (Diff revision 7)
> > +  // Imported from WebConsole actor
> > +  _netEvents: new Map(),
> > +  _networkEventActorsByURL: new Map(),
> > +
> > +  // NetworkMonitorActor is instanciated from WebConsoleActor.startListeners
> > +  // Either in the same process, for debugging service worker requests or when debugger
> 
> nit: debugger -> debugging

fixed.
Comment on attachment 8996325 [details]
Bug 1444132 - Always wait for full Connector setup in getHarExportConnector.

https://reviewboard.mozilla.org/r/260466/#review267952

Works for me if try is green!
Attachment #8996325 - Flags: review?(jdescottes) → review+
Pushed by apoirot@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/853c9128ee24
Spawn a NetworkMonitorActor directly in the parent process. r=jryans
https://hg.mozilla.org/integration/autoland/rev/13e0438d7362
Fix stacktrace support with the new setup. r=jryans
https://hg.mozilla.org/integration/autoland/rev/550cdc18a51b
HSTS redirections now have stack traces. r=jryans
https://hg.mozilla.org/integration/autoland/rev/ace2f8e7a9c9
Fix stylesheet retrieval from network monitor with the new setup. r=jryans
https://hg.mozilla.org/integration/autoland/rev/48ac26a8a15c
Remove the old proxy code. r=jryans
https://hg.mozilla.org/integration/autoland/rev/c993f615d1e6
Fix netmonitor preferences with the new setup. r=jryans
https://hg.mozilla.org/integration/autoland/rev/bb4fb7bb9ab2
Fix sendHTTPRequest with the new setup. r=jryans
https://hg.mozilla.org/integration/autoland/rev/50e7a79bb869
Fetch stacks from content process only on-demand. r=jryans
https://hg.mozilla.org/integration/autoland/rev/3ba0fd5c1def
Prevent race in browser_net_view_source-debugger.js if the stack button is lazily created. r=jryans
https://hg.mozilla.org/integration/autoland/rev/3baa0a93d12d
Fix the order of requests done by html_cause-test-page.html test. r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/8b61609228c9
Always wait for full Connector setup in getHarExportConnector. r=jdescottes
DAMP perf wins thanks to this:

== Change summary for alert #14704 (as of Thu, 02 Aug 2018 09:56:28 GMT) ==

Improvements:

  3%  damp linux64-qr opt e10s stylo     290.35 -> 281.62
  3%  damp windows7-32 pgo e10s stylo    240.10 -> 233.04
  2%  damp windows10-64 opt e10s stylo   254.12 -> 248.57

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=14704
Looks like this sped up all the complicated.reload and complicated.close by ~10% ! Simple and custom are not impacted but that makes sense because they don't have any network activity.
Depends on: 1487284
Depends on: 1490927
Whiteboard: dt-fission
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: