Bug 85119 – Only allow non-null pointers in the WeakSet

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

[Geoffrey Garen]

Only allow non-null pointers in the WeakSet

[Geoffrey Garen]

Created attachment 139326 [details]
Patch

[Darin Adler]

Comment on attachment 139326 [details]
Patch

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

> Source/JavaScriptCore/heap/PassWeak.h:127
>  template<typename T> inline PassWeak<T>::PassWeak(JSGlobalData& globalData, typename PassWeak<T>::GetType getType, WeakHandleOwner* weakOwner, void* context)
> -    : m_impl(globalData.heap.weakSet()->allocate(getType, weakOwner, context))
> +    : m_impl(0)
>  {
> +    if (!getType)
> +        return;
> +    m_impl = globalData.heap.weakSet()->allocate(getType, weakOwner, context);
>  }

As with your prior WebCore patch, I think there are two other ways you could do this:

1) A helper function that returns 0 if getType is 0.
2) A trinary expression getType ? xxx : 0.

The “initialize to 0 and then use an early exit” doesn’t seem quite as good as those.

> Source/JavaScriptCore/heap/WeakBlock.cpp:109
>          const JSValue& jsValue = weakImpl->jsValue();
> -        if (!jsValue || !jsValue.isCell())
> -            continue;
> -
>          JSCell* jsCell = jsValue.asCell();
>          if (Heap::isMarked(jsCell))
>              continue;

Why bother with all these local variables? I like this:

    if (Heap::isMarked(weakImpl->jsValue().asCell()))

> Source/JavaScriptCore/heap/WeakBlock.cpp:136
>          const JSValue& jsValue = weakImpl->jsValue();
> -        if (!jsValue || !jsValue.isCell())
> -            continue;
> -
>          JSCell* jsCell = jsValue.asCell();
>          if (Heap::isMarked(jsCell))
>              continue;

Why bother with all these local variables? I like this:

    if (Heap::isMarked(weakImpl->jsValue().asCell()))

[Geoffrey Garen]

> As with your prior WebCore patch, I think there are two other ways you could do this:
> 
> 1) A helper function that returns 0 if getType is 0.
> 2) A trinary expression getType ? xxx : 0.

OK, I did 2.

> Why bother with all these local variables? I like this:

I kept jsValue in one function, since it's reused later, but removed all the other locals.

[Geoffrey Garen]

Committed r115534: <http://trac.webkit.org/changeset/115534>