Bug 28185 – Inspector: Show Hidden Cookie Data

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector (Deprecated)

[Joseph Pecoraro]

Inspecting cookie data currently uses "document.cookie" in the inspected window and thus only has access to the names and values of cookies.  However, there is far more information, namely path, expires, domain, httpOnly, sessionOnly, and secure.  This information should be made available to the Inspector.

[Joseph Pecoraro]

Created attachment 34592 [details]
Working on OS X Leopard

This is pretty much my first experience with C++, so I'd appreciate it if you be explicit about any of the C++ mistakes I made, or build fixes that will be needed for other platforms.  I changed the CookieJar.h header so I expect that I will need to fix a number of other files.  Thanks in advance.

[Timothy Hatcher]

Patch looks pretty good. I think rawCookies will be better named as cookies. And rename the existing cookies to cokkiesString or somthing more specific.

I'll review more later and provide guidance o nwhat files to edit for other platforms.

[Darin Adler]

Comment on attachment 34592 [details]
Working on OS X Leopard

> +    Vector<Cookie> cookies;
> +    Document* document = ic->inspectedPage()->mainFrame()->domWindow()->document();
> +    cookies = rawCookies(document, document->cookieURL());

You should declare the cookies vector on the same line where you initialize it. But, you should not use Vector for a return value; copying it to return it is very inefficient. Instead you should name this function getRawCookies and use a Vector<Cookie>& as an out parameter.

> +        Cookie cookie = cookies[i];

To avoid making an extra copy of each cookie in the vector this can be a "const Cookie&" instead of Cookie.

> +        JSObject* cookieObj = constructEmptyObject(exec);

We prefer not to abbreviate, so please say cookieObject.

> +        cookieObj->putDirect(Identifier(exec, "name"), jsString(exec, cookie.name()));
> +        cookieObj->putDirect(Identifier(exec, "value"), jsString(exec, cookie.value()));
> +        cookieObj->putDirect(Identifier(exec, "domain"), jsString(exec, cookie.domain()));
> +        cookieObj->putDirect(Identifier(exec, "path"), jsString(exec, cookie.path()));
> +        cookieObj->putDirect(Identifier(exec, "expires"), jsString(exec, cookie.expires()));
> +        cookieObj->putDirect(Identifier(exec, "httpOnly"), jsBoolean(cookie.isHttpOnly()));
> +        cookieObj->putDirect(Identifier(exec, "secure"), jsBoolean(cookie.isSecure()));
> +        cookieObj->putDirect(Identifier(exec, "session"), jsBoolean(cookie.isSession()));

This will be relatively slow since it's going to create each of these identifiers from the constant string each time through the loop, which is a hash table lookup every time. It might be good to instead create local variables for each identifier so each will be created exactly once. On the other hand, the jsString operations are relatively costly too, so it's probably not all that important to optimize.

> +        result.append( cookieObj );

The spaces around the argument are not the correct style.

> +    // return jsNumber(exec, cookies.size());

Oops, left in this commented code!

> +    class Cookie {
> +    public:
> +        Cookie(const String& name, const String& value, const String& domain,
> +            const String& path, const String& expires, const bool httpOnly,
> +            const bool secure, const bool session)

No need for the "const" in "const bool". Would be nice to indent this so the lines don't exactly line up with the ":". I often use indent by an extra tab stop.

For a pure data holder like this, you might want to consider just using a struct instead of a class with getter functions.

>      class KURL;
>      class String;
>      class Document;
> +    class Cookie;

These should be sorted alphabetically. Not sure why Document wasn't already.

>  #import "config.h"
> +#import "Cookie.h"
>  #import "CookieJar.h"

The new include should be added to the paragraph below, not put above the file's own header. The file's own header always comes first.

> +        NSString* name = [cookie name];
> +        NSString* value = [cookie value];
> +        NSString* domain = [cookie domain];
> +        NSString* path = [cookie path];
> +        NSString* expires = [[cookie expiresDate] description];

It's a bit strange to put this date into this string form, but I don't have a better suggestion at the moment.

Our coding style is to put the * next to the variable name for Objective-C types like this one.

> +        bool httpOnly = [cookie isHTTPOnly];
> +        bool secure = [cookie isSecure];
> +        bool session = [cookie isSessionOnly];
> +        Cookie cookieData = Cookie(name, value, domain, path, expires, httpOnly, secure, session);
> +        rawCookies.append(cookieData);
> +    }
> +
> +    return rawCookies;

You shouldn't need this return statement here. It's OK to fall through to the return outside the block exceptions macro.

This patch will break all platforms other than Mac OS X. You can't land it like that, so you'll need to figure out what to do for those other platforms.

[Joseph Pecoraro]

Created attachment 34634 [details]
Fixed Based on Review

This is an attempt at a real workable version that hopefully won't break the build.

[Joseph Pecoraro]

(In reply to comment #3)
> You should declare the cookies vector on the same line where you initialize it.
> But, you should not use Vector for a return value; copying it to return it is
> very inefficient. Instead you should name this function getRawCookies and use a
> Vector<Cookie>& as an out parameter.

Done. Made the Vector an out parameter, but that requires the declaration to again be on another line.

> To avoid making an extra copy of each cookie in the vector this can be a "const
> Cookie&" instead of Cookie.

Done.

> We prefer not to abbreviate, so please say cookieObject.

Done. Noted.

> This will be relatively slow since it's going to create each of these
> identifiers from the constant string each time through the loop, which is a
> hash table lookup every time. It might be good to instead create local
> variables for each identifier so each will be created exactly once. On the
> other hand, the jsString operations are relatively costly too, so it's probably
> not all that important to optimize.

Done. I moved Initializers out and made them const.

> The spaces around the argument are not the correct style.

Done. Including all other style fixes.

> No need for the "const" in "const bool". Would be nice to indent this so the
> lines don't exactly line up with the ":". I often use indent by an extra tab
> stop.

Done.

> For a pure data holder like this, you might want to consider just using a
> struct instead of a class with getter functions.

Done. Made a struct.  Including a new field "size" (see below).

> The new include should be added to the paragraph below, not put above the
> file's own header. The file's own header always comes first.

Done. Noted.

> It's a bit strange to put this date into this string form, but I don't have a
> better suggestion at the moment.

Done. Noted.

> Our coding style is to put the * next to the variable name for Objective-C
> types like this one.

Done. Noted.

> This patch will break all platforms other than Mac OS X. You can't land it like
> that, so you'll need to figure out what to do for those other platforms.

Attempted.  I searched and identified, hopefully all, other platforms.  Most don't really make support for this easy, and I have no way of testing on them at all.  What is the best course of action for something like this?

------------

I added another field to the Cookie struct, "size".  Firebug's "size" parameter seems to be the (size of the name + size of the value).  I decided to put this into the struct, and then make it available in the constructed jsObjects for the inspector.

[Joseph Pecoraro]

Oh, one final question.  Inside CookieJar.h I include <wtf/Vector.h>.  This seems to make it unnecessary to include again in each of the platform specific implementations... but I included them anyways.  I could probably do the same for Cookie.h  What is the proper way to go about this?

  - Include in just the header
  - Include in both the header and the implementation

Seeing as it compiled for me without declaring in both places I'm guessing this is a style issue, and I'm guessing certain editors might work better with the declaration in both files.

[Darin Adler]

Comment on attachment 34634 [details]
Fixed Based on Review

> +    Document* document = ic->inspectedPage()->mainFrame()->domWindow()->document();

From mainFrame() you can get to the document directly. No need to go through domWindow().

> +    const Identifier nameIdentifier = Identifier(exec, "name");
> +    const Identifier valueIdentifier = Identifier(exec, "value");
> +    const Identifier domainIdentifier = Identifier(exec, "domain");
> +    const Identifier pathIdentifier = Identifier(exec, "path");
> +    const Identifier expiresIdentifier = Identifier(exec, "expires");
> +    const Identifier sizeIdentifier = Identifier(exec, "size");
> +    const Identifier httpOnlyIdentifier = Identifier(exec, "httpOnly");
> +    const Identifier secureIdentifier = Identifier(exec, "secure");
> +    const Identifier sessionIdentifier = Identifier(exec, "session");

You can use construction syntax instead of assignment for these. Also, no real reason to make them const. After all, almost any local variable in most programs could be marked const, and for brevity we normally don't do it unless there's a special reason to.

    Identifier nameIdentifier(exec, "name");

and so on.

> +        const String name;
> +        const String value;
> +        const String domain;
> +        const String path;
> +        const String expires;
> +        const bool httpOnly;
> +        const bool secure;
> +        const bool session;
> +        const unsigned size;

Making all these data members const isn't really all that good an idea. If someone wants to use a Cookie in some non-const way, perhaps modifying one of these, then declaring it this way would get in the way, for no good reason. On the other hand, clients who don't want to modify a Cookie object get the same effect you got here by just saying "const Cookie". I'd leave off all the const on the individual members.

> +void getRawCookies(const Document*, const KURL& url, Vector<Cookie>& rawCookies)
> +{
> +    rawCookies.clear();
> +    BEGIN_BLOCK_OBJC_EXCEPTIONS;
> +
> +    NSURL *cookieURL = url;
> +    NSArray *cookies = [[NSHTTPCookieStorage sharedHTTPCookieStorage] cookiesForURL:cookieURL];
> +
> +    NSUInteger count = [cookies count];

For best memory use and performance, you will want to do this here:

    rawCookies.reserveCapacity(count);

This allocates the memory for the vector rather than sizing it up each time you hit the limit while appending. It's faster and saves a bit on extra storage. Once you've done that change, it is then safe to use the slightly faster uncheckedAppend function below inside the loop instead of append. But even if you chose not to do uncheckedAppend, reserveCapacity is a good idea.

> +    for (NSUInteger i = 0; i < count; ++i) {
> +        NSHTTPCookie *cookie = (NSHTTPCookie *)[cookies objectAtIndex:i];
> +        NSString *name = [cookie name];
> +        NSString *value = [cookie value];
> +        NSString *domain = [cookie domain];
> +        NSString *path = [cookie path];
> +        NSString *expires = [[cookie expiresDate] description];
> +        bool httpOnly = [cookie isHTTPOnly];
> +        bool secure = [cookie isSecure];
> +        bool session = [cookie isSessionOnly];
> +        Cookie cookieData = Cookie(name, value, domain, path, expires, httpOnly, secure, session);

Again could use construction syntax here:

    Cookie cookieData(name, value, domain, path, expires, httpOnly, secure, session);

> +        rawCookies.append(cookieData);

Or you could construct right in the append function call:

    rawCookies.uncheckedAppend(Cookie(name, value, domain, path, expires, httpOnly, secure, session));

Otherwise this looks really great.

None of my comments are critical, so I'll say r=me on this version, but feel free to do a revised version if you like some of the things I propose above.

[Joseph Pecoraro]

Created attachment 34660 [details]
Improved Based on Review

[Joseph Pecoraro]

(In reply to comment #7)
> From mainFrame() you can get to the document directly. No need to go through
> domWindow().

Done. Noted.
 
> You can use construction syntax instead of assignment for these. Also, no real
> reason to make them const. After all, almost any local variable in most
> programs could be marked const, and for brevity we normally don't do it unless
> there's a special reason to.

Done. This is the first time I've heard of construction syntax. Interesting!

> Making all these data members const isn't really all that good an idea.

Done.

> For best memory use and performance, you will want to do this here:
>     rawCookies.reserveCapacity(count);
> Once you've done that change, it is then safe to use the slightly faster
> uncheckedAppend function below inside the loop instead of append. 

Done. I had thought of this instead of the clear(), but I realize now that doing both still makes sense.

> Or you could construct right in the append function call:

Done.

> None of my comments are critical, so I'll say r=me on this version, but feel
> free to do a revised version if you like some of the things I propose above.

I don't have commit access so it would be helpful to use the commit-queue.  Thanks!

----

Any comments on my header include question from comment #6?

[Darin Adler]

Comment on attachment 34660 [details]
Improved Based on Review

> +        cookieObject->putDirect(sizeIdentifier, jsNumber(exec, cookie.size));

I'm so sorry I didn't mention this earlier. It seems clear to me that if "size" is just name.length() + value.length() then it should not be stored in a Cookie struct. Instead it should be computed, perhaps in JavaScript code, or perhaps in this function, or perhaps in a function member of the Cookie struct.

Another possibility is that this "name.length() + value.length()" computation is just an estimate that's being used because the cookie API on Mac OS X does not supply the length. In that case, the logic to compute this size estimate should be in the getRawCookies function in CookieJarMac.mm, not in the cross-platform code, and the size should be a constructor argument.

Everything else is perfect, and this is a minor issue.

r=me

[Darin Adler]

(In reply to comment #6)
> Oh, one final question.  Inside CookieJar.h I include <wtf/Vector.h>.  This
> seems to make it unnecessary to include again in each of the platform specific
> implementations... but I included them anyways.  I could probably do the same
> for Cookie.h  What is the proper way to go about this?
> 
>   - Include in just the header
>   - Include in both the header and the implementation

It should be included in just the header.

[Joseph Pecoraro]

Created attachment 34679 [details]
Improved Based on Review

[Joseph Pecoraro]

(In reply to comment #10)
> I'm so sorry I didn't mention this earlier. It seems clear to me that if "size"
> is just name.length() + value.length() then it should not be stored in a Cookie
> struct. Instead it should be computed, perhaps in JavaScript code, or perhaps
> in this function, or perhaps in a function member of the Cookie struct.

Done. I moved this to the final point in the chain, inside JSInspectorBackendCustom.cpp. I put it here rather then performing it in the JavaScript so it will hopefully be faster in the C++.  It might be useful to throw some ASSERTS() that the name and value are not null, but I'm not entirely clear on WebKit asserts yet.  I actually build the Release versions anyways.


(In reply to comment #11)
> It should be included in just the header.

Sounds good. In the latest patch I removed the duplicate includes from all of the implementation files I edited.

----

Thanks for the reviews and help Darin. =)

[Joseph Pecoraro]

It probably makes more sense to store the expires as milliseconds since the epoch?  Hopefully there is a standard way to calculate this for all platforms and JavaScript can use Date.parse() to convert it into a Date Object.  Or, if there is a way to do this in the C++ like jsNumber, jsBoolean etc that would be useful.

[Joseph Pecoraro]

Created attachment 34692 [details]
Improved Based on Review - expires as epoch double

[Darin Adler]

Comment on attachment 34692 [details]
Improved Based on Review - expires as epoch double

> +        NSTimeInterval expires = [[cookie expiresDate] timeIntervalSince1970] * 1000;

I'd say double here since this isn't really an NSTimeInterval since it's not in the standard units for NSTimeInterval.

r=me

[Joseph Pecoraro]

(In reply to comment #16)
> (From update of attachment 34692 [details])
> > +        NSTimeInterval expires = [[cookie expiresDate] timeIntervalSince1970] * 1000;
> 
> I'd say double here since this isn't really an NSTimeInterval since it's not in
> the standard units for NSTimeInterval.

I'm not familiar with Obj-C, however the documentation I looked at said that NSTimeInterval was a typedef of a "double".  Are you saying because I multiple by 1000 I should change the type to indicate its not really a Time Interval anymore, but should be treated as just a double?


(In reply to comment #14)
> Hopefully there is a standard way to calculate this for all platforms
> and JavaScript can use Date.parse() to convert it into a Date Object.  Or, if
> there is a way to do this in the C++ like jsNumber, jsBoolean etc that would be
> useful.

I used jsNumber, although I guess the alternative could have been constructDate
after including <runtime/DateConstructor.h>?

[Darin Adler]

(In reply to comment #17)
> Are you saying because I multiply
> by 1000 I should change the type to indicate its not really a Time Interval
> anymore, but should be treated as just a double?

Yes.

> I used jsNumber, although I guess the alternative could have been constructDate
> after including <runtime/DateConstructor.h>?

That's right. I think Date would be pretty neat for this. But at this point we're in the "tweak forever" mode, so we should get this landed soon!

[Joseph Pecoraro]

Okay!  If its good to land please flip the commit-queue bit.  Or, if you'd rather have me do those last few tweaks let me know.

As a heads up here is what it looks like in the Web Inspector, its what I've been playing with today:
http://bogojoker.com/pub/inspector/cookies.png

This needs a couple of r+ patches to land, including this patch and bug 27202.  When all is well I'll start advancing this new stuff.

[Eric Seidel (no email)]

Joe you can set cq=? if you like.  I can't tell from the comments if this is ready for landing or not.  committers can set cq+ and the script will auto-land the patch.

[Eric Seidel (no email)]

Comment on attachment 34692 [details]
Improved Based on Review - expires as epoch double

Rejecting patch 34692 from commit-queue.  This patch will require manual commit.

Patch https://bugs.webkit.org/attachment.cgi?id=34692 from bug 28185 failed to download and apply.

[Eric Seidel (no email)]

Applying 34692 from bug 28185.
patching file WebCore/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file WebCore/WebCore.xcodeproj/project.pbxproj
Hunk #1 succeeded at 4399 (offset 15 lines).
Hunk #2 succeeded at 9331 (offset 33 lines).
Hunk #3 succeeded at 14134 (offset 67 lines).
Hunk #4 FAILED at 17255.
1 out of 4 hunks FAILED -- saving rejects to file WebCore/WebCore.xcodeproj/project.pbxproj.rej
patch -p0 "WebCore/WebCore.xcodeproj/project.pbxproj" returned 1.  Pass --force to ignore patch failures.

[Joseph Pecoraro]

Created attachment 34713 [details]
Merged with ToT

Merged with ToT. There was a merge in the xcodeproj file. Odd, because I only added this to XCode to help others, I don't actually use XCode myself =P.

[Eric Seidel (no email)]

Comment on attachment 34713 [details]
Merged with ToT

Clearing flags on attachment: 34713

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	C	WebCore/platform/haiku/CookieJarHaiku.cpp => WebCore/platform/Cookie.h
	M	WebCore/ChangeLog
	M	WebCore/WebCore.xcodeproj/project.pbxproj
	M	WebCore/bindings/js/JSInspectorBackendCustom.cpp
	M	WebCore/inspector/InspectorBackend.idl
	M	WebCore/platform/CookieJar.h
	M	WebCore/platform/haiku/CookieJarHaiku.cpp
	M	WebCore/platform/mac/CookieJar.mm
	M	WebCore/platform/network/chromium/CookieJarChromium.cpp
	M	WebCore/platform/network/curl/CookieJarCurl.cpp
	M	WebCore/platform/network/soup/CookieJarSoup.cpp
	M	WebCore/platform/network/win/CookieJarCFNetWin.cpp
	M	WebCore/platform/network/win/CookieJarWin.cpp
	M	WebCore/platform/qt/CookieJarQt.cpp
Committed r47185
	M	WebCore/ChangeLog
	M	WebCore/platform/qt/CookieJarQt.cpp
	A	WebCore/platform/Cookie.h
	M	WebCore/platform/mac/CookieJar.mm
	M	WebCore/platform/haiku/CookieJarHaiku.cpp
	M	WebCore/platform/network/chromium/CookieJarChromium.cpp
	M	WebCore/platform/network/soup/CookieJarSoup.cpp
	M	WebCore/platform/network/curl/CookieJarCurl.cpp
	M	WebCore/platform/network/win/CookieJarCFNetWin.cpp
	M	WebCore/platform/network/win/CookieJarWin.cpp
	M	WebCore/platform/CookieJar.h
	M	WebCore/bindings/js/JSInspectorBackendCustom.cpp
	M	WebCore/inspector/InspectorBackend.idl
	M	WebCore/WebCore.xcodeproj/project.pbxproj
r47185 = c2e6ddc0489c8b1b557dce21d734c8613f4eee30 (trunk)
No changes between current HEAD and refs/remotes/trunk
Resetting to the latest refs/remotes/trunk
http://trac.webkit.org/changeset/47185

[Eric Seidel (no email)]

All reviewed patches have been landed.  Closing bug.

[Mark Rowe (bdash)]

This broke the Windows build (<http://build.webkit.org/builders/Windows%20Release%20%28Build%29/builds/3854/steps/compile-webkit/logs/stdio>).  I can't immediately see what is wrong so I'll look at rolling it out.

[Mark Rowe (bdash)]

Rolled out in r47195.

Hopefully you can find someone familiar with Windows to help determine what is going on and this can be re-landed again soon!

[Joseph Pecoraro]

Did I fail to add the header file to any particular build files?

I took a look at another changeset (http://trac.webkit.org/changeset/47170) where files were added and he specifically added the new files to a couple build files:

  - trunk/WebCore/GNUmakefile.am 
  - trunk/WebCore/WebCore.gypi 

Do you think this would solve my problem.  Is there a list of build files that I should update whenever I add a new file?

[Joseph Pecoraro]

Created attachment 34742 [details]
Attempted Build Fix for Windows

aroben made an excellent point in IRC. Even though I change Cookie to a struct, I didn't change the short declaration "class Cookie" to "struct Cookie" inside CookieJar.h.  Hopefully this will fix the Windows Build (I noticed someone fixed Chromium shortly after this initially landed).

Let me know about the build files that I should update.

[Joseph Pecoraro]

Created attachment 34744 [details]
Attempted Build Fix for Windows

Whitespace

[Joseph Pecoraro]

Created attachment 34746 [details]
Fix Extra Semicolons in Haiku

Sorry for the spam =(.

[Joseph Pecoraro]

It looks like someone else just included a Haiku build fix for the Haiku port for  their CookiesJar implementation, and its better then mine:
https://bugs.webkit.org/show_bug.cgi?id=28253

Hopefully there won't be any conflicts, seeing as his does the same fixes as mine and more.  However, his is being applied from a rolled out patch.  I think maybe we should cancel his and I'll update mine to include his changes?

[Joseph Pecoraro]

Created attachment 34749 [details]
Improved Haiku [final I swear]

Sorry yet again.

[Maxime Simon]

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

[Eric Seidel (no email)]

Comment on attachment 34749 [details]
Improved Haiku [final I swear]

Clearing flags on attachment: 34749

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	C	WebCore/platform/haiku/CookieJarHaiku.cpp => WebCore/platform/Cookie.h
	M	WebCore/ChangeLog
	M	WebCore/WebCore.xcodeproj/project.pbxproj
	M	WebCore/bindings/js/JSInspectorBackendCustom.cpp
	M	WebCore/inspector/InspectorBackend.idl
	M	WebCore/platform/CookieJar.h
	M	WebCore/platform/haiku/CookieJarHaiku.cpp
	M	WebCore/platform/mac/CookieJar.mm
	M	WebCore/platform/network/chromium/CookieJarChromium.cpp
	M	WebCore/platform/network/curl/CookieJarCurl.cpp
	M	WebCore/platform/network/soup/CookieJarSoup.cpp
	M	WebCore/platform/network/win/CookieJarCFNetWin.cpp
	M	WebCore/platform/network/win/CookieJarWin.cpp
	M	WebCore/platform/qt/CookieJarQt.cpp
Committed r47205
	M	WebCore/ChangeLog
	M	WebCore/platform/qt/CookieJarQt.cpp
	A	WebCore/platform/Cookie.h
	M	WebCore/platform/mac/CookieJar.mm
	M	WebCore/platform/haiku/CookieJarHaiku.cpp
	M	WebCore/platform/network/chromium/CookieJarChromium.cpp
	M	WebCore/platform/network/soup/CookieJarSoup.cpp
	M	WebCore/platform/network/curl/CookieJarCurl.cpp
	M	WebCore/platform/network/win/CookieJarCFNetWin.cpp
	M	WebCore/platform/network/win/CookieJarWin.cpp
	M	WebCore/platform/CookieJar.h
	M	WebCore/bindings/js/JSInspectorBackendCustom.cpp
	M	WebCore/inspector/InspectorBackend.idl
	M	WebCore/WebCore.xcodeproj/project.pbxproj
r47205 = ce6d838c267e15719f6ce0180a65d8d395df2d2b (trunk)
No changes between current HEAD and refs/remotes/trunk
Resetting to the latest refs/remotes/trunk
http://trac.webkit.org/changeset/47205

[Eric Seidel (no email)]

All reviewed patches have been landed.  Closing bug.