Who owns the UI thread's NotificationService?

Jonathan Dixon

unread,

Oct 17, 2012, 8:14:38 PM10/17/12

to Thiago Farina, John Abd-El-Malek, sgu...@chromium.org, conten...@chromium.org, boliu

In navigation_controller_impl.cc there are many unconditional calls to NotificationService::current()->Notify() -- i.e. that do not guard against current() returning NULL .

Therefore it seems to be a pre-requisite of using content API that the UI thread must have installed a NotificationService?

I can't work out in chrome who it is the creates and owns that instance. 3 candidates are listed below (but the _mac one clearly isn't it, and the other two look unlikely)

chrome/browser/password_manager/password_store_mac.cc: notification_service_.reset(content::NotificationService::Create());

chrome/browser/sync/glue/typed_url_change_processor.cc: notification_service_.reset(content::NotificationService::Create());

chrome/browser/webdata/web_database.cc: notification_service_.reset(content::NotificationService::Create());

(This doesn't seem to have changed much since http://codereview.chromium.org/8913009/)

Background is, for android webview, we're trying to remove dependency on the chrome code, which means we need to explicitly create our own one. If it makes sense to do so, I'd like to do this in a way consistent with chrome.

Alternative is we start adding NULL checks in navigation_controller_impl.cc... e.g.

https://codereview.chromium.org/11187032/diff/9001/content/browser/web_contents/navigation_controller_impl.cc ... ?

John Abd-El-Malek

unread,

Oct 17, 2012, 8:26:01 PM10/17/12

to Jonathan Dixon, Thiago Farina, sgu...@chromium.org, conten...@chromium.org, boliu

it's owned by content, see

http://code.google.com/searchframe#OAMlx_jo-ck/src/content/browser/browser_main_runner.cc&exact_package=chromium&q=new%20NotificationServiceImpl&type=cs&l=72

On Wed, Oct 17, 2012 at 5:14 PM, Jonathan Dixon <jo...@google.com> wrote:

In navigation_controller_impl.cc there are many unconditional calls to NotificationService::current()->Notify() -- i.e. that do not guard against current() returning NULL .

Therefore it seems to be a pre-requisite of using content API that the UI thread must have installed a NotificationService?

I can't work out in chrome who it is the creates and owns that instance. 3 candidates are listed below (but the _mac one clearly isn't it, and the other two look unlikely)

chrome/browser/password_manager/password_store_mac.cc: notification_service_.reset(content::NotificationService::Create());

this is on another thread (not UI)

chrome/browser/sync/glue/typed_url_change_processor.cc: notification_service_.reset(content::NotificationService::Create());

this is for unittests

chrome/browser/webdata/web_database.cc: notification_service_.reset(content::NotificationService::Create());

this is for unittests

(This doesn't seem to have changed much since http://codereview.chromium.org/8913009/)

Background is, for android webview, we're trying to remove dependency on the chrome code, which means we need to explicitly create our own one. If it makes sense to do so, I'd like to do this in a way consistent with chrome.

Alternative is we start adding NULL checks in navigation_controller_impl.cc... e.g.
https://codereview.chromium.org/11187032/diff/9001/content/browser/web_contents/navigation_controller_impl.cc ... ?

that shouldn't be done, we assume that it's always non-null

Jonathan Dixon

unread,

Oct 17, 2012, 8:29:33 PM10/17/12

to John Abd-El-Malek, Thiago Farina, sgu...@chromium.org, conten...@chromium.org, boliu

On 17 October 2012 17:26, John Abd-El-Malek <j...@chromium.org> wrote:

it's owned by content, see
http://code.google.com/searchframe#OAMlx_jo-ck/src/content/browser/browser_main_runner.cc&exact_package=chromium&q=new%20NotificationServiceImpl&type=cs&l=72

there it is! Thanks. I had a grep failure (I was only looking for calls to ::Create)

On Wed, Oct 17, 2012 at 5:14 PM, Jonathan Dixon <jo...@google.com> wrote:
In navigation_controller_impl.cc there are many unconditional calls to NotificationService::current()->Notify() -- i.e. that do not guard against current() returning NULL .

Therefore it seems to be a pre-requisite of using content API that the UI thread must have installed a NotificationService?

I can't work out in chrome who it is the creates and owns that instance. 3 candidates are listed below (but the _mac one clearly isn't it, and the other two look unlikely)

chrome/browser/password_manager/password_store_mac.cc: notification_service_.reset(content::NotificationService::Create());
this is on another thread (not UI)

chrome/browser/sync/glue/typed_url_change_processor.cc: notification_service_.reset(content::NotificationService::Create());
this is for unittests

chrome/browser/webdata/web_database.cc: notification_service_.reset(content::NotificationService::Create());
this is for unittests

(This doesn't seem to have changed much since http://codereview.chromium.org/8913009/)

Background is, for android webview, we're trying to remove dependency on the chrome code, which means we need to explicitly create our own one. If it makes sense to do so, I'd like to do this in a way consistent with chrome.

Alternative is we start adding NULL checks in navigation_controller_impl.cc... e.g.
https://codereview.chromium.org/11187032/diff/9001/content/browser/web_contents/navigation_controller_impl.cc ... ?

that shouldn't be done, we assume that it's always non-null

Totally agree. So there must be some other issue causing NotificationService::current() to return NULL. Will dig into that.

Thanks

Reply all

Reply to author

Forward