Bug 64494 – Fix leveldb crash when compacting during destruction

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

[David Grogan]

Fix leveldb crash when compacting during destruction

[David Grogan]

Created attachment 100740 [details]
Patch

[David Grogan]

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.

[Hans Wennborg]

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;

[David Grogan]

Created attachment 101887 [details]
Patch

[David Grogan]

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.

[David Grogan]

Nate, sorry to bug you again.  Could you do another, quicker, review?

[Nate Chapin]

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.

[David Grogan]

Created attachment 101898 [details]
Patch

[Nate Chapin]

Comment on attachment 101898 [details]
Patch

Great, thanks!

[WebKit Review Bot]

Comment on attachment 101898 [details]
Patch

Clearing flags on attachment: 101898

Committed r91721: <http://trac.webkit.org/changeset/91721>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.