Closed Bug 997065 Opened 7 years ago Closed 7 years ago

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


(DevTools :: Netmonitor, defect)

Not set


(Not tracked)

Firefox 33


(Reporter: thomas, Assigned: sjakthol)




(2 files, 2 obsolete files)

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:
(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.
Component: Untriaged → Developer Tools: Netmonitor
This issue could be related to bug 984687
Summary: Network Monitor: media/webm request issues → Network Monitor: Multiple request header blocks for media/webm requests
I'm having a separate issue with the webm file not showing up on every reload.

Victor, any theories?
Flags: needinfo?(vporof)
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.
Flags: needinfo?(vporof)
> 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
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.
Attachment #8444153 - Flags: feedback?(vporof)
The previous patch contained an error. Here's an updated one without test failures. Sorry for the noise.
Attachment #8444153 - Attachment is obsolete: true
Attachment #8444153 - Flags: feedback?(vporof)
Attachment #8444155 - Flags: feedback?(vporof)
Sorry for the long delay Sami, and thank you for the patch! I'll have a look at this asap.
Comment on attachment 8444155 [details] [diff] [review]

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] {
Attachment #8444155 - Flags: feedback?(vporof) → review+
Ever confirmed: true
Thanks for the review. Here's a patch with the if-else if block simplified.

Try run:
Assignee: nobody → sjakthol
Attachment #8444155 - Attachment is obsolete: true
Attachment #8449554 - Flags: review?(vporof)
Comment on attachment 8449554 [details] [diff] [review]

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

Attachment #8449554 - Flags: review?(vporof) → review+
Please add a checkin-needed keyword to this bug if the try run is green.
Duplicate of this bug: 984687
Duplicate of this bug: 991172
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
Closed: 7 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 33
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.