Bug 16531 – Web Inspector: Show all headers in the Request Headers section of the Resource details sidebar

• Component: Web Inspector
• Status: RESOLVED FIXED

[Danny Bloemendaal]

I'm missing the cookie data that's being sent to the server in the request information when you click on an item in the Network section inside web inspector. (I have to debug a problem there so that's why I am missing that information).

[Adam Roben (:aroben)]

<rdar://problem/5712895>

[Brian Weinstein]

Now that we have a Cookie storage view, is this enough to have here, or should there be more?

[Timothy Hatcher]

I think showing them with resources might be nice, but can be confusing too.

We should only show the cookies that were sent to / set by the resource.

[Brian Weinstein]

Do we have that data from CFNetwork? I don't think we do, we can get all the cookies that the resource has access to, but not the ones just set by that resource.

[Timothy Hatcher]

We should have the Cookies header that they send back.

[Tauren Mills]

(In reply to comment #2)
> Now that we have a Cookie storage view, is this enough to have here, or should there be more?

I was told that my post on the Chromium group relates to this issue.
http://groups.google.com/a/chromium.org/group/chromium-discuss/browse_thread/thread/c44361ebe6e03184#

Personally, I think that showing cookie information in the Resource tab is very important, and that it shouldn't only be available in the Cookie storage view. I'm not familiar with WebKit besides using Chrome, but I'm assuming this is referring to the "Cookies and Other Data" dialog.

I don't understand why cookies would not be included with the Request Headers and Response Headers in Developer Tools so that developers can get a very clear picture of everything related to a request. When debugging a cookie related problem, it is extremely cumbersome to have to go into a separate location to view cookie details. Having an indicator showing when a new cookie is set would be nice (perhaps bold it or something).

Also, the separate cookie storage view simply shows the state of cookies at a certain point in time - when you look at it. It doesn't give you an easy way to see which request received a cookie, or which ones sent a cookie. For instance, if there are a series of ajax requests, one of which receives a cookie, and the following ones send the cookie, it would be hard to determine which ones used cookies and which didn't.

[Andrey Kosyakov]

*** Bug 48226 has been marked as a duplicate of this bug. ***

[Andrey Kosyakov]

Created attachment 71772 [details]
patch

See https://bugs.webkit.org/show_bug.cgi?id=48226 for previous discussion of the patch.

> > WebCore/WebCore.gypi:4424
> > +            'inspector/front-end/ResourceCookiesView.js',
> 
> This is a tab, not a view.

Renamed to ResourceCookiesTab and moved it to ResourceView.js

> > WebCore/inspector/front-end/CookieItemsViewBase.js:33
> > +WebInspector.CookieItemsViewBase = function()
> 
> This could simply reside in CookieItemsView in form of helper methods / component.

Moving this to CookieItemsView.js, but leaving everything else intact (we do use a bit of polymorphism there).

> > WebCore/inspector/front-end/CookieParser.js:57
> > +                this._addCookie(kv, "request");
> 
> Could you use constants for these?

Done.

> > WebCore/inspector/front-end/DataGrid.js:320
> > +        var children = maxDescentLevel ? this._enumerateChildren(this, [], maxDescentLevel) : this.children;
> 
> So if I pass "1" as maxDescentLevel, the code will work as if it was "1".

Quite so, and similar statements hold true for all other possible values of maxDescentLevel ;-)
Changed the base for the level so that 0 is the old behavior (top level only) and 1 is one level below it, etc.

> > WebCore/inspector/front-end/DataGrid.js:404
> > +        if (!this._columnWidthsInitialized && this.element.enclosingNodeOrSelfWithNodeName("body")) {
> 
> You could also check this.element.offsetWidth for === 0.

Done.

[Pavel Feldman]

Comment on attachment 71772 [details]
patch

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

> WebCore/inspector/front-end/CookieItemsView.js:30
> +WebInspector.CookieItemsViewBase = function()

Can we call this CookiesTable?

[Andrey Kosyakov]

Manually committed r70529: http://trac.webkit.org/changeset/70529

Note this functionality depends on platform's ability to deliver Cookie/Set-Cookie headers from the network stack, which is not yet available on many platforms yet -- so this patch won't have immediate effect on these platforms. Chromium, for one, has this working.

[Eric Seidel (no email)]

Seems this is fixed.  Please consider using webkit-patch land, as it will close bugs for you when landing.

[Andrey Kosyakov]

I left it open on purpose (see comment #10), as this was only fixed for Chromium -- this functionality depends on the network stack supplying actual request headers, which is only implemented in case of Chromium presently. Hence, the cookies tab is currently enabled conditionally (off by default). I thought it wouldn't be appropriate to close this bug until it gets fixed in other browsers -- Safari in particular, given Radar references above.

[Eric Seidel (no email)]

Attachment 71772 [details] was posted by a committer and has review+, assigning to Andrey Kosyakov for commit.

[Eric Seidel (no email)]

Comment on attachment 71772 [details]
patch

Marking obsolete since this was landed.

[Rob Colburn]

This is ticket is marked REOPENED, but should be marked CLOSED/FIXED.
See:
Comment #14 From Eric Seidel 2010-12-20 22:59:48 PST

[Joseph Pecoraro]

This information is now available to WebKit on macOS via CFNetwork's public didFinishCollectingMetrics delegate message.

WebKit has access to a complete NSURLRequest which includes all of the request headers that were sent, including Cookie headers, Authorization headers (Basic Auth), and others (Host, Accept*) which may be added underneath WebKit.

[Joseph Pecoraro]

Created attachment 306241 [details]
[IMAGE] Before - Missing Cookie Header

[Joseph Pecoraro]

Created attachment 306242 [details]
[IMAGE] After - Include Cookie, Accept-Language, Accept-Encoding

[Joseph Pecoraro]

Created attachment 306244 [details]
[PATCH] Proposed Fix

Some things I have to test:

1. Is it possible to send a request with duplicate header? if so I need a test for that
2. I have concerns about potential performance issues:
  - this makes a HTTPHeaderMap for every request and serializes between the NetworkProcess and WebProcess (by putting it in NetworkLoadMetrics)
  - I suspect this will be normally be small but it could be large and if so I want to make sure we don't negatively impact load performance

[Timothy Hatcher]

Comment on attachment 306244 [details]
[PATCH] Proposed Fix

Awesome!

[youenn fablet]

(In reply to Joseph Pecoraro from comment #19)
> Created attachment 306244 [details]
> [PATCH] Proposed Fix
> 
> Some things I have to test:
> 
> 1. Is it possible to send a request with duplicate header? if so I need a
> test for that

Except for cookie headers, duplicated headers should be combined.

> 2. I have concerns about potential performance issues:
>   - this makes a HTTPHeaderMap for every request and serializes between the
> NetworkProcess and WebProcess (by putting it in NetworkLoadMetrics)
>   - I suspect this will be normally be small but it could be large and if so
> I want to make sure we don't negatively impact load performance

This will happen in all cases or just when web inspector is set?

[Build Bot]

Comment on attachment 306244 [details]
[PATCH] Proposed Fix

Attachment 306244 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/3475445

New failing tests:
http/tests/inspector/network/resource-request-headers.html

[Build Bot]

Created attachment 306249 [details]
Archive of layout-test-results from ews107 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Joseph Pecoraro]

Oh, el-capitan doesn't use the NSURLSession path, so I'll have to mark this as failing there like I did for the WK1 ports.

[Joseph Pecoraro]

(In reply to youenn fablet from comment #21)
> (In reply to Joseph Pecoraro from comment #19)
> > Created attachment 306244 [details]
> > [PATCH] Proposed Fix
> > 
> > Some things I have to test:
> > 
> > 1. Is it possible to send a request with duplicate header? if so I need a
> > test for that
> 
> Except for cookie headers, duplicated headers should be combined.

Yeah, I don't see a way for web developers to ever create a case of duplicate request headers (XHR and Fetch both have set/append so only 1 key). I may have missed a case where the browser might do it, but in that case I concluded it would be enough of an edge case that I can worry about it later.


> > 2. I have concerns about potential performance issues:
> >   - this makes a HTTPHeaderMap for every request and serializes between the
> > NetworkProcess and WebProcess (by putting it in NetworkLoadMetrics)
> >   - I suspect this will be normally be small but it could be large and if so
> > I want to make sure we don't negatively impact load performance
> 
> This will happen in all cases or just when web inspector is set?

Bug 170525 convert only doing this when Web Inspector is open. If that gets reviewed positively I'll land this confidently (I don't want to impact performance of general browsing).

[Joseph Pecoraro]

<https://trac.webkit.org/changeset/215062/webkit>