Attempt 3 at: Splits ChromeURLDataManager into 2 chunks:...

Attempt 3 at: Splits ChromeURLDataManager into 2 chunks:
. ChromeURLDataManager is no longer a singleton and is always used on
  the UI thread. ChromeURLDataManager is now profile specific (you get
  from the profile).
. ChromeURLDataManagerBackend handles the URLRequests and the
  DataSources. ChromeURLDataManagerBackend is created by
  ChromeURLRequestContext.

All DataSources are now profile specific. There were two that wanted
to be global, but have been converted.

This differs from the last version in that DataSource::SendResponse
does nothing if the DataSource is schedule for deletion. This is
necessary otherwise SendResponse will up the ref count and by the time
the request is processed on the IO thread chances are the DataSource
will have been deleted. And if it wasn't deleted, it'll get scheduled
for deletion again. <insert comment here about the perils of using
delayed deletion> This has always been possible, it just appears to be
more likely with my patch.

BUG=52022
71868
TEST=none


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

[sky, 2011-02-10 03:25:23]

I'm hoping 3 is the lucky number!

  -Scott

[sky, 2011-02-10 03:30:01]

Forgot to mention, patchset 1 is last patchset you reviewed.

This crash was being triggered because between the time the DataSource was
scheduled for deletion and the time we did the actual deletion SendResponse was
being invoked. SendResponse does an AddRef so that we end up deleting the
DataSource and still trying to use it. Oi! Yes, delayed deletion is painful.

  -Scott

[willchan no longer on Chromium, 2011-02-10 03:51:36]

LGTM, even though it's ugly.

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

http://codereview.chromium.org/6479007/diff/7012/chrome/browser/dom_ui/chrome...
chrome/browser/dom_ui/chrome_url_data_manager.cc:122: if
(IsScheduledForDeletion(this)) {
If we're scheduled for deletion, that means the refcount is 0. How is
SendResponse() getting invoked? This is a bug. I realize you didn't cause this
bug, but we should at least document it (file a bug for it). If you have any
idea which DataSource is being evil (someone must be holding a raw pointer),
then please note that down in the bug.

I'm OK with this hack, but we should definitely file a bug to fix this. Please
reference the bug here.

[Lei Zhang, 2011-03-04 12:30:55]

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

http://codereview.chromium.org/6479007/diff/5025/chrome/browser/dom_ui/chrome...
chrome/browser/dom_ui/chrome_url_data_manager.h:56: // data race.  The
|DeleteOnUIThread| trait is used to enforce this.
This comment is out of date. Can you fix?

[sky, 2011-03-04 16:22:06]

Certainly. http://codereview.chromium.org/6624019/ is sent your way.

  -Scott

On Fri, Mar 4, 2011 at 4:30 AM,  <thestig@chromium.org> wrote:
>
>
http://codereview.chromium.org/6479007/diff/5025/chrome/browser/dom_ui/chrome...
> File chrome/browser/dom_ui/chrome_url_data_manager.h (right):
>
>
http://codereview.chromium.org/6479007/diff/5025/chrome/browser/dom_ui/chrome...
> chrome/browser/dom_ui/chrome_url_data_manager.h:56: // data race.  The
> |DeleteOnUIThread| trait is used to enforce this.
> This comment is out of date. Can you fix?
>
> http://codereview.chromium.org/6479007/
>

[joth, 2011-07-25 11:39:04]

A late drive-by comment that I noticed whilst looking into something else
(http://codereview.chromium.org/7489044/)
Let me know if you'd prefer I open a bug for this?

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

http://codereview.chromium.org/6479007/diff/5025/chrome/browser/dom_ui/chrome...
chrome/browser/dom_ui/chrome_url_data_manager.cc:122: if
(IsScheduledForDeletion(this)) {
This doesn't seem quite sufficient? Between this function returning 'true' and
the PostTask being executed, another thread could still jump in decrement the
refcount, and schedule this data source for deletion.

Afraid I can't really propose any fix to that other than avoid SendResponse
being called on a data source when the reference maybe about to go away on it.

[sky, 2011-07-25 14:20:26]

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

http://codereview.chromium.org/6479007/diff/5025/chrome/browser/dom_ui/chrome...
chrome/browser/dom_ui/chrome_url_data_manager.cc:122: if
(IsScheduledForDeletion(this)) {
On 2011/07/25 11:39:04, joth wrote:
> This doesn't seem quite sufficient? Between this function returning 'true' and
> the PostTask being executed, another thread could still jump in decrement the
> refcount, and schedule this data source for deletion.
> 
> Afraid I can't really propose any fix to that other than avoid SendResponse
> being called on a data source when the reference maybe about to go away on it.

This is a problem with any delayed deletion.

I don't have a good idea on how to protect this either. In general the
DataSources are deleted on the same thread that SendResponse is going to be
invoked on, but there is nothing guaranteeing that. I also don't know if that's
true of all implementations.

[joth, 2011-07-25 14:58:36]

On 25 July 2011 15:20, <sky@chromium.org> wrote:

>
> http://codereview.chromium.**org/6479007/diff/5025/chrome/**
>
browser/dom_ui/chrome_url_**data_manager.cc<http://codereview.chromium.org/6479007/diff/5025/chrome/browser/dom_ui/chrome_url_data_manager.cc>
> File chrome/browser/dom_ui/chrome_**url_data_manager.cc (right):
>
> http://codereview.chromium.**org/6479007/diff/5025/chrome/**
>
browser/dom_ui/chrome_url_**data_manager.cc#newcode122<http://codereview.chromium.org/6479007/diff/5025/chrome/browser/dom_ui/chrome_url_data_manager.cc#newcode122>
> chrome/browser/dom_ui/chrome_**url_data_manager.cc:122: if
> (IsScheduledForDeletion(this)) {
> On 2011/07/25 11:39:04, joth wrote:
>
>> This doesn't seem quite sufficient? Between this function returning
>>
> 'true' and
>
>> the PostTask being executed, another thread could still jump in
>>
> decrement the
>
>> refcount, and schedule this data source for deletion.
>>
>
>  Afraid I can't really propose any fix to that other than avoid
>>
> SendResponse
>
>> being called on a data source when the reference maybe about to go
>>
> away on it.
>
> This is a problem with any delayed deletion.
>
> I don't have a good idea on how to protect this either. In general the
> DataSources are deleted on the same thread that SendResponse is going to
> be invoked on, but there is nothing guaranteeing that. I also don't know
> if that's true of all implementations.
>
>
> I think what confuses me is, if the refcount has reached zero then
logically there should be no 'owning' pointers (scoped_refptr) pointing at
this object. In which case the caller is calling SendResponse via a raw
pointer (either the object calling it on itself via 'this', or some other
raw pointer). Could this be the error? How could we detect it?

[sky, 2011-07-25 15:48:11]

On Mon, Jul 25, 2011 at 7:58 AM, Jonathan Dixon <joth@chromium.org> wrote:
>
>
> On 25 July 2011 15:20, <sky@chromium.org> wrote:
>>
>>
>>
http://codereview.chromium.org/6479007/diff/5025/chrome/browser/dom_ui/chrome...
>> File chrome/browser/dom_ui/chrome_url_data_manager.cc (right):
>>
>>
>>
http://codereview.chromium.org/6479007/diff/5025/chrome/browser/dom_ui/chrome...
>> chrome/browser/dom_ui/chrome_url_data_manager.cc:122: if
>> (IsScheduledForDeletion(this)) {
>> On 2011/07/25 11:39:04, joth wrote:
>>>
>>> This doesn't seem quite sufficient? Between this function returning
>>
>> 'true' and
>>>
>>> the PostTask being executed, another thread could still jump in
>>
>> decrement the
>>>
>>> refcount, and schedule this data source for deletion.
>>
>>> Afraid I can't really propose any fix to that other than avoid
>>
>> SendResponse
>>>
>>> being called on a data source when the reference maybe about to go
>>
>> away on it.
>>
>> This is a problem with any delayed deletion.
>>
>> I don't have a good idea on how to protect this either. In general the
>> DataSources are deleted on the same thread that SendResponse is going to
>> be invoked on, but there is nothing guaranteeing that. I also don't know
>> if that's true of all implementations.
>>
> I think what confuses me is, if the refcount has reached zero then logically
> there should be no 'owning' pointers (scoped_refptr) pointing at this
> object. In which case the caller is calling SendResponse via a raw pointer
> (either the object calling it on itself via 'this', or some other raw
> pointer). Could this be the error? How could we detect it?

I don't know if there are cases an object keeping the raw pointer
around, but there are definitely cases of the object itself trying to
invoke Send when the refcount hits 0. This happens with requests for
data from history. They schedule something on the history thread from
start, and only cancel that request from the destructor. This
necessitated the code in
ChromeURLDataManager::DataSource::SendResponse.

Even if we didn't have delayed deletion, the issue you mention would
still be a problem, but instead of hitting DCHECKs, it would likely
cause a crash.

  -Scott