Bug 54144 - IndexedDB event targets need to ensure their wrappers arent garbage collected
Summary: IndexedDB event targets need to ensure their wrappers arent garbage collected
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Jeremy Orlow
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-02-09 14:22 PST by Jeremy Orlow
Modified: 2011-02-10 21:10 PST (History)
10 users (show)

See Also:


Attachments
Patch (11.43 KB, patch)
2011-02-09 14:25 PST, Jeremy Orlow
no flags Details | Formatted Diff | Diff
Patch (11.68 KB, patch)
2011-02-09 14:40 PST, Jeremy Orlow
no flags Details | Formatted Diff | Diff
Patch (12.44 KB, patch)
2011-02-09 17:14 PST, Jeremy Orlow
no flags Details | Formatted Diff | Diff
Patch (12.67 KB, patch)
2011-02-10 11:52 PST, Jeremy Orlow
japhet: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jeremy Orlow 2011-02-09 14:22:15 PST
IndexedDB event targets need to ensure their wrappers arent garbage collected
Comment 1 Jeremy Orlow 2011-02-09 14:25:52 PST
Created attachment 81875 [details]
Patch
Comment 2 Jeremy Orlow 2011-02-09 14:26:51 PST
Please take a look
Comment 3 Jeremy Orlow 2011-02-09 14:40:05 PST
Created attachment 81878 [details]
Patch
Comment 4 Jeremy Orlow 2011-02-09 14:40:46 PST
New version.  Pro: only blocks gc if we have event listeners.  Con: has to use const_cast to do it.
Comment 5 Jeremy Orlow 2011-02-09 14:56:36 PST
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.
Comment 6 Jeremy Orlow 2011-02-09 17:14:10 PST
Created attachment 81896 [details]
Patch
Comment 7 anton muhin 2011-02-10 06:29:33 PST
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?)
Comment 8 Jeremy Orlow 2011-02-10 11:51:47 PST
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.)
Comment 9 Jeremy Orlow 2011-02-10 11:52:27 PST
Created attachment 82018 [details]
Patch
Comment 10 Nate Chapin 2011-02-10 14:10:34 PST
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?
Comment 11 Jeremy Orlow 2011-02-10 15:42:44 PST
(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.
Comment 12 Jeremy Orlow 2011-02-10 15:49:33 PST
Committed r78280: <http://trac.webkit.org/changeset/78280>
Comment 13 WebKit Review Bot 2011-02-10 21:10:00 PST
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