144321 2015-04-28 03:55:35 -0700 SharedBuffer::copy is not computing the buffer size correctly when having m_dataArray 2015-04-30 08:38:48 -0700 1 1 1 Unclassified WebKit Platform 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 1 youennf youennf ap commit-queue darin psolanki oldest_to_newest 1089459 0 youennf 2015-04-28 03:55:35 -0700 SharedBuffer::copy is not computing the buffer size correctly when having m_dataArray. The size of the copy is not always the same as the side of the source, which should be the case. This is exemplified by https://bugs.webkit.org/show_bug.cgi?id=144272 1089460 1 251835 youennf 2015-04-28 03:59:14 -0700 Created attachment 251835 Patch 1089518 2 251835 darin 2015-04-28 08:18:32 -0700 Comment on attachment 251835 Patch Is there a way to test this? 1089519 3 251835 darin 2015-04-28 08:18:51 -0700 Comment on attachment 251835 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=251835&action=review > Source/WebCore/ChangeLog:8 > + Patch correctness covered by existing tests. How can this be? Which test was failing? 1089571 4 251835 youennf 2015-04-28 09:56:55 -0700 Comment on attachment 251835 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=251835&action=review >> Source/WebCore/ChangeLog:8 >> + Patch correctness covered by existing tests. > > How can this be? Which test was failing? I wrote "Patch correctness covered" not "Patch covered"... I agree it is a little bit of a lexical abuse. With the added ASSERT, the tests would now crash in debug mode and not only in guard malloc mode, which will make things easier to spot when landing the patch if there are still some issues within copy. And, of course, it will be covered by appcache test once bug 143711 will be landed. The only current copy() user I know is SubresourceLoader when loading multipart content. I guess there are tests in http/tests/multipart for this code path. Do some of them have issues with guard malloc? Is it possible to monitor this somewhere? SharedBuffer as well as some other platform/WTF classes would best be covered by direct unit testing, which does not seem to be WebKit tradition, at least for Source/WebCore stuff. 1089580 5 ap 2015-04-28 10:03:49 -0700 There are no known issues with GuardMalloc at the time, although it's possible that affected tests are simply disabled. You can run GuardMalloc tests yourself with -g option to run-webkit-tests. 1089596 6 darin 2015-04-28 10:35:21 -0700 (In reply to comment #4) > SharedBuffer as well as some other platform/WTF classes would best be > covered by direct unit testing, which does not seem to be WebKit tradition, > at least for Source/WebCore stuff. We should start adding more direct unit testing. Not doing these sorts of tests is obsolete project thinking; we now like to add such tests. 1089608 7 251835 commit-queue 2015-04-28 10:46:55 -0700 Comment on attachment 251835 Patch Clearing flags on attachment: 251835 Committed r183489: <http://trac.webkit.org/changeset/183489> 1089609 8 commit-queue 2015-04-28 10:46:59 -0700 All reviewed patches have been landed. Closing bug. 1090096 9 youennf 2015-04-29 11:58:12 -0700 (In reply to comment #6) > (In reply to comment #4) > > SharedBuffer as well as some other platform/WTF classes would best be > > covered by direct unit testing, which does not seem to be WebKit tradition, > > at least for Source/WebCore stuff. > > We should start adding more direct unit testing. Not doing these sorts of > tests is obsolete project thinking; we now like to add such tests. This sounds great. Is somebody working on bringing C++ Unit tests to WebKit tools infastructure? Is it discussed somewhere? I guess I would use it for bug 144192. 1090105 10 ap 2015-04-29 12:13:46 -0700 I'm not sure what you mean by test infrastructure. Something like bug 106596, which is about making EWS run API tests? 1090442 11 youennf 2015-04-30 08:38:48 -0700 (In reply to comment #10) > I'm not sure what you mean by test infrastructure. Something like bug > 106596, which is about making EWS run API tests? That would be useful yes. I hope this is done by the commit queue though. But also adding tests require modifying the build system files which is tedious. 251835 2015-04-28 03:59:14 -0700 2015-04-28 10:46:55 -0700 Patch bug-144321-20150428125805.patch text/plain 1490 youennf U3VidmVyc2lvbiBSZXZpc2lvbjogMTgzMzg4CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggMGQ1MTY5ZTEyNDFkZDJk ZDA4ZTM4ZjEwM2NmMThlMzgzMDIxZDE5My4uNmQxODZmZDA3ZmEwMjZkN2FjMTJkMDk2NTEzOGRi ZWE0MWFjYzU0ZCAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDE1IEBACisyMDE1LTA0LTI4ICBZb3Vl bm4gRmFibGV0ICA8eW91ZW5uLmZhYmxldEBjcmYuY2Fub24uZnI+CisKKyAgICAgICAgU2hhcmVk QnVmZmVyOjpjb3B5IGlzIG5vdCBjb21wdXRpbmcgdGhlIGJ1ZmZlciBzaXplIGNvcnJlY3RseSB3 aGVuIGhhdmluZyBtX2RhdGFBcnJheQorICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9z aG93X2J1Zy5jZ2k/aWQ9MTQ0MzIxCisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BT ISkuCisKKyAgICAgICAgUGF0Y2ggY29ycmVjdG5lc3MgY292ZXJlZCBieSBleGlzdGluZyB0ZXN0 cy4KKworICAgICAgICAqIHBsYXRmb3JtL1NoYXJlZEJ1ZmZlci5jcHA6CisgICAgICAgIChXZWJD b3JlOjpTaGFyZWRCdWZmZXI6OmNvcHkpOiBEaXJlY3QgYXBwZW5kaW5nIHRvIG1fZGF0YUFycmF5 IGFuZCBub3QgdXNpbmcgYXBwZW5kIG1ldGhvZCBhcyB0aGlzIG1ldGhvZCB1cGRhdGVzIHRoZSBT aGFyZWRCdWZmZXIgc2l6ZS4KKwogMjAxNS0wNC0yNCAgUGhpbGlwcGUgTm9ybWFuZCAgPHBub3Jt YW5kQGlnYWxpYS5jb20+CiAKICAgICAgICAgW0pIQnVpbGRdIE1vdmUgdG8gdXBzdHJlYW0gT3Bl bldlYlJUQwpkaWZmIC0tZ2l0IGEvU291cmNlL1dlYkNvcmUvcGxhdGZvcm0vU2hhcmVkQnVmZmVy LmNwcCBiL1NvdXJjZS9XZWJDb3JlL3BsYXRmb3JtL1NoYXJlZEJ1ZmZlci5jcHAKaW5kZXggYWU2 ZWVlNWViZjUwMGVjY2UwM2E1NDExNDNhZTlhNzU1ODhlYzFlZi4uYWZiYjY1MzFmZDcxNGI4Njdh ZmQ2ZDJmZjEyN2NlOTk2NmUwZGViOSAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvcGxhdGZv cm0vU2hhcmVkQnVmZmVyLmNwcAorKysgYi9Tb3VyY2UvV2ViQ29yZS9wbGF0Zm9ybS9TaGFyZWRC dWZmZXIuY3BwCkBAIC0yNDQsOCArMjQ0LDkgQEAgUGFzc1JlZlB0cjxTaGFyZWRCdWZmZXI+IFNo YXJlZEJ1ZmZlcjo6Y29weSgpIGNvbnN0CiAgICAgICAgIGNsb25lLT5tX2J1ZmZlci0+ZGF0YS5h cHBlbmQoc2VnbWVudCwgc2VnbWVudFNpemUpOwogI2Vsc2UKICAgICBmb3IgKGF1dG8mIGRhdGEg OiBtX2RhdGFBcnJheSkKLSAgICAgICAgY2xvbmUtPmFwcGVuZChkYXRhLmdldCgpKTsKKyAgICAg ICAgY2xvbmUtPm1fZGF0YUFycmF5LmFwcGVuZChkYXRhLmdldCgpKTsKICNlbmRpZgorICAgIEFT U0VSVChjbG9uZS0+c2l6ZSgpID09IHNpemUoKSk7CiAKICAgICByZXR1cm4gY2xvbmUucmVsZWFz ZSgpOwogfQo=