Bug 63313 - The return value of SharedBuffer::createWithContentsOfFile must have valid m_size.
Summary: The return value of SharedBuffer::createWithContentsOfFile must have valid m_...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-06-23 22:48 PDT by Kim, kyusun
Modified: 2011-07-07 10:29 PDT (History)
5 users (show)

See Also:


Attachments
Patch (1.38 KB, patch)
2011-06-23 22:54 PDT, Kim, kyusun
no flags Details | Formatted Diff | Diff
Patch (1.77 KB, patch)
2011-07-07 02:23 PDT, Kim, kyusun
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kim, kyusun 2011-06-23 22:48:57 PDT
SharedBufferPOSIX.cpp - SharedBuffer::m_size has to be set
Comment 1 Kim, kyusun 2011-06-23 22:54:03 PDT
Created attachment 98460 [details]
Patch
Comment 2 Eric Seidel (no email) 2011-07-05 17:57:35 PDT
Comment on attachment 98460 [details]
Patch

How do we test this?  Is there a public function we should call instead of setting m_size directly?
Comment 3 Eric Seidel (no email) 2011-07-05 18:01:24 PDT
Comment on attachment 98460 [details]
Patch

In general this looks fine though.
Comment 4 Eric Seidel (no email) 2011-07-05 18:01:50 PDT
How did you detect that this was broken?
Comment 5 Eric Seidel (no email) 2011-07-05 18:04:51 PDT
CCing anders who originally created the SharedBuffer class.  It's strange to me that m_size is not used by most ports.
Comment 6 Eric Seidel (no email) 2011-07-05 18:06:10 PDT
Comment on attachment 98460 [details]
Patch

Clearing cq+.

It seems to me it would be better for this code to read into a Vector and then create the SharedBuffer from the Vector.  SharedBuffer doesn't seem to be designed to be read into directly, it's meant to wrap other data storage mechanisms.  Since this POSIX implementation doesn't have any "native" data storage, seems we should use Vector.
Comment 7 Eric Seidel (no email) 2011-07-05 18:07:39 PDT
We already have a adoptVector(Vector) which gets all this right, including setting m_size.  I worry that there are other things that might be missing from this implementation in the future.  re-using the adoptVector(Vector) method may be more future-proof.
Comment 8 Kim, kyusun 2011-07-05 22:13:39 PDT
(In reply to comment #2)
> (From update of attachment 98460 [details])
> How do we test this?

Currently, there is no port in upstream that uses this file. So, we cannot test it for now.

> Is there a public function we should call instead of setting m_size directly?

There is no setter for m_size.
Comment 9 Kim, kyusun 2011-07-05 22:14:13 PDT
(In reply to comment #4)
> How did you detect that this was broken?

When I used the linux port that is mentioned in https://bugs.webkit.org/show_bug.cgi?id=39348#c6, I found this bug.
Comment 10 Kim, kyusun 2011-07-05 22:16:13 PDT
(In reply to comment #7)
> We already have a adoptVector(Vector) which gets all this right, including setting m_size.  I worry that there are other things that might be missing from this implementation in the future.  re-using the adoptVector(Vector) method may be more future-proof.

Ok. After modifying it, I'll submit new patch.
Comment 11 Kim, kyusun 2011-07-07 02:23:56 PDT
Created attachment 99961 [details]
Patch
Comment 12 Kim, kyusun 2011-07-07 02:42:21 PDT
Changed to use adoptVector(Vector).
Comment 13 Eric Seidel (no email) 2011-07-07 10:18:10 PDT
Comment on attachment 99961 [details]
Patch

Nice!
Comment 14 WebKit Review Bot 2011-07-07 10:29:32 PDT
Comment on attachment 99961 [details]
Patch

Clearing flags on attachment: 99961

Committed r90574: <http://trac.webkit.org/changeset/90574>
Comment 15 WebKit Review Bot 2011-07-07 10:29:37 PDT
All reviewed patches have been landed.  Closing bug.