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

Jason Liu
Reported 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
Attachments
Patch (20.83 KB, patch)
2012-02-29 01:59 PST, Jason Liu
no flags
Patch (20.86 KB, patch)
2012-02-29 02:07 PST, Jason Liu
no flags
Patch (17.63 KB, patch)
2012-03-01 00:59 PST, Jason Liu
no flags
Patch (20.32 KB, patch)
2012-03-01 01:14 PST, Jason Liu
no flags
Jason Liu
Comment 1 2012-02-29 01:59:05 PST
Jason Liu
Comment 2 2012-02-29 02:07:56 PST
Joe Mason
Comment 3 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; }
Jason Liu
Comment 4 2012-03-01 00:59:20 PST
Jason Liu
Comment 5 2012-03-01 01:14:00 PST
Joe Mason
Comment 6 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.
WebKit Review Bot
Comment 7 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
WebKit Review Bot
Comment 8 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>
WebKit Review Bot
Comment 9 2012-03-01 20:26:58 PST
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.