Bug 86067 - [BlackBerry] Possible to clobber httponly cookie.
Summary: [BlackBerry] Possible to clobber httponly cookie.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit BlackBerry (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other Other
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 87208 87255
Blocks:
  Show dependency treegraph
 
Reported: 2012-05-10 01:30 PDT by Jason Liu
Modified: 2012-06-10 20:44 PDT (History)
11 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Jason Liu 2012-05-10 01:30:11 PDT
The httponly cookies should be changed by javaScript.
Comment 1 Jason Liu 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.
Comment 2 Jason Liu 2012-05-17 23:51:09 PDT
Created attachment 142647 [details]
Patch
Comment 3 jochen 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?
Comment 4 Jason Liu 2012-05-18 02:29:28 PDT
Created attachment 142673 [details]
Patch
Comment 5 George Staikos 2012-05-18 06:07:22 PDT
Comment on attachment 142647 [details]
Patch

This seems incredibly inefficient. Is this the best we can do?
Comment 6 jochen 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
Comment 7 Jason Liu 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.
Comment 8 Jason Liu 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.
Comment 9 Jason Liu 2012-05-22 00:43:53 PDT
Created attachment 143215 [details]
Patch
Comment 10 jochen 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?
Comment 11 Jason Liu 2012-05-22 00:57:39 PDT
Created attachment 143218 [details]
Patch
Comment 12 Jason Liu 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.
Comment 13 Jason Liu 2012-05-22 02:28:46 PDT
Created attachment 143241 [details]
Patch
Comment 14 jochen 2012-05-22 02:52:57 PDT
The change looks good to me, thanks

(I'm no reviewer, so I can't r+ it..)
Comment 15 Jason Liu 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. :)
Comment 16 Rob Buis 2012-05-22 04:56:14 PDT
Comment on attachment 143241 [details]
Patch

Looks good.
Comment 17 WebKit Review Bot 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>
Comment 18 WebKit Review Bot 2012-05-22 19:49:35 PDT
All reviewed patches have been landed.  Closing bug.
Comment 19 WebKit Review Bot 2012-05-23 05:48:41 PDT
Re-opened since this is blocked by 87255
Comment 20 Jason Liu 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.
Comment 21 Jason Liu 2012-05-23 20:40:04 PDT
Will I commit my test case in our own platform
Comment 22 jochen 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?
Comment 23 Jason Liu 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.
: (
Comment 24 Jason Liu 2012-06-03 23:18:49 PDT
Created attachment 145520 [details]
Patch
Comment 25 Jason Liu 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.
Comment 26 jochen 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
Comment 27 Jason Liu 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
Comment 28 Jason Liu 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?
Comment 29 jochen 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)
Comment 30 Rob Buis 2012-06-04 12:05:57 PDT
Comment on attachment 145520 [details]
Patch

R- based on Jochen's comments.
Comment 31 Jason Liu 2012-06-05 03:40:49 PDT
Created attachment 145748 [details]
Patch
Comment 32 jochen 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>
Comment 33 Jason Liu 2012-06-05 04:02:32 PDT
Created attachment 145753 [details]
Patch
Comment 34 Jason Liu 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. : )
Comment 35 jochen 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
Comment 36 Jason Liu 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.
Comment 37 jochen 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?
Comment 38 Jason Liu 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?
Comment 39 Jason Liu 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.
Comment 40 jochen 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.
Comment 41 Jason Liu 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.
Comment 42 jochen 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
Comment 43 Jason Liu 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.
Comment 44 WebKit Review Bot 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
Comment 45 WebKit Review Bot 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
Comment 46 Jason Liu 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.
Comment 47 Jason Liu 2012-06-05 23:40:07 PDT
Created attachment 145945 [details]
Patch
Comment 48 jochen 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
Comment 49 Jason Liu 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.
Comment 50 jochen 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
Comment 51 Jason Liu 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.
Comment 52 Jason Liu 2012-06-06 01:36:14 PDT
Created attachment 145962 [details]
Patch
Comment 53 jochen 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
Comment 54 Rob Buis 2012-06-08 07:04:17 PDT
Comment on attachment 145962 [details]
Patch

Looks good.
Comment 55 WebKit Review Bot 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>
Comment 56 WebKit Review Bot 2012-06-10 19:16:43 PDT
All reviewed patches have been landed.  Closing bug.