Speculative fix for crash in DOMUIThumbnailSource.

Speculative fix for crash in DOMUIThumbnailSource.

DOMUIThumbnailSource objects are deleted on the IO thread, while they are used
on the UI thread. The DataSource documentation says that they should not live
on the IO thread, but for almost all DataSources, the only reference held is
the one by ChromeURLDataManager, which lives on the IO thread. Since I had a racy
stack where the object was being used on the UI thread while its destructor was
running on the IO thread, forcing destruction on the UI thread should fix
the crash.

Pretty much everything but changing the templated base class of DataSource to
always DeleteOnUIThread is my usual cleanup.

BUG=34115
TEST=none

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

[Elliot Glaysher, 2010-07-26 21:20:27]

ping?

[Elliot Glaysher, 2010-07-27 00:52:52]

Can you review this, tony? (The other person who's worked around here, according
to git)

[tony, 2010-07-27 16:55:17]

I am confused by how the destructor was running while code was running on the UI
thread.  The code that runs on the UI thread, if it is from a Task, doesn't it
automatically get a ref count?  Or was it that some other code called a method
on DOMUIThumbnailSource while already on the UI thread?  Maybe we could use a
PostTask to make sure that the code has a ref while running?

http://codereview.chromium.org/3061009/diff/1/3
File base/ref_counted_memory.cc (right):

http://codereview.chromium.org/3061009/diff/1/3#newcode7
base/ref_counted_memory.cc:7: RefCountedMemory::~RefCountedMemory() {
Nit: I think if this is in the header, subclasses don't need a vtable since the
function is empty.  Maybe.  I think darin often suggests this?  I don't have a
strong opinion either way.

http://codereview.chromium.org/3061009/diff/1/4
File base/ref_counted_memory.h (left):

http://codereview.chromium.org/3061009/diff/1/4#oldcode42
base/ref_counted_memory.h:42: virtual const unsigned char* front() const {
return data_; }
Can you explain to me why it's better to put these simple getters in the .cc
file?

http://codereview.chromium.org/3061009/diff/1/8
File chrome/browser/dom_ui/dom_ui_favicon_source.h (right):

http://codereview.chromium.org/3061009/diff/1/8#newcode40
chrome/browser/dom_ui/dom_ui_favicon_source.h:40: virtual ~DOMUIFavIconSource();
Yeah, I would ask darin about these.  I think we get better perf for inlining
empty virtual destructors.

http://codereview.chromium.org/3061009/diff/1/12
File chrome/browser/dom_ui/dom_ui_thumbnail_source.h (right):

http://codereview.chromium.org/3061009/diff/1/12#newcode10
chrome/browser/dom_ui/dom_ui_thumbnail_source.h:10: #include <vector>
Nit: I think you can remove vector and utility now.

[Elliot Glaysher, 2010-07-27 17:39:25]

> I am confused by how the destructor was running while code was running on the
UI
> thread.  The code that runs on the UI thread, if it is from a Task, doesn't it
> automatically get a ref count?

You are thinking about RunnableMethod (base/task.h). RunnableMethod
automatically adds refcounts. CancelableRequest does not (it's sort of the point
that the owner of a CancelableRequest can be destructed before the task
completes). CancelableRequests are not tasks, but another parallel callback
system in our code.

> Maybe we could use a
> PostTask to make sure that the code has a ref while running?

That was my first changelist for this. I still have it, but it's more
complicated and I want the simplest thing that could possibly work.

I will talk to Darin about the virtual empty destructors. In general, I don't
think people are making conscious decisions about what is inlined (see virtual
getters, and other large methods!). Not inlining anything would be a better
default, at least from a .o and compile time perspective.

As always,
http://www.parashift.com/c++-faq-lite/inline-functions.html#faq-9.3

http://codereview.chromium.org/3061009/diff/1/4
File base/ref_counted_memory.h (left):

http://codereview.chromium.org/3061009/diff/1/4#oldcode42
base/ref_counted_memory.h:42: virtual const unsigned char* front() const {
return data_; }
On 2010/07/27 16:55:17, tony wrote:
> Can you explain to me why it's better to put these simple getters in the .cc
> file?

Because it's virtual, isn't a simple getter, and will almost never be inlined.
Inlining here can only happen by explicit invocation
("RefCountedStaticMemory::font()") or by having the complete object in scope
(RefCountedStaticMemory on the stack or as a full member of the class), which
will never happen since this isa RefCountedThreadSafe.

[Elliot Glaysher, 2010-07-27 18:11:39]

Talked to Darin:

http://codereview.chromium.org/3061009/diff/1/3
File base/ref_counted_memory.cc (right):

http://codereview.chromium.org/3061009/diff/1/3#newcode7
base/ref_counted_memory.cc:7: RefCountedMemory::~RefCountedMemory() {
On 2010/07/27 16:55:17, tony wrote:
> Nit: I think if this is in the header, subclasses don't need a vtable since
the
> function is empty.  Maybe.  I think darin often suggests this?  I don't have a
> strong opinion either way.

You are correct. I talked to Darin and in the case of a totally empty base class
with no data members, there are optimizations the compiler can do with inlined
virtual destructors.

http://codereview.chromium.org/3061009/diff/1/8
File chrome/browser/dom_ui/dom_ui_favicon_source.h (right):

http://codereview.chromium.org/3061009/diff/1/8#newcode40
chrome/browser/dom_ui/dom_ui_favicon_source.h:40: virtual ~DOMUIFavIconSource();
But this case is different, because this isn't a base class, and more
importantly, isn't an empty destructor. This definitely shouldn't be in the
interface file--we could forward declare RefCountedMemory if it wasn't already
included in a dependent header.

[tony, 2010-07-27 18:47:22]

LGTM.

I think it would be better to leave all empty virtual destructors in header
files so the compiler can optimize (and so it's easier to remember whether to do
this or not), but I don't think the time difference is measurable.

http://codereview.chromium.org/3061009/diff/10001/11015
File chrome/browser/dom_ui/new_tab_ui.h (right):

http://codereview.chromium.org/3061009/diff/10001/11015#newcode70
chrome/browser/dom_ui/new_tab_ui.h:70: virtual ~NewTabHTMLSource();
Nit: This seems safe to put the empty body here.