SQLiteCursor could be destroyed in Java object's finalize method, and UI...

SQLiteCursor could be destroyed in Java object's finalize method, and UI message loop might already shutdown, we need to check if the post task succeeds, then wait for callback.

BUG=239482

Review URL: https://chromiumcodereview.appspot.com/15701011

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@203953 0039d316-1c4b-4281-b951-d872f2087c98

chrome/browser/history/android/sqlite_cursor.cc

@@ -132,7 +132,17 @@ jint SQLiteCursor::GetColumnType(JNIEnv* env, jobject obj, jint column) {
 }
 
 void SQLiteCursor::Destroy(JNIEnv* env, jobject obj) {
-  delete this;
+  // We do our best to cleanup when Destroy() is called from Java's finalize()
+  // where the UI message loop might stop running or in the process of shutting
+  // down, as the whole process will be destroyed soon, it's fine to leave some
+  // objects out there.
+  if (BrowserThread::CurrentlyOn(BrowserThread::UI)) {
+    DestroyOnUIThread();
+  } else if (!BrowserThread::PostTask(BrowserThread::UI, FROM_HERE,
+                 base::Bind(&SQLiteCursor::DestroyOnUIThread,
+                     base::Unretained(this)))) {
+    delete this;
+  }
 }
 
 SQLiteCursor::SQLiteCursor(const std::vector<std::string>& column_names,
@@ -150,25 +160,16 @@ SQLiteCursor::SQLiteCursor(const std::vector<std::string>& column_names,
 }
 
 SQLiteCursor::~SQLiteCursor() {
+}
+
+void SQLiteCursor::DestroyOnUIThread() {
   // Consumer requests were set in the UI thread. They must be cancelled
   // using the same thread.
-  if (BrowserThread::CurrentlyOn(BrowserThread::UI)) {
-    CancelAllRequests(NULL);
-  } else {
-    base::WaitableEvent event(false, false);
-    BrowserThread::PostTask(
-        BrowserThread::UI,
-        FROM_HERE,
-        base::Bind(&SQLiteCursor::CancelAllRequests, base::Unretained(this),
-                   &event));
-    event.Wait();
-  }
-
-  BrowserThread::PostTask(
-      BrowserThread::UI,
-      FROM_HERE,
-      base::Bind(&AndroidHistoryProviderService::CloseStatement,
-                 base::Unretained(service_), statement_));
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  consumer_.reset();
+  tracker_.reset();
+  service_->CloseStatement(statement_);
+  delete this;
 }
 
 bool SQLiteCursor::GetFavicon(chrome::FaviconID id,
@@ -226,17 +227,6 @@ void SQLiteCursor::OnMoved(AndroidHistoryProviderService::Handle handle,
     test_observer_->OnGetMoveToResult();
 }
 
-void SQLiteCursor::CancelAllRequests(base::WaitableEvent* finished) {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-
-  // Destruction will cancel all pending tasks.
-  consumer_.reset();
-  tracker_.reset();
-
-  if (finished)
-    finished->Signal();
-}
-
 SQLiteCursor::JavaColumnType SQLiteCursor::GetColumnTypeInternal(int column) {
   if (column == statement_->favicon_index())
     return SQLiteCursor::BLOB;

chrome/browser/history/android/sqlite_cursor.h

@@ -136,6 +136,10 @@ class SQLiteCursor {
 
   virtual ~SQLiteCursor();
 
+  // Destory SQLiteCursor object on UI thread. All cleanup need finish in UI
+  // thread.
+  void DestroyOnUIThread();
+
   // This method is for testing only.
   void set_test_observer(TestObserver* test_observer) {
     test_observer_ = test_observer;
@@ -155,9 +159,6 @@ class SQLiteCursor {
   // The callback function of MoveTo().
   void OnMoved(AndroidHistoryProviderService::Handle handle, int pos);
 
-  // Used to cancel all request on the UI thread during shutdown.
-  void CancelAllRequests(base::WaitableEvent* finished);
-
   JavaColumnType GetColumnTypeInternal(int column);
 
   // Runs the MoveStatement on UI thread.

chrome/browser/history/android/sqlite_cursor_unittest.cc

@@ -197,28 +197,25 @@ TEST_F(SQLiteCursorTest, Run) {
 
   FaviconService* favicon_service = new FaviconService(hs_);
 
-  // Wraps cursor in a block, so the destructor will be called after that.
-  {
-    SQLiteCursor cursor(column_names, statement, service_.get(),
-                        favicon_service);
-    cursor.set_test_observer(this);
-    JNIEnv* env = base::android::AttachCurrentThread();
-    EXPECT_EQ(1, cursor.GetCount(env, NULL));
-    EXPECT_EQ(0, cursor.MoveTo(env, NULL, 0));
-    EXPECT_EQ(row.url().spec(), base::android::ConvertJavaStringToUTF8(
-        cursor.GetString(env, NULL, 0)).c_str());
-    EXPECT_EQ(history::ToDatabaseTime(row.last_visit_time()),
-              cursor.GetLong(env, NULL, 1));
-    EXPECT_EQ(row.visit_count(), cursor.GetInt(env, NULL, 2));
-    base::android::ScopedJavaLocalRef<jbyteArray> data =
-        cursor.GetBlob(env, NULL, 3);
-    std::vector<uint8> out;
-    base::android::JavaByteArrayToByteVector(env, data.obj(), &out);
-    EXPECT_EQ(data_bytes->data().size(), out.size());
-    EXPECT_EQ(data_bytes->data()[0], out[0]);
-  }
-
-  // Cursor's destructor post the task in UI thread, run Message loop to release
-  // the statement etc.
+  SQLiteCursor* cursor = new SQLiteCursor(column_names, statement,
+      service_.get(), favicon_service);
+  cursor->set_test_observer(this);
+  JNIEnv* env = base::android::AttachCurrentThread();
+  EXPECT_EQ(1, cursor->GetCount(env, NULL));
+  EXPECT_EQ(0, cursor->MoveTo(env, NULL, 0));
+  EXPECT_EQ(row.url().spec(), base::android::ConvertJavaStringToUTF8(
+      cursor->GetString(env, NULL, 0)).c_str());
+  EXPECT_EQ(history::ToDatabaseTime(row.last_visit_time()),
+      cursor->GetLong(env, NULL, 1));
+  EXPECT_EQ(row.visit_count(), cursor->GetInt(env, NULL, 2));
+  base::android::ScopedJavaLocalRef<jbyteArray> data =
+      cursor->GetBlob(env, NULL, 3);
+  std::vector<uint8> out;
+  base::android::JavaByteArrayToByteVector(env, data.obj(), &out);
+  EXPECT_EQ(data_bytes->data().size(), out.size());
+  EXPECT_EQ(data_bytes->data()[0], out[0]);
+  cursor->Destroy(env, NULL);
+  // Cursor::Destroy posts the task in UI thread, run Message loop to release
+  // the statement, delete SQLiteCursor itself etc.
   content::RunAllPendingInMessageLoop();
 }