Fixing 27834 by always deleting ExtensionMessageService on...

Fixing 27834 by always deleting ExtensionMessageService on
UI thread.


BUG=27834
TEST=none


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

[Erik does not do reviews, 2009-11-25 01:27:33]

http://codereview.chromium.org/440012/diff/1/2
File chrome/browser/extensions/extension_message_service.cc (right):

http://codereview.chromium.org/440012/diff/1/2#newcode131
chrome/browser/extensions/extension_message_service.cc:131:
CHECK(ChromeThread::CurrentlyOn(thread_id_));
given that you added the DeleteOnUIThread trait, we don't need these checks here
(we should rely on the unit tests for that code).  What I was proposing was that
you add these checks to NotificationRegistrar to try to catch these problems
generically.

[huanr, 2009-11-25 01:43:40]

http://codereview.chromium.org/440012/diff/1/2
File chrome/browser/extensions/extension_message_service.cc (right):

http://codereview.chromium.org/440012/diff/1/2#newcode131
chrome/browser/extensions/extension_message_service.cc:131:
CHECK(ChromeThread::CurrentlyOn(thread_id_));
I see. In that case I'd add checks to NotificationRegistrar in a separate CL so
they can be merged individually. This change is pretty safe for beta branch. For
the check to catch generic problem, I will try it out on dev branch first.

I have removed the CHECK here but kept the CHECK in ProfileDestroyed.

On 2009/11/25 01:27:33, Erik Kay wrote:
> given that you added the DeleteOnUIThread trait, we don't need these checks
here
> (we should rely on the unit tests for that code).  What I was proposing was
that
> you add these checks to NotificationRegistrar to try to catch these problems
> generically.

[Erik does not do reviews, 2009-11-25 03:06:17]

LGTM

http://codereview.chromium.org/440012/diff/1005/5
File chrome/browser/extensions/extension_message_service.cc (right):

http://codereview.chromium.org/440012/diff/1005/5#newcode118
chrome/browser/extensions/extension_message_service.cc:118: if
(!ChromeThread::GetCurrentThreadIdentifier(&thread_id_)) {
or this

http://codereview.chromium.org/440012/diff/1005/5#newcode140
chrome/browser/extensions/extension_message_service.cc:140: if (thread_id_ !=
ChromeThread::ID_COUNT)
if you still want to do this (I don't think it's necessary), you could just
explicitly check that current thread is th UI thread

http://codereview.chromium.org/440012/diff/1005/6
File chrome/browser/extensions/extension_message_service.h (right):

http://codereview.chromium.org/440012/diff/1005/6#newcode202
chrome/browser/extensions/extension_message_service.h:202: ChromeThread::ID
thread_id_;
you don't need this any more

[huanr, 2009-11-25 04:19:09]

http://codereview.chromium.org/440012/diff/1005/5
File chrome/browser/extensions/extension_message_service.cc (right):

http://codereview.chromium.org/440012/diff/1005/5#newcode140
chrome/browser/extensions/extension_message_service.cc:140: if (thread_id_ !=
ChromeThread::ID_COUNT)
If we get created in unit test, GetCurrentThreadIdentifier above fails. So I
assign thread_id_ to an ID not used and check it here.

I have updated comments.


On 2009/11/25 03:06:17, Erik Kay wrote:
> if you still want to do this (I don't think it's necessary), you could just
> explicitly check that current thread is th UI thread

[Erik does not do reviews, 2009-11-25 15:52:11]

http://codereview.chromium.org/440012/diff/4004/7004
File chrome/browser/extensions/extension_message_service.cc (right):

http://codereview.chromium.org/440012/diff/4004/7004#newcode142
chrome/browser/extensions/extension_message_service.cc:142:
CHECK(ChromeThread::CurrentlyOn(thread_id_));
If GetCurrentThreadIdentifier fails, I don't see how CurrentlyOn could work

[huanr, 2009-11-25 17:16:37]

http://codereview.chromium.org/440012/diff/4004/7004
File chrome/browser/extensions/extension_message_service.cc (right):

http://codereview.chromium.org/440012/diff/4004/7004#newcode142
chrome/browser/extensions/extension_message_service.cc:142:
CHECK(ChromeThread::CurrentlyOn(thread_id_));
If GetCurrentThreadIdentifier fails, the thread_id_ will be ID_COUNT.

On 2009/11/25 15:52:11, Erik Kay wrote:
> If GetCurrentThreadIdentifier fails, I don't see how CurrentlyOn could work

[Erik does not do reviews, 2009-11-25 17:32:49]

http://codereview.chromium.org/440012/diff/4004/7004
File chrome/browser/extensions/extension_message_service.cc (right):

http://codereview.chromium.org/440012/diff/4004/7004#newcode142
chrome/browser/extensions/extension_message_service.cc:142:
CHECK(ChromeThread::CurrentlyOn(thread_id_));
On 2009/11/25 17:16:38, huanr wrote:
> If GetCurrentThreadIdentifier fails, the thread_id_ will be ID_COUNT.
> 
> On 2009/11/25 15:52:11, Erik Kay wrote:
> > If GetCurrentThreadIdentifier fails, I don't see how CurrentlyOn could work
> 
> 

I think you're missing my point.  CurrentlyOn uses the same mechanism as
GetCurrentThreadIdentifier.  If one of them doesn't work, neither will the other
one.

In fact, it's worse than this.  Looking at the code, both methods DCHECK if
identifier is >= ID_COUNT, so you're going to hit DCHECKs anyway.

[huanr, 2009-11-25 17:41:55]

GetCurrentThreadIdentifier fails because the current thread is not a well-known
thread. If that happens then we assign thread_id_ with ID_COUNT, a value that
GetCurrentThreadIdentifier will not return if the function succeeds.

So we only call CurrentlyOn if previous GetCurrentThreadIdentifier succeeds. Do
I miss anything?

 On 2009/11/25 17:16:37, huanr wrote:
> http://codereview.chromium.org/440012/diff/4004/7004
> File chrome/browser/extensions/extension_message_service.cc (right):
> 
> http://codereview.chromium.org/440012/diff/4004/7004#newcode142
> chrome/browser/extensions/extension_message_service.cc:142:
> CHECK(ChromeThread::CurrentlyOn(thread_id_));
> If GetCurrentThreadIdentifier fails, the thread_id_ will be ID_COUNT.
> 
> On 2009/11/25 15:52:11, Erik Kay wrote:
> > If GetCurrentThreadIdentifier fails, I don't see how CurrentlyOn could work

[Erik does not do reviews, 2009-11-25 17:51:43]

I see it now.  I misread that check.  Thanks.

Erik


On Wed, Nov 25, 2009 at 9:41 AM, <huanr@chromium.org> wrote:

> GetCurrentThreadIdentifier fails because the current thread is not a
> well-known
> thread. If that happens then we assign thread_id_ with ID_COUNT, a value
> that
> GetCurrentThreadIdentifier will not return if the function succeeds.
>
> So we only call CurrentlyOn if previous GetCurrentThreadIdentifier
> succeeds. Do
> I miss anything?
>
>
>  On 2009/11/25 17:16:37, huanr wrote:
>
>> http://codereview.chromium.org/440012/diff/4004/7004
>> File chrome/browser/extensions/extension_message_service.cc (right):
>>
>
>  http://codereview.chromium.org/440012/diff/4004/7004#newcode142
>> chrome/browser/extensions/extension_message_service.cc:142:
>> CHECK(ChromeThread::CurrentlyOn(thread_id_));
>> If GetCurrentThreadIdentifier fails, the thread_id_ will be ID_COUNT.
>>
>
>  On 2009/11/25 15:52:11, Erik Kay wrote:
>> > If GetCurrentThreadIdentifier fails, I don't see how CurrentlyOn could
>> work
>>
>
>
>
> http://codereview.chromium.org/440012
>