[webkit-reviews] review granted: [Bug 209042] reportZappedCellAndCrash should handle PreciseAllocation in IsoSubspace : [Attachment 393567] Patch

[bugzilla-daemon at webkit.org]

Mark Lam <mark.lam at apple.com> has granted Yusuke Suzuki <ysuzuki at apple.com>'s
request for review:
Bug 209042: reportZappedCellAndCrash should handle PreciseAllocation in
IsoSubspace
https://bugs.webkit.org/show_bug.cgi?id=209042

Attachment 393567: Patch

https://bugs.webkit.org/attachment.cgi?id=393567&action=review




--- Comment #5 from Mark Lam <mark.lam at apple.com> ---
Comment on attachment 393567
  --> https://bugs.webkit.org/attachment.cgi?id=393567
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=393567&action=review

r=me

>>> Source/JavaScriptCore/runtime/JSCell.cpp:387
>>> +		 variousState |= static_cast<uint64_t>(!isFreeListed) << 1;
>> 
>> Is this correct?  Does !!isFreeListed mean isAllocated?
> 
> typo: I meant !isFreeListed

Nevermind, I've read thru the PreciseAllocation code and this patch some more. 
This is correct because foundPreciseAllocation is only true if this cell is
found in the lower tier of this IsoSubspace, and it is only allocated if it's
not on the free list.

> Source/JavaScriptCore/runtime/JSCell.cpp:390
> +		   variousState |=
static_cast<uint64_t>(foundPreciseAllocation->isEmpty()) << 2;
> +		   variousState |=
static_cast<uint64_t>(foundPreciseAllocation->isNewlyAllocated()) << 4;

The isEmpty and isNewlyAllocated flags previously meant that the cell pointer
was found in a MarkedBlock of that state.  For PreciseAllocations, I think
knowing whether it isFreeListed or isAllocated is sufficient.  I'm not quite
sure what isEmpty and isNewlyAllocated would mean in this case.  I suggest just
leaving these unset.

Instead, can you also set the cellIsProperlyAligned flag ( << 5) in
variousState based on whether (foundPreciseAllocation->cell() == cell)?  If the
cell pointer is found in a PreciseAllocation but does not equal it's expected
cell() pointer, then we have a corrupted pointer or someone unscrupulous is
making bad pointers.

Can you also set the needsDestruction flag in variousState based on whether the
SubSpace CellAttributes says it NeedsDestruction or not?