Fixed missing pushed objects in the HAR file

[marty90]

Hi, I noticed that pushed H2 objects are often missing in the HAR file. I found the cause: lines 72-78 initialize the cache params for the pushed entry. As such, the object is considered as coming from the disk cache and not written in the HAR (line 654). Adding the simple check that I propose solves the issue.

[soulgalore]

Ah thanks @marty90 great! Do you have a json with a push that we can add in the test/perflogs and add a small test so we make sure we continue to do right?

[soulgalore]

Or if you don't have time for the test, adding an example json with a push would be great!

[marty90]

Do you mean a HAR file that includes a pushed object?

[soulgalore]

No I was thinking if you have Chrome perf log like https://github.com/sitespeedio/chrome-har/blob/master/test/perflogs/run.sitespeed.io.json with pushed request(s), I think we missed added any test for that. Just thinking since you fixed, maybe you have a log we can add?

[marty90]

Yes, find attached a Chrome perf log with 3 pushed objects.

File: chromePerflog-1.json.gz

[soulgalore]

Hi @marty90 and thanks! Will push an updated version as soon as npm is working correctly (it's been up/down the last couple of hours).

[tobli]

Hi, thanks for the PR! I'll revert this for now, the generated HARs don't comply with the HAR schema after this change. I'll add the perflog you attached to the test suite to catch that. There's certainly something not right in how we handle "fromDiskCache": true. My simplistic assumption that it's meant "responses that are cached from an earlier load", but that doesn't seem to hold up. Seems it's also true for responses that were pushed before the client discovered it needed to request it.

[tobli]

reverted in 15a92c03aa4a01c3b1a55050559e6562df96f46d

[marty90]

Ho tobli! I see the point. The issue is quite subtle. According to the H2 standard the push cache is something at a lower level than the disk cache, and it is associated to a socket. However I agree that definining a cached object becomes difficult!

[marty90]

Why are the HAR files different? The modified line should affect only pushed responses.

[tobli]

Because of additional bugs. =) When including them, it turns out that they’re not built correctly. So additional/other fixes are needed. Now that your example file is included in the tests, you can apply your change, run npm test and see the error.

[tobli]

I think a step in the right direction is to not treat fromDiskCache responses as cached, if they also have a pushStart > 0.

[marty90]

I see the problem: in the code there is an access to response.pushStart, that actually does not appear to be a valid field according to the DevTool Specs. I guess that who wrote the code meant response.timing.pushStart ! I guess that fixing that line should solve the issue ;)

Edit: Fixing that line does not generate errors in the tests.

[marty90]

May I PR that line? ;)

Martino

[soulgalore]

@marty90 please go ahead :)