Bug 64494 - Fix leveldb crash when compacting during destruction
Summary: Fix leveldb crash when compacting during destruction
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: David Grogan
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-07-13 17:48 PDT by David Grogan
Modified: 2011-07-25 16:28 PDT (History)
4 users (show)

See Also:


Attachments
Patch (2.29 KB, patch)
2011-07-13 17:50 PDT, David Grogan
no flags Details | Formatted Diff | Diff
Patch (2.11 KB, patch)
2011-07-25 11:37 PDT, David Grogan
no flags Details | Formatted Diff | Diff
Patch (2.44 KB, patch)
2011-07-25 13:13 PDT, David Grogan
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description David Grogan 2011-07-13 17:48:21 PDT
Fix leveldb crash when compacting during destruction
Comment 1 David Grogan 2011-07-13 17:50:08 PDT
Created attachment 100740 [details]
Patch
Comment 2 David Grogan 2011-07-13 18:13:47 PDT
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 3 Hans Wennborg 2011-07-25 04:18:15 PDT
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;
Comment 4 David Grogan 2011-07-25 11:37:48 PDT
Created attachment 101887 [details]
Patch
Comment 5 David Grogan 2011-07-25 11:52:12 PDT
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.
Comment 6 David Grogan 2011-07-25 12:00:42 PDT
Nate, sorry to bug you again.  Could you do another, quicker, review?
Comment 7 Nate Chapin 2011-07-25 12:32:17 PDT
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.
Comment 8 David Grogan 2011-07-25 13:13:00 PDT
Created attachment 101898 [details]
Patch
Comment 9 Nate Chapin 2011-07-25 13:14:20 PDT
Comment on attachment 101898 [details]
Patch

Great, thanks!
Comment 10 WebKit Review Bot 2011-07-25 16:28:51 PDT
Comment on attachment 101898 [details]
Patch

Clearing flags on attachment: 101898

Committed r91721: <http://trac.webkit.org/changeset/91721>
Comment 11 WebKit Review Bot 2011-07-25 16:28:56 PDT
All reviewed patches have been landed.  Closing bug.