.
Created attachment 72011 [details] patch
LGTM
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?
(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.
> 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.
(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.
I can has r+?
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.
Committed r71411: <http://trac.webkit.org/changeset/71411>
(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.