731311 - Network monitor should allow to replay and edit requests

Status: RESOLVED FIXED

(DevTools :: Netmonitor, defect, P3)

Description

[Mihai Sucan [:msucan]]

A common use case when developing web applications and especially when looking into deployment is to analyze HTTP requests and responses. The current network panel is a perfect fit for that, but it lacks the ability to replay network requests.

We need a way to replay any of the logged requests and equally important is the ability to edit the request headers.


This is based on a use case I had this weekend: had to deploy a web app and I needed to replay specific network requests and I also needed to make custom network requests. I had to use Opera's Dragonfly for this. Chrome's Web Inspector and Firebug did not provide obvious ways to do this (if they do at all).

Comment 1

[Kevin Dangoor]

Firebug has a "resend" menu item on the network panel context menu for a request. Just thought I'd mention that, but I agree with the idea of this bug.

Comment 2

[Mihai Sucan [:msucan]]

(In reply to Kevin Dangoor from comment #1)
> Firebug has a "resend" menu item on the network panel context menu for a
> request. Just thought I'd mention that, but I agree with the idea of this
> bug.

This is why I mentioned "no obvious way". Thanks! I just learned something.

For Firebug it would be very useful to improve the UI. Like add a "replay" button in the request/response headers view.

Comment 3

[Kevin Dangoor]

(In reply to Mihai Sucan [:msucan] from comment #2)
> For Firebug it would be very useful to improve the UI. Like add a "replay"
> button in the request/response headers view.

Agreed... context menus are not super discoverable. In fact, the only reason I know about this is that it's a relatively recent addition to Firebug and I've mentioned it in a couple of talks where I highlighted some recent Firebug additions.

of course, the challenge is not filling the screen with every possible command. I like your suggestion of putting it in the header view.

Comment 4

[Heather Arthur [:harth]]

I started working on this. I'll post a prototype soon for feedback.

Comment 5

[Victor Porof [:vporof][:vp]]

That's awesome, thanks Heather!

Comment 6

[Heather Arthur [:harth]]

Attachment: WIP prototype - resend + edit request

This is a prototype. The code is pretty hacky, I'm just looking for feedback on the functionality and UX.

Comment 7

[Mihai Sucan [:msucan]]

Comment on attachment 758367 [details] [diff] [review]
WIP prototype - resend + edit request

Review of attachment 758367 [details] [diff] [review]:
-----------------------------------------------------------------

Definitely f+! This is really great Heather! Thanks for taking this bug.

Tested the patch and I have the following comments:

- for some network requests Resend doesn't do anything. Load my ugly page [1] and try Resend for test.html. The new request form doesn't disappear but the "new request" item in the requests list disappears. Maybe there's an exception, but Promises probably eat it away (browser console doesn't show anything unusual).

- the "new request" form always shows the last user input values, even if I go from one request to another.

- the "Resend" context menu item has the "C" access key - that is odd.

Given this is a work-in-progress patch, I don't worry about any of the above comments, or anything I mention below. I am just writing these comments so we don't forget these ideas.

[1] http://mihaisucan.github.io/mozilla-work/test.html

::: toolkit/devtools/server/actors/webconsole.js
@@ +1009,5 @@
>      return actor;
>    },
>  
> +  getNetworkEventActor: function(aChannel) {
> +    if (this._netEvents.has(aChannel)) {

Is each unique request associated to a unique channel? Do you know if different requests can reuse channels?

@@ +1030,5 @@
> +      request.setRequestHeader(name, value);
> +    }
> +    request.send(aRequest.body);
> +
> +    let actor = this.getNetworkEventActor(request.channel);

I see you use the channel's map to NetworkEventActors map for http requests we send. This makes sense. However, do we want to include in the map all of the other network events?

I see you added the |channel| property to network event actors and NetworkMonitor from WebConsoleUtils exposes it to the actor. I see why you did it, but I wonder if we could avoid it / limit it. I specifically wanted to separate out any actual lower-level network request objects - such that the network event actor doesn't know/use them at all. I would like the NetworkMonitor to keep things abstracted out as much as possible.

I'm not making this a blocker for review - I am just putting this out, maybe we can find something that avoids exposing the http channel. If not, that's not a problem.

Comment 8

[Heather Arthur [:harth]]

(In reply to Mihai Sucan [:msucan] from comment #7)
> - for some network requests Resend doesn't do anything. Load my ugly page
> [1] and try Resend for test.html. The new request form doesn't disappear but
> the "new request" item in the requests list disappears. Maybe there's an
> exception, but Promises probably eat it away (browser console doesn't show
> anything unusual).

That could be because it gets cached? bug 764958. But I can make the form disappear at least.

> Given this is a work-in-progress patch, I don't worry about any of the above
> comments, or anything I mention below. I am just writing these comments so
> we don't forget these ideas.
> 
> [1] http://mihaisucan.github.io/mozilla-work/test.html
> 
> ::: toolkit/devtools/server/actors/webconsole.js
> @@ +1009,5 @@
> >      return actor;
> >    },
> >  
> > +  getNetworkEventActor: function(aChannel) {
> > +    if (this._netEvents.has(aChannel)) {
> 
> Is each unique request associated to a unique channel? Do you know if
> different requests can reuse channels?

They are unique to each request, as far as I can tell from researching.

> 
> @@ +1030,5 @@
> > +      request.setRequestHeader(name, value);
> > +    }
> > +    request.send(aRequest.body);
> > +
> > +    let actor = this.getNetworkEventActor(request.channel);
> 
> I see you use the channel's map to NetworkEventActors map for http requests
> we send. This makes sense. However, do we want to include in the map all of
> the other network events?

Pardon my ignorance, what other network events? I created the map so that I could associate the sent request with the exact network event that ends up being created for it, in that sense, it wouldn't be necessary for other kinds of events.

> 
> I see you added the |channel| property to network event actors and
> NetworkMonitor from WebConsoleUtils exposes it to the actor. I see why you
> did it, but I wonder if we could avoid it / limit it. I specifically wanted
> to separate out any actual lower-level network request objects - such that
> the network event actor doesn't know/use them at all. I would like the
> NetworkMonitor to keep things abstracted out as much as possible.
> 
> I'm not making this a blocker for review - I am just putting this out, maybe
> we can find something that avoids exposing the http channel. If not, that's
> not a problem.

I see your point. I can't think of another property that could uniquely identify an event, but I'll look into it.

Comment 9

[Mihai Sucan [:msucan]]

(In reply to Heather Arthur [:harth] from comment #8)
> (In reply to Mihai Sucan [:msucan] from comment #7)
> > - for some network requests Resend doesn't do anything. Load my ugly page
> > [1] and try Resend for test.html. The new request form doesn't disappear but
> > the "new request" item in the requests list disappears. Maybe there's an
> > exception, but Promises probably eat it away (browser console doesn't show
> > anything unusual).
> 
> That could be because it gets cached? bug 764958. But I can make the form
> disappear at least.

That's possible. Please double check.


> > Is each unique request associated to a unique channel? Do you know if
> > different requests can reuse channels?
> 
> They are unique to each request, as far as I can tell from researching.

Good then.


> > @@ +1030,5 @@
> > > +      request.setRequestHeader(name, value);
> > > +    }
> > > +    request.send(aRequest.body);
> > > +
> > > +    let actor = this.getNetworkEventActor(request.channel);
> > 
> > I see you use the channel's map to NetworkEventActors map for http requests
> > we send. This makes sense. However, do we want to include in the map all of
> > the other network events?
> 
> Pardon my ignorance, what other network events? I created the map so that I
> could associate the sent request with the exact network event that ends up
> being created for it, in that sense, it wouldn't be necessary for other
> kinds of events.

Sorry, my wording was confusing. Please let me reword the above: you use the channel's map to NetworkEventActors (_netEvents object) for the sendHTTPRequest packet type, so you can return the network event actor grip immediately for the new XMLHTTPRequest, based on its channel. This is fine. The getNetworkEventActor() method is used for all network requests, not only for those generated by onSendHTTPRequest(). I just wanted to know if we could keep this map smaller/limited only to the network events that are generated by onSendHTTPRequest(), since that's what we need the map for. What do you think about this?

The NetworkMonitor in WCU cleans-up all references to the request objects, like the HTTP channel, once the network request completes, to avoid memory leaks. I would suggest that this is also done for the _netEvents map. Afaik, Gecko has some smarts about garbage-collecting request channels once there is no JS object holding onto them. In this patch _netEvents is cleared only once the NEA is released, which could be much later in time.


> > I'm not making this a blocker for review - I am just putting this out, maybe
> > we can find something that avoids exposing the http channel. If not, that's
> > not a problem.
> 
> I see your point. I can't think of another property that could uniquely
> identify an event, but I'll look into it.

Thank you!

Comment 10

[Heather Arthur [:harth]]

(In reply to Mihai Sucan [:msucan] from comment #9)
> (In reply to Heather Arthur [:harth] from comment #8)
> > (In reply to Mihai Sucan [:msucan] from comment #7)
> > > - for some network requests Resend doesn't do anything. Load my ugly page
> > > [1] and try Resend for test.html. The new request form doesn't disappear but
> > > the "new request" item in the requests list disappears. Maybe there's an
> > > exception, but Promises probably eat it away (browser console doesn't show
> > > anything unusual).
> > 
> > That could be because it gets cached? bug 764958. But I can make the form
> > disappear at least.
> 
> That's possible. Please double check.

Confirmed that these requests don't appear because they're hitting the cache. Should definitely fix bug 764958, because this would be confusing.

Thanks for the feedback Mihai, I addressed all your other concerns. I indeed did not need to add any other events to the Map, and could even delete them right after checking for them the first time. Patch forthcoming.

Comment 11

[Heather Arthur [:harth]]

Attachment: Resend + edit requests

This adds a "Resend" button and context menu item. When selecting it will open up an editable form in the sidebar populated with the details of the request.

It could be cool to re-use the details view for this instead of showing a form, but it seemed like it would take more work to make it editable enough.

Comment 12

[Victor Porof [:vporof][:vp]]

Comment on attachment 763307 [details] [diff] [review]
Resend + edit requests

Review of attachment 763307 [details] [diff] [review]:
-----------------------------------------------------------------

Really clean patch, thank you Heather. r+ with the comments below addressed. 

First, some small findings and papercuts:

1. While editing stuff when replaying, I can switch to a different request and click replay again, at which point two "New request" badges appear in the menu. This behavior should either not be allowed, or the badges should contain more information about what initiated them.

2. The query string textarea should always be present, or there should be a way of activating it, even when the original request url didn't contain any query. I think it's a reasonable use case to be able to add, not only modify, queries.

3. The excessive contrast between dark background in the sidebar and the form items hurts my eyes a bit, and doesn't look very good. I think it would work much better with a white background, the one in the Options panel.

4. Nit: the Resend button adds a bit of extra margin to the "Status code" label. It feels awkward to me, but you might find good arguments for it.

5. (followup) For multipart/form-data requests, it'd be nice to have a way of adding/handling binary data, like files, images etc. Right now, a form which sends a file adds a blob of garbled text in the "Request Body" form. Maybe integrate with bug 812616.

::: browser/devtools/netmonitor/netmonitor-view.js
@@ +364,5 @@
> +  /**
> +   * Create a new custom request form populated with the data from
> +   * the currently selected request.
> +   */
> +  cloneRequest: function() {

Maybe rename this to cloneCurrentlySelectedRequest?

@@ +366,5 @@
> +   * the currently selected request.
> +   */
> +  cloneRequest: function() {
> +    let label = document.createElement("label");
> +    label.setAttribute("value", "New Request");

Need to localize this string.

@@ +372,5 @@
> +    let selected = this.selectedItem.attachment;
> +
> +    let body;
> +    if (selected.requestPostData) {
> +      body = selected.requestPostData.postData.text;

Need to use gNetwork.getString here since |text| is a longString and you're not guaranteed here that it's already fetched.

@@ +376,5 @@
> +      body = selected.requestPostData.postData.text;
> +    }
> +
> +    let newItem = this.push(label, {
> +      attachment: {

Just clone the previous attachment. It'd be easier than manually copying all of these.

let newItem = this.push(label, {
  attachment: Object.create(this.selectedItem.attachment, {
    isCustom: { value: true }
  })
});

@@ +385,5 @@
> +        body: body
> +      }
> +    });
> +
> +    // immediately switch to new request pane

Nit: Capitalize I and end with a period please.

@@ +392,5 @@
> +
> +  /**
> +   * Send a new HTTP request using the data in the custom request form.
> +   */
> +  sendRequest: function() {

sendCustomRequest? :)

@@ +397,5 @@
> +    let data = this.selectedItem.attachment;
> +
> +    NetMonitorController.webConsoleClient.sendHTTPRequest(data, (response) => {
> +      let id = response.eventActor.actor;
> +      this._itemToSelectId = id;

This is a bit dirty and seems fragile. If you have multiple "New request"s, then this fails doesn't it?

In any case, it'd be nice to have a better way of handling such things (there is a preferredValue setter, for example, in MenuContainers, but that doesn't handle attachments). I will think of a thing, but this is ok for now. Maybe renaming it to _preferredRequestId would make more sense until I make the preferredValue setter smarter.

@@ +1151,5 @@
>  
>    /**
> +   * Handle the context menu opening. Hide items if no request is selected.
> +   */
> +  onContextShowing: function() {

Please mark this as private.

@@ +2046,5 @@
> + * @return string aText
> + *         List of headers in text format
> + */
> +function writeHeaderText(aHeaders) {
> +  return [(name + ": " + value) for ({name, value} of aHeaders)].join("\n");

This is so pretty! <3

@@ +2058,5 @@
> + * @return string
> + *         List of query params in text format
> + */
> +function writeQueryText(aParams) {
> +  return [(name + "=" + value) for ({name, value} of aParams)].join("\n");

Oh my!

@@ +2090,5 @@
>  NetMonitorView.Toolbar = new ToolbarView();
>  NetMonitorView.RequestsMenu = new RequestsMenuView();
>  NetMonitorView.NetworkDetails = new NetworkDetailsView();
> +NetMonitorView.CustomRequest = new CustomRequestView();
> +NetMonitorView.Sidebar = new SidebarView();

Uber nit: It'd be cleaner if you instantiated these views in the order in which they are defined: Sidebar, CustomRequest, NetworkDetails.

::: browser/devtools/netmonitor/netmonitor.xul
@@ +20,5 @@
> +  <popupset id="networkPopupSet">
> +    <menupopup id="network-request-popup"
> +               onpopupshowing="NetMonitorView.RequestsMenu.onContextShowing(event);">
> +      <menuitem id="request-menu-context-resend"
> +        label="&netmonitorUI.summary.resend;"

Uber nit: please align attributes vertically.

@@ +226,5 @@
> +          <textbox id="custom-method-value"
> +             oninput="NetMonitorView.CustomRequest.onUpdate();"
> +             multiline="true"
> +             cols="6"
> +             rows="1"/>

Ditto.

@@ +451,1 @@
>    </box>

Even though It'll add more noise to the patch, please just indent the <deck> and all of its contents as necessary.

::: browser/devtools/netmonitor/test/browser_net_resend.js
@@ +1,5 @@
> +/* Any copyright is dedicated to the Public Domain.
> +   http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +let gPanelWin;
> +let gPanelDoc;

Nit: you could avoid globals altogether by defining everything inside test(), but I really don't care that much.

@@ +139,5 @@
> +function finishUp(aMonitor) {
> +  gPanelWin = null;
> +  gPanelDoc = null;
> +
> +  teardown(aMonitor).then(finish);

This was a beautifully written test, good work!

Comment 13

[Mihai Sucan [:msucan]]

Comment on attachment 763307 [details] [diff] [review]
Resend + edit requests

Review of attachment 763307 [details] [diff] [review]:
-----------------------------------------------------------------

Went through the patch. Thanks for the updates.

I'm still worried about the HTTP channels still being referenced by NEA instances for longer than strictly needed. Maybe you could clear netEventActor.channel when it is removed from _netEvents?

Just a couple of nits below, otherwise the patch has an (un)official r+ from me. Really good work!

::: toolkit/devtools/server/actors/webconsole.js
@@ +88,5 @@
>    this.dbg = new Debugger();
>  
>    this._protoChains = new Map();
>    this._dbgGlobals = new Map();
> +  this._netEvents = new Map();

Please also add _netEvents: null, in the prototype definition, with a comment describing what it is used for.

@@ +1269,5 @@
> +   *
> +   * @param object aNetworkEvent
> +   *        The network event associated with this actor.
> +   */
> +  setEvent: function setEvent(aNetworkEvent)

nit: add NEA_ prefix to function name.

Also, maybe init() is more suited than setEvent()?

Comment 14

[Heather Arthur [:harth]]

Attachment: Resend + edit requests v2

Thanks for the reviews. I think I fixed the issues except:

> 3. The excessive contrast between dark background in the sidebar and the
> form items hurts my eyes a bit, and doesn't look very good. I think it would
> work much better with a white background, the one in the Options panel.

I agree it's harsh. I tried out the light background: http://i.imgur.com/cAzYdeX.png, but I think the dark still looks better.

> 4. Nit: the Resend button adds a bit of extra margin to the "Status code"
> label. It feels awkward to me, but you might find good arguments for it.

That was unintentional. I thought it looked awkward too, and maybe we could file separately? Some serious XUL fu is needed, I think.

> > +   * Handle the context menu opening. Hide items if no request is selected.
> > +   */
> > +  onContextShowing: function() {
> 
> Please mark this as private.

It's referenced in netmonitor.xul, and I felt weird saying _onContextShowing from there. Is that what you mean by mark as private?

Comment 15

[Victor Porof [:vporof][:vp]]

(In reply to Heather Arthur [:harth] from comment #14)
> Created attachment 766204 [details] [diff] [review]
> Resend + edit requests v2
> 
> Thanks for the reviews. I think I fixed the issues except:
> 
> > 3. The excessive contrast between dark background in the sidebar and the
> > form items hurts my eyes a bit, and doesn't look very good. I think it would
> > work much better with a white background, the one in the Options panel.
> 
> I agree it's harsh. I tried out the light background:
> http://i.imgur.com/cAzYdeX.png, but I think the dark still looks better.
> 

You're right, that looks even more weird. We needs some shorlander magic here.

> > 4. Nit: the Resend button adds a bit of extra margin to the "Status code"
> > label. It feels awkward to me, but you might find good arguments for it.
> 
> That was unintentional. I thought it looked awkward too, and maybe we could
> file separately? Some serious XUL fu is needed, I think.
> 

Yeah, don't worry about it. It's just UI nitpicking.

> > > +   * Handle the context menu opening. Hide items if no request is selected.
> > > +   */
> > > +  onContextShowing: function() {
> > 
> > Please mark this as private.
> 
> It's referenced in netmonitor.xul, and I felt weird saying _onContextShowing
> from there. Is that what you mean by mark as private?

It does feel weird, but all methods that shouldn't be called directly by an API consumer (which is another human writing code) need to be marked as private. At least, that's one pattern I tried to keep in the netmonitor codebase.

Comment 16

[Victor Porof [:vporof][:vp]]

Comment on attachment 766204 [details] [diff] [review]
Resend + edit requests v2

Review of attachment 766204 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/devtools/netmonitor/netmonitor-view.js
@@ +377,5 @@
> +
> +    if (selected.requestPostData) {
> +      let body = selected.requestPostData.postData.text;
> +      gNetwork.getString(body).then((aString) => {
> +        attachment.body = aString;

This isn't synchronous, so when you call _createMenuView below, the .body property may have not been set yet. Methinks you should just clone the existing attachment (with Object.create) and try to get the the actual post data body (with gNetwork.getString) inside CustomRequestView.populate. What do you think?

@@ +384,5 @@
> +
> +    // Create the element node for the network request item.
> +    let menuView = this._createMenuView(selected.method, selected.url);
> +
> +    let newItem = this.push(menuView, {

This will become
> let newItem = this.push([menuView], {
since bug 884006.

Comment 17

[Victor Porof [:vporof][:vp]]

Comment on attachment 766204 [details] [diff] [review]
Resend + edit requests v2

Shorlander, how can we make this look prettier?

Comment 18

[Victor Porof [:vporof][:vp]]

(In reply to Victor Porof [:vp] from comment #17)
> Shorlander, how can we make this look prettier?

Umm, I mean for http://i.imgur.com/cAzYdeX.png

Comment 19

[Victor Porof [:vporof][:vp]]

(In reply to Heather Arthur [:harth] from comment #14)
> Created attachment 766204 [details] [diff] [review]
> Resend + edit requests v2
> 

This needs a small rebase.

Comment 20

[Heather Arthur [:harth]]

Attachment: Resend + edit requests v3

Addressed latest comment.

Any chance of saving UI changes for a followup? 1) People using it could provide feedback 2) This patch adds a context menu to the requests list, which other bugs could consume (bug 794152, bug 859059)

Comment 21

[Victor Porof [:vporof][:vp]]

(In reply to Heather Arthur [:harth] from comment #20)
> Created attachment 767583 [details] [diff] [review]
> Resend + edit requests v3
> 
> Addressed latest comment.
> 
> Any chance of saving UI changes for a followup? 1) People using it could
> provide feedback 

Yes, of course! It shouldn't block this patch for landing.

> 2) This patch adds a context menu to the requests list,
> which other bugs could consume (bug 794152, bug 859059)

\o/

Comment 22

[Victor Porof [:vporof][:vp]]

Filed bug 887135 and bug 887134.

Comment 23

[Victor Porof [:vporof][:vp]]

Landed, but with the wrong patch author (I didn't catch the fact that it didn't have any author, so when importing it, hg automatically assumed it's me): [0] so I backed out: [1] and landed again [2]. Sorry about the mess.

[0] https://hg.mozilla.org/integration/fx-team/rev/5414c83bb989
[1] https://hg.mozilla.org/integration/fx-team/rev/b57d9f8db028
[2] https://hg.mozilla.org/integration/fx-team/rev/a4d856768629

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a4d856768629

Comment 25

[Mihai Sucan [:msucan]]

Do we have documentation for web devs about this feature?

We also need to update the docs for the Web Console actor about the changes in this patch:

https://developer.mozilla.org/en-US/docs/Tools/Web_Console/remoting