Fix leveldb crash when compacting during destruction
Created attachment 100740 [details] Patch
Hans, this is the patch I mentioned in crbug.com/88944. This is obviously a lame way to control order of destruction among sibling objects. Open to suggestions.
Comment on attachment 100740 [details] Patch Thanks for looking at this! View in context: https://bugs.webkit.org/attachment.cgi?id=100740&action=review > Source/WebCore/platform/leveldb/LevelDBDatabase.cpp:97 > + delete db; I was staring a bit at this to see what was going on. This makes us destroy the leveldb::DB object before we destroy m_comparatorAdapter, which is probably exactly the right thing to do. But we should do it by re-ordering the members of the class instead. > Source/WebCore/storage/IDBLevelDBBackingStore.cpp:130 > + delete db; Ditto. I'm not set up to reproduce this at the moment. Can you see if the following patch does the trick? diff --git a/Source/WebCore/platform/leveldb/LevelDBDatabase.h b/Source/WebCore/platform/leveldb/LevelDBDatabase.h index cbc28f6..c8f6b56 100644 --- a/Source/WebCore/platform/leveldb/LevelDBDatabase.h +++ b/Source/WebCore/platform/leveldb/LevelDBDatabase.h @@ -60,9 +60,9 @@ public: private: LevelDBDatabase(); - OwnPtr<leveldb::DB> m_db; const LevelDBComparator* m_comparator; OwnPtr<leveldb::Comparator> m_comparatorAdapter; + OwnPtr<leveldb::DB> m_db; }; } diff --git a/Source/WebCore/storage/IDBLevelDBBackingStore.h b/Source/WebCore/storage/IDBLevelDBBackingStore.h index d28a90c..ea6ec48 100644 --- a/Source/WebCore/storage/IDBLevelDBBackingStore.h +++ b/Source/WebCore/storage/IDBLevelDBBackingStore.h @@ -83,8 +83,8 @@ private: String m_identifier; RefPtr<IDBFactoryBackendImpl> m_factory; - OwnPtr<LevelDBDatabase> m_db; OwnPtr<LevelDBComparator> m_comparator; + OwnPtr<LevelDBDatabase> m_db; RefPtr<LevelDBTransaction> m_currentTransaction;
Created attachment 101887 [details] Patch
I couldn't reproduce the original problem on latest ToT. Going back to r92772, picked somewhat arbitrarily, worked though. Hans, your re-order suggestion indeed fixed the problem. I had forgotten that member variables are guaranteed to be destroyed in reverse order of declaration.
Nate, sorry to bug you again. Could you do another, quicker, review?
Comment on attachment 101887 [details] Patch Rather than depending on the ordering in the header file, we should probably manually destroy the OwnPtr<>s in the destructor by calling .clear() on them in the proper order.
Created attachment 101898 [details] Patch
Comment on attachment 101898 [details] Patch Great, thanks!
Comment on attachment 101898 [details] Patch Clearing flags on attachment: 101898 Committed r91721: <http://trac.webkit.org/changeset/91721>
All reviewed patches have been landed. Closing bug.