Closed Bug 1479524 Opened 6 years ago Closed 6 years ago

Use message manager mocks to share the same codepath for all NetworkMonitorActor instances

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

Details

(Whiteboard: dt-fission)

Attachments

(3 files)

Bug 1479524 - Always use message manager with NetworkMonitorActor.
59 bytes, text/x-review-board-request
jdescottes
: review+
Details
Bug 1479524 - Remove unused appId filter.
59 bytes, text/x-review-board-request
jdescottes
: review+
Details
Bug 1479524 - Lazy load network-monitor.js dependencies.
59 bytes, text/x-review-board-request
jdescottes
: review+
Details 
Bug 1444132 introduces NetworkMonitorActor but instanciate them in two distinct ways:
* across processes, with a message manager argument,
* in the same process (when the console actor runs in parent process), without a message manager, but a direct reference to stack trace collector.

It would be great to use message manager API in both cases,
but as the code has to live in the same process as the code creating it, we can't use message manager.
Here there is no tab involved. We would need to have a mock to intantiate two message manager in the same process.
Blocks: 1482070
Comment on attachment 8996280 [details]
Bug 1479524 - Always use message manager with NetworkMonitorActor.

https://reviewboard.mozilla.org/r/260440/#review269312

Thanks for the patch Alex, I think this is on the right track. I have a few comments, I will be happy to take another look, but since I am leaving on PTO on wednesday, I will set r+ now in case you don't have time to update the patch before that. My comments are mostly about readability anyway. 

Note that unless I am mistaken and the "actorID" is actually used to destroy the proper netmonitor instance, then we don't have to store the actorID in the `netmonitors` array, and we don't have to add the actors to the pool early?

::: devtools/server/actors/network-monitor.js:32
(Diff revision 4)
>     * @param number parentID (optional)
>     *        To be removed, specify the ID of the Web console actor.
>     *        This is used to fake emitting an event from it to prevent changing RDP
>     *        behavior.
> -   * @param nsIMessageManager messageManager (optional)
> -   *        Passed only when it is instanciated across processes. This is the manager to
> +   * @param nsIMessageManager messageManager
> +   *        This is the manager to use to communicate with the other process.

Should we expand this a bit and say that this is to communicate with the webconsole actor, either with a real nsIMessageManager if both actors live in different processes or with a MockMessageManager in case both actors are in the samme process?

::: devtools/server/actors/network-monitor.js:145
(Diff revision 4)
>      }
>    },
>  
>    onGetNetworkEventActor({ data }) {
>      const actor = this.getNetworkEventActor(data.channelId);
> -    this.messageManager.sendAsyncMessage("debug:get-network-event-actor", actor.form());
> +    this.messageManager.sendAsyncMessage("debug:get-network-event-actor", {

Not related to your change, but I am a bit confused by the messages passed around with the message manager. If I understand correctly, the webconsole actor sends "debug:get-network-event-actor", here the network-monitor listens to this message and responds to is with another "debug:get-network-event-actor" but this time to return the result? Would it make sense to have different names here, maybe with kind of naming pattern? "debug:something-something-request" "debug:something-something-result"

::: devtools/server/actors/webconsole.js:330
(Diff revision 4)
>      if (this.consoleServiceListener) {
>        this.consoleServiceListener.destroy();
>        this.consoleServiceListener = null;
>      }
> -    if (this.networkMonitorActor) {
> -      this.networkMonitorActor.destroy();
> +    if (this.netmonitors) {
> +      for (const { actorId, messageManager } of this.netmonitors) {

actorID?

::: devtools/server/actors/webconsole.js:332
(Diff revision 4)
> -    }
> -    if (this.networkMonitorActorId) {
> -      const messageManager = this.parentActor.messageManager;
> -      if (messageManager) {
>          messageManager.sendAsyncMessage("debug:destroy-network-monitor", {
> -          actorId: this.networkMonitorActorId
> +          actorId

actorID? really used? (similar comment down below)

::: devtools/server/actors/webconsole.js:618
(Diff revision 4)
>              break;
>            }
> -          if (!this.networkMonitorActorId && !this.networkMonitorActor) {
> -            // Create a StackTraceCollector that's going to be shared both by
> -            // the NetworkMonitorActor running in the same process for service worker
> -            // requests, as well with the NetworkMonitorActor running in the parent
> +          if (!this.netmonitors) {
> +            // Check if the actor is running in a child process (but only if
> +            // Services.appinfo exists, to prevent startListeners to fail
> +            // when the target is a Worker).

Why is the isWorker guard above not enough to prevent this issue?

::: devtools/server/actors/webconsole.js:626
(Diff revision 4)
> -            this.stackTraceCollector.init();
>  
> -            if (messageManager && processBoundary) {
> +            // Instanciate a fake message manager used for service worker's netmonitor
> +            // when running in the content process, and for netmonitor running in the
> +            // same process when running in the parent process.
> +            const messageManagerMocks = createMessageManagerMocks();

If we were using distinct messages for each action (instead of reusing the same name for request+response, see prev. comment), could we use only a single message-manager-mock for both the webconsole-actor side and the netmonitor-actor side?

We are not just getting an array of mocks, we are getting a couple of connected objects that must be used in a specific way. We should try to explain this coupling either:
- via a comment (`createMessageManagerMocks returns a couple of connected messages managers that pass messages to each other to simulate the process boundary. We will use the first one for the webconsole-actor and the second one will be used by the netmonitor-actor`)
- via explicit APIs (eg instanciate the two managers here and call a method "connectToOtherMessageManager" on one of them etc...)

::: devtools/server/actors/webconsole.js:628
(Diff revision 4)
> -            if (messageManager && processBoundary) {
> +            // Instanciate a fake message manager used for service worker's netmonitor
> +            // when running in the content process, and for netmonitor running in the
> +            // same process when running in the parent process.
> +            const messageManagerMocks = createMessageManagerMocks();
> +
> +            // Maintain le list of message manager we should message to/listen from

nit: "le list of..." :)

::: devtools/server/actors/webconsole.js:631
(Diff revision 4)
> +            const messageManagerMocks = createMessageManagerMocks();
> +
> +            // Maintain le list of message manager we should message to/listen from
> +            // to support the netmonitor instances, also records actorID of each
> +            // NetworkMonitorActor.
> +            // Array of `{ actorId, messageManager, parentProcess }`.

actorId -> actorID

::: devtools/server/actors/webconsole.js:638
(Diff revision 4)
> +            // parent process.
> +            this.netmonitors = [];
> +
> +            if (processBoundary && this.parentActor.messageManager) {
>                // Start a network monitor in the parent process to listen to
> -              // most requests than happen in parent
> +              // most requests than happen in parent. This one will communicated through

than -> that,
communicated -> communicate

::: devtools/server/actors/webconsole.js:655
(Diff revision 4)
> -              // Spawn also one in the child to listen to service workers
> -              this.networkMonitorChildActor = new NetworkMonitorActor(this.conn,
> +              // Spawn also one in the child to listen to service workers that will
> +              // communicate through `messageManagerMock`

can we rephrase that to say "Spawn also one in the child to listen to requests that happen in the child process (for instance service workers requests)". It might seem redundant, but the whole setup is pretty complex.

::: devtools/server/actors/webconsole.js:673
(Diff revision 4)
> +              const actor = new NetworkMonitorActor(this.conn, { window },
> +                this.actorID, messageManagerMocks[0]);

Can we use the same indentation as L657-660?

Most of the code is shared between the two blocks. Maybe mutualizing and extracting some things to functions could make the differences clearer? If I may, I would like to suggest something along the lines of

if (isRunningInChildProcess) {
  createNetActorInParent();
  createNetActorInSameProcess({ isParent: false });
} else {
  createNetActorInSameProcess({ isParent: true });
}

Just a suggestion, feel free to ignore.

::: devtools/server/actors/webconsole.js:793
(Diff revision 4)
>            }
>            stoppedListeners.push(listener);
>            break;
>          case "NetworkActivity":
> -          if (this.networkMonitorActor) {
> -            this.networkMonitorActor.destroy();
> +          if (this.netmonitors) {
> +            for (const { actorId, messageManager } of this.netmonitors) {

actorID ?

::: devtools/server/actors/webconsole.js:795
(Diff revision 4)
> -          }
> -          if (this.networkMonitorActorId) {
> -            const messageManager = this.parentActor.messageManager;
> -            if (messageManager) {
>                messageManager.sendAsyncMessage("debug:destroy-network-monitor", {
> -                actorId: this.networkMonitorActorId
> +                actorId

I don't see where the actorID/Id is used when catching this message?

In the long run, I think we should extract the netmonitor-actor part of the webconsole to a separate class & file.

::: devtools/server/actors/webconsole.js:1835
(Diff revision 4)
> -          if (data.url == url) {
> +        if (data.url != url) {
> +          return;
> +        }
> +        // Either use the first response with a content, or return a null content
> +        // if we received the responses from all the message managers.
> +        if (data.content || ++messagesReceived == this.netmonitors.length) {

Can we increment messagesReceived outside of this if? 

Even if this technically works, incrementing messagesReceived is not technically connected to not having data content. A message has been received regardless.

::: devtools/server/actors/webconsole.js:1898
(Diff revision 4)
> -      return {
> -        eventActor: actor.form()
> +    const netmonitor = this.netmonitors.filter(({ parentProcess }) => parentProcess)[0];
> +    const { messageManager } = netmonitor;

This works because by design we always have at least a parentProcess, but for an external reader these lines may seem fragile. Can we add a comment to reinforce that this code is safe here?
Attachment #8996280 - Flags: review?(jdescottes) → review+
Comment on attachment 8998780 [details]
Bug 1479524 - Remove unused appId filter.

https://reviewboard.mozilla.org/r/261652/#review269316

::: devtools/shared/webconsole/network-monitor.js
(Diff revision 1)
>        }
>      }
>    }
>  
> -  if (filters.appId) {
> -    const appId = NetworkHelper.getAppIdForRequest(channel);

getAppIdForRequest is no longer used and can be removed.
Attachment #8998780 - Flags: review?(jdescottes) → review+
Comment on attachment 8998781 [details]
Bug 1479524 - Lazy load network-monitor.js dependencies.

https://reviewboard.mozilla.org/r/261654/#review269318
Attachment #8998781 - Flags: review?(jdescottes) → review+
(In reply to Julian Descottes [:jdescottes][:julian] from comment #8)
> Note that unless I am mistaken and the "actorID" is actually used to destroy
> the proper netmonitor instance, then we don't have to store the actorID in
> the `netmonitors` array, and we don't have to add the actors to the pool
> early?

That's exact. I meant to check for the actorID on the network monitor side, but forgot doing it.
I imagine we can drop actorID in `netmonitors` array and compare against web console ID instead.
I switched to that.

> ::: devtools/server/actors/network-monitor.js:32
> (Diff revision 4)
> >     * @param number parentID (optional)
> >     *        To be removed, specify the ID of the Web console actor.
> >     *        This is used to fake emitting an event from it to prevent changing RDP
> >     *        behavior.
> > -   * @param nsIMessageManager messageManager (optional)
> > -   *        Passed only when it is instanciated across processes. This is the manager to
> > +   * @param nsIMessageManager messageManager
> > +   *        This is the manager to use to communicate with the other process.
> 
> Should we expand this a bit and say that this is to communicate with the
> webconsole actor, either with a real nsIMessageManager if both actors live
> in different processes or with a MockMessageManager in case both actors are
> in the samme process?

Expanded!
 
> ::: devtools/server/actors/network-monitor.js:145
> (Diff revision 4)
> >      }
> >    },
> >  
> >    onGetNetworkEventActor({ data }) {
> >      const actor = this.getNetworkEventActor(data.channelId);
> > -    this.messageManager.sendAsyncMessage("debug:get-network-event-actor", actor.form());
> > +    this.messageManager.sendAsyncMessage("debug:get-network-event-actor", {
> 
> Not related to your change, but I am a bit confused by the messages passed
> around with the message manager. If I understand correctly, the webconsole
> actor sends "debug:get-network-event-actor", here the network-monitor
> listens to this message and responds to is with another
> "debug:get-network-event-actor" but this time to return the result? Would it
> make sense to have different names here, maybe with kind of naming pattern?
> "debug:something-something-request" "debug:something-something-result"

Yes, I realize it can be disturbing. I'll fix that in a dedicated followup.

> ::: devtools/server/actors/webconsole.js:618
> (Diff revision 4)
> >              break;
> >            }
> > -          if (!this.networkMonitorActorId && !this.networkMonitorActor) {
> > -            // Create a StackTraceCollector that's going to be shared both by
> > -            // the NetworkMonitorActor running in the same process for service worker
> > -            // requests, as well with the NetworkMonitorActor running in the parent
> > +          if (!this.netmonitors) {
> > +            // Check if the actor is running in a child process (but only if
> > +            // Services.appinfo exists, to prevent startListeners to fail
> > +            // when the target is a Worker).
> 
> Why is the isWorker guard above not enough to prevent this issue?

I think `isWorker` is enough, I copied this `processBoundary` definition line from above,
where `isWorker` isn't bailing out early.
So I removed this comment and check against Services.appinfo.

> ::: devtools/server/actors/webconsole.js:626
> (Diff revision 4)
> > -            this.stackTraceCollector.init();
> >  
> > -            if (messageManager && processBoundary) {
> > +            // Instanciate a fake message manager used for service worker's netmonitor
> > +            // when running in the content process, and for netmonitor running in the
> > +            // same process when running in the parent process.
> > +            const messageManagerMocks = createMessageManagerMocks();
> 
> If we were using distinct messages for each action (instead of reusing the
> same name for request+response, see prev. comment), could we use only a
> single message-manager-mock for both the webconsole-actor side and the
> netmonitor-actor side?

We could, but I would prefer keeping the same behavior than message manager to avoid debugging in-same-process only bugs.
The whole point of message manager is to have the exact same behavior between in-same-process and OOP.

> We are not just getting an array of mocks, we are getting a couple of
> connected objects that must be used in a specific way. We should try to
> explain this coupling either:
> - via a comment (`createMessageManagerMocks returns a couple of connected
> messages managers that pass messages to each other to simulate the process
> boundary. We will use the first one for the webconsole-actor and the second
> one will be used by the netmonitor-actor`)
> - via explicit APIs (eg instanciate the two managers here and call a method
> "connectToOtherMessageManager" on one of them etc...)

I went for the comment and named the array values. Hopefully, it is clearer now:
  const [ mmMockParent, mmMockChild ] = createMessageManagerMocks();

> ::: devtools/server/actors/webconsole.js:655
> (Diff revision 4)
> > -              // Spawn also one in the child to listen to service workers
> > -              this.networkMonitorChildActor = new NetworkMonitorActor(this.conn,
> > +              // Spawn also one in the child to listen to service workers that will
> > +              // communicate through `messageManagerMock`
> 
> can we rephrase that to say "Spawn also one in the child to listen to
> requests that happen in the child process (for instance service workers
> requests)". It might seem redundant, but the whole setup is pretty complex.

Rephrased.

> 
> ::: devtools/server/actors/webconsole.js:673
> (Diff revision 4)
> > +              const actor = new NetworkMonitorActor(this.conn, { window },
> > +                this.actorID, messageManagerMocks[0]);
> 
> Can we use the same indentation as L657-660?

Done.
 
> Most of the code is shared between the two blocks. Maybe mutualizing and
> extracting some things to functions could make the differences clearer? If I
> may, I would like to suggest something along the lines of
> 
> if (isRunningInChildProcess) {
>   createNetActorInParent();
>   createNetActorInSameProcess({ isParent: false });
> } else {
>   createNetActorInSameProcess({ isParent: true });
> }
> 
> Just a suggestion, feel free to ignore.

Instead of function, I merged the two branches into only one.
I found it easier as this code uses a couple of local variables.
I had to tweak comments again.

> ::: devtools/server/actors/webconsole.js:795
> (Diff revision 4)
> > -          }
> > -          if (this.networkMonitorActorId) {
> > -            const messageManager = this.parentActor.messageManager;
> > -            if (messageManager) {
> >                messageManager.sendAsyncMessage("debug:destroy-network-monitor", {
> > -                actorId: this.networkMonitorActorId
> > +                actorId
> 
> I don't see where the actorID/Id is used when catching this message?

It is used for destroy message, but I realized that I'm not checking it on the other side, in network-monitor.js...
I fixed that.

> In the long run, I think we should extract the netmonitor-actor part of the
> webconsole to a separate class & file.

That's the plan! I do want to use the resource actor to do that.
Bug 1478683 is meant to do that even if its title isn't really clear about that.

> ::: devtools/server/actors/webconsole.js:1835
> (Diff revision 4)
> > -          if (data.url == url) {
> > +        if (data.url != url) {
> > +          return;
> > +        }
> > +        // Either use the first response with a content, or return a null content
> > +        // if we received the responses from all the message managers.
> > +        if (data.content || ++messagesReceived == this.netmonitors.length) {
> 
> Can we increment messagesReceived outside of this if? 
> 
> Even if this technically works, incrementing messagesReceived is not
> technically connected to not having data content. A message has been
> received regardless.

Good catch, fixed.

> ::: devtools/server/actors/webconsole.js:1898
> (Diff revision 4)
> > -      return {
> > -        eventActor: actor.form()
> > +    const netmonitor = this.netmonitors.filter(({ parentProcess }) => parentProcess)[0];
> > +    const { messageManager } = netmonitor;
> 
> This works because by design we always have at least a parentProcess, but
> for an external reader these lines may seem fragile. Can we add a comment to
> reinforce that this code is safe here?

Done.


By the way, do not hesitate to suggest any followups if you see something to be improved/fixed unrelated to this patch!
Blocks: 1482990
Comment on attachment 8996280 [details]
Bug 1479524 - Always use message manager with NetworkMonitorActor.

https://reviewboard.mozilla.org/r/260440/#review269418

Thanks!

::: devtools/server/actors/webconsole.js:668
(Diff revision 5)
> -                this.actorID,
> +              this.actorID,
> -                null,
> -                this.stackTraceCollector);
> -            } else {
> -              this.networkMonitorActor = new NetworkMonitorActor(this.conn, { window },
> -                this.actorID, null, this.stackTraceCollector);
> +              mmMockParent);
> +
> +            // We don't use NeworkMonitorActor as an actor yet, but as we want an
> +            // actorID, we have to add it to a pool.
> +            this._actorPool.addActor(actor);

This is no longer needed (or maybe for a different reason than the one suggested by the comment)?
Thanks for addressing my comments!

(In reply to Alexandre Poirot [:ochameau] from comment #14)
> 
> By the way, do not hesitate to suggest any followups if you see something to
> be improved/fixed unrelated to this patch!

Simple thing, but I would love to split devtools/shared/webconsole/network-monitor.js in several files (I think it contains ~5 classes) but I forgot to mention it during the review I think.
(In reply to Julian Descottes [:jdescottes][:julian] from comment #16)
> Thanks for addressing my comments!
> 
> (In reply to Alexandre Poirot [:ochameau] from comment #14)
> > 
> > By the way, do not hesitate to suggest any followups if you see something to
> > be improved/fixed unrelated to this patch!
> 
> Simple thing, but I would love to split
> devtools/shared/webconsole/network-monitor.js in several files (I think it
> contains ~5 classes) but I forgot to mention it during the review I think.

There is already bug 1482070 on file, that will hopefully help a bit on this.
Pushed by apoirot@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0f25b95e9740
Always use message manager with NetworkMonitorActor. r=jdescottes
https://hg.mozilla.org/integration/mozilla-inbound/rev/7f6bb6c2bc01
Remove unused appId filter. r=jdescottes
https://hg.mozilla.org/integration/mozilla-inbound/rev/fa0a81fe2da6
Lazy load network-monitor.js dependencies. r=jdescottes
See Also: → 1484764
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: