Summary: | Only allow non-null pointers in the WeakSet | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Geoffrey Garen <ggaren> | ||||
Component: | New Bugs | Assignee: | Geoffrey Garen <ggaren> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | ||||||
Priority: | P2 | ||||||
Version: | 528+ (Nightly build) | ||||||
Hardware: | Unspecified | ||||||
OS: | Unspecified | ||||||
Attachments: |
|
Description
Geoffrey Garen
2012-04-27 19:22:10 PDT
Created attachment 139326 [details]
Patch
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())) > 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. Committed r115534: <http://trac.webkit.org/changeset/115534> |