Fix crash in V8 during serializing objects requiring alignment.

Fix crash in V8 during serializing objects requiring alignment.

[Slava Chigrin, 2014-12-10 18:11:08]

vchigrin@yandex-team.ru changed reviewers:
+ yangguo@chromium.org

[Slava Chigrin, 2014-12-10 18:13:58]

https://codereview.chromium.org/793753002/diff/1/src/serialize.cc
File src/serialize.cc (right):

https://codereview.chromium.org/793753002/diff/1/src/serialize.cc#newcode1669
src/serialize.cc:1669: if (needs_double_align)
Deserializer::ReadObject expects DoubleAlignmentSentinel be just before size
field, not before "NewObject" tag.

[Yang, 2014-12-10 20:45:48]

lgtm

https://codereview.chromium.org/793753002/diff/1/src/serialize.cc
File src/serialize.cc (right):

https://codereview.chromium.org/793753002/diff/1/src/serialize.cc#newcode1669
src/serialize.cc:1669: if (needs_double_align)
On 2014/12/10 18:13:58, Slava Chigrin wrote:
> Deserializer::ReadObject expects DoubleAlignmentSentinel be just before size
> field, not before "NewObject" tag.

Thanks for catching this! I guess I got confused when refactoring a couple of
things when implementing this.

[Yang, 2014-12-10 20:45:52]

The CQ bit was checked by yangguo@chromium.org

[commit-bot: I haz the power, 2014-12-10 20:46:26]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/793753002/1

[Yang, 2014-12-10 20:46:27]

The CQ bit was unchecked by yangguo@chromium.org

[Yang, 2014-12-10 20:47:58]

https://codereview.chromium.org/793753002/diff/1/src/serialize.cc
File src/serialize.cc (left):

https://codereview.chromium.org/793753002/diff/1/src/serialize.cc#oldcode1667
src/serialize.cc:1667: sink_->Put(kNewObject + reference_representation_ +
space, "NewObject");
How about we simply put this line (outputting the new object) before the if-else
clause?

[Slava Chigrin, 2014-12-11 09:16:40]

https://codereview.chromium.org/793753002/diff/1/src/serialize.cc
File src/serialize.cc (left):

https://codereview.chromium.org/793753002/diff/1/src/serialize.cc#oldcode1667
src/serialize.cc:1667: sink_->Put(kNewObject + reference_representation_ +
space, "NewObject");
I am afraid this will not work - Serializer::Allocate may output kNextChunk to
the sink, so resulting binary stream will differ. It will be

kNewObject, [kNewChunk, chunk_space], [kDoubleAlignmentSentinel], object_size, 

whereas correct order is 
[kNewChunk, chunk_space], kNewObject, [kDoubleAlignmentSentinel], object_size

On 2014/12/10 20:47:58, Yang wrote:
> How about we simply put this line (outputting the new object) before the
if-else
> clause?

[Yang, 2014-12-11 09:26:12]

On 2014/12/11 09:16:40, Slava Chigrin wrote:
> https://codereview.chromium.org/793753002/diff/1/src/serialize.cc
> File src/serialize.cc (left):
> 
> https://codereview.chromium.org/793753002/diff/1/src/serialize.cc#oldcode1667
> src/serialize.cc:1667: sink_->Put(kNewObject + reference_representation_ +
> space, "NewObject");
> I am afraid this will not work - Serializer::Allocate may output kNextChunk to
> the sink, so resulting binary stream will differ. It will be
> 
> kNewObject, [kNewChunk, chunk_space], [kDoubleAlignmentSentinel], object_size,

> 
> whereas correct order is 
> [kNewChunk, chunk_space], kNewObject, [kDoubleAlignmentSentinel], object_size
> 
> On 2014/12/10 20:47:58, Yang wrote:
> > How about we simply put this line (outputting the new object) before the
> if-else
> > clause?

Ah yes. LGTM.

[Yang, 2014-12-11 09:26:16]

The CQ bit was checked by yangguo@chromium.org

[commit-bot: I haz the power, 2014-12-11 09:27:21]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/793753002/1

[commit-bot: I haz the power, 2014-12-11 09:27:37]

Committed patchset #1 (id:1)