sql::MetaTable.next_download_id, DownloadManager::GetNextId()

chrome/browser/renderer_host/download_resource_handler.cc

Index: chrome/browser/renderer_host/download_resource_handler.cc
diff --git a/chrome/browser/renderer_host/download_resource_handler.cc b/chrome/browser/renderer_host/download_resource_handler.cc
index f35d5e4166ce7a127ce62a813137ecc2081b24e6..53194d40eed5fe0985616c763edc4976f3cce10b 100644
--- a/chrome/browser/renderer_host/download_resource_handler.cc
+++ b/chrome/browser/renderer_host/download_resource_handler.cc
@@ -13,6 +13,8 @@
 #include "chrome/browser/download/download_create_info.h"
 #include "chrome/browser/download/download_item.h"
 #include "chrome/browser/download/download_file_manager.h"
+#include "chrome/browser/download/download_manager.h"
+#include "chrome/browser/download/download_prefs.h"
 #include "chrome/browser/download/download_request_handle.h"
 #include "chrome/browser/download/download_util.h"
 #include "content/browser/browser_thread.h"
@@ -77,7 +79,6 @@ bool DownloadResourceHandler::OnResponseStarted(int request_id,
   const ResourceDispatcherHostRequestInfo* request_info =
     ResourceDispatcherHost::InfoForRequest(request_);
 
-  download_id_ = download_file_manager_->GetNextId();
 
   // Deleted in DownloadManager.
   DownloadCreateInfo* info = new DownloadCreateInfo;
@@ -87,7 +88,6 @@ bool DownloadResourceHandler::OnResponseStarted(int request_id,
   info->received_bytes = 0;
   info->total_bytes = content_length_;
   info->state = DownloadItem::IN_PROGRESS;
-  info->download_id = download_id_;
   info->has_user_gesture = request_info->has_user_gesture();
   info->request_handle = DownloadRequestHandle(rdh_,
                                                global_id_.child_id,
@@ -108,10 +108,8 @@ bool DownloadResourceHandler::OnResponseStarted(int request_id,
       save_as_ && save_info_.file_path.empty();
   info->referrer_charset = request_->context()->referrer_charset();
   info->save_info = save_info_;
-  BrowserThread::PostTask(
-      BrowserThread::UI, FROM_HERE,
-      NewRunnableMethod(
-          download_file_manager_, &DownloadFileManager::StartDownload, info));
+  BrowserThread::PostTask(BrowserThread::UI, FROM_HERE, NewRunnableMethod(
+      this, &DownloadResourceHandler::StartDownloadOnUIThread, info));
 
   // We can't start saving the data before we create the file on disk.
   // The request will be un-paused in DownloadFileManager::CreateDownloadFile.
@@ -120,6 +118,16 @@ bool DownloadResourceHandler::OnResponseStarted(int request_id,
   return true;
 }
 
+void DownloadResourceHandler::StartDownloadOnUIThread(
+    DownloadCreateInfo* info) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  download_id_ = info->request_handle.GetDownloadManager()->download_prefs()->

[Randy Smith (Not in Mondays), 2011/07/10 23:49:55]

I'm concerned about this line--it's setting a value on the UI thread that'll be
referenced on the IO thread, quite possibly in a racy way.  Can we come up with
a way of setting the download_id_ for the DownloadResourceHandler() that doesn't
require this kind of cross thread race concern?    Even if we can convince
ourselves that the assignment will always be strictly before the reading, it's
fragile code.  

[benjhayden, 2011/07/11 15:57:50]

Ah, I see it now.
The primary reason that I moved GetNextId to the UI thread is because
GetDownloadManager() is on the UI thread.
I could move GetNextId back to the IO thread if I make the
ResourceDispatcherHost pass a DownloadPrefs into DownloadResourceHander along
with the DownloadFileManager, or if I attach a DownloadPrefs to the
DownloadFileManager. Which would you prefer, or is there a better alternative?

[Randy Smith (Not in Mondays), 2011/07/12 18:12:52]

I think DownloadPrefs is a UI thread object, and I'd be reluctant to change that
without a strong reason.  But digging into this issue has a bit of a tugging on
a string feeling (as is appropriate for threading issues :-J):
* If we require an atomic update of the prefs on each GetNextID() we must do it
on the UI thread (PrefService is definitely a UI thread object).
* If we don't require an atomic updates of the prefs (e.g. just update every n
seconds, or at browser shutdown) we can allow allocation of the ID on any thread
we want, or on all threads via the atomic increment idea.
* But in that case, we don't persist the id in the event of a browser crash,
which sounds bad (we'll have multiple downloads allocated with the same id).  
* But even if we do atomically update the prefs on each GetNextID(), are we
guaranteed that that update will make it out to the file system in the event of
a crash?  I'm guessing not.  Which means we need to deal with the desyncing of
the DB from the browser pref *anyway*.  

So I think what this means is that we need to figure out what guarantees the
pref service makes about persistence of preferences, and figure out what
implications that has for the system, then we can come back to the threading
issue.  Does that make sense?  

I don't know how to do the prefs persistence evaluation--I'd either trace the
code or use git blame to get some names of people to ask about it--so I think it
makes sense for you to start that process.  But please grab me if it gets bogged
down.  And definitely grab me if any of this doesn't sound right to you
(possibly we should sync up tomorrow anyway if you're in--this is getting
complicated).

+    GetNextId();
+  VLOG(20) << __PRETTY_FUNCTION__ << " " << download_id_;
+  info->download_id = download_id_;
+  download_file_manager_->StartDownload(info);
+}
+
 bool DownloadResourceHandler::OnWillStart(int request_id,
                                           const GURL& url,
                                           bool* defer) {