Bug 154061 - Modern IDB: Ref cycle between IDBObjectStore and IDBTransaction
Summary: Modern IDB: Ref cycle between IDBObjectStore and IDBTransaction
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: Other
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Brady Eidson
URL:
Keywords:
Depends on:
Blocks: 149117 154015
  Show dependency treegraph
 
Reported: 2016-02-09 21:03 PST by Brady Eidson
Modified: 2016-02-11 16:17 PST (History)
6 users (show)

See Also:


Attachments
Patch v1 (2.95 KB, patch)
2016-02-09 21:05 PST, Brady Eidson
buildbot: commit-queue-
Details | Formatted Diff | Diff
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 Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brady Eidson 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.
Comment 1 Brady Eidson 2016-02-09 21:05:26 PST
Created attachment 270969 [details]
Patch v1
Comment 2 Build Bot 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
Comment 3 Build Bot 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
Comment 4 Build Bot 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
Comment 5 Build Bot 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
Comment 6 Brady Eidson 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.
Comment 7 Brady Eidson 2016-02-10 10:12:26 PST
Easy bug. Building and testing now.
Comment 8 Brady Eidson 2016-02-10 10:31:44 PST
Created attachment 271004 [details]
Patch v2
Comment 9 WebKit Commit Bot 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>
Comment 10 WebKit Commit Bot 2016-02-10 11:28:04 PST
All reviewed patches have been landed.  Closing bug.
Comment 11 Darin Adler 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?
Comment 12 Brady Eidson 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!
Comment 13 Brady Eidson 2016-02-11 16:11:49 PST
Reopening to track that feedback so I don't forget it.
Comment 14 Alex Christensen 2016-02-11 16:17:50 PST
http://trac.webkit.org/changeset/196455