Bug 86067

Summary:

[BlackBerry] Possible to clobber httponly cookie.

Product:

WebKit

Reporter:

Jason Liu <jasonliuwebkit>

Component:

WebKit BlackBerry

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

cdumez, dannin, dglazkov, jochen, joenotcharles, pnormand, rakuco, rwlbuis, tonikitoo, webkit.review.bot, zan

Priority:

Version:

528+ (Nightly build)

Hardware:

Other

OS:

Other

Bug Depends on:

87208, 87255

Bug Blocks:

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Archive of layout-test-results from ec2-cr-linux-04	none
Patch	none
Patch	none

Jason Liu

Reported 2012-05-10 01:30:11 PDT

The httponly cookies should be changed by javaScript.

Attachments
Patch (8.36 KB, patch) 2012-05-17 23:51 PDT, Jason Liu	no flags	Details Formatted Diff Diff
Patch (9.00 KB, patch) 2012-05-18 02:29 PDT, Jason Liu	no flags	Details Formatted Diff Diff
Patch (17.57 KB, patch) 2012-05-22 00:43 PDT, Jason Liu	no flags	Details Formatted Diff Diff
Patch (17.61 KB, patch) 2012-05-22 00:57 PDT, Jason Liu	no flags	Details Formatted Diff Diff
Patch (17.47 KB, patch) 2012-05-22 02:28 PDT, Jason Liu	no flags	Details Formatted Diff Diff
Patch (17.09 KB, patch) 2012-06-03 23:18 PDT, Jason Liu	no flags	Details Formatted Diff Diff
Patch (17.31 KB, patch) 2012-06-05 03:40 PDT, Jason Liu	no flags	Details Formatted Diff Diff
Patch (17.22 KB, patch) 2012-06-05 04:02 PDT, Jason Liu	no flags	Details Formatted Diff Diff
Archive of layout-test-results from ec2-cr-linux-04 (843.46 KB, application/zip) 2012-06-05 22:56 PDT, WebKit Review Bot	no flags	Details
Patch (18.15 KB, patch) 2012-06-05 23:40 PDT, Jason Liu	no flags	Details Formatted Diff Diff
Patch (18.16 KB, patch) 2012-06-06 01:36 PDT, Jason Liu	no flags	Details Formatted Diff Diff
Show Obsolete (9) View All Add attachment proposed patch, testcase, etc.

Jason Liu

Comment 1 2012-05-17 22:47:02 PDT

(In reply to comment #0) > The httponly cookies should be changed by javaScript. Sorry. The httponly cookies should not be changed by javaScript.

Jason Liu

Comment 2 2012-05-17 23:51:09 PDT

Created attachment 142647 [details] Patch

jochen

Comment 3 2012-05-18 00:57:06 PDT

Comment on attachment 142647 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=142647&action=review > Source/WebCore/platform/blackberry/CookieManager.h:82 > + void setCookies(const KURL&, const String& value, bool isHttpCookie = true); Would it be more consistent with the other methods in this class to define an enum instead of using a bool?

Jason Liu

Comment 4 2012-05-18 02:29:28 PDT

Created attachment 142673 [details] Patch

George Staikos

Comment 5 2012-05-18 06:07:22 PDT

Comment on attachment 142647 [details] Patch This seems incredibly inefficient. Is this the best we can do?

jochen

Comment 6 2012-05-18 12:21:55 PDT

Comment on attachment 142673 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=142673&action=review > Source/WebCore/platform/blackberry/CookieManager.cpp:513 > + getRawCookies(cookies, url, WithHttpOnlyCookies); the only way I see this could be optimized is to move getRawCookies out of the loop. In the end, to insert a cookie, you'll have to check all cookies for the given url, so there's not really much to be improved > LayoutTests/http/tests/cookies/js-get-and-set-http-only-cookie.php:17 > + nit. no empty lines at eof

Jason Liu

Comment 7 2012-05-20 19:18:36 PDT

(In reply to comment #3) > (From update of attachment 142647 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=142647&action=review > > > Source/WebCore/platform/blackberry/CookieManager.h:82 > > + void setCookies(const KURL&, const String& value, bool isHttpCookie = true); > > Would it be more consistent with the other methods in this class to define an enum instead of using a bool? (In reply to comment #5) > (From update of attachment 142647 [details]) > This seems incredibly inefficient. Is this the best we can do? I thought the cookies are always set by http. And they are seldom set by JavaScript. So I didn't want to add more code into setCookies' deeper code path which may decrease the performance of http cookies. Maybe above idea is wrong or there is a better method. Let me think it over.

Jason Liu

Comment 8 2012-05-21 04:22:50 PDT

(In reply to comment #7) > (In reply to comment #3) > > (From update of attachment 142647 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=142647&action=review > > > > > Source/WebCore/platform/blackberry/CookieManager.h:82 > > > + void setCookies(const KURL&, const String& value, bool isHttpCookie = true); > > > > Would it be more consistent with the other methods in this class to define an enum instead of using a bool? > > (In reply to comment #5) > > (From update of attachment 142647 [details] [details]) > > This seems incredibly inefficient. Is this the best we can do? > > I thought the cookies are always set by http. And they are seldom set by JavaScript. So I didn't want to add more code into setCookies' deeper code path which may decrease the performance of http cookies. > > Maybe above idea is wrong or there is a better method. > Let me think it over. I will change my patch.

Jason Liu

Comment 9 2012-05-22 00:43:53 PDT

Created attachment 143215 [details] Patch

jochen

Comment 10 2012-05-22 00:55:41 PDT

Comment on attachment 143215 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=143215&action=review > Source/WebCore/platform/blackberry/CookieMap.cpp:57 > +ParsedCookie* CookieMap::addOrReplaceCookie(ParsedCookie* cookie, CookieFilter filter) I think the semantics that returning cookie means the cookie was not added is very unclear. Can you make it return a bool indicating whether the cookie was added or not, and add a parameter ParsedCookie** replacedCookie which is set to the replaced cookie if the return value is true?

Jason Liu

Comment 11 2012-05-22 00:57:39 PDT

Created attachment 143218 [details] Patch

Jason Liu

Comment 12 2012-05-22 01:08:00 PDT

(In reply to comment #10) > (From update of attachment 143215 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=143215&action=review > > > Source/WebCore/platform/blackberry/CookieMap.cpp:57 > > +ParsedCookie* CookieMap::addOrReplaceCookie(ParsedCookie* cookie, CookieFilter filter) > > I think the semantics that returning cookie means the cookie was not added is very unclear. > > Can you make it return a bool indicating whether the cookie was added or not, and add a parameter ParsedCookie** replacedCookie which is set to the replaced cookie if the return value is true? Let me have a look. Thanks for your high-speed reply and this good suggestion.

Jason Liu

Comment 13 2012-05-22 02:28:46 PDT

Created attachment 143241 [details] Patch

jochen

Comment 14 2012-05-22 02:52:57 PDT

The change looks good to me, thanks (I'm no reviewer, so I can't r+ it..)

Jason Liu

Comment 15 2012-05-22 03:09:44 PDT

(In reply to comment #14) > The change looks good to me, thanks > > (I'm no reviewer, so I can't r+ it..) Thanks. ：）

Rob Buis

Comment 16 2012-05-22 04:56:14 PDT

Comment on attachment 143241 [details] Patch Looks good.

WebKit Review Bot

Comment 17 2012-05-22 19:49:16 PDT

Comment on attachment 143241 [details] Patch Clearing flags on attachment: 143241 Committed r118105: <http://trac.webkit.org/changeset/118105>

WebKit Review Bot

Comment 18 2012-05-22 19:49:35 PDT

All reviewed patches have been landed. Closing bug.

WebKit Review Bot

Comment 19 2012-05-23 05:48:41 PDT

Re-opened since this is blocked by 87255

Jason Liu

Comment 20 2012-05-23 20:19:10 PDT

(In reply to comment #15) > (In reply to comment #14) > > The change looks good to me, thanks > > > > (I'm no reviewer, so I can't r+ it..) > Thanks. ：） Any idea about my test case? I don't think it can cause the other test case failing. Thanks.

Jason Liu

Comment 21 2012-05-23 20:40:04 PDT

Will I commit my test case in our own platform

jochen

Comment 22 2012-05-24 11:37:38 PDT

(In reply to comment #21) > Will I commit my test case in our own platform I think that's not really desirable. It seems that no port should have the ability to override http cookies from javascript. Do you have any idea why the test passed on the EWS (at least the cr-linux bot also executes tests) but then failed when you committed it? can you reproduce this locally on any port?

Jason Liu

Comment 23 2012-05-24 19:46:35 PDT

(In reply to comment #22) > (In reply to comment #21) > > Will I commit my test case in our own platform > > I think that's not really desirable. It seems that no port should have the ability to override http cookies from javascript. > > Do you have any idea why the test passed on the EWS (at least the cr-linux bot also executes tests) but then failed when you committed it? can you reproduce this locally on any port? Maybe there are problems in their own platform DRT running. I don't have the other platform's environment. So can't try to reproduce it. I really can't find any problems in my test case. : (

Jason Liu

Comment 24 2012-06-03 23:18:49 PDT

Created attachment 145520 [details] Patch

Jason Liu

Comment 25 2012-06-03 23:23:11 PDT

(In reply to comment #24) > Created an attachment (id=145520) [details] > Patch I can't test all the other platforms. And so don't know why (In reply to comment #23) > (In reply to comment #22) > > (In reply to comment #21) > > > Will I commit my test case in our own platform > > > > I think that's not really desirable. It seems that no port should have the ability to override http cookies from javascript. > > > > Do you have any idea why the test passed on the EWS (at least the cr-linux bot also executes tests) but then failed when you committed it? can you reproduce this locally on any port? > > Maybe there are problems in their own platform DRT running. > I don't have the other platform's environment. So can't try to reproduce it. > I really can't find any problems in my test case. > : ( I put this test case to blackberry platform.

jochen

Comment 26 2012-06-04 01:07:28 PDT

(In reply to comment #25) > I put this test case to blackberry platform. Given that the test case described behavior that should work on all platforms, and should work the same on all platforms, I think this is the wrong solution. What operating system are you developing on? Building other ports usually doesn't require much effort

Jason Liu

Comment 27 2012-06-04 01:30:25 PDT

(In reply to comment #26) > (In reply to comment #25) > > I put this test case to blackberry platform. > > Given that the test case described behavior that should work on all platforms, and should work the same on all platforms, I think this is the wrong solution. > > What operating system are you developing on? Building other ports usually doesn't require much effort

Jason Liu

Comment 28 2012-06-04 01:32:36 PDT

(In reply to comment #27) > (In reply to comment #26) > > (In reply to comment #25) > > > I put this test case to blackberry platform. > > > > Given that the test case described behavior that should work on all platforms, and should work the same on all platforms, I think this is the wrong solution. > > > > What operating system are you developing on? Building other ports usually doesn't require much effort Please have a look at 87208. They are Mac, GTK, and qt. Would you help me if you have these platform's building already?

jochen

Comment 29 2012-06-04 07:14:55 PDT

Comment on attachment 145520 [details] Patch In order to successfully contribute to WebKit, you should be able to build a few other ports.. View in context: https://bugs.webkit.org/attachment.cgi?id=145520&action=review > LayoutTests/platform/blackberry/http/tests/cookies/js-get-and-set-http-only-cookie.php:2 > +setcookie("httpOnlyCookie", "value", time()+36000000, "", "", 0, 1); you should use the cookie scripts located in http/tests/cookies/resources as in your previos patch to set the cookie. The bug in your previous patch was that you use php to set the cookie, instead of the setCookie helper function. Therefore, the call to clearCookies() had no effect. Please revert to the previous version, but change your test to not use PHP to set the cookie. Please also remove the baselines committed by other ports (such as LayoutTests/platform/mac/http/tests/cookies/js-get-and-set-http-only-cookie-expected.txt)

Rob Buis

Comment 30 2012-06-04 12:05:57 PDT

Comment on attachment 145520 [details] Patch R- based on Jochen's comments.

Jason Liu

Comment 31 2012-06-05 03:40:49 PDT

Created attachment 145748 [details] Patch

jochen

Comment 32 2012-06-05 03:44:13 PDT

Comment on attachment 145748 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=145748&action=review > Source/WebCore/ChangeLog:11 > + Test: platform/blackberry/http/tests/cookies/js-get-and-set-http-only-cookie.php nit: the test is not in platform/blackberry anymore > LayoutTests/ChangeLog:8 > + * platform/blackberry/http/tests/cookies/js-get-and-set-http-only-cookie-expected.txt: Added. also here. Please also delete the old test expectations/unskip the test and update this list accordingly > LayoutTests/http/tests/cookies/js-get-and-set-http-only-cookie.html:1 > +<!DCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN"> <!DOCTYPE html>

Jason Liu

Comment 33 2012-06-05 04:02:32 PDT

Created attachment 145753 [details] Patch

Jason Liu

Comment 34 2012-06-05 04:03:45 PDT

(In reply to comment #32) > (From update of attachment 145748 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=145748&action=review > > > Source/WebCore/ChangeLog:11 > > + Test: platform/blackberry/http/tests/cookies/js-get-and-set-http-only-cookie.php > > nit: the test is not in platform/blackberry anymore > > > LayoutTests/ChangeLog:8 > > + * platform/blackberry/http/tests/cookies/js-get-and-set-http-only-cookie-expected.txt: Added. > > also here. Please also delete the old test expectations/unskip the test and update this list accordingly > > > LayoutTests/http/tests/cookies/js-get-and-set-http-only-cookie.html:1 > > +<!DCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN"> > > <!DOCTYPE html> Thanks a lot for your help. : )

jochen

Comment 35 2012-06-05 04:09:21 PDT

Comment on attachment 145753 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=145753&action=review > LayoutTests/ChangeLog:9 > + * http/tests/cookies/js-get-and-set-http-only-cookie.html: Added. there's still LayoutTests/platform/mac/http/tests/cookies/js-get-and-set-http-only-cookie-expected.txt can you delete that file as part of this CL? otherwise, it won't pass on the mac bots

Jason Liu

Comment 36 2012-06-05 04:24:17 PDT

(In reply to comment #35) > (From update of attachment 145753 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=145753&action=review > > > LayoutTests/ChangeLog:9 > > + * http/tests/cookies/js-get-and-set-http-only-cookie.html: Added. > > there's still LayoutTests/platform/mac/http/tests/cookies/js-get-and-set-http-only-cookie-expected.txt > > can you delete that file as part of this CL? otherwise, it won't pass on the mac bots I think mac will fail with the new test, too. So LayoutTests/platform/mac/http/tests/cookies/js-get-and-set-http-only-cookie-expected.txt is needed to make mac pass.

jochen

Comment 37 2012-06-05 04:26:27 PDT

(In reply to comment #36) > (In reply to comment #35) > > (From update of attachment 145753 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=145753&action=review > > > > > LayoutTests/ChangeLog:9 > > > + * http/tests/cookies/js-get-and-set-http-only-cookie.html: Added. > > > > there's still LayoutTests/platform/mac/http/tests/cookies/js-get-and-set-http-only-cookie-expected.txt > > > > can you delete that file as part of this CL? otherwise, it won't pass on the mac bots > > I think mac will fail with the new test, too. > So LayoutTests/platform/mac/http/tests/cookies/js-get-and-set-http-only-cookie-expected.txt is needed to make mac pass. Why do you think so?

Jason Liu

Comment 38 2012-06-05 04:32:22 PDT

(In reply to comment #37) > (In reply to comment #36) > > (In reply to comment #35) > > > (From update of attachment 145753 [details] [details] [details]) > > > View in context: https://bugs.webkit.org/attachment.cgi?id=145753&action=review > > > > > > > LayoutTests/ChangeLog:9 > > > > + * http/tests/cookies/js-get-and-set-http-only-cookie.html: Added. > > > > > > there's still LayoutTests/platform/mac/http/tests/cookies/js-get-and-set-http-only-cookie-expected.txt > > > > > > can you delete that file as part of this CL? otherwise, it won't pass on the mac bots > > > > I think mac will fail with the new test, too. > > So LayoutTests/platform/mac/http/tests/cookies/js-get-and-set-http-only-cookie-expected.txt is needed to make mac pass. > > Why do you think so? In my opinion, mac added this *.txt to make the old test *.php pass. Although I changed the test case, mac's result will be also the same as *.txt. So it is needed for mac all the same. Do you agree with me?

Jason Liu

Comment 39 2012-06-05 04:33:15 PDT

(In reply to comment #38) > (In reply to comment #37) > > (In reply to comment #36) > > > (In reply to comment #35) > > > > (From update of attachment 145753 [details] [details] [details] [details]) > > > > View in context: https://bugs.webkit.org/attachment.cgi?id=145753&action=review > > > > > > > > > LayoutTests/ChangeLog:9 > > > > > + * http/tests/cookies/js-get-and-set-http-only-cookie.html: Added. > > > > > > > > there's still LayoutTests/platform/mac/http/tests/cookies/js-get-and-set-http-only-cookie-expected.txt > > > > > > > > can you delete that file as part of this CL? otherwise, it won't pass on the mac bots > > > > > > I think mac will fail with the new test, too. > > > So LayoutTests/platform/mac/http/tests/cookies/js-get-and-set-http-only-cookie-expected.txt is needed to make mac pass. > > > > Why do you think so? > > In my opinion, mac added this *.txt to make the old test *.php pass. > Although I changed the test case, mac's result will be also the same as *.txt. > So it is needed for mac all the same. > Do you agree with me? If I remove it, mac will fail again.

jochen

Comment 40 2012-06-05 04:35:54 PDT

(In reply to comment #38) > In my opinion, mac added this *.txt to make the old test *.php pass. > Although I changed the test case, mac's result will be also the same as *.txt. > So it is needed for mac all the same. > Do you agree with me? I'd hope that now that you use the cookies testing framework, the test passes on all platforms.

Jason Liu

Comment 41 2012-06-05 04:41:48 PDT

(In reply to comment #40) > (In reply to comment #38) > > In my opinion, mac added this *.txt to make the old test *.php pass. > > Although I changed the test case, mac's result will be also the same as *.txt. > > So it is needed for mac all the same. > > Do you agree with me? > > I'd hope that now that you use the cookies testing framework, the test passes on all platforms. I think mac fails because its httpOnly cookies can be changed by JavaScript.

jochen

Comment 42 2012-06-05 05:06:25 PDT

(In reply to comment #41) > (In reply to comment #40) > > (In reply to comment #38) > > > In my opinion, mac added this *.txt to make the old test *.php pass. > > > Although I changed the test case, mac's result will be also the same as *.txt. > > > So it is needed for mac all the same. > > > Do you agree with me? > > > > I'd hope that now that you use the cookies testing framework, the test passes on all platforms. > > I think mac fails because its httpOnly cookies can be changed by JavaScript. Ok, I manually verified this. Note that the platform/mac suppression will also apply to other ports, so when you land this change, there will be some redness. Anyway, can you comment on https://bugs.webkit.org/show_bug.cgi?id=87208 that the failure on mac is because of an actual bug in their cookie jar? otherwise, the patch looks good

Jason Liu

Comment 43 2012-06-05 05:29:39 PDT

(In reply to comment #42) > (In reply to comment #41) > > (In reply to comment #40) > > > (In reply to comment #38) > > > > In my opinion, mac added this *.txt to make the old test *.php pass. > > > > Although I changed the test case, mac's result will be also the same as *.txt. > > > > So it is needed for mac all the same. > > > > Do you agree with me? > > > > > > I'd hope that now that you use the cookies testing framework, the test passes on all platforms. > > > > I think mac fails because its httpOnly cookies can be changed by JavaScript. > > Ok, I manually verified this. > > Note that the platform/mac suppression will also apply to other ports, so when you land this change, there will be some redness. > > Anyway, can you comment on https://bugs.webkit.org/show_bug.cgi?id=87208 that the failure on mac is because of an actual bug in their cookie jar? > > otherwise, the patch looks good I've added comments for 87208. Thanks a lot for your help.

WebKit Review Bot

Comment 44 2012-06-05 22:56:36 PDT

Comment on attachment 145753 [details] Patch Attachment 145753 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12905704 New failing tests: http/tests/cookies/js-get-and-set-http-only-cookie.html

WebKit Review Bot

Comment 45 2012-06-05 22:56:42 PDT

Created attachment 145936 [details] Archive of layout-test-results from ec2-cr-linux-04 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-04 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

Jason Liu

Comment 46 2012-06-05 23:26:15 PDT

(In reply to comment #44) > (From update of attachment 145753 [details]) > Attachment 145753 [details] did not pass chromium-ews (chromium-xvfb): > Output: http://queues.webkit.org/results/12905704 > > New failing tests: > http/tests/cookies/js-get-and-set-http-only-cookie.html This is caused by LayoutTests/platform/mac/http/tests/cookies/js-get-and-set-http-only-cookie-expected.txt. I have to delete it since it effects chromium's right result.

Jason Liu

Comment 47 2012-06-05 23:40:07 PDT

Created attachment 145945 [details] Patch

jochen

Comment 48 2012-06-06 00:26:35 PDT

(In reply to comment #46) > (In reply to comment #44) > > (From update of attachment 145753 [details] [details]) > > Attachment 145753 [details] [details] did not pass chromium-ews (chromium-xvfb): > > Output: http://queues.webkit.org/results/12905704 > > > > New failing tests: > > http/tests/cookies/js-get-and-set-http-only-cookie.html > > This is caused by LayoutTests/platform/mac/http/tests/cookies/js-get-and-set-http-only-cookie-expected.txt. > > I have to delete it since it effects chromium's right result. I think the better solution is to add the right results to platform/chromium/http/tests/cookies/... because mac will need to failing results

Jason Liu

Comment 49 2012-06-06 00:59:43 PDT

(In reply to comment #48) > (In reply to comment #46) > > (In reply to comment #44) > > > (From update of attachment 145753 [details] [details] [details]) > > > Attachment 145753 [details] [details] [details] did not pass chromium-ews (chromium-xvfb): > > > Output: http://queues.webkit.org/results/12905704 > > > > > > New failing tests: > > > http/tests/cookies/js-get-and-set-http-only-cookie.html > > > > This is caused by LayoutTests/platform/mac/http/tests/cookies/js-get-and-set-http-only-cookie-expected.txt. > > > > I have to delete it since it effects chromium's right result. > > I think the better solution is to add the right results to platform/chromium/http/tests/cookies/... because mac will need to failing results And shall we do this on the other platforms? Why the other ports follow LayoutTests/platform/mac/http/tests/cookies/js-get-and-set-http-only-cookie-expected.txt? It seems better to remove this *.txt if there are more ports failing under this wrong result. I think it is better to delete this wrong result and ask mac to fix the cookieJar's bug.

jochen

Comment 50 2012-06-06 01:05:46 PDT

(In reply to comment #49) > (In reply to comment #48) > > (In reply to comment #46) > > > (In reply to comment #44) > > > > (From update of attachment 145753 [details] [details] [details] [details]) > > > > Attachment 145753 [details] [details] [details] [details] did not pass chromium-ews (chromium-xvfb): > > > > Output: http://queues.webkit.org/results/12905704 > > > > > > > > New failing tests: > > > > http/tests/cookies/js-get-and-set-http-only-cookie.html > > > > > > This is caused by LayoutTests/platform/mac/http/tests/cookies/js-get-and-set-http-only-cookie-expected.txt. > > > > > > I have to delete it since it effects chromium's right result. > > > > I think the better solution is to add the right results to platform/chromium/http/tests/cookies/... because mac will need to failing results > > And shall we do this on the other platforms? > Why the other ports follow LayoutTests/platform/mac/http/tests/cookies/js-get-and-set-http-only-cookie-expected.txt? Because mac is somewhat like the "main" port. > > It seems better to remove this *.txt if there are more ports failing under this wrong result. > I think it is better to delete this wrong result and ask mac to fix the cookieJar's bug. Until they fix this, they will add back the suppression. Anyway, that's what I meant in comment 42 - just leave the expectations as is, and let the gardeners figure out what to do

Jason Liu

Comment 51 2012-06-06 01:19:30 PDT

(In reply to comment #50) > (In reply to comment #49) > > (In reply to comment #48) > > > (In reply to comment #46) > > > > (In reply to comment #44) > > > > > (From update of attachment 145753 [details] [details] [details] [details] [details]) > > > > > Attachment 145753 [details] [details] [details] [details] [details] did not pass chromium-ews (chromium-xvfb): > > > > > Output: http://queues.webkit.org/results/12905704 > > > > > > > > > > New failing tests: > > > > > http/tests/cookies/js-get-and-set-http-only-cookie.html > > > > > > > > This is caused by LayoutTests/platform/mac/http/tests/cookies/js-get-and-set-http-only-cookie-expected.txt. > > > > > > > > I have to delete it since it effects chromium's right result. > > > > > > I think the better solution is to add the right results to platform/chromium/http/tests/cookies/... because mac will need to failing results > > > > And shall we do this on the other platforms? > > Why the other ports follow LayoutTests/platform/mac/http/tests/cookies/js-get-and-set-http-only-cookie-expected.txt? > > Because mac is somewhat like the "main" port. > > > > > > It seems better to remove this *.txt if there are more ports failing under this wrong result. > > I think it is better to delete this wrong result and ask mac to fix the cookieJar's bug. > > Until they fix this, they will add back the suppression. Anyway, that's what I meant in comment 42 - just leave the expectations as is, and let the gardeners figure out what to do Ok, I will add the right result to chromium's platform.

Jason Liu

Comment 52 2012-06-06 01:36:14 PDT

Created attachment 145962 [details] Patch

jochen

Comment 53 2012-06-08 04:24:41 PDT

(In reply to comment #52) > Created an attachment (id=145962) [details] > Patch That patch looks good to me

Rob Buis

Comment 54 2012-06-08 07:04:17 PDT

Comment on attachment 145962 [details] Patch Looks good.

WebKit Review Bot

Comment 55 2012-06-10 19:16:36 PDT

Comment on attachment 145962 [details] Patch Clearing flags on attachment: 145962 Committed r119947: <http://trac.webkit.org/changeset/119947>

WebKit Review Bot

Comment 56 2012-06-10 19:16:43 PDT

All reviewed patches have been landed. Closing bug.

Raphael Kubo da Costa (:rakuco)

Comment 57 2012-06-10 20:44:08 PDT

This appears to have broken the EFL and GTK+ bots: http://build.webkit.org/builders/GTK%20Linux%2064-bit%20Debug/builds/33967/steps/layout-test/logs/stdio http://build.webkit.org/builders/EFL%20Linux%2064-bit%20Debug/builds/1051/steps/layout-test/logs/stdio The error diffs can be obtained via http://build.webkit.org/results/.

Note You need to log in before you can comment on or make changes to this bug.