Bug 154061

Summary: Modern IDB: Ref cycle between IDBObjectStore and IDBTransaction
Product: WebKit Reporter: Brady Eidson <beidson>
Component: WebCore Misc.Assignee: Brady Eidson <beidson>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, alecflett, buildbot, commit-queue, jsbell, rniwa
Priority: P2    
Version: Other   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 149117, 154015    
Attachments:
Description Flags
Patch v1
buildbot: commit-queue-
Archive of layout-test-results from ews103 for mac-yosemite
none
Archive of layout-test-results from ews113 for mac-yosemite
none
Patch v2 none

Brady Eidson
Reported 2016-02-09 21:03:32 PST
Modern IDB: Ref cycle between IDBObjectStore and IDBTransaction IDBTransaction has to retain a list of all object stores referenced during its lifetime, and object stores need a pointer to their transaction. Fortunately it's easy to break this cycle: Once the transaction is aborting or committing, accessing its referenced object stores is no longer possible. So at that time, the set of references object stores can be cleared, breaking the cycle.
Attachments
Patch v1 (2.95 KB, patch)
2016-02-09 21:05 PST, Brady Eidson 		buildbot: commit-queue-
DetailsFormatted DiffDiff
Archive of layout-test-results from ews103 for mac-yosemite (954.59 KB, application/zip)
2016-02-09 21:51 PST, Build Bot 		no flags
Details
Archive of layout-test-results from ews113 for mac-yosemite (865.61 KB, application/zip)
2016-02-09 22:00 PST, Build Bot 		no flags
Details
Patch v2 (3.17 KB, patch)
2016-02-10 10:31 PST, Brady Eidson 		no flags
DetailsFormatted DiffDiff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.
Brady Eidson
Comment 1 2016-02-09 21:05:26 PST
Created attachment 270969 [details] Patch v1
Build Bot
Comment 2 2016-02-09 21:51:14 PST
Comment on attachment 270969 [details] Patch v1 Attachment 270969 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/807768 New failing tests: storage/indexeddb/objectstore-basics-private.html storage/indexeddb/modern/abort-objectstore-info-private.html storage/indexeddb/modern/abort-objectstore-info.html storage/indexeddb/metadata.html storage/indexeddb/objectstore-basics.html storage/indexeddb/metadata-private.html
Build Bot
Comment 3 2016-02-09 21:51:16 PST
Created attachment 270974 [details] Archive of layout-test-results from ews103 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews103 Port: mac-yosemite Platform: Mac OS X 10.10.5
Build Bot
Comment 4 2016-02-09 22:00:24 PST
Comment on attachment 270969 [details] Patch v1 Attachment 270969 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/807775 New failing tests: storage/indexeddb/objectstore-basics-private.html storage/indexeddb/modern/abort-objectstore-info-private.html storage/indexeddb/modern/abort-objectstore-info.html storage/indexeddb/metadata.html storage/indexeddb/objectstore-basics.html storage/indexeddb/metadata-private.html
Build Bot
Comment 5 2016-02-09 22:00:26 PST
Created attachment 270975 [details] Archive of layout-test-results from ews113 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews113 Port: mac-yosemite Platform: Mac OS X 10.10.5
Brady Eidson
Comment 6 2016-02-10 08:45:04 PST
(In reply to comment #4) > Comment on attachment 270969 [details] > Patch v1 > > Attachment 270969 [details] did not pass mac-debug-ews (mac): > Output: http://webkit-queues.webkit.org/results/807775 > > New failing tests: > storage/indexeddb/objectstore-basics-private.html > storage/indexeddb/modern/abort-objectstore-info-private.html > storage/indexeddb/modern/abort-objectstore-info.html > storage/indexeddb/metadata.html > storage/indexeddb/objectstore-basics.html > storage/indexeddb/metadata-private.html Very surprising, looking locally.
Brady Eidson
Comment 7 2016-02-10 10:12:26 PST
Easy bug. Building and testing now.
Brady Eidson
Comment 8 2016-02-10 10:31:44 PST
Created attachment 271004 [details] Patch v2
WebKit Commit Bot
Comment 9 2016-02-10 11:27:59 PST
Comment on attachment 271004 [details] Patch v2 Clearing flags on attachment: 271004 Committed r196373: <http://trac.webkit.org/changeset/196373>
WebKit Commit Bot
Comment 10 2016-02-10 11:28:04 PST
All reviewed patches have been landed. Closing bug.
Darin Adler
Comment 11 2016-02-11 11:29:43 PST
Comment on attachment 271004 [details] Patch v2 View in context: https://bugs.webkit.org/attachment.cgi?id=271004&action=review > Source/WebCore/Modules/indexeddb/client/IDBTransactionImpl.cpp:183 > + ASSERT(!isFinishedOrFinishing()); > + m_state = state; Can we add this: ASSERT(isFinishedOrFinishing()); after setting m_state?
Brady Eidson
Comment 12 2016-02-11 16:11:27 PST
(In reply to comment #11) > Comment on attachment 271004 [details] > Patch v2 > > View in context: > https://bugs.webkit.org/attachment.cgi?id=271004&action=review > > > Source/WebCore/Modules/indexeddb/client/IDBTransactionImpl.cpp:183 > > + ASSERT(!isFinishedOrFinishing()); > > + m_state = state; > > Can we add this: > > ASSERT(isFinishedOrFinishing()); > > after setting m_state? Not a bad idea!
Brady Eidson
Comment 13 2016-02-11 16:11:49 PST
Reopening to track that feedback so I don't forget it.
Alex Christensen
Comment 14 2016-02-11 16:17:50 PST
http://trac.webkit.org/changeset/196455
Note You need to log in before you can comment on or make changes to this bug.