Bug 54144 – IndexedDB event targets need to ensure their wrappers arent garbage collected

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

[Jeremy Orlow]

IndexedDB event targets need to ensure their wrappers arent garbage collected

[Jeremy Orlow]

Created attachment 81875 [details]
Patch

[Jeremy Orlow]

Please take a look

[Jeremy Orlow]

Created attachment 81878 [details]
Patch

[Jeremy Orlow]

New version.  Pro: only blocks gc if we have event listeners.  Con: has to use const_cast to do it.

[Jeremy Orlow]

Actually, the second version of the patch is less correct.  If we check for event listeners, we need to check whether the parents have them as well.  It's probably best to just go with this for now.

I'll add FIXMEs before landing though since we could make it a bit better if we wanted to.

[Jeremy Orlow]

Created attachment 81896 [details]
Patch

[anton muhin]

Comment on attachment 81896 [details]
Patch

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

LGTM, but I know nothing of indexed dbs

> Source/WebCore/ChangeLog:5
> +        IndexedDB event targets need to ensure their wrappers arent garbage collected

nit: arent => aren't

> Source/WebCore/storage/IDBDatabase.cpp:145
> +    // FIXME: In an ideal world, we should return true as long as anyone has a or can

nit: do you need 'a' between has and or?

> Source/WebCore/storage/IDBDatabase.cpp:155
> +    close();

Just curious: what if I just call close()?  Is it fine meaning you don't set m_stopped to false (I know nothing of indexed dbs, so, please, bear with me?)

[Jeremy Orlow]

Nate can you please give me a final review?

(In reply to comment #7)
> (From update of attachment 81896 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=81896&action=review
> 
> LGTM, but I know nothing of indexed dbs
> 
> > Source/WebCore/ChangeLog:5
> > +        IndexedDB event targets need to ensure their wrappers arent garbage collected
> 
> nit: arent => aren't

done
 
> > Source/WebCore/storage/IDBDatabase.cpp:145
> > +    // FIXME: In an ideal world, we should return true as long as anyone has a or can
> 
> nit: do you need 'a' between has and or?

done
 
> > Source/WebCore/storage/IDBDatabase.cpp:155
> > +    close();
> 
> Just curious: what if I just call close()?  Is it fine meaning you don't set m_stopped to false (I know nothing of indexed dbs, so, please, bear with me?)

Even after a close, events can still fire, so we still need to keep it alive.  If we kept track of all pending requests, then we could stop returning true when it goes to 0.  Right now the way the objects relate to each other is already a mess though, so I want to leave such a thing alone for now.  (There shouldn't be too many IDBDatabases open, so it's OK to not let them be collected until the document navigates in the mean time, I think.)

[Jeremy Orlow]

Created attachment 82018 [details]
Patch

[Nate Chapin]

Comment on attachment 82018 [details]
Patch

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

Code LGTM

> Source/WebCore/storage/IDBDatabase.cpp:146
> +    //        get a handle to us or any deritivive transaction/request object and any

Nit: deritivive -> derivative?

> Source/WebCore/storage/IDBRequest.cpp:172
> +    // fire again (unless the transaction completes

Nit: += ').'

> Source/WebCore/storage/IDBRequest.h:100
> +    bool m_finished; // Is it possible that we'll fire any more events? If not, we're finished.

Is there some significance to IDBRequest and IDBTransaction using m_finished and IDBDatabase using m_stopped?  I assume there's a good reason that these variables don't have the same name for a similar concept?

[Jeremy Orlow]

(In reply to comment #10)
> (From update of attachment 82018 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=82018&action=review
> 
> Code LGTM
> 
> > Source/WebCore/storage/IDBDatabase.cpp:146
> > +    //        get a handle to us or any deritivive transaction/request object and any
> 
> Nit: deritivive -> derivative?

thanks.  fixed

> > Source/WebCore/storage/IDBRequest.cpp:172
> > +    // fire again (unless the transaction completes
> 
> Nit: += ').'

thanks.  fixed
 
> > Source/WebCore/storage/IDBRequest.h:100
> > +    bool m_finished; // Is it possible that we'll fire any more events? If not, we're finished.
> 
> Is there some significance to IDBRequest and IDBTransaction using m_finished and IDBDatabase using m_stopped?  I assume there's a good reason that these variables don't have the same name for a similar concept?

For IDBRequest and IDBTransaction, we know when it's done doing anything.  With IDBDatabase, we only know when the page is going away.  It'd be ideal if we know know when all outstanding requests are done (and thus when it's finished), but it's not worth the time at the moment I think.

[Jeremy Orlow]

Committed r78280: <http://trac.webkit.org/changeset/78280>

[WebKit Review Bot]

http://trac.webkit.org/changeset/78280 might have broken GTK Linux 32-bit Release
The following tests are not passing:
http/tests/misc/acid2-pixel.html
http/tests/misc/acid2.html