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
Created attachment 251835 [details] Patch
Comment on attachment 251835 [details] Patch Is there a way to test this?
Comment on attachment 251835 [details] 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?
Comment on attachment 251835 [details] 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.
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.
(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.
Comment on attachment 251835 [details] Patch Clearing flags on attachment: 251835 Committed r183489: <http://trac.webkit.org/changeset/183489>
All reviewed patches have been landed. Closing bug.
(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.
I'm not sure what you mean by test infrastructure. Something like bug 106596, which is about making EWS run API tests?
(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.