[webkit-dev] rolling out a buggy security patch
Hi All, https://trac.webkit.org/changeset/145482 which is a security fix, broke 33 JSC tests and made zillion layout test timeout on all platform. (It seems the author forgot to run tests at least on his own platform and watching the bots after landing.) It made bots early exit and very long test runtime. Now bots can't catch any new regression because of this patch. I tried to ping the author and reviewer on #webkit, but they are unavailable. Unfortunately rolling out isn't possible with sheriffbot. And I don't think if I have authorization for rolling out a security fix without review irrespectively of its goodness/buginess. Additionally EWS bots can't test security patches without security group access. And gardeners can't comment the original security bug report because of the same reason. So I filed a new bug report about this serious and blocker regression: https://bugs.webkit.org/show_bug.cgi?id=112112 and I propose that we should roll it out until the author can fix it offline. Could you review this rollout patch, please? Otherwise it would be great if EWS bots can test security patches before committing to avoid similar problems. I noticed that a security fix broke the build and/or many tests several times. br, Ossy ___ webkit-dev mailing list webkit-dev@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] rolling out a buggy security patch
Hi, Rollout patch was already r+ -ed, thanks for the quick r+. But my question is still open about how can we avoid similar problems in the future. Why can't we let the EWS bots to build and test security patches before commit. br, Ossy Osztrogonác Csaba írta: https://trac.webkit.org/changeset/145482 which is a security fix, broke 33 JSC tests and made zillion layout test timeout on all platform. (It seems the author forgot to run tests at least on his own platform and watching the bots after landing.) It made bots early exit and very long test runtime. Now bots can't catch any new regression because of this patch. I tried to ping the author and reviewer on #webkit, but they are unavailable. Unfortunately rolling out isn't possible with sheriffbot. And I don't think if I have authorization for rolling out a security fix without review irrespectively of its goodness/buginess. Additionally EWS bots can't test security patches without security group access. And gardeners can't comment the original security bug report because of the same reason. So I filed a new bug report about this serious and blocker regression: https://bugs.webkit.org/show_bug.cgi?id=112112 and I propose that we should roll it out until the author can fix it offline. Could you review this rollout patch, please? Otherwise it would be great if EWS bots can test security patches before committing to avoid similar problems. I noticed that a security fix broke the build and/or many tests several times. ___ webkit-dev mailing list webkit-dev@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] rolling out a buggy security patch
It seems like Oliver has already r+ed the patch. I wish we had a better way of dealing with regressions from security bug fixes. In theory, sheriffbot should be able to roll out security bug fixes without having to access the original bug. - R. Niwa On Tue, Mar 12, 2013 at 1:15 AM, Osztrogonác Csaba o...@inf.u-szeged.huwrote: Hi All, https://trac.webkit.org/**changeset/145482https://trac.webkit.org/changeset/145482which is a security fix, broke 33 JSC tests and made zillion layout test timeout on all platform. (It seems the author forgot to run tests at least on his own platform and watching the bots after landing.) It made bots early exit and very long test runtime. Now bots can't catch any new regression because of this patch. I tried to ping the author and reviewer on #webkit, but they are unavailable. Unfortunately rolling out isn't possible with sheriffbot. And I don't think if I have authorization for rolling out a security fix without review irrespectively of its goodness/buginess. Additionally EWS bots can't test security patches without security group access. And gardeners can't comment the original security bug report because of the same reason. So I filed a new bug report about this serious and blocker regression: https://bugs.webkit.org/show_**bug.cgi?id=112112https://bugs.webkit.org/show_bug.cgi?id=112112and I propose that we should roll it out until the author can fix it offline. Could you review this rollout patch, please? Otherwise it would be great if EWS bots can test security patches before committing to avoid similar problems. I noticed that a security fix broke the build and/or many tests several times. br, Ossy __**_ webkit-dev mailing list webkit-dev@lists.webkit.org https://lists.webkit.org/**mailman/listinfo/webkit-devhttps://lists.webkit.org/mailman/listinfo/webkit-dev ___ webkit-dev mailing list webkit-dev@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] rolling out a buggy security patch
On Tue, Mar 12, 2013 at 1:36 AM, Osztrogonác Csaba o...@inf.u-szeged.hu wrote: But my question is still open about how can we avoid similar problems in the future. Why can't we let the EWS bots to build and test security patches before commit. This topic was discussed on the webkit-security mailing list in May 2010. Unfortunately, the archives of that list are not viewable publicly. Maciej's concerns at the time are summaries in his message below: On Tue, Oct 19, 2010 at 6:16 PM, Maciej Stachowiak m...@apple.com wrote: The commit bot is not a person and therefore can't agree to the security group policy, as required for security group membership. If a specific person or persons want to take responsibility for an additional email account and bugzilla account having security access, then that's not categorically excluded. But I'd like to understand who currently has access to the commit bot's email account and bugzilla account, what the policies are for more people getting access, and whether there are indirect ways of getting access such as by modifying the commit bot's code, or by uploading a patch that tries to abuse the EWS testers. And I'd like to see at least one person named to take responsibility for ensuring that the commit bot is not used as a means of violating the policy. Of course, it's entirely possible that his views have changed since then. Adam ___ webkit-dev mailing list webkit-dev@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] rolling out a buggy security patch
On Mar 12, 2013, at 1:48 AM, Adam Barth aba...@webkit.org wrote: On Tue, Mar 12, 2013 at 1:36 AM, Osztrogonác Csaba o...@inf.u-szeged.hu wrote: But my question is still open about how can we avoid similar problems in the future. Why can't we let the EWS bots to build and test security patches before commit. This topic was discussed on the webkit-security mailing list in May 2010. Unfortunately, the archives of that list are not viewable publicly. Maciej's concerns at the time are summaries in his message below: On Tue, Oct 19, 2010 at 6:16 PM, Maciej Stachowiak m...@apple.com wrote: The commit bot is not a person and therefore can't agree to the security group policy, as required for security group membership. If a specific person or persons want to take responsibility for an additional email account and bugzilla account having security access, then that's not categorically excluded. But I'd like to understand who currently has access to the commit bot's email account and bugzilla account, what the policies are for more people getting access, and whether there are indirect ways of getting access such as by modifying the commit bot's code, or by uploading a patch that tries to abuse the EWS testers. And I'd like to see at least one person named to take responsibility for ensuring that the commit bot is not used as a means of violating the policy. Of course, it's entirely possible that his views have changed since then. I am still curious who has access to the commit bot's bugzilla account. Is a small set of known people, is it a large set, is the password sitting around somewhere that others may get at it? I do not recall this being answered at the time, or perhaps I have forgotten. If the set with access is a small set of known people who are willing to be identified and be in the security group themselves (or already are), then I am personally fine with it. Regards, Maciej ___ webkit-dev mailing list webkit-dev@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] rolling out a buggy security patch
On Tue, Mar 12, 2013 at 2:26 AM, Maciej Stachowiak m...@apple.com wrote: On Mar 12, 2013, at 1:48 AM, Adam Barth aba...@webkit.org wrote: On Tue, Mar 12, 2013 at 1:36 AM, Osztrogonác Csaba o...@inf.u-szeged.hu wrote: But my question is still open about how can we avoid similar problems in the future. Why can't we let the EWS bots to build and test security patches before commit. This topic was discussed on the webkit-security mailing list in May 2010. Unfortunately, the archives of that list are not viewable publicly. Maciej's concerns at the time are summaries in his message below: On Tue, Oct 19, 2010 at 6:16 PM, Maciej Stachowiak m...@apple.com wrote: The commit bot is not a person and therefore can't agree to the security group policy, as required for security group membership. If a specific person or persons want to take responsibility for an additional email account and bugzilla account having security access, then that's not categorically excluded. But I'd like to understand who currently has access to the commit bot's email account and bugzilla account, what the policies are for more people getting access, and whether there are indirect ways of getting access such as by modifying the commit bot's code, or by uploading a patch that tries to abuse the EWS testers. And I'd like to see at least one person named to take responsibility for ensuring that the commit bot is not used as a means of violating the policy. Of course, it's entirely possible that his views have changed since then. I am still curious who has access to the commit bot's bugzilla account. Is a small set of known people, is it a large set, is the password sitting around somewhere that others may get at it? I do not recall this being answered at the time, or perhaps I have forgotten. The approach we've taken is to use different bugzilla accounts for the different bot administrators. The commit-queue, the cr-linux-ews, the style-queue, and sheriffbot share one account (webkit.review.bot@gmail), whereas the queues that Ossy runs use a different account. Approximately eight people have access to the account because they have ssh access to the machines that run those queues. I can send you the list of people, if you're interested, but there are certainly folks on that list who are not members of the WebKit Security Group. In addition to the bugzilla account, we should also consider the set of people who have access to the underlying email address (since the email address can be used to reset the bugzilla password). In the case of webkit.review.bot, I'm the only person who has access to the underlying email account. (That's probably not ideal from a bus-factor point-of-view, however.) If the set with access is a small set of known people who are willing to be identified and be in the security group themselves (or already are), then I am personally fine with it. The set of people who are active maintainers of those machines is smaller than set of people who have access. A good first step would be for me to narrow down the list (and obviously rotate the password). Adam ___ webkit-dev mailing list webkit-dev@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] Overtype mode in WebKit for editable content?
En 12/03/13 05:11, Ryosuke Niwa escribiu: So I guess the question boils down to something like: if we have changes that are generally useful, but not used in the major WebKit applications (i.e. Chrome, Safari, Opera), does it make sense to upstream it to WebKit for the benefit of the general community? Yes. It makes a lot of sense for this feature to be in WebKit. We can probably expose some member functions on Editor or add new execCommand only executable via menu/keyboard binding to toggle this feature. I don't think it makes a sense for WebCore to detect Insert Key and automatically move into the inse Will handle it here http://wkb.ug/112126 then. BR ___ webkit-dev mailing list webkit-dev@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] Overtype mode in WebKit for editable content?
On Tue, Mar 12, 2013 at 12:18 AM, Ryosuke Niwa rn...@webkit.org wrote: Have you tested your code with bidirectional text? I don't think using block caret will work due to the way offsets at bidi-level boundary works in WebKit. In particular, suppose we have, in the document/byte order, ABC123 where ABC are strongly RTL letters in a LTR block. Then this text is rendered as: 123CBA If we were to place logical/DOM offset at where they appear visually, we have: (0)1(1)2(2)3C(5)B(4)A(3)(6) Oh oops, I'm sorry. I messed this up. It should be: (0)1(5)2(4)3C(2)B(1)A(3)(6) instead. i.e. At offset 3, the caret will visually appear on the right of A. If you were just setting the caret width, the caret will look as if we're inserting a character on the right of A, which is extremely misleading to a user. Our application right now doesn't allow for bidi text, so this isn't currently an issue for us. I haven't looked at the details, but if we were to support this in the future, I think the caret rendering code would simply determine whether the next character is RTL or not, and if so, it would extend its width in the opposite direction. Also, I don't understand why offset 3 would be rendered after A -- shouldn't it be rendered before 1 since that is where the next character would be inserted? (or maybe between 3 and C, I'm not familiar enough with the bidi rules). If it is rendered after A right now, then I'm guessing that is just a bug, and whether or not the caret is 1px or 20px wide seems equally misleading to the user. ___ webkit-dev mailing list webkit-dev@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] rolling out a buggy security patch
Unfortunately rolling out isn't possible with sheriffbot. And I don't think if I have authorization for rolling out a security fix without review irrespectively of its goodness/buginess. It looks like the necessary review took just under 13 minutes: Comment #1 From Csaba Osztrogonac 2013-03-12 01:04:20 PST (-) [reply] Created an attachment (id=192662) [details] rollout Comment #2 From Oliver Hunt 2013-03-12 01:17:16 PST (-) [reply] (From update of attachment 192662 [details] ) wtf? My bad What problem are we trying to solve here? Geoff ___ webkit-dev mailing list webkit-dev@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] Overtype mode in WebKit for editable content?
On Tue, Mar 12, 2013 at 8:29 AM, Shezan Baig shezbaig...@gmail.com wrote: On Tue, Mar 12, 2013 at 12:18 AM, Ryosuke Niwa rn...@webkit.org wrote: Have you tested your code with bidirectional text? I don't think using block caret will work due to the way offsets at bidi-level boundary works in WebKit. In particular, suppose we have, in the document/byte order, ABC123 where ABC are strongly RTL letters in a LTR block. Then this text is rendered as: 123CBA If we were to place logical/DOM offset at where they appear visually, we have: (0)1(1)2(2)3C(5)B(4)A(3)(6) Oh oops, I'm sorry. I messed this up. It should be: (0)1(5)2(4)3C(2)B(1)A(3)(6) instead. i.e. At offset 3, the caret will visually appear on the right of A. If you were just setting the caret width, the caret will look as if we're inserting a character on the right of A, which is extremely misleading to a user. Our application right now doesn't allow for bidi text, so this isn't currently an issue for us. I haven't looked at the details, but if we were to support this in the future, I think the caret rendering code would simply determine whether the next character is RTL or not, and if so, it would extend its width in the opposite direction. That doesn't work. At a bidi-level boundary, offset can jump from one place to another. Consider a much simpler example; the same sequence of letters, in logical order, ABC123 in a RTL block, which will be rendered as: 123CBA with logical/DOM offsets placed at: (6)1(4)2(5)3(3)C(2)B(1)A(0) Again, at offset 3, what we're going to replace is 1 so we can't place the block caret on top of either C or 3. Also, I don't understand why offset 3 would be rendered after A -- shouldn't it be rendered before 1 since that is where the next character would be inserted? (or maybe between 3 and C, I'm not familiar enough with the bidi rules). If it is rendered after A right now, then I'm guessing that is just a bug, and whether or not the caret is 1px or 20px wide seems equally misleading to the user. There is no rule or specification for this. We followed NSTextView's convention and this is the expected behavior. In particular, it is NOT a bug. If we're supporting the overtype mode with block caret, we need to solve this problem. - R. Niwa ___ webkit-dev mailing list webkit-dev@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-dev