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

chrome/browser/renderer_host/download_resource_handler.cc

 // Copyright (c) 2011 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/renderer_host/download_resource_handler.h"
 
 #include <string>
 
 #include "base/logging.h"
 #include "base/metrics/histogram.h"
 #include "base/metrics/stats_counters.h"
 #include "base/stringprintf.h"
 #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"
 #include "content/browser/renderer_host/global_request_id.h"
 #include "content/browser/renderer_host/resource_dispatcher_host.h"
 #include "content/browser/renderer_host/resource_dispatcher_host_request_info.h"
 #include "content/common/resource_response.h"
 #include "net/base/io_buffer.h"
 #include "net/http/http_response_headers.h"
 #include "net/url_request/url_request_context.h"
@@ skipping 44 matching lines @@
   download_start_time_ = base::TimeTicks::Now();
   std::string content_disposition;
   request_->GetResponseHeaderByName("content-disposition",
                                     &content_disposition);
   set_content_disposition(content_disposition);
   set_content_length(response->response_head.content_length);
 
   const ResourceDispatcherHostRequestInfo* request_info =
     ResourceDispatcherHost::InfoForRequest(request_);
 
-  download_id_ = download_file_manager_->GetNextId();
 
   // Deleted in DownloadManager.
   DownloadCreateInfo* info = new DownloadCreateInfo;
   info->url_chain = request_->url_chain();
   info->referrer_url = GURL(request_->referrer());
   info->start_time = base::Time::Now();
   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,
                                                render_view_id_,
                                                global_id_.request_id);
   info->content_disposition = content_disposition_;
   info->mime_type = response->response_head.mime_type;
   // TODO(ahendrickson) -- Get the last modified time and etag, so we can
   // resume downloading.
 
   std::string content_type_header;
   if (!response->response_head.headers ||
       !response->response_head.headers->GetMimeType(&content_type_header))
     content_type_header = "";
   info->original_mime_type = content_type_header;
 
   info->prompt_user_for_save_location =
       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.
   rdh_->PauseRequest(global_id_.child_id, global_id_.request_id, true);
 
   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) {
   return true;
 }
 
 // Create a new buffer, which will be handed to the download thread for file
 // writing and deletion.
 bool DownloadResourceHandler::OnWillRead(int request_id, net::IOBuffer** buf,
                                          int* buf_size, int min_size) {
@@ skipping 122 matching lines @@
                             " render_view_id_ = " "%d"
                             " save_info_.file_path = \"%" PRFilePath "\""
                             " }",
                             request_->url().spec().c_str(),
                             download_id_,
                             global_id_.child_id,
                             global_id_.request_id,
                             render_view_id_,
                             save_info_.file_path.value().c_str());
 }