Fix for crash when deleting DataSource's in IO thread.

Fix for crash when deleting |DataSource|s on the IO thread.

Although |ChromeURLDataManager::DataSource|s are |RefCountedThreadSafe|, at least two derived classes contain |CancellationFlag| members, which check that they are deleted in the same thread as they are created.

Normally, the |DataSource|s are destructed when |ChromeURLDataManager| (instanced via a Singleton) is destroyed, on the main thread.  This is not an issue as the classes with a |CancellationFlag| are created on that thread too.

However, occasionally (see the bug) |AddDataSource()| is called when there is already an entry in |data_sources_|, leading to a |DataSource| being released and destructed in the IO thread.

To fix this problem, I've incremented the reference count and posted a task to the main (UI) thread that will release the reference and so destruct it there.

BUG=49121
TEST=none

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

[ahendrickson, 2011-01-18 16:50:19]

Ping.

[eroman, 2011-01-19 22:47:01]

This approach is fragile -- final reference to a refcounted class can be
released from many places and this could easily break in the future.

The general way to solve this is to by passing a trait to BaseRefCounted which
controls the deletion (by posting it to another thread if necessary).

For instance, you can declare DataSource as:

class DataSource : public base::RefCountedThreadSafe<DataSource,
BrowserThread::DeleteOnUIThread> { ... }

This will guarantee that the destructor is only called from the UI thread.

That should work and "fix" this bug.

However be aware that in general sharing a reference-counted datastructure
between the UI and IO thread is a broken paradigm ... which unfortunately we use
in other places in Chrome too.

Specifically consider what happens at shutdown: if the final reference is
released from the IO thread, then necessarily we post a task to the UI thread to
delete it. HOWEVER, since we are in shutdown, the UI message loop is being
stopped and subsequent tasks that were posted to it will not be run. Hence we
leak those tasks, and will leak the DataSource. Sadly we have this class of bug
in other places in Chrome too.

http://codereview.chromium.org/6285002/diff/2001/chrome/browser/dom_ui/chrome...
File chrome/browser/dom_ui/chrome_url_data_manager.cc (right):

http://codereview.chromium.org/6285002/diff/2001/chrome/browser/dom_ui/chrome...
chrome/browser/dom_ui/chrome_url_data_manager.cc:8: #include
"base/debug/stack_trace.h"
Is this intentional? Please remove.

[ahendrickson, 2011-01-21 16:04:31]

http://codereview.chromium.org/6285002/diff/2001/chrome/browser/dom_ui/chrome...
File chrome/browser/dom_ui/chrome_url_data_manager.cc (right):

http://codereview.chromium.org/6285002/diff/2001/chrome/browser/dom_ui/chrome...
chrome/browser/dom_ui/chrome_url_data_manager.cc:8: #include
"base/debug/stack_trace.h"
On 2011/01/19 22:47:01, eroman wrote:
> Is this intentional? Please remove.

Cleanup fail.

Removed.

[eroman, 2011-01-24 18:26:39]

lgtm

http://codereview.chromium.org/6285002/diff/8001/chrome/browser/dom_ui/chrome...
File chrome/browser/dom_ui/chrome_url_data_manager.cc (right):

http://codereview.chromium.org/6285002/diff/8001/chrome/browser/dom_ui/chrome...
chrome/browser/dom_ui/chrome_url_data_manager.cc:203: // |DataSource| now uses
the |DeleteOnUIThread| trait.
I don't like this comment, in particular how it says "now uses".

Comments written about how things used to be don't make sense in the future.

Please remove or reword this.

http://codereview.chromium.org/6285002/diff/8001/chrome/browser/dom_ui/chrome...
File chrome/browser/dom_ui/chrome_url_data_manager.h (right):

http://codereview.chromium.org/6285002/diff/8001/chrome/browser/dom_ui/chrome...
chrome/browser/dom_ui/chrome_url_data_manager.h:106: };
what happened to the indentation here? looks off.

[ahendrickson, 2011-01-24 19:58:20]

http://codereview.chromium.org/6285002/diff/8001/chrome/browser/dom_ui/chrome...
File chrome/browser/dom_ui/chrome_url_data_manager.cc (right):

http://codereview.chromium.org/6285002/diff/8001/chrome/browser/dom_ui/chrome...
chrome/browser/dom_ui/chrome_url_data_manager.cc:203: // |DataSource| now uses
the |DeleteOnUIThread| trait.
On 2011/01/24 18:26:39, eroman wrote:
> I don't like this comment, in particular how it says "now uses".
> 
> Comments written about how things used to be don't make sense in the future.
> 
> Please remove or reword this.

Done.

http://codereview.chromium.org/6285002/diff/8001/chrome/browser/dom_ui/chrome...
File chrome/browser/dom_ui/chrome_url_data_manager.h (right):

http://codereview.chromium.org/6285002/diff/8001/chrome/browser/dom_ui/chrome...
chrome/browser/dom_ui/chrome_url_data_manager.h:106: };
On 2011/01/24 18:26:39, eroman wrote:
> what happened to the indentation here? looks off.

Fixed.  Didn't notice when Visual Studio reformatted.

[Nico, 2011-01-25 18:29:14]

Hi guys, it looks like this caused many memory leaks ( http://crbug.com/70782 ).
I guess leaks are better than races, so unless I hear otherwise, I won't revert
this.

[ahendrickson, 2011-01-25 19:05:59]

I'm on it.

On 2011/01/25 18:29:14, Nico wrote:
> Hi guys, it looks like this caused many memory leaks ( http://crbug.com/70782
).
> I guess leaks are better than races, so unless I hear otherwise, I won't
revert
> this.