Bug 30104 – Inspector should show cookies of sub-resources on the page.

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

[Brian Weinstein]

Created attachment 40677 [details]
iFrame test case

Inspector should show cookies of sub-resources on the page.

[Brian Weinstein]

That test case should show the user's cookie's on webkit.org, but it doesn't.

[Brian Weinstein]

Created attachment 40678 [details]
Work in Progress Screenshot

This is a rough pass at the look/functionality. One issue I am struggling with is that cookies are defined by their full URL, so to differentiate between resources, we need the full URL instead of the domain, but the full URL is long, so it can be cut off (as it is in this image).

[Timothy Hatcher]

(In reply to comment #2)
> Created an attachment (id=40678) [details]
> Work in Progress Screenshot
> 
> This is a rough pass at the look/functionality. One issue I am struggling with
> is that cookies are defined by their full URL, so to differentiate between
> resources, we need the full URL instead of the domain, but the full URL is
> long, so it can be cut off (as it is in this image).

Cookies are not defined by their full URL. Cokkies are defined by a domain suffix and a path prefix. A full URL matches against that.

I proposed we should show all cookies that match the shortest domain.

So if a resource is from images.apple.com, we show all cookies for apple.com as an item in the sidebar.

And if another resource is from ads.doubleclick.net, then we show another item in the sidebar for doubleclick.net.

Make sense?

[Timothy Hatcher]

Also why why are there only two columns in the data grid?

[Joseph Pecoraro]

(In reply to comment #4)
> Also why why are there only two columns in the data grid?

It looks like he is on Windows. My guess is access to raw cookies isn't implemented in Windows (I only did it for Mac) and so its falling back to the JavaScript version?

[Timothy Hatcher]

Ah, makes sense.

Brian, make sure you try on the Mac, since it uses a different code path.

Also you should implent what is need on Windows to get the full data like Mac.

[Brian Weinstein]

(In reply to comment #3)
> (In reply to comment #2)
> > Created an attachment (id=40678) [details] [details]
> > Work in Progress Screenshot
> > 
> > This is a rough pass at the look/functionality. One issue I am struggling with
> > is that cookies are defined by their full URL, so to differentiate between
> > resources, we need the full URL instead of the domain, but the full URL is
> > long, so it can be cut off (as it is in this image).
> 
> Cookies are not defined by their full URL. Cokkies are defined by a domain
> suffix and a path prefix. A full URL matches against that.
> 
> I proposed we should show all cookies that match the shortest domain.
> 
> So if a resource is from images.apple.com, we show all cookies for apple.com as
> an item in the sidebar.
> 
> And if another resource is from ads.doubleclick.net, then we show another item
> in the sidebar for doubleclick.net.
> 
> Make sense?

Yeah, I had my wires crossed about a couple things I was thinking about, it makes sense, and I'll get a new screen shot.

[Brian Weinstein]

(In reply to comment #6)
> Ah, makes sense.
> 
> Brian, make sure you try on the Mac, since it uses a different code path.
> 
> Also you should implent what is need on Windows to get the full data like Mac.

I'm testing on both, but I'm not sure if we have the data from CFNetwork to get all that cookie information, but I'll see what I can do.

[Brian Weinstein]

Created attachment 40747 [details]
GetRawCookies on windows

[Brian Weinstein]

Created attachment 40748 [details]
Fix duplicate variable names

[Brady Eidson]

Comment on attachment 40748 [details]
Fix duplicate variable names

Looks fine.

Are you sure we use kCFAbsoluteTimeIntervalSince1970 and not ReferenceDate?  (Jan 1 2001)?

I'm not sure, I'm just asking.

[Brian Weinstein]

(In reply to comment #11)
> (From update of attachment 40748 [details])
> Looks fine.
> 
> Are you sure we use kCFAbsoluteTimeIntervalSince1970 and not ReferenceDate? 
> (Jan 1 2001)?
> 
> I'm not sure, I'm just asking.

The mac uses NSTimeInterval expires = [[cookie expiresDate] timeIntervalSince1970] * 1000, and the JavaScript parses it based on 1970, so the kCFAbsoluteTimeIntervalSince1970 is necessary. Thanks for the review!

[Brian Weinstein]

Part 1 landed in http://trac.webkit.org/changeset/49210.

[Brian Weinstein]

Created attachment 40756 [details]
Cookies from Subdomains Screenshot

[Brian Weinstein]

Created attachment 40760 [details]
Fix

[Joseph Pecoraro]

A few comments and questions for my own education.

> this._cookieView = [];
> ... later on ...
> var view = this._cookieView[cookieDomain];

You're using _cookieView like an object / hashtable but initialize it as an array. Although it works as an array "[]", it would be clearer as an object "{}".

> this._cookieDomains = [];
> ...
> // Eliminate duplicate domains from the list.
> if (this._cookieDomains.indexOf(domain) != -1)
>   return;

Although its probably negligible in this case because the # of domains is likely to be very small, is it better to turn this into an Object with properties and look for duplicates that way?  Is there a heuristic for choosing one approach over the other?

  if (!this._cookieDomains[domain]) {
    this._cookieDomains[domain] = true; // Prevent duplicates
    // ...
  }


Any ideas on implementing deleteCookie for Windows?  I honestly can't find a scrap of documentation on CFHTTPCookieStorage stuff. And when the trail finally ended it was in a binary lib file =(.

[Brian Weinstein]

(In reply to comment #16)
> A few comments and questions for my own education.
> 
> > this._cookieView = [];
> > ... later on ...
> > var view = this._cookieView[cookieDomain];
> 
> You're using _cookieView like an object / hashtable but initialize it as an
> array. Although it works as an array "[]", it would be clearer as an object
> "{}".

I was trying to make it behave like an associative array, this._cookieView['string'] = CookieTreeElement or null, wasn't sure the best way to handle this in JavaScript.

> 
> > this._cookieDomains = [];
> > ...
> > // Eliminate duplicate domains from the list.
> > if (this._cookieDomains.indexOf(domain) != -1)
> >   return;
> 
> Although its probably negligible in this case because the # of domains is
> likely to be very small, is it better to turn this into an Object with
> properties and look for duplicates that way?  Is there a heuristic for choosing
> one approach over the other?
> 
>   if (!this._cookieDomains[domain]) {
>     this._cookieDomains[domain] = true; // Prevent duplicates
>     // ...
>   }
> 

Not sure which way is the best, like I mentioned above. I figure I'll take Tim's opinion when he reviews the patch.

> Any ideas on implementing deleteCookie for Windows?  I honestly can't find a
> scrap of documentation on CFHTTPCookieStorage stuff. And when the trail finally
> ended it was in a binary lib file =(.

Yeah, if you want to file a bug about it, I should be able to implement that tomorrow, assuming CFNetwork has the API I hope it does. You can assign it to me if you do want to file it.

Thanks for the feedback! I'll see what Tim has to say when he reviews it, and then go from there with his and your feedback.

[Darin Adler]

Comment on attachment 40760 [details]
Fix

> +    // If we can't get raw cookies - fallback to String representation

The verb "fall back" is two words.

> +    String cookieList = String();

Strings automatically start out with null value, so there's no need to say "= String()".

Naming cookiesList and cookieList almost the same is a bad idea. Give them more clearly different names, please.

> +        Document* doc = it->second->frame()->document();

I prefer document rather than "doc".

> +        if (doc->url().host() == host)
> +        {

Brace goes on same line as if statement.

> +                    if (cookiesList.find(docCookiesList[i]) == WTF::notFound)
> +                        cookiesList.append(docCookiesList);

If you're going to do a find like, it seems to me you should be use a ListHashSet rather than a Vector so it won't get super-slow.

You should be able to use just notFound rather than WTF::notFound. If not, this should be fixed in WTF by adding a using in the file that defines notFound.

> +    if (!isImplemented)
> +        m_frontend->didGetCookies(callId, m_frontend->newScriptArray(), cookieList);
> +    else
> +        m_frontend->didGetCookies(callId, buildArrayForCookies(cookiesList), String());

I find the variable name "isImplemented" confusing in this longer function although it was clear in the shorter function you created it from. It's also strange that it gets set repeatedly each time through the loop. Are you sure it will have the same value every time? Could it return different values? I think it's ugly that you get the value repeatedly and assume it's the same every time.

> +    unsigned length = cookiesList.size();

Some places you are using unsigned and other places int. Normally I suggest using size_t to iterate a Vector.

> +        const Cookie& cookie = cookiesList[i];
> +        cookies.set(i, buildObjectForCookie(cookie));

I don't think the local variable adds value here. You should just write it in one line.

> +    ScriptObject value = m_frontend->newScriptObject();
> +    value.set("name", cookie.name);
> +    value.set("value", cookie.value);
> +    value.set("domain", cookie.domain);
> +    value.set("path", cookie.path);
> +    value.set("expires", cookie.expires);
> +    value.set("size", static_cast<int>(cookie.name.length() + cookie.value.length()));
> +    value.set("httpOnly", cookie.httpOnly);
> +    value.set("secure", cookie.secure);
> +    value.set("session", cookie.session);
> +    return value;

Does the static_cast<int> really need to be there? Could you explain why?

> +        if (document->url().host() == domain) {
> +            WebCore::deleteCookie(document, document->cookieURL(), cookieName);
> +        }

No braces on a single-line if statement.

> +    ScriptObject buildObjectForCookie(const Cookie& cookie);
> +    ScriptArray buildArrayForCookies(const Vector<Cookie>& cookiesList);

We normally omit argument names in cases like this where the type makes it clear alone.

> +        void addCookieDomainForDocument(const Document* document);

Normally we would just use Document*, not const Document* and not give a name for the argument.

> -        delete this._cookieView;
> +        this._cookieDomains = [];
> +        this._cookieView = [];

It seems strange to set _cookieView to an empty array. Why that instead of setting it to an empty object, or to null, or empty string, or deleting the property?

> +        // Eliminate duplicate domains from the list.
> +        if (this._cookieDomains.indexOf(domain) != -1)
> +            return;

This is only efficient if we are guaranteed there are a small number of domains. If there's a larger number, then you should keep another object that uses the domains as property names so you can use hashing to quickly check if the domain is in the list.

> +        bool operator==(const Cookie& cookie) const
> +        {
> +            return cookie.name == name && cookie.domain == domain && cookie.path == path && cookie.secure == secure;
> +        }

Normally it's better to make operator== a non-member function instead of a member.

Normally we define != too if we define ==.

review- because there are enough comments that I think you'll want to make at least some changes

[Joseph Pecoraro]

Some of these comments refer to my older code that Brian moved between files.

> > +    value.set("size", static_cast<int>(cookie.name.length() + cookie.value.length()));
> 
> Does the static_cast<int> really need to be there? Could you explain why?

ScriptObject.set() does not work with an unsigned (the return value from String.length()). int can sufficiently hold the value and satisfied the compiler warnings.  Perhaps a better solution would be to add that signature and implementation to ScriptObject?

[Brian Weinstein]

Created attachment 40817 [details]
Addresses Darin's + Joe's comments

[Brian Weinstein]

Created attachment 40818 [details]
Style Fixes + deleteCookie support on Windows

[Pavel Feldman]

Comment on attachment 40818 [details]
Style Fixes + deleteCookie support on Windows

> +bool ScriptObject::set(const char* name, unsigned value)
> +{
> +    JSLock lock(SilenceAssertionsOnly);
> +    PutPropertySlot slot;
> +    jsObject()->put(m_scriptState, Identifier(m_scriptState, name), jsNumber(m_scriptState, value), slot);
> +    return handleException(m_scriptState);
> +}

I'd like to do that for v8 prior to landing this.

> +void InspectorFrontend::addCookieDomainForDocument(Document* document)
> +{
> +    OwnPtr<ScriptFunctionCall> function(newFunctionCall("addCookieDomain"));
> +    function->appendArgument(document->url().host());
> +    function->call();
> +}
> +

Names of InspectorFrontend::* and corresponding WebInspector.* methods should match. InspectorFrontend is a C++ API to the frontend. It is pretty much reflecting the WebInspector.* API in JS that InspectorController is calling into. We could even try generating it. I'd suggest introducing

void InspectorFrontend::addCookieDomain(const String& domain)

instead.

[Pavel Feldman]

Created attachment 40822 [details]
[V8 bindings] ScriptObject::set(const char*, unsigned)

[Timothy Hatcher]

Comment on attachment 40818 [details]
Style Fixes + deleteCookie support on Windows

> +        this._cookieView = {};

This is better called _cookieViews.

[Brian Weinstein]

pfeldman's patch landed in http://trac.webkit.org/changeset/49262, my patch landed in http://trac.webkit.org/changeset/49263.

[Yury Semikhatsky]

Created attachment 40947 [details]
patch fixing Chromium breakage

In Chromium Storage panel is still hidden so the WebInspector.addCookieDomain fails there with exception.

[Yury Semikhatsky]

Created attachment 40948 [details]
 patch fixing Chromium breakage

Fixed Changelog entry

[Dimitri Glazkov (Google)]

Comment on attachment 40948 [details]
 patch fixing Chromium breakage

r=me.

[Brian Weinstein]

Sorry guys, I missed the check there. Thanks for the fix!