Bug 48416 – IDBFactoryBackend's reference to IDBDatabaseBackend should be weak

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Jeremy Orlow]

.

[Jeremy Orlow]

Created attachment 72011 [details]
patch

[Andrei Popescu]

LGTM

[Steve Block]

Comment on attachment 72011 [details]
patch

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

> WebCore/storage/IDBDatabaseBackendImpl.h:93
> +    RefPtr<IDBFactoryBackendImpl> m_factory;

You mention that the factory has the lifetime of the application. If so, why do we need to take a ref to it?

[Jeremy Orlow]

(In reply to comment #3)
> (From update of attachment 72011 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=72011&action=review
> 
> > WebCore/storage/IDBDatabaseBackendImpl.h:93
> > +    RefPtr<IDBFactoryBackendImpl> m_factory;
> 
> You mention that the factory has the lifetime of the application. If so, why do we need to take a ref to it?

The other objects are ref counted and held by javascript.  Are we guaranteed that a GC will happen and the GC will catch _all_ references before the page group goes away?

Is there any reason to do this either?  Given that the factory is a pretty light weight object (since it only has weak references) there seems like no advantage and big disadvantages (possible security bugs) for changing this behavior.

[Steve Block]

> The other objects are ref counted and held by javascript.  Are we guaranteed that a GC will happen and
> the GC will catch _all_ references before the page group goes away?
So you mean the factory has page lifetime? If so, that makes sense. Perhaps you could update the comment to make that clear.

[Jeremy Orlow]

(In reply to comment #5)
> > The other objects are ref counted and held by javascript.  Are we guaranteed that a GC will happen and
> > the GC will catch _all_ references before the page group goes away?
> So you mean the factory has page lifetime? If so, that makes sense. Perhaps you could update the comment to make that clear.

No, it's page group lifetime which is a superset of the pages' lifetime.  But pages will own IDBDatabase objects.  And once the last one goes away, the IDBDatabaseBackendImpl will go to 0 which will cause the IDBFactory to clear its pointer.

Basically the case where IDBFactory could possibly go away first is on shutdown where it's not clear whether all the pages' resources (especially those owned by JavaScript) will be killed before the page group goes away.

[Jeremy Orlow]

I can has r+?

[Steve Block]

Comment on attachment 72011 [details]
patch

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

r=me

>>> WebCore/storage/IDBDatabaseBackendImpl.h:93
>>> +    RefPtr<IDBFactoryBackendImpl> m_factory;
>> 
>> You mention that the factory has the lifetime of the application. If so, why do we need to take a ref to it?
> 
> The other objects are ref counted and held by javascript.  Are we guaranteed that a GC will happen and the GC will catch _all_ references before the page group goes away?
> 
> Is there any reason to do this either?  Given that the factory is a pretty light weight object (since it only has weak references) there seems like no advantage and big disadvantages (possible security bugs) for changing this behavior.

OK, makes sense. I still think the comment could be more clear.

[Jeremy Orlow]

Committed r71411: <http://trac.webkit.org/changeset/71411>

[Jeremy Orlow]

(In reply to comment #8)
> (From update of attachment 72011 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=72011&action=review
> 
> r=me
> 
> >>> WebCore/storage/IDBDatabaseBackendImpl.h:93
> >>> +    RefPtr<IDBFactoryBackendImpl> m_factory;
> >> 
> >> You mention that the factory has the lifetime of the application. If so, why do we need to take a ref to it?
> > 
> > The other objects are ref counted and held by javascript.  Are we guaranteed that a GC will happen and the GC will catch _all_ references before the page group goes away?
> > 
> > Is there any reason to do this either?  Given that the factory is a pretty light weight object (since it only has weak references) there seems like no advantage and big disadvantages (possible security bugs) for changing this behavior.
> 
> OK, makes sense. I still think the comment could be more clear.

I added a comment.