997065 - Network Monitor: Multiple request header blocks for media/webm requests

Status: RESOLVED FIXED

(DevTools :: Netmonitor, defect)

Description

[Thomas Andersen]

Attachment: Screen Shot 2014-04-16 at 10.54.04.png

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:28.0) Gecko/20100101 Firefox/28.0 (Beta/Release)
Build ID: 20140314220517

Steps to reproduce:

1. Make sure the browser cache is cleared.
2. Open the Network panel
3. Navigate to: http://mr-andersen.no/gfcycat-companion-test/issues/7.html
(warning: the page loads a video w/sound using autoplay)
4. Select the trailer.webm entry in the request list quickly when the page is loaded.
 


Actual results:

If you click the trailer.webm entry before the video is buffered properly the Headers tab is populated with multiple Request Header blocks (see the attached screenshot)


Expected results:

Only one Request Header block after selecting the entry in the list.

Comment 1

[Thomas Andersen]

This issue could be related to bug 984687

Comment 2

[Rob Campbell [:rc] (:robcee)]

I'm having a separate issue with the webm file not showing up on every reload.

Victor, any theories?

Comment 3

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

Yes, this might be definitely related to bug 984687. It seems that if a request is inspected before it finishes (or all the information about it is available), some parts end up being displayed twice.

Comment 4

[Thomas Andersen]

> I'm having a separate issue with the webm file not showing up on every reload.

Maybe related to the issue discussed here, bug 764958

Comment 5

[Sami Jaktholm]

Attachment: netmonitor-no-duplicate-content-in-details.patch

Here's a patch that should fix the issues mentioned here and in bug 984687. The problem here is that the update is performed in an asynchronous task. If the data is updated often there were multiple tasks running simultaneously. This caused the information to be added multiple times.

The patch fixes the problem by keeping track of the update task. If a new update of a tab is required while another one is running it waits for the first one to finish until performing the final update. If multiple updates are triggered during an update only the last one is performed once the pending update finishes.

The attached test case tries to ensure the tabs don't contain duplicated information by waiting for the final "TAB_UPDATED" event and checking the content of the tabs (Headers, Cookies and Params). To test this case manually you need to inspect a request before it finishes.

Test browser_net_status-codes.js was relying on the old behavior where summary would be updated synchronously even if another update was running. So instead of .thening on "TAB_UPDATED" event I decided to refactor the test to be a Task and use loops. Without that it would've looked a lot like this:

<click on a request>
<wait for a details pane to update>.then
<perform checks>

<click on a request>
<wait for a details pane to update>.then
<perform checks>

and so on for each request twice (10 times)...

This bug is a bit nasty to deal with so feedback is more than welcome.

Comment 6

[Sami Jaktholm]

Attachment: netmonitor-no-duplicate-content-in-details.patch

The previous patch contained an error. Here's an updated one without test failures. Sorry for the noise.

Comment 7

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

Sorry for the long delay Sami, and thank you for the patch! I'll have a look at this asap.

Comment 8

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

Comment on attachment 8444155 [details] [diff] [review]
netmonitor-no-duplicate-content-in-details.patch

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

Beautiful test and I'm liking the new logic. Thank you, let's land this! Have you pushed to try? Do you want me to do it?

::: browser/devtools/netmonitor/netmonitor-view.js
@@ +2110,2 @@
>        }
> +      else if (tab == this.widget.selectedIndex) {

Nit: you could more cleanly write this as 

if (tab == this.widget.selectedIndex) {
  if (viewState.dirty[tab]) {
    ...
  } else {
    ...
  }
} else {
  if (viewState.dirty[tab] {
    ...
  }
}

Comment 9

[Sami Jaktholm]

Attachment: netmonitor-no-duplicate-content-in-sidebar.patch

Thanks for the review. Here's a patch with the if-else if block simplified.

Try run: https://tbpl.mozilla.org/?tree=Try&rev=74f673e71887

Comment 10

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

Comment on attachment 8449554 [details] [diff] [review]
netmonitor-no-duplicate-content-in-sidebar.patch

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

Splendid.

Comment 11

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

Please add a checkin-needed keyword to this bug if the try run is green.

Comment 14

[Sami Jaktholm]

All green: https://tbpl.mozilla.org/?tree=Try&rev=74f673e71887

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/610cfd0139c1

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/610cfd0139c1