Summary: | [BlackBerry]Array of Cookies in HTTP request header are not in order. | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Jason Liu <jasonliuwebkit> | ||||||||||
Component: | New Bugs | Assignee: | 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
Jason Liu
2012-02-28 22:09:29 PST
Created attachment 129419 [details]
Patch
Created attachment 129420 [details]
Patch
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; } Created attachment 129657 [details]
Patch
Created attachment 129660 [details]
Patch
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 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 on attachment 129660 [details] Patch Clearing flags on attachment: 129660 Committed r109494: <http://trac.webkit.org/changeset/109494> All reviewed patches have been landed. Closing bug. |