Bug 79870

Summary: [BlackBerry]Array of Cookies in HTTP request header are not in order.
Product: WebKit Reporter: Jason Liu <jasonliuwebkit>
Component: New BugsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: charles.wei, jasonliuwebkit, joenotcharles, kpiascik, staikos, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Other   
OS: Other   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch none

Description Jason Liu 2012-02-28 22:09:29 PST
Test Case:
----------
Setting array of cookies as follow:
setcookie("setArraycookie[three]", "cookiethree");
setcookie("setArraycookie[two]", "cookietwo");
setcookie("setArraycookie[one]", "cookieone");

Expected Results:
-----------------
Order of cookies in the HTTP request should be the same order as the HTTP
response sent from the server:

Cookie:setArraycookie[three]=cookiethree; setArraycookie[two]=cookietwo;
setArraycookie[one]=cookieone

Actual results:
---------------
Cookie Array are not in the same order:

Cookie:setArraycookie[one]=cookieone; setArraycookie[three]=cookiethree;
setArraycookie[two]=cookietwo
Comment 1 Jason Liu 2012-02-29 01:59:05 PST
Created attachment 129419 [details]
Patch
Comment 2 Jason Liu 2012-02-29 02:07:56 PST
Created attachment 129420 [details]
Patch
Comment 3 Joe Mason 2012-02-29 12:32:48 PST
The refactor of removeCookie doesn't do the right thing.  Without this patch it was:

ParsedCookie* CookieMap::removeCookie(const ParsedCookie* cookie)
{
    // Find a previous entry for deletion
    String key = cookie->name() + cookie->path();
    ParsedCookie* prevCookie = m_cookieMap.take(key);

    if (!prevCookie)
        return 0;

    if (prevCookie == m_oldestCookie)
        updateOldestCookie();
    else if (prevCookie != cookie) {
        // The cookie we used to search is force expired, we must do the same
        // to the cookie in memory too.
        if (cookie->isForceExpired())
            prevCookie->forceExpire();
        delete cookie;
    }

    if (!prevCookie->isSession())
        cookieManager().removedCookie();
    return prevCookie;
}

And it was called from two places.  The new removeCookieAtIndex only does the equivalent of m_cookieMap.take(key), and so when it's called from getAllCookies, all the other bookkeeping done by removeCookie is skipped.

removeCookieAtIndex should be:

ParsedCookie* CookieMap::removeCookieAtIndex(size_t position)
{
    ASSERT(0 <= position && position < m_cookieVector.size());
    ParsedCookie* prevCookie = m_cookieVector[position];
    m_cookieVector.remove(position);

    if (prevCookie == m_oldestCookie)
        updateOldestCookie();
    else if (prevCookie != cookie) {
        // The cookie we used to search is force expired, we must do the same
        // to the cookie in memory too.
        if (cookie->isForceExpired())
            prevCookie->forceExpire();
        delete cookie;
    }

    if (!prevCookie->isSession())
        cookieManager().removedCookie();
    return prevCookie;
}

And then removeCookie is just:

ParsedCookie* CookieMap::removeCookie(const ParsedCookie* cookie)
{
    size_t cookieCount = m_cookieVector.size();
    for (int position = 0; position < cookieCount; ++position) {
        if (m_cookieVector[position]->name() == cookie->name() && m_cookieVector[position]->path() == cookie->path())
            return removeCookieAtIndex(position);
    }
    return 0;
}
Comment 4 Jason Liu 2012-03-01 00:59:20 PST
Created attachment 129657 [details]
Patch
Comment 5 Jason Liu 2012-03-01 01:14:00 PST
Created attachment 129660 [details]
Patch
Comment 6 Joe Mason 2012-03-01 10:47:17 PST
I was a little surprised to find out that removeCookieAtIndex still needs the "cookie" parameter as well as the index, but after I thought about it a bit, it makes sense: this is for the case where a cookie has been updated (ie. a new cookie with the same name and path overwrote the old one), and removeCookie is called based on the old cookie.  In this case there is extra cleanup to do, so the old cookie still needs to be passed in.

A comment explaining this would be good.  Otherwise looks good to me.
Comment 7 WebKit Review Bot 2012-03-01 11:00:02 PST
Comment on attachment 129660 [details]
Patch

Rejecting attachment 129660 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1

ERROR: /mnt/git/webkit-commit-queue/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

Full output: http://queues.webkit.org/results/11769567
Comment 8 WebKit Review Bot 2012-03-01 20:26:54 PST
Comment on attachment 129660 [details]
Patch

Clearing flags on attachment: 129660

Committed r109494: <http://trac.webkit.org/changeset/109494>
Comment 9 WebKit Review Bot 2012-03-01 20:26:58 PST
All reviewed patches have been landed.  Closing bug.