Bug 85119 - Only allow non-null pointers in the WeakSet
Summary: Only allow non-null pointers in the WeakSet
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Geoffrey Garen
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-04-27 19:22 PDT by Geoffrey Garen
Modified: 2012-04-27 20:42 PDT (History)
0 users

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Geoffrey Garen 2012-04-27 19:22:10 PDT
Only allow non-null pointers in the WeakSet
Comment 1 Geoffrey Garen 2012-04-27 19:35:21 PDT
Created attachment 139326 [details]
Patch
Comment 2 Darin Adler 2012-04-27 19:47:15 PDT
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()))
Comment 3 Geoffrey Garen 2012-04-27 20:41:38 PDT
> 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 Geoffrey Garen 2012-04-27 20:42:34 PDT
Committed r115534: <http://trac.webkit.org/changeset/115534>