[webkit-reviews] review denied: [Bug 18205] DOMNode objects are garbage collected although there are strong references : [Attachment 25829] Fix for the resurrection bug of DOM wrapper objects using BUILDING_ON_TIGER

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Kai Brüning <kai at granus.net>'s request
for review:
Bug 18205: DOMNode objects are garbage collected although there are strong
references
https://bugs.webkit.org/show_bug.cgi?id=18205

Attachment 25829: Fix for the resurrection bug of DOM wrapper objects using
BUILDING_ON_TIGER
https://bugs.webkit.org/attachment.cgi?id=25829&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
> +    NSMapTableKeyCallBacks keyCallBacks;
> +    keyCallBacks.hash = 0;	   // use pointer hashing
> +    keyCallBacks.isEqual = 0;   // use pointer equality
> +    keyCallBacks.retain = 0;
> +    keyCallBacks.release = 0;
> +    keyCallBacks.describe = 0;
> +    keyCallBacks.notAKeyMarker = 0;
> +    
> +    NSMapTableValueCallBacks valueCallBacks;
> +    valueCallBacks.retain = 0;
> +    valueCallBacks.release = 0;
> +    valueCallBacks.describe = describeDOMWrapper;
> +    
> +    DOMWrapperCache = NSCreateMapTable(keyCallBacks, valueCallBacks, 0);

You should use NSNonOwnedPointerMapKeyCallBacks and
NSNonRetainedObjectMapValueCallBacks instead of defining your own.

> +    // Use the GC-aware NSMapTable if available (that is, under Leopard and
newer).

We normally prefer to have comments like to explain the old version, and not
the new one. Eventually we'll delete the old version, and at that point the
comment about "if available" is a little strange; it's better if it just gets
deleted along with the old unused code.

> +    DOMWrapperCache = [[NSMapTable alloc]
> +			  initWithKeyOptions:NSPointerFunctionsOpaqueMemory |
NSPointerFunctionsOpaquePersonality
> +			  valueOptions:NSPointerFunctionsZeroingWeakMemory |
NSPointerFunctionsObjectPersonality
> +			  capacity:0];

This is not typical indentation style for code in the WebKit project; we'd
normally just indent subsequent lines 4 spaces and not try to line up with the
opening square bracket.

Won't the DOMWrapperCache object itself get garbage collected? Does the GC
runtime properly handle an object referenced only by a global variable without
an explicit call to CFRetain? In the past we've had to explicitly CFRetain in
some cases like this.

> -    if (!DOMWrapperCache)
> -	   return nil;
> -    return DOMWrapperCache->get(impl);
> +    return DOMWrapperCache ? static_cast<NSObject*>
(NSMapGet(DOMWrapperCache, impl)) : nil;

There shouldn't be a space between the > character and the ( character. It's
also unnecessary to change this to ?: since the old if statement would have
worked fine as is. Unless there's something better about the new style. I think
part of the reason for the old style was to be consistent with the other two
functions.

>  void addDOMWrapper(NSObject* wrapper, DOMObjectInternal* impl)
>  {
>      if (!DOMWrapperCache)
> -	   DOMWrapperCache = new DOMWrapperMap;
> -    DOMWrapperCache->set(impl, wrapper);
> +	   createDOMWrapperCache();
> +
> +    NSMapInsert(DOMWrapperCache, impl, wrapper);
>  }

No need to add the blank line here. Better to stay as close to the old version
as possible.

>  void removeDOMWrapper(DOMObjectInternal* impl)
>  {
> -    if (!DOMWrapperCache)
> -	   return;
> -    DOMWrapperCache->remove(impl);
> +    assert(DOMWrapperCache);
> +    NSMapRemove(DOMWrapperCache, impl);
>  }

The use of "assert" here is incorrect. We use the ASSERT (capitalized) macro
from <wtf/Assertions.h>, not the assert macro from <assert.h>.

The old code was safe to call even if the DOM wrapper hadn't been added
earlier. The new version will crash instead. I think it's not likely that the
old behavior was ever helpful, but it seems unnecessary to change the code to
be stricter in this way.

This is fine, but what about the identical code in WebScriptObject.mm that
maintains JSWrapperMap? DOMObject is a subclass of WebScriptObject, so that
code will be running too.

How did you test this?

I'm going to say review- mainly because of the JSWrapperMap issue. I also have
some questions about the basic assumptions in this patch. I'll follow up with
those.