Closed Bug 1406610 Opened 7 years ago Closed 7 years ago

No HTTP details for requests executed before the Console is activated

Categories

(DevTools :: Console, defect, P1)

Product:
DevTools
Component:
Console
Type:
defect

Tracking

(firefox57 wontfix, firefox58 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 58
Tracking Flags:
Tracking Status
firefox57 --- wontfix
firefox58 --- fixed

People

(Reporter: Honza, Assigned: Honza)

Details

(Whiteboard: [reserve-console-html])

Attachments

(2 files)

Bug 1406610 - No HTTP details for requests executed before the Console is activated;
59 bytes, text/x-review-board-request
nchevobbe
: review+
Details
Bug 1406610 - Implementing test;
59 bytes, text/x-review-board-request
nchevobbe
: review+
Details 
STR:

1) Open DevTools, make sure the Net panel is the default
2) Load google.com
3) Switch to the Console panel
4) Open one of the existing HTTP requests
5) No Headers (and other details) -> BUG

Honza
I did analyze this issue and:

1) The problem happens for requests that are executed before the Console is attached to the backend.

2) The attach happens in `WebConsoleConnectionProxy._onAttachConsole`

3) This method registers a listener for "networkEvent" & "networkEventUpdate"

4) Important is the "networkEventUpdate" since other messages (as well as network events) are cached. But all 'update' net events are lost.

5) If 'update' events are not handled, than `NETWORK_MESSAGE_UPDATE` are not fired and "networkMessagesUpdateById" in `messages` reducer are not updated (an entry in the "networkMessagesUpdateById" not created). So, NETWORK_UPDATE_REQUEST has no effect since it needs that entry.

6) The patch is related to `enableNetProvider` (implemented in webconsole/new-console-output/store.js). It executes NETWORK_MESSAGE_UPDATE when network log is expanded for the first time, so the entry in "networkMessagesUpdateById" is created.
However, this should only probably happen only for requests that has been executed before console attach. Or at least we should make sure there is no perf regression if we do it for every request.

Honza
Whiteboard: [reserve-console-html]
Assignee: nobody → odvarko
Status: NEW → ASSIGNED
Priority: -- → P1
Comment on attachment 8916530 [details]
Bug 1406610 - No HTTP details for requests executed before the Console is activated;

https://reviewboard.mozilla.org/r/187666/#review196754

::: devtools/client/webconsole/new-console-output/store.js:195
(Diff revision 2)
> +          // If there is no network request update received for this
> +          // request-log, it's likely that it comes from cache.
> +          // I.e. it's been executed before the console panel started
> +          // listening from network events. Let fix that by updating
> +          // the reducer now.
> +          if (!updates[action.id]) {
> +            newState = reducer(newState, {
> +              type: NETWORK_MESSAGE_UPDATE,
> +              message: message,
> +            });
> +          }

I have trouble understanding this.
Executing the reducer will gives us the data we need ? Don't we need to fetch it ?
(In reply to Nicolas Chevobbe [:nchevobbe] from comment #5)
> > +          // If there is no network request update received for this
> > +          // request-log, it's likely that it comes from cache.
> > +          // I.e. it's been executed before the console panel started
> > +          // listening from network events. Let fix that by updating
> > +          // the reducer now.
> > +          if (!updates[action.id]) {
> > +            newState = reducer(newState, {
> > +              type: NETWORK_MESSAGE_UPDATE,
> > +              message: message,
> > +            });
> > +          }
> 
> I have trouble understanding this.
> Executing the reducer will gives us the data we need ? Don't we need to
> fetch it ?
Executing the reducer means that the `networkMessagesUpdateById` is updated
(a key with actor created). The key is needed for proper handling
NETWORK_UPDATE_REQUEST event (in the same reducer).

Honza
Comment on attachment 8916530 [details]
Bug 1406610 - No HTTP details for requests executed before the Console is activated;

https://reviewboard.mozilla.org/r/187666/#review196766

Okay, so now I get it. We do have the data in the message, but this was not reflected in the store because we never got through the reducer that does that, since all was done in the cache.
Can we add a sentence to the comment saying something alike, so it's clear for the reader that we just need to shape the data in a certain way.

This would need a test as well, a mocha one might be enough maybe ?
Attachment #8916530 - Flags: review?(nchevobbe) → review+
(In reply to Nicolas Chevobbe [:nchevobbe] from comment #7)
> Can we add a sentence to the comment saying something alike, so it's clear
> for the reader that we just need to shape the data in a certain way.
Done

> This would need a test as well, a mocha one might be enough maybe ?
Mocha test would be better, but here is what I need:
- select the net panel by default make sure console doesn't listen
- make a request, wait till it's done (payload fetched)
- selected console (activate) and expand the request (from cache)
- check that HTTP details are there.

We already test this (browser_webconsole_network_messages_expand.js), the only difference
is that console panel wasn't listening when the request was executed.

Can we cover that by Mocha test?

Honza
your patch check if have some data on the store and if we don't call a reducer.
This looks like something that is testable with mocha.
We could have a stub representing a cached message and checking that when we try to call open on it, we do what we expect.
I guess this might be a bit hard to have everything in place in mocha ? So if a mochitest is easier, I'm fine with it too :)
Comment on attachment 8922362 [details]
Bug 1406610 - Implementing test;

https://reviewboard.mozilla.org/r/193408/#review199060

Some style comment but this is good to land. test is green in verify mode locally :)

::: devtools/client/webconsole/new-console-output/test/mochitest/browser_webconsole_network_attach.js:13
(Diff revision 1)
> +Services.prefs.setBoolPref(NET_PREF, false);
> +Services.prefs.setBoolPref(XHR_PREF, true);
> +registerCleanupFunction(() => {
> +  Services.prefs.clearUserPref(NET_PREF);
> +  Services.prefs.clearUserPref(XHR_PREF);
> +});

I think we could use pushPref here ? http://searchfox.org/mozilla-central/source/devtools/client/framework/test/shared-head.js#557-561

::: devtools/client/webconsole/new-console-output/test/mochitest/browser_webconsole_network_attach.js:70
(Diff revision 1)
> +  return new Promise(resolve => {
> +    panel.once("NetMonitor:PayloadReady", () => {
> +      info("NetMonitor:PayloadReady received");
> +      resolve();
> +    });
> +  });

I think we can simplify this in

```js
await panel.once("NetMonitor:PayloadReady");
info("NetMonitor:PayloadReady received");
```

and given how small it is, and only used once in the test, we could remove waitForNetPayloadReady and inline this

::: devtools/client/webconsole/new-console-output/test/mochitest/browser_webconsole_network_attach.js:83
(Diff revision 1)
> +  return new Promise(resolve => {
> +    ui.jsterm.hud.on("network-request-payload-ready", () => {
> +      info("network-request-payload-ready received");
> +      resolve();
> +    });
> +  });

this could be simplified into:
```js
await ui.jsterm.hud.on("network-request-payload-ready");
info("network-request-payload-ready received");
```

and same thing thant the previous comment, since ` waitForConsolePayloadReady` is quite smqll qnd used once, we could inline its content into the main function
Attachment #8922362 - Flags: review?(nchevobbe) → review+
(In reply to Nicolas Chevobbe [:nchevobbe] from comment #13)
> Comment on attachment 8922362 [details]
> Bug 1406610 - Implementing test;
> 
> https://reviewboard.mozilla.org/r/193408/#review199060
> 
> Some style comment but this is good to land. test is green in verify mode
> locally :)
All comments resolved, patch updated, thanks!

Honza
Pushed by jodvarko@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/bb0c71d42196
No HTTP details for requests executed before the Console is activated; r=nchevobbe
https://hg.mozilla.org/integration/autoland/rev/7b41783886cc
Implementing test; r=nchevobbe
https://hg.mozilla.org/mozilla-central/rev/bb0c71d42196
https://hg.mozilla.org/mozilla-central/rev/7b41783886cc
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox58: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: