Bug 85119 - Only allow non-null pointers in the WeakSet
: Only allow non-null pointers in the WeakSet
Status: RESOLVED FIXED
: WebKit
New Bugs
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To:
:
:
:
:
  Show dependency treegraph
 
Reported: 2012-04-27 19:22 PST by
Modified: 2012-04-27 20:42 PST (History)


Attachments
Patch (8.62 KB, patch)
2012-04-27 19:35 PST, Geoffrey Garen
darin: review+
Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2012-04-27 19:22:10 PST
Only allow non-null pointers in the WeakSet
------- Comment #1 From 2012-04-27 19:35:21 PST -------
Created an attachment (id=139326) [details]
Patch
------- Comment #2 From 2012-04-27 19:47:15 PST -------
(From update of attachment 139326 [details])
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()))
------- Comment #3 From 2012-04-27 20:41:38 PST -------
> 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.
------- Comment #4 From 2012-04-27 20:42:34 PST -------
Committed r115534: <http://trac.webkit.org/changeset/115534>