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

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[youenn fablet]

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

[youenn fablet]

Created attachment 251835 [details]
Patch

[Darin Adler]

Comment on attachment 251835 [details]
Patch

Is there a way to test this?

[Darin Adler]

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 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]

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]

(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 on attachment 251835 [details]
Patch

Clearing flags on attachment: 251835

Committed r183489: <http://trac.webkit.org/changeset/183489>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[youenn fablet]

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

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

[youenn fablet]

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