1674622 - har import/export - incorrect handling of the timings/time fields

Status: RESOLVED FIXED

(DevTools :: Netmonitor, defect, P2)

Description

[Florent Viard]

Attachment: patch-devtools-fixes-timings-har-import-export.diff

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:80.0) Gecko/20100101 Firefox/80.0

Steps to reproduce:

1. I did export a browsing session with "save all as har", and looked at the content of the generated "har" file.

2. I did import another "har" file that was not generated by Firefox.
   Inside this file, one item of "entries" with the ["response"]["timings"] field that has only 3 elements instead of the 7 possibles:
   "timings": {
   "receive": -1,
   "send": -1,
   "wait": 22
   }

(FYI, All the possible elements:
const TIMING_KEYS = [
"blocked",
"dns",
"connect",
"ssl",
"send",
"wait",
"receive",
];
)

Actual results:

Patch to fix these issues provided as attachment.
In fact, the code in the exporter (har-builder.js) and the importer (har-importer.js) had one difference on each side being the fix to the other issue.

For 1)
The exporter will calculate the "entry.time" based on entry.timings elements, but will not ignore the "-1" elements and count them as if it was "negative" times.
("-1" is in fact supposed to mean that the information is not available)
Ex:
"time": 23,
"timings": {
"blocked": -1,
"connect": 0,
"dns": 0,
"receive": 0,
"send": 0,
"ssl": 0,
"wait": 24
}

For 2)
The missing elements are returning an undefined that is not ignored, and so the sum result become a NaN.

Expected results:

For 1 and 2:
-1 and undefined values should both be ignored so that the correct "time" could be exported in the "har" file and the correct timings imported and displayed.

Comment 1

[Hubert Boma Manilla (:bomsy)]

Hey florent,
Thanks for reporting and the diff for the fix.
We accept patches on phabricator now.
Would you be willing to open this patch using phabricator? See this https://moz-conduit.readthedocs.io/en/latest/phabricator-user.html# for details.

I can assign to you if you are interested.

Thanks

Comment 2

[Florent Viard]

Hi,
Thank you for your reply.
I'm ok to use phabricator.
I was just thinking that it was not possible for an individual contributor without a special level to post a patch there.
So, you can assign me the issue there :-)

Thanks

Comment 3

[Hubert Boma Manilla (:bomsy)]

Cool assigned it to you.
If you have any questions , please feel free to ask.

Comment 4

[Florent Viard]

I have a little question to proceed.
I don't see anything to post the patch using the phabricator web page.
So, my understanding is that I have to do a "hg commit" locally and then use moz-phab to send the patch.
Am I correct?

Also, what is the reviewer string ("r=???") that I should put in my commit?
Thanks

Comment 5

[Florent Viard]

(In reply to Florent Viard from comment #4)

Also, what is the reviewer string ("r=???") that I should put in my commit?
Here I mean who should I put there?

Comment 6

[Hubert Boma Manilla (:bomsy)]

(In reply to Florent Viard from comment #4)

I have a little question to proceed.
I don't see anything to post the patch using the phabricator web page.
So, my understanding is that I have to do a "hg commit" locally and then use moz-phab to send the patch.
Am I correct?

Yes that is correct!

Also, what is the reviewer string ("r=???") that I should put in my commit?

The reviewer is the person you want to review the patch. You can add me as the reviewer whic would be r=bomsy

Comment 7

[Florent Viard]

Attachment: Bug 1674622 - Fixes timings/time fields for har import/export; r=bomsy

Comment 8

[Florent Viard]

Done and in "review" state.
I don't have anything more to do now?

Just more my knowledge, if I was to do another patch for something else, how do you find which reviewer to associate with your commit?
Is there a maintainer list of the different modules of the code somewhere?

Comment 9

[Hubert Boma Manilla (:bomsy)]

(In reply to Florent Viard from comment #8)

Done and in "review" state.
I don't have anything more to do now?

Great! i'll review when i get a chance.

Just more my knowledge, if I was to do another patch for something else, how do you find which reviewer to associate with your commit?
Is there a maintainer list of the different modules of the code somewhere?

For any bug you want to work on, the easiest thing is you can set the mentor or the triage owner as the reviewer of the patch.
Also you can see the list of owners for the firefox devtools here https://firefox-dev.tools/

Comment 10

[Florent Viard]

Hi,
Thanks for the review and approval.
What is the next step for the patch?
Will it get merged soon, automatically?
Or do I have anything more to do to send to "merge"?
Thanks

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f6f29c9cc744
Fixes timings/time fields for har import/export; r=bomsy

Comment 12

[Narcis Beleuzu [:NarcisB]]

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

Comment 13

[Florent Viard]

Fyi, I think that the previous regression report, is erroneous and not related to the fix for the current issue or the day it was pushed.