[webkit-reviews] review requested: [Bug 18205] DOMNode objects are garbage collected although there are strong references : [Attachment 25982] Fix for the resurrection bug of a varity of wrapper objects

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Fri Dec 12 08:33:33 PST 2008

Kai Brüning <kai at granus.net> has asked	for review:
Bug 18205: DOMNode objects are garbage collected although there are strong

Attachment 25982: Fix for the resurrection bug of a varity of wrapper objects

------- Additional Comments from Kai Brüning <kai at granus.net>
New fix with the changes according to Darins comments and including both the
caches in WebScriptObject.mm and DOMRGBColor.mm.

I followed Darins though to share the creation function for the caches. I put
it into DOMInternal.h because this header is included in all three places
anyway. The function is parameterized by the key personality.

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

Sure, done. Just didn’t know of their existence.

> 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.


> 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
> opening square bracket.

Done - but it’s ugly now.

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

> some cases like this.

>From the GC guide:

"Typically, the garbage collector treats global object pointers as root objects
and so does not consider them candidates for collection (see “Root Set and
Reference Types”). Globals of Objective-C objects or other __strong pointer
variables, and function-level static variables, are written to with a
write-barrier. Note that although this is true for Objective-C or
Objective-C++, writing to globals from C or C++ is not supported."

Therefore we should be fine as long as the cache objects are created in .mm

> 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
> worked fine as is. Unless there's something better about the new style. I
> part of the reason for the old style was to be consistent with the other two
> functions.

Sure - this happend because I took a detour in the code. Changed it back.

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

Copied it from a few lines below. But now it’s gone again anyway.

> 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.

Makes sense - done.

> 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?

Mainly by using the Leopard build with my project, which used to crash due to
this bug before and now no longer does.
The Tiger build is untested because unfortunately I do not have a suitable
Tiger system.
Is there any way to build for Tiger under Leopard?

More information about the webkit-reviews mailing list