Add defensive code in pickle to preclude realloc of shared header_ memory....

Add defensive code in pickle to preclude realloc of shared header_ memory.

Since I was able to (some how) generate a problem with header_ being
double freed (perhaps, because it was shared in some way??), this change
adds several lines of defensive coding.  The current assignment operator
appears very dangerous, as it allowed Resize to be called when the header_
was not owned by the instance (it was read-only).  I haven't found a path
to cause a problem, but we may as well be defensive.

BUG=17027
BUG=17088
r=CPU

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

[darin (slow to review), 2009-08-03 06:32:18]

LGTM

http://codereview.chromium.org/160490/diff/1/2
File base/pickle.cc (right):

http://codereview.chromium.org/160490/diff/1/2#newcode68
Line 68: if (capacity_ >= kCapacityReadOnly) {
nit: the >= check seems a bit odd to me since kCapacityReadOnly is intended as a
flag / sentinel value.  i think == would be better... it would also be more
consistent with the check in ~Pickle.  moreover, there is no way for capacity_
to be larger than kCapacityReadOnly, so it reads a bit odd to have a >= here.

[jar (doing other things), 2009-08-03 06:48:35]

Changes made per Darin's suggestions.

http://codereview.chromium.org/160490/diff/1/2
File base/pickle.cc (right):

http://codereview.chromium.org/160490/diff/1/2#newcode68
Line 68: if (capacity_ >= kCapacityReadOnly) {
On 2009/08/03 06:32:18, darin wrote:
> nit: the >= check seems a bit odd to me since kCapacityReadOnly is intended as
a
> flag / sentinel value.  i think == would be better... it would also be more
> consistent with the check in ~Pickle.  moreover, there is no way for capacity_
> to be larger than kCapacityReadOnly, so it reads a bit odd to have a >= here.

Done.

[cpu_(ooo_6.6-7.5), 2009-08-03 16:18:57]

http://codereview.chromium.org/160490/diff/1005/7
File base/pickle.cc (right):

http://codereview.chromium.org/160490/diff/1005/7#newcode67
Line 67: Pickle& Pickle::operator=(const Pickle& other) {
this operator is lacking a check against self assignment.

http://codereview.chromium.org/160490/diff/1005/7#newcode77
Line 77: bool resized = Resize(header_size_ + other.header_->payload_size);
I don't get the change in line 73. It seems more clear the previous version.
What about changing line 79 so the two match?

[jar (doing other things), 2009-08-03 20:11:22]

http://codereview.chromium.org/160490/diff/1005/7
File base/pickle.cc (right):

http://codereview.chromium.org/160490/diff/1005/7#newcode67
Line 67: Pickle& Pickle::operator=(const Pickle& other) {
On 2009/08/03 16:18:57, cpu wrote:
> this operator is lacking a check against self assignment.
> 

I'll create a CL that handles this explicitly.

http://codereview.chromium.org/160490/diff/1005/7#newcode77
Line 77: bool resized = Resize(header_size_ + other.header_->payload_size);
On 2009/08/03 16:18:57, cpu wrote:
> I don't get the change in line 73. It seems more clear the previous version.
> What about changing line 79 so the two match?

The goal was to have the argument of the memcpy (two lines from now) have
EXACTLY the same size argument as we have in the resize (which was not the case
before). 

It can be deduced from the last 4 lines that they have to be the same, but it
seemed better to use the same text in both users (resize and memcpy).

If you'd rather I use the longer one in both cases, I can do that.  I'll add it
to the CL containing more defensive code for the self-assignment.

[jim.roskind, 2009-08-08 01:18:00]

LGTM