DownloadId

Retry r100017. This time, prevent a potential shutdown race by moving the call to DownloadManager.Shutdown() from ExitedOffTheRecordMode() to ~OTRProfileImpl() to match ProfileImpl and guarantee that Shutdown() is called.

Make a new integer field in sql::MetaTable (a per-profile db) containing a counter for the next download id, so that this id is unique across sessions. This id will allow us to merge download id with db_handle and merge most/all of the maps in DownloadManager in future CLs.

Make DownloadManager read this field to initialize its next_id_ counter in Init().

Put a fine-grained mutex in DownloadManager::GetNextId() so that it can be called directly from any thread.
Define a thunk wrapping DM::GNI() to be passed around between threads to guard against other threads calling any other DM methods.

This thunk owns a scoped_refptr<DM> to manage life-time issues. This pattern is implemented for DM elsewhere.
Store this thunk in ResourceContext to be called by ResourceDispatchHost/DownloadThrottlingResourceHandler on the IO thread. Pass the returned DownloadId into DownloadResourceHandler.

The alternative way to obtain ids on the IO thread is to jump over to the UI thread and back. This way would add significant latency to a critical path. GetNextId() should be fast and easily accessible from any thread.

Now that ids are per-profile, define a class DownloadId containing a per-profile id and an indication of which profile, currently the DownloadManager*. DownloadIds are hashable, comparable, globally unique, not persistent, and are used by DownloadFileManager.

When the download is added to the history, MetaTable.next_download_id will be set to the new download's id +1 if that number is greater than MT.next_download_id. Increasing this counter at the same time as the download is added to the db prevents the counter from desyncing from the db, which was the primary concern re storing the counter in the BrowserPrefs.

Owners:
Randy: downloads/* (no change from 7776012)
(Done) John: content/browser/{renderer_host/,resource_context.}* (no change from 7776012)
(Done) Will, Miranda: profiles/* (small bugfix)

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=102149

[benjhayden, 2011-09-09 14:24:51]

Hi Will,
Retrying 7776012/r100017. It was reverted because of the stack trace below. It
appears that either the OTRProfileImpl is being deleted without running
ExitedOffTheRecordMode() or else its IOData is being initialized after
ExitedOffTheRecordMode() runs, causing the DownloadManager to not be Shutdown()
properly. I'm unable to repro locally or on the try bots, so this is a bit of a
guess. The 'Callback' in the trace is ProfileIOData::next_download_id_thunk_,
which holds a scoped_refptr to the DownloadManager.

You can just look at the delta between patches 1 and 2. I've added a boolean
OTRProfileImpl::is_shutdown_ to indicate whether ExitedOffTheRecordMode() has
been called, in which case GetDownloadManager() will refuse to create a
DownloadManager. This protects against the case where the OTR IOData is
initialized after EOTRM(). I could try to call EOTRM() in ~OTRProfileImpl() to
protect against the case where EOTRM() is never called, but I don't know if the
timing would cause wrinkles elsewhere.

Thank you again for your time.

NOTE(self): Update all the callers of GetDownloadManager() to allow it to return
NULL, and make EOTRM() idempotent if I call it from ~OTRProfileImpl.


[ RUN      ] ProfileManagerTest.CreateProfileAsyncMultipleRequests
[1833:1833:0907/164017:107215722:ERROR:input_method_util.cc(708)]
intl.hardware_keyboard is not registered
[1833:1833:0907/164017:107219696:WARNING:network_library.cc(4807)]
NetworkLibrary: falling back to stub impl.
[1833:1833:0907/164017:107234541:FATAL:download_manager.cc(71)] Check failed:
!shutdown_needed_. 
Backtrace:
	base::debug::StackTrace::StackTrace() [0x2a781ba]
	logging::LogMessage::~LogMessage() [0x2a9ab56]
	DownloadManager::~DownloadManager() [0x42ee97f]
	BrowserThread::DeleteOnThread<>::Destruct<>() [0x7ff5d9]
	base::RefCountedThreadSafe<>::Release() [0x7fe356]
	base::internal::MaybeRefcount<>::Release() [0x42fbd7a]
	base::internal::InvokerStorage1<>::~InvokerStorage1() [0x42fb942]
	base::RefCountedThreadSafe<>::DeleteInternal() [0x60df82]
	base::DefaultRefCountedThreadSafeTraits<>::Destruct() [0x60d960]
	base::RefCountedThreadSafe<>::Release() [0x60cdb4]
	scoped_refptr<>::~scoped_refptr() [0x60c4df]
	base::internal::CallbackBase::~CallbackBase() [0x2a75f32]
	base::Callback<>::~Callback() [0x19596a8]
	ProfileIOData::~ProfileIOData() [0x1956cf3]
	OffTheRecordProfileIOData::~OffTheRecordProfileIOData() [0x1ebec28]
	ProfileIOData::ShutdownOnUIThread() [0x1958f7d]
	OffTheRecordProfileIOData::Handle::~Handle() [0x1ebe36e]
	OffTheRecordProfileImpl::~OffTheRecordProfileImpl() [0x1ebb5aa]
	scoped_ptr<>::~scoped_ptr() [0x8b882b]
	ProfileImpl::~ProfileImpl() [0x194466b]
	scoped_ptr<>::~scoped_ptr() [0x8b882b]
	ProfileManager::ProfileInfo::~ProfileInfo() [0x1962c12]
	linked_ptr<>::depart() [0x19640a1]
	linked_ptr<>::~linked_ptr() [0x19632de]
	std::pair<>::~pair() [0x1962e02]
	__gnu_cxx::new_allocator<>::destroy() [0x1965e7c]
	std::_Rb_tree<>::_M_destroy_node() [0x1964d9f]
	std::_Rb_tree<>::_M_erase() [0x196382b]
	std::_Rb_tree<>::~_Rb_tree() [0x1962f1e]
	std::map<>::~map() [0x1962dca]
	ProfileManager::~ProfileManager() [0x195fdfb]
	ProfileManagerWithoutInit::~ProfileManagerWithoutInit() [0x1967934]
	scoped_ptr<>::reset() [0xc28e6c]
	ProfileManagerTest::TearDown() [0xc2863b]
	testing::internal::HandleSehExceptionsInMethodIfSupported<>() [0x23e52f1]
	testing::internal::HandleExceptionsInMethodIfSupported<>() [0x23e29e8]
	testing::Test::Run() [0x23d7de7]
	testing::TestInfo::Run() [0x23d85c2]
	testing::TestCase::Run() [0x23d8cb8]
	testing::internal::UnitTestImpl::RunAllTests() [0x23dda8f]
	testing::internal::HandleSehExceptionsInMethodIfSupported<>() [0x23e6118]
	testing::internal::HandleExceptionsInMethodIfSupported<>() [0x23e3197]
	testing::UnitTest::Run() [0x23dc5ac]
	base::TestSuite::Run() [0x4885be7]
	main [0x24cca0a]

[benjhayden, 2011-09-09 18:57:22]

Hi Pawel,
I'm hunting down a possible race in OffTheRecordProfileImpl wrt DownloadManager
creation, destruction, and Shutdown.
I see in rev 59139 that you chose to shutdown the download manager in
ExitedOffTheRecordMode() (called from OnBrowserRemoved()) instead of
OTRProfileImpl's destructor, as ProfileImpl does. Would you mind commenting why?
I think that shutting down the download manager in ~OTRProfileImpl instead of
ExitedOTRMode will prevent a race between ProfileIOData::InitializeOnUIThread
(which calls GetDownloadManager in this change) and ExitedOTRMode.

[Randy Smith (Not in Mondays), 2011-09-09 20:55:59]

Quick note while we're tracking down the larger issues.

http://codereview.chromium.org/7847027/diff/34/content/browser/download/downl...
File content/browser/download/download_manager.cc (right):

http://codereview.chromium.org/7847027/diff/34/content/browser/download/downl...
content/browser/download/download_manager.cc:140: shutdown_needed_ = false;
Why eliminate this line?  My memory is that we rely on it elsewhere in the file
...

[benjhayden, 2011-09-09 21:13:32]

Pawels says the he doesn't know who null'd the dm in eotrm or why, he just added
the Shutdown().

http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/profiles/off_t...
says
initial.co	15	mit

http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/profiles/off_t...
returns 400

It feels like I'm chasing a ghost, so I think I'll just try moving the
shutdown/null to the destructor and see if anything explodes.

http://codereview.chromium.org/7847027/diff/34/content/browser/download/downl...
File content/browser/download/download_manager.cc (right):

http://codereview.chromium.org/7847027/diff/34/content/browser/download/downl...
content/browser/download/download_manager.cc:140: shutdown_needed_ = false;
On 2011/09/09 20:56:00, rdsmith wrote:
> Why eliminate this line?  My memory is that we rely on it elsewhere in the
file
> ...

It's redundant to L81.
I'd rather keep the one on L81 because it's closer to its usage on L79.

[willchan no longer on Chromium, 2011-09-09 21:14:25]

Sounds fine to me. Let me know when it's ready for review (is it already?).

On Fri, Sep 9, 2011 at 2:13 PM,  <benjhayden@chromium.org> wrote:
> Pawels says the he doesn't know who null'd the dm in eotrm or why, he just
> added
> the Shutdown().
>
>
http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/profiles/off_t...
> says
> initial.co      15      mit
>
>
http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/profiles/off_t...
> returns 400
>
> It feels like I'm chasing a ghost, so I think I'll just try moving the
> shutdown/null to the destructor and see if anything explodes.
>
>
>
http://codereview.chromium.org/7847027/diff/34/content/browser/download/downl...
> File content/browser/download/download_manager.cc (right):
>
>
http://codereview.chromium.org/7847027/diff/34/content/browser/download/downl...
> content/browser/download/download_manager.cc:140: shutdown_needed_ =
> false;
> On 2011/09/09 20:56:00, rdsmith wrote:
>>
>> Why eliminate this line?  My memory is that we rely on it elsewhere in
>
> the file
>>
>> ...
>
> It's redundant to L81.
> I'd rather keep the one on L81 because it's closer to its usage on L79.
>
> http://codereview.chromium.org/7847027/
>

[benjhayden, 2011-09-12 20:58:36]

Hi again, Miranda,
Randy and I would appreciate your thoughts on this change to
off_the_record_profile_impl.cc along with Will's and Pawel's. The potential race
that I'm trying to address is evidenced by the traceback in the first Message of
this CL; the last scoped_refptr<DownloadManager> is being reaped before/without
running DM.Shutdown().

Will, Pawel, Randy, PTAL.

Thank you all for your time.

[willchan no longer on Chromium, 2011-09-12 22:09:41]

LGTM

http://codereview.chromium.org/7847027/diff/17001/chrome/browser/profiles/off...
File chrome/browser/profiles/off_the_record_profile_impl.cc (right):

http://codereview.chromium.org/7847027/diff/17001/chrome/browser/profiles/off...
chrome/browser/profiles/off_the_record_profile_impl.cc:535: virtual void
ExitedOffTheRecordMode() {
Can we get rid of this? Or is it still needed?

[Miranda Callahan, 2011-09-13 08:51:43]

On 2011/09/12 22:09:41, willchan wrote:
> LGTM
> 
>
http://codereview.chromium.org/7847027/diff/17001/chrome/browser/profiles/off...
> File chrome/browser/profiles/off_the_record_profile_impl.cc (right):
> 
>
http://codereview.chromium.org/7847027/diff/17001/chrome/browser/profiles/off...
> chrome/browser/profiles/off_the_record_profile_impl.cc:535: virtual void
> ExitedOffTheRecordMode() {
> Can we get rid of this? Or is it still needed?

The OTR profile change makes sense to me -- agree with willchan that you can now
get rid of "ExitedOffTheRecordMode()" (and the contents of the
"OnBrowserRemoved" method).

[Miranda Callahan, 2011-09-13 08:51:59]

On 2011/09/13 08:51:43, Miranda Callahan wrote:
> On 2011/09/12 22:09:41, willchan wrote:
> > LGTM
> > 
> >
>
http://codereview.chromium.org/7847027/diff/17001/chrome/browser/profiles/off...
> > File chrome/browser/profiles/off_the_record_profile_impl.cc (right):
> > 
> >
>
http://codereview.chromium.org/7847027/diff/17001/chrome/browser/profiles/off...
> > chrome/browser/profiles/off_the_record_profile_impl.cc:535: virtual void
> > ExitedOffTheRecordMode() {
> > Can we get rid of this? Or is it still needed?
> 
> The OTR profile change makes sense to me -- agree with willchan that you can
now
> get rid of "ExitedOffTheRecordMode()" (and the contents of the
> "OnBrowserRemoved" method).

Oh yeah -- LGTM! :-)

[benjhayden, 2011-09-14 19:25:42]

http://codereview.chromium.org/7847027/diff/17001/chrome/browser/profiles/off...
File chrome/browser/profiles/off_the_record_profile_impl.cc (right):

http://codereview.chromium.org/7847027/diff/17001/chrome/browser/profiles/off...
chrome/browser/profiles/off_the_record_profile_impl.cc:535: virtual void
ExitedOffTheRecordMode() {
On 2011/09/12 22:09:41, willchan wrote:
> Can we get rid of this? Or is it still needed?

Done.

[benjhayden, 2011-09-19 17:55:06]

Hi, John, would you mind rubber-stamping this? It's a reland of 7776012 (aka
7237034) with a small bugfix in profiles/.

[jam, 2011-09-19 18:26:54]

rubberstamp lgtm

[commit-bot: I haz the power, 2011-09-19 21:15:03]

Can't apply patch for file chrome/browser/history/download_database.h.
While running patch -p1 --forward --force;
patching file chrome/browser/history/download_database.h
Hunk #1 FAILED at 7.
Hunk #2 succeeded at 27 (offset 3 lines).
Hunk #3 FAILED at 65.
2 out of 3 hunks FAILED -- saving rejects to file
chrome/browser/history/download_database.h.rej

[commit-bot: I haz the power, 2011-09-21 19:32:05]

Change committed as 102149