Bug 144321

Summary:

SharedBuffer::copy is not computing the buffer size correctly when having m_dataArray

Product:

WebKit

Reporter:

youenn fablet <youennf>

Component:

Platform

Assignee:

youenn fablet <youennf>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

ap, commit-queue, darin, psolanki

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
Patch	none

youenn fablet

Reported 2015-04-28 03:55:35 PDT

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

Attachments
Patch (1.46 KB, patch) 2015-04-28 03:59 PDT, youenn fablet	no flags	Details Formatted Diff Diff
View All Add attachment proposed patch, testcase, etc.

youenn fablet

Comment 1 2015-04-28 03:59:14 PDT

Created attachment 251835 [details] Patch

Darin Adler

Comment 2 2015-04-28 08:18:32 PDT

Comment on attachment 251835 [details] Patch Is there a way to test this?

Darin Adler

Comment 3 2015-04-28 08:18:51 PDT

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?

youenn fablet

Comment 4 2015-04-28 09:56:55 PDT

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.

Alexey Proskuryakov

Comment 5 2015-04-28 10:03:49 PDT

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.

Darin Adler

Comment 6 2015-04-28 10:35:21 PDT

(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.

WebKit Commit Bot

Comment 7 2015-04-28 10:46:55 PDT

Comment on attachment 251835 [details] Patch Clearing flags on attachment: 251835 Committed r183489: <http://trac.webkit.org/changeset/183489>

WebKit Commit Bot

Comment 8 2015-04-28 10:46:59 PDT

All reviewed patches have been landed. Closing bug.

youenn fablet

Comment 9 2015-04-29 11:58:12 PDT

(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.

Alexey Proskuryakov

Comment 10 2015-04-29 12:13:46 PDT

I'm not sure what you mean by test infrastructure. Something like bug 106596, which is about making EWS run API tests?

youenn fablet

Comment 11 2015-04-30 08:38:48 PDT

(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.

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