BinaryValue does not support NULL buffer.

Allowed BinaryValue to take a NULL buffer.

BUG=127630
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=138044

[asargent_no_longer_on_chrome, 2012-05-11 21:22:46]

https://chromiumcodereview.appspot.com/10389088/diff/1/base/values.h
File base/values.h (right):

https://chromiumcodereview.appspot.com/10389088/diff/1/base/values.h#newcode184
base/values.h:184: static BinaryValue* Create(char* buffer, size_t size);
Instead of just changing the behavior of the Create method, let's instead get
rid of it and do 2 things:

-Make the existing constructor public, and remove the comment on it about "..so
that only objects with valid buffer pointers and size values can be created..".
Also, change its signature to:

BinaryValue(scoped_ptr<char> buffer, size_t size);

this gives us compile-time checking that forces callers to transfer ownership of
memory by calling .Pass() on the scoped_ptr that they pass in.

-create a no-argument constructor, which initializes the buffer_ to NULL and
size_ to 0.

-Add a warning comment above the 2 GetBuffer methods that they can return NULL.


BTW, I did a quick search in the code and the only user of Create() was us
working around this problem of not supporting empty buffers. 


You'll also probably need to update one or more existing tests in
values_unittest.cc, and it would be good to add a test or two to cover the new
behavior of allowing 0-length buffers.

[mitchellwrosen, 2012-05-12 00:22:05]

On 2012/05/11 21:22:46, Antony Sargent wrote:
> https://chromiumcodereview.appspot.com/10389088/diff/1/base/values.h
> File base/values.h (right):
> 
>
https://chromiumcodereview.appspot.com/10389088/diff/1/base/values.h#newcode184
> base/values.h:184: static BinaryValue* Create(char* buffer, size_t size);
> Instead of just changing the behavior of the Create method, let's instead get
> rid of it and do 2 things:
> 
> -Make the existing constructor public, and remove the comment on it about
"..so
> that only objects with valid buffer pointers and size values can be
created..".
> Also, change its signature to:
> 
> BinaryValue(scoped_ptr<char> buffer, size_t size);
> 
> this gives us compile-time checking that forces callers to transfer ownership
of
> memory by calling .Pass() on the scoped_ptr that they pass in.
> 
> -create a no-argument constructor, which initializes the buffer_ to NULL and
> size_ to 0.
> 
> -Add a warning comment above the 2 GetBuffer methods that they can return
NULL.
> 
> 
> BTW, I did a quick search in the code and the only user of Create() was us
> working around this problem of not supporting empty buffers. 
> 
> 
> You'll also probably need to update one or more existing tests in
> values_unittest.cc, and it would be good to add a test or two to cover the new
> behavior of allowing 0-length buffers.

Sure thing. What's to come of BinaryValue::CreateWithCopiedBuffer, for
"situations where you want to keep ownership of your buffer"? Should BinaryValue
take a scoped_refptr instead?

[asargent_no_longer_on_chrome, 2012-05-14 16:50:28]

> Sure thing. What's to come of BinaryValue::CreateWithCopiedBuffer, for
> "situations where you want to keep ownership of your buffer"? Should
BinaryValue
> take a scoped_refptr instead?

I think it's fine to have CreateWithCopiedBuffer continue to just take a const
char* - that's a pretty clear signal to the caller that it continues to own the
bytes. Adding ref-counting might be useful down the road in some situations
(avoiding unnecessary copying when going from the IPC system to the extensions
browser/renderer code comes to mind), but that's orthogonal to this patch and
should be left for future work.

[mitchellwrosen, 2012-05-15 00:17:48]

On 2012/05/14 16:50:28, Antony Sargent wrote:
> > Sure thing. What's to come of BinaryValue::CreateWithCopiedBuffer, for
> > "situations where you want to keep ownership of your buffer"? Should
> BinaryValue
> > take a scoped_refptr instead?
> 
> I think it's fine to have CreateWithCopiedBuffer continue to just take a const
> char* - that's a pretty clear signal to the caller that it continues to own
the
> bytes. Adding ref-counting might be useful down the road in some situations
> (avoiding unnecessary copying when going from the IPC system to the extensions
> browser/renderer code comes to mind), but that's orthogonal to this patch and
> should be left for future work.

So just to clarify, replace BinaryValue::Create with a public constructor, but
leave BinaryValue::CreateWithCopiedBuffer as it is?

[asargent_no_longer_on_chrome, 2012-05-15 00:37:43]

On 2012/05/15 00:17:48, mitchellwrosen wrote:
> On 2012/05/14 16:50:28, Antony Sargent wrote:
> > > Sure thing. What's to come of BinaryValue::CreateWithCopiedBuffer, for
> > > "situations where you want to keep ownership of your buffer"? Should
> > BinaryValue
> > > take a scoped_refptr instead?
> > 
> > I think it's fine to have CreateWithCopiedBuffer continue to just take a
const
> > char* - that's a pretty clear signal to the caller that it continues to own
> the
> > bytes. Adding ref-counting might be useful down the road in some situations
> > (avoiding unnecessary copying when going from the IPC system to the
extensions
> > browser/renderer code comes to mind), but that's orthogonal to this patch
and
> > should be left for future work.
> 
> So just to clarify, replace BinaryValue::Create with a public constructor, but
> leave BinaryValue::CreateWithCopiedBuffer as it is?

Correct.

[asargent_no_longer_on_chrome, 2012-05-18 22:49:49]

LGTM w/ one comment nit. 

+brettw for base/ (since I'm not in base/OWNERS, and I happened to mention this
to him the other day).

https://chromiumcodereview.appspot.com/10389088/diff/8001/base/values.h
File base/values.h (right):

https://chromiumcodereview.appspot.com/10389088/diff/8001/base/values.h#newco...
base/values.h:183: BinaryValue(scoped_ptr<char> buffer, size_t size);
nit: it might be worth adding a comment above this second constructor -
something along the lines of "Creates a BinaryValue, taking ownership of the
bytes pointed to by |buffer|".

[mitchellwrosen, 2012-05-18 23:04:29]

https://chromiumcodereview.appspot.com/10389088/diff/8001/base/values.h
File base/values.h (right):

https://chromiumcodereview.appspot.com/10389088/diff/8001/base/values.h#newco...
base/values.h:183: BinaryValue(scoped_ptr<char> buffer, size_t size);
On 2012/05/18 22:49:49, Antony Sargent wrote:
> nit: it might be worth adding a comment above this second constructor -
> something along the lines of "Creates a BinaryValue, taking ownership of the
> bytes pointed to by |buffer|".

Done.

[commit-bot: I haz the power, 2012-05-18 23:04:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mitchellwrosen@chromium.org/10389088/3002

[commit-bot: I haz the power, 2012-05-18 23:04:55]

Presubmit check for 10389088-3002 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit Messages **
If this change requires manual test instructions to QA team, add
TEST=[instructions].

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    base

Presubmit checks took 1.5s to calculate.

[mitchellwrosen, 2012-05-18 23:06:16]

Should have seen that one coming. Trigger finger...

[brettw, 2012-05-18 23:20:31]

lgtm

https://chromiumcodereview.appspot.com/10389088/diff/3002/base/values.cc
File base/values.cc (right):

https://chromiumcodereview.appspot.com/10389088/diff/3002/base/values.cc#newc...
base/values.cc:302: : Value(TYPE_BINARY),
Indent these initializers 2 more spaces. Same for the other constructor.

[brettw, 2012-05-18 23:20:31]

lgtm

lgtm

[commit-bot: I haz the power, 2012-05-19 17:24:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mitchellwrosen@chromium.org/10389088/1...

[commit-bot: I haz the power, 2012-05-19 17:32:10]

Try job failure for 10389088-17001 (retry) on mac_rel for step "compile"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2012-05-19 18:08:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mitchellwrosen@chromium.org/10389088/6005

[commit-bot: I haz the power, 2012-05-19 19:31:19]

Change committed as 138044

[mitchellwrosen, 2012-05-19 22:19:54]

On 2012/05/19 19:31:19, I haz the power (commit-bot) wrote:
> Change committed as 138044

So I'm failing valgrind:

http://build.chromium.org/p/chromium.memory.fyi/builders/Linux%20Tests%20%28v...

http://build.chromium.org/p/chromium.memory.fyi/builders/Chromium%20Mac%20%28...

http://build.chromium.org/p/chromium.memory.fyi/builders/Windows%20Tests%20%2...

I believe the problem is that I was using scoped_ptr<char> instead of
scoped_array<char>. I have a new CL, but I'm not sure what to do... should I
wait until a sheriff reverts my change, then push a new patch set here and wait
for another LGTM from you two? Or should I make a whole new issue?

[asargent_no_longer_on_chrome, 2012-05-21 17:27:13]

Probably the memory sheriffs will put in a suppression and file a bug for
you.

I'm actually not sure off the top of my head if there's a reason to prefer
allocating the memory for these with "new int[size]" vs "malloc(size)" - we
just need to make sure we are consistent so we call delete or free as
appropriate. I'll touch base with a couple of folks and see if anyone has
an opinion (or fall back to asking chromium-dev).


On Sat, May 19, 2012 at 3:19 PM, <mitchellwrosen@chromium.org> wrote:

> On 2012/05/19 19:31:19, I haz the power (commit-bot) wrote:
>
>> Change committed as 138044
>>
>
> So I'm failing valgrind:
>
> http://build.chromium.org/p/**chromium.memory.fyi/builders/**
> Linux%20Tests%20%28valgrind%**29%283%29/builds/9478/steps/**
>
memory%20test%3A%20base/logs/**stdio<http://build.chromium.org/p/chromium.memory.fyi/builders/Linux%20Tests%20%28valgrind%29%283%29/builds/9478/steps/memory%20test%3A%20base/logs/stdio>
>
> http://build.chromium.org/p/**chromium.memory.fyi/builders/**
> Chromium%20Mac%20%28valgrind%**29%281%29/builds/8966/steps/**
>
memory%20test%3A%20base/logs/**stdio<http://build.chromium.org/p/chromium.memory.fyi/builders/Chromium%20Mac%20%28valgrind%29%281%29/builds/8966/steps/memory%20test%3A%20base/logs/stdio>
>
> http://build.chromium.org/p/**chromium.memory.fyi/builders/**
> Windows%20Tests%20%28DrMemory%**29/builds/3452/steps/memory%**
>
20test%3A%20base/logs/stdio<http://build.chromium.org/p/chromium.memory.fyi/builders/Windows%20Tests%20%28DrMemory%29/builds/3452/steps/memory%20test%3A%20base/logs/stdio>
>
> I believe the problem is that I was using scoped_ptr<char> instead of
> scoped_array<char>. I have a new CL, but I'm not sure what to do... should
> I
> wait until a sheriff reverts my change, then push a new patch set here and
> wait
> for another LGTM from you two? Or should I make a whole new issue?
>
>
>
https://chromiumcodereview.**appspot.com/10389088/<https://chromiumcodereview...
>

[asargent_no_longer_on_chrome, 2012-05-21 17:42:11]

Oh, looks like it just got reverted. No big deal - I'll still follow up with an
answer for you on new vs. malloc.