Bug 205712 – [JSC] MarkedBlock::Handle and BlockDirectory should be shrunk

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Yusuke Suzuki]

[JSC] MarkedBlock::Handle and BlockDirectory should be shrunk

[Yusuke Suzuki]

Created attachment 386662 [details]
Patch

[Mark Lam]

Comment on attachment 386662 [details]
Patch

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

r=me with a suggestion.

> Source/JavaScriptCore/heap/MarkedBlock.h:557
> +// This function must return size_t instead of unsigned since pointer |p| is not guaranteed that this is within MarkedBlock.
> +// See MarkedBlock::isAtom which can accept out-of-bound pointers.
>  inline size_t MarkedBlock::atomNumber(const void* p)

Seems like isAtom() is the only case that can potential use atomNumber with a bad pointer while lots of other clients expect the pointer to be valid.  How about doing this instead?
1. rename this function to candidateAtomNumber() and change isAtom() to use it.
2. add a new inline function atomNumber() that calls candidateAtomNumber() in its implementation, ASSERT(!(atomNumber % handle().m_atomsPerCell)), and return the atomNumber() as an unsigned value.

[Mark Lam]

(In reply to Mark Lam from comment #2)
> > Source/JavaScriptCore/heap/MarkedBlock.h:557
> > +// This function must return size_t instead of unsigned since pointer |p| is not guaranteed that this is within MarkedBlock.
> > +// See MarkedBlock::isAtom which can accept out-of-bound pointers.
> >  inline size_t MarkedBlock::atomNumber(const void* p)
> 
> Seems like isAtom() is the only case that can potential use atomNumber with
> a bad pointer while lots of other clients expect the pointer to be valid. 
> How about doing this instead?
> 1. rename this function to candidateAtomNumber() and change isAtom() to use
> it.
> 2. add a new inline function atomNumber() that calls candidateAtomNumber()
> in its implementation, ASSERT(!(atomNumber % handle().m_atomsPerCell)), and
> return the atomNumber() as an unsigned value.

One more thing: HeapUtil::findGCObjectPointersForMarking() does a test for "candidate->atomNumber(alignedPointer) > 0" which is always true because atomNumber() returned a size_t.  This bug was not introduced by your patch, but your patch did shed a light on it.  Can you also fix that bug, or file a bug to have it revisited later?  Thanks.

[Yusuke Suzuki]

Comment on attachment 386662 [details]
Patch

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

>> Source/JavaScriptCore/heap/MarkedBlock.h:557
>>  inline size_t MarkedBlock::atomNumber(const void* p)
> 
> Seems like isAtom() is the only case that can potential use atomNumber with a bad pointer while lots of other clients expect the pointer to be valid.  How about doing this instead?
> 1. rename this function to candidateAtomNumber() and change isAtom() to use it.
> 2. add a new inline function atomNumber() that calls candidateAtomNumber() in its implementation, ASSERT(!(atomNumber % handle().m_atomsPerCell)), and return the atomNumber() as an unsigned value.

Sounds better! Fixed.

[Yusuke Suzuki]

(In reply to Mark Lam from comment #3)
> (In reply to Mark Lam from comment #2)
> > > Source/JavaScriptCore/heap/MarkedBlock.h:557
> > > +// This function must return size_t instead of unsigned since pointer |p| is not guaranteed that this is within MarkedBlock.
> > > +// See MarkedBlock::isAtom which can accept out-of-bound pointers.
> > >  inline size_t MarkedBlock::atomNumber(const void* p)
> > 
> > Seems like isAtom() is the only case that can potential use atomNumber with
> > a bad pointer while lots of other clients expect the pointer to be valid. 
> > How about doing this instead?
> > 1. rename this function to candidateAtomNumber() and change isAtom() to use
> > it.
> > 2. add a new inline function atomNumber() that calls candidateAtomNumber()
> > in its implementation, ASSERT(!(atomNumber % handle().m_atomsPerCell)), and
> > return the atomNumber() as an unsigned value.
> 
> One more thing: HeapUtil::findGCObjectPointersForMarking() does a test for
> "candidate->atomNumber(alignedPointer) > 0" which is always true because
> atomNumber() returned a size_t.  This bug was not introduced by your patch,
> but your patch did shed a light on it.  Can you also fix that bug, or file a
> bug to have it revisited later?  Thanks.

Nice catch! Yeah, we should fix it. I'll do it in this patch.

[Yusuke Suzuki]

(In reply to Yusuke Suzuki from comment #5)
> (In reply to Mark Lam from comment #3)
> > (In reply to Mark Lam from comment #2)
> > > > Source/JavaScriptCore/heap/MarkedBlock.h:557
> > > > +// This function must return size_t instead of unsigned since pointer |p| is not guaranteed that this is within MarkedBlock.
> > > > +// See MarkedBlock::isAtom which can accept out-of-bound pointers.
> > > >  inline size_t MarkedBlock::atomNumber(const void* p)
> > > 
> > > Seems like isAtom() is the only case that can potential use atomNumber with
> > > a bad pointer while lots of other clients expect the pointer to be valid. 
> > > How about doing this instead?
> > > 1. rename this function to candidateAtomNumber() and change isAtom() to use
> > > it.
> > > 2. add a new inline function atomNumber() that calls candidateAtomNumber()
> > > in its implementation, ASSERT(!(atomNumber % handle().m_atomsPerCell)), and
> > > return the atomNumber() as an unsigned value.
> > 
> > One more thing: HeapUtil::findGCObjectPointersForMarking() does a test for
> > "candidate->atomNumber(alignedPointer) > 0" which is always true because
> > atomNumber() returned a size_t.  This bug was not introduced by your patch,
> > but your patch did shed a light on it.  Can you also fix that bug, or file a
> > bug to have it revisited later?  Thanks.
> 
> Nice catch! Yeah, we should fix it. I'll do it in this patch.

I'll still need to understand the meaning of this original code. So I'll open a new bug for this.

[Yusuke Suzuki]

(In reply to Yusuke Suzuki from comment #6)
> (In reply to Yusuke Suzuki from comment #5)
> > (In reply to Mark Lam from comment #3)
> > > (In reply to Mark Lam from comment #2)
> > > > > Source/JavaScriptCore/heap/MarkedBlock.h:557
> > > > > +// This function must return size_t instead of unsigned since pointer |p| is not guaranteed that this is within MarkedBlock.
> > > > > +// See MarkedBlock::isAtom which can accept out-of-bound pointers.
> > > > >  inline size_t MarkedBlock::atomNumber(const void* p)
> > > > 
> > > > Seems like isAtom() is the only case that can potential use atomNumber with
> > > > a bad pointer while lots of other clients expect the pointer to be valid. 
> > > > How about doing this instead?
> > > > 1. rename this function to candidateAtomNumber() and change isAtom() to use
> > > > it.
> > > > 2. add a new inline function atomNumber() that calls candidateAtomNumber()
> > > > in its implementation, ASSERT(!(atomNumber % handle().m_atomsPerCell)), and
> > > > return the atomNumber() as an unsigned value.
> > > 
> > > One more thing: HeapUtil::findGCObjectPointersForMarking() does a test for
> > > "candidate->atomNumber(alignedPointer) > 0" which is always true because
> > > atomNumber() returned a size_t.  This bug was not introduced by your patch,
> > > but your patch did shed a light on it.  Can you also fix that bug, or file a
> > > bug to have it revisited later?  Thanks.
> > 
> > Nice catch! Yeah, we should fix it. I'll do it in this patch.
> 
> I'll still need to understand the meaning of this original code. So I'll
> open a new bug for this.

Talked with Phil. This condition is checking whether the atomNumber is not zero. If the pointer is pointing the first atom, this is never pointing ButterflyEnd + sizeof(IndexingHeader) since no previous object exists (Note that MarkedBlock has footer, so the end of MarkedBlock is not usable for objects).

[Mark Lam]

(In reply to Yusuke Suzuki from comment #7)
> (In reply to Yusuke Suzuki from comment #6)
> > (In reply to Yusuke Suzuki from comment #5)
> > > (In reply to Mark Lam from comment #3)
> > > > (In reply to Mark Lam from comment #2)
> > > > > > Source/JavaScriptCore/heap/MarkedBlock.h:557
> > > > > > +// This function must return size_t instead of unsigned since pointer |p| is not guaranteed that this is within MarkedBlock.
> > > > > > +// See MarkedBlock::isAtom which can accept out-of-bound pointers.
> > > > > >  inline size_t MarkedBlock::atomNumber(const void* p)
> > > > > 
> > > > > Seems like isAtom() is the only case that can potential use atomNumber with
> > > > > a bad pointer while lots of other clients expect the pointer to be valid. 
> > > > > How about doing this instead?
> > > > > 1. rename this function to candidateAtomNumber() and change isAtom() to use
> > > > > it.
> > > > > 2. add a new inline function atomNumber() that calls candidateAtomNumber()
> > > > > in its implementation, ASSERT(!(atomNumber % handle().m_atomsPerCell)), and
> > > > > return the atomNumber() as an unsigned value.
> > > > 
> > > > One more thing: HeapUtil::findGCObjectPointersForMarking() does a test for
> > > > "candidate->atomNumber(alignedPointer) > 0" which is always true because
> > > > atomNumber() returned a size_t.  This bug was not introduced by your patch,
> > > > but your patch did shed a light on it.  Can you also fix that bug, or file a
> > > > bug to have it revisited later?  Thanks.
> > > 
> > > Nice catch! Yeah, we should fix it. I'll do it in this patch.
> > 
> > I'll still need to understand the meaning of this original code. So I'll
> > open a new bug for this.
> 
> Talked with Phil. This condition is checking whether the atomNumber is not
> zero. If the pointer is pointing the first atom, this is never pointing
> ButterflyEnd + sizeof(IndexingHeader) since no previous object exists (Note
> that MarkedBlock has footer, so the end of MarkedBlock is not usable for
> objects).

Sorry for the noise.  I misread > 0 as >= 0.

[Yusuke Suzuki]

Committed r254023: <https://trac.webkit.org/changeset/254023>

[Radar WebKit Bug Importer]

<rdar://problem/58310059>