Closed Bug 1421335 Opened 2 years ago Closed 2 years ago

Right response info link disappears after request is expanded

Categories

(DevTools :: Console, defect, P1)

defect

Tracking

(firefox59 fixed)

RESOLVED FIXED
Firefox 59
Tracking Status
firefox59 --- fixed

People

(Reporter: abhinav.koppula, Assigned: Honza)

References

Details

(Whiteboard: [newconsole-mvp])

Attachments

(2 files)

STR:
- Fire some request with webconsole open,
- When I fire some request, I see the status code on the right, something like this - https://snag.gy/EBaQ6M.jpg
- Now if I click the request, so that it shows the details, the response info on the right goes away. https://snag.gy/G1eVOv.jpg
- It doesn't come back also once request is again collapsed.

Expected:
Response info should remain visible like the other things on the left(method and url).
Priority: -- → P3
Hey Nicolas,
This happens when totalTime is undefined in first highlight in [1] and hence the condition(second highlight in [1]) fails.
I am guessing totalTime is not being sent by the server when collapse/expand happens.

[1] https://dxr.mozilla.org/mozilla-central/source/devtools/client/webconsole/new-console-output/components/message-types/NetworkEventMessage.js#62-65,76-77

Is this something we need to fix on the server side?
Flags: needinfo?(nchevobbe)
probably not, i guess there is something weird with netmonitor reducer
Flags: needinfo?(nchevobbe)
Assignee: nobody → odvarko
Status: NEW → ASSIGNED
I am working on a test, but I don't have solid STR to repro the issue.
I'll probably update the existing browser_webconsole_network_messages_expand.js
test to check the time value after expand.

Honza
Comment on attachment 8935399 [details]
Bug 1421335 - Right response info link disappears after request is expanded;

https://reviewboard.mozilla.org/r/206298/#review211926

this looks good to me. Could not test it though to see if I don't see the error anymore

::: devtools/client/webconsole/new-console-output/store.js
(Diff revision 1)
> -          // 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.
> -          // Executing the reducer means that the `networkMessagesUpdateById`
> -          // is updated (a actor key created). The key is needed for proper
> -          // handling NETWORK_UPDATE_REQUEST event (in the same reducer).
> -          if (!updates[action.id]) {
> -            newState = reducer(newState, {
> -              type: NETWORK_MESSAGE_UPDATE,
> -              message: message,
> -            });
> -          }

great
Attachment #8935399 - Flags: review?(nchevobbe) → review+
Patch with update test attached. It works nicely and fails without the patch.
(I even found yet one issue, now fixed in the first patch)

Honza
Comment on attachment 8936242 [details]
Bug 1421335 - Update test;

https://reviewboard.mozilla.org/r/207000/#review213876
Attachment #8936242 - Flags: review?(nchevobbe) → review+
Comment on attachment 8935399 [details]
Bug 1421335 - Right response info link disappears after request is expanded;

https://reviewboard.mozilla.org/r/206298/#review213878

::: devtools/client/netmonitor/src/connector/firefox-data-provider.js:429
(Diff revision 3)
>        case "eventTimings":
> +        // Total time doesn't have to be always set e.g. net provider enhancer
> +        // in Console panel is using this method to fetch data when network log
> +        // is expanded. So, make sure to not push undefined into the payload queue
> +        // (it could overwrite an existing value).
> +        if (networkInfo.totalTime) {

it seems a bit silly, but could totalTime be 0 ? For example in the case of a cached request (regular cache or service worker) ?

We could test specifically for undefined or null here
Priority: P3 → P1
Whiteboard: [newconsole-mvp]
Honza, is this patch ready to land ?
Flags: needinfo?(odvarko)
(In reply to Nicolas Chevobbe [:nchevobbe] from comment #11)
> it seems a bit silly, but could totalTime be 0 ? For example in the case of
> a cached request (regular cache or service worker) ?
> 
> We could test specifically for undefined or null here
Done, tested for undefined.

Ready to land.

Honza
Flags: needinfo?(odvarko)
Pushed by jodvarko@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/eaf338dcf3ea
Right response info link disappears after request is expanded; r=nchevobbe
https://hg.mozilla.org/integration/autoland/rev/787fbdc766c4
Update test; r=nchevobbe
Pushed by apavel@mozilla.com:
https://hg.mozilla.org/mozilla-central/rev/7f796e33aa5b
Right response info link disappears after request is expanded; r=nchevobbe
https://hg.mozilla.org/mozilla-central/rev/bd91c94565e3
Update test; r=nchevobbe
https://hg.mozilla.org/mozilla-central/rev/7f796e33aa5b
https://hg.mozilla.org/mozilla-central/rev/bd91c94565e3
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 59
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.