Bug 76398 - Web Inspector: HAR pageref attributes are wrong and inconsistent with pages array
Summary: Web Inspector: HAR pageref attributes are wrong and inconsistent with pages a...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Andrey Kosyakov
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-01-16 11:59 PST by Andrey Kosyakov
Modified: 2012-01-23 01:03 PST (History)
10 users (show)

See Also:


Attachments
Patch (19.65 KB, patch)
2012-01-20 02:30 PST, Andrey Kosyakov
no flags Details | Formatted Diff | Diff
Patch (21.69 KB, patch)
2012-01-20 06:48 PST, Andrey Kosyakov
no flags Details | Formatted Diff | Diff
Patch (21.11 KB, patch)
2012-01-20 08:25 PST, Andrey Kosyakov
pfeldman: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andrey Kosyakov 2012-01-16 11:59:09 PST
Original report by Steve Souders:

====
A simple case is to just load two simple pages - Bing and Craigslist:
1. Clear your cache.
2. Select "Preserve Log upon Navigation" 
3. Go to Bing & Craigslist. In the waterfall chart UI verify that you see all the requests for both pages.
4. Right-click (2-finger click) and select "Save All as HAR" and save as chrome-bing-craigslist.har.
5. Open the .har file in a text editor - very that info for all requests is present - for example AutoSug.js (from Bing) and craigslist.css.
6. View the .har file in various tools included pcapperf, Har to Page Speed, and HAR Viewer. In the first two tools, only 3 requests for Craigslist are shown. In HAR Viewer only error messages are shown.

This might be the same bug, but when I try to get a HAR file for Gmail a similar thing happens. I can see all the requests in the Chrome dev tools (eg, logo_small.png) and also see them in the .har file, but when I try to view that file only 4 requests are shown (instead of ~30).
====

There are two related problems that cause this:
- The pages array only contains one page, the one after the last navigation; the correct behavior is to populate pages array with all pages that are present in the log;
- pageref attribute is incorrect for some HAR entries -- notably for those that followed the main resource redirect and those loaded from subframes.
Comment 1 Andrey Kosyakov 2012-01-20 02:30:29 PST
Created attachment 123275 [details]
Patch
Comment 2 Andrey Kosyakov 2012-01-20 06:48:54 PST
Created attachment 123306 [details]
Patch
Comment 3 Pavel Feldman 2012-01-20 07:15:02 PST
Comment on attachment 123306 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=123306&action=review

A bunch of small nits.

> Source/WebCore/inspector/front-end/HAREntry.js:325
> +    _convertPage: function(page)

If you move this method to the Page class, you wouldn't need to expose all of the page's properties to public.

> Source/WebCore/inspector/front-end/NetworkLog.js:56
> +    pageForResource: function(resource)

pageLoadForResource ?

> Source/WebCore/inspector/front-end/NetworkLog.js:58
> +        return resource.__page;

Please use Map for this.

> Source/WebCore/inspector/front-end/NetworkLog.js:76
> +                resource.__page = this._currentPageLoad;

ditto

> Source/WebCore/inspector/front-end/NetworkLog.js:88
> +        var frame = WebInspector.resourceTreeModel.frameForId(resource.frameId);

this seems to be unused.

> Source/WebCore/inspector/front-end/NetworkLog.js:89
> +        resource.__page = this._currentPageLoad;

ditto
Comment 4 Andrey Kosyakov 2012-01-20 08:23:24 PST
(In reply to comment #3)
> (From update of attachment 123306 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=123306&action=review
> 
> A bunch of small nits.
> 
> > Source/WebCore/inspector/front-end/HAREntry.js:325
> > +    _convertPage: function(page)
> 
> If you move this method to the Page class, you wouldn't need to expose all of the page's properties to public.

As discussed offline, this is not quite practical, as convertPage() relies on certain methods internal to HAR. Besides, we pretend this is a more generic, model level than HAR support -- I think this may be eventually reused in NetworkPanel, perhaps in audits. As agreed, I'm just removing getters and use fields directly.

> 
> > Source/WebCore/inspector/front-end/NetworkLog.js:56
> > +    pageForResource: function(resource)
> 
> pageLoadForResource ?

done.

> 
> > Source/WebCore/inspector/front-end/NetworkLog.js:58
> > +        return resource.__page;
> 
> Please use Map for this.

That would be cumbersome due to need to eventually release map entries. For HAR at least, the resource entries are primary and we only need PageLoad entries as long as there are some (network) Resources that refer to them. The life-time of resource is a bit complicated, as some may be cleaned manually in the network panel, some may survive page reloads (again due to network panel), and life-time of resource references in NetworkLog is different from the Network panel.
 
WebInspector.resourceTreeModel.frameForId(resource.frameId);
> 
> this seems to be unused.

Thanks, fixed!
Comment 5 Andrey Kosyakov 2012-01-20 08:25:01 PST
Created attachment 123319 [details]
Patch
Comment 6 Andrey Kosyakov 2012-01-23 01:03:24 PST
Committed r105596: <http://trac.webkit.org/changeset/105596>