Bug 79870 - [BlackBerry]Array of Cookies in HTTP request header are not in order.
Summary: [BlackBerry]Array of Cookies in HTTP request header are not in order.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other Other
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-02-28 22:09 PST by Jason Liu
Modified: 2012-03-01 20:26 PST (History)
6 users (show)

See Also:


Attachments
Patch (20.83 KB, patch)
2012-02-29 01:59 PST, Jason Liu
no flags Details | Formatted Diff | Diff
Patch (20.86 KB, patch)
2012-02-29 02:07 PST, Jason Liu
no flags Details | Formatted Diff | Diff
Patch (17.63 KB, patch)
2012-03-01 00:59 PST, Jason Liu
no flags Details | Formatted Diff | Diff
Patch (20.32 KB, patch)
2012-03-01 01:14 PST, Jason Liu
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.