Bug 79870 – [BlackBerry]Array of Cookies in HTTP request header are not in order.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Jason Liu]

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

[Jason Liu]

Created attachment 129419 [details]
Patch

[Jason Liu]

Created attachment 129420 [details]
Patch

[Joe Mason]

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;
}

[Jason Liu]

Created attachment 129657 [details]
Patch

[Jason Liu]

Created attachment 129660 [details]
Patch

[Joe Mason]

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.

[WebKit Review Bot]

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

[WebKit Review Bot]

Comment on attachment 129660 [details]
Patch

Clearing flags on attachment: 129660

Committed r109494: <http://trac.webkit.org/changeset/109494>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.