[GTK] Crash in DOMObjectCache when a wrapped object owned by the cache is unreffed by the user

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Carlos Garcia Campos]

This is a case we claim to support, but it only works if the object has only one reference. In that case, when the user unrefs it, the weak ref notify callback removes the object from the cache. However, if the object has more than one ref, the cache doesn't know the user unreffed it, and when clearing the cache we try to remove more references that what the object actually has, causing a crash in g_object_unref. See the backtrace in bug #118788.

[Carlos Garcia Campos]

Created attachment 250347 [details]
Patch

[Martin Robinson]

Comment on attachment 250347 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=250347&action=review

> Source/WebCore/ChangeLog:13
> +        try to remove more references that what the object actually has,

Nit: that what -> than what

> Source/WebCore/bindings/gobject/DOMObjectCache.cpp:48
> +        cacheReferences = std::min(static_cast<unsigned>(object->ref_count), cacheReferences);

It might be worth dropping a comment here explaining why cacheReferences might be incorrect at this point.

[Sergio Villar Senin]

Comment on attachment 250347 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=250347&action=review

>> Source/WebCore/bindings/gobject/DOMObjectCache.cpp:48
>> +        cacheReferences = std::min(static_cast<unsigned>(object->ref_count), cacheReferences);
> 
> It might be worth dropping a comment here explaining why cacheReferences might be incorrect at this point.

Also, coulnd't we completely forget about cacheReferences and simply use GObject's ref_count ?

[Martin Robinson]

(In reply to comment #3)
 
> Also, coulnd't we completely forget about cacheReferences and simply use
> GObject's ref_count ?

I don't think we can, because if the user explicitly refs the object, we should not delete it.

[Carlos Garcia Campos]

(In reply to comment #4)
> (In reply to comment #3)
>  
> > Also, coulnd't we completely forget about cacheReferences and simply use
> > GObject's ref_count ?
> 
> I don't think we can, because if the user explicitly refs the object, we
> should not delete it.

Exactly, cacheReferences only keeps track of the references owned by the cache itself. In a normal case, we just drop our refs, but users can do weird things.

[Carlos Garcia Campos]

(In reply to comment #2)
> Comment on attachment 250347 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=250347&action=review
> 
> > Source/WebCore/ChangeLog:13
> > +        try to remove more references that what the object actually has,
> 
> Nit: that what -> than what

I always doubt :-P
 
> > Source/WebCore/bindings/gobject/DOMObjectCache.cpp:48
> > +        cacheReferences = std::min(static_cast<unsigned>(object->ref_count), cacheReferences);
> 
> It might be worth dropping a comment here explaining why cacheReferences
> might be incorrect at this point.

Sure!

[Carlos Garcia Campos]

Committed r182537: <http://trac.webkit.org/changeset/182537>