Using nullptr for RemoteSuggestionsScheduler causes brittle code |
||||
Issue descriptionThere have been three crashes recently where using the ContentSuggestionsService or the RemoteSuggestionsScheduler caused a crash: https://crbug.com/647920 https://crbug.com/648201 https://crbug.com/647920 https://crbug.com/678483 Everytime, the member content_suggestions_service_ or its member function remote_suggestions_scheduler() were accessed and returned a nullptr which was supposed to be a object. The ContentSuggestionsServiceFactory should always provide a default/empty implementation for objects like the scheduler instead of just omitting their construction.
,
Jan 25 2017
It is fairly common for factories for KeyedServices to return nullptr in some cases (incognito, mostly). I think we just have to deal with that. Once you do have a non-null service, it still makes sense IMO to guarantee nothing else will be null, such as the scheduler.
,
Feb 6 2017
,
Sep 11 2017
,
Jan 25 2018
As stated, this is rather common and can be changed if the behavior becomes more complex. |
||||
►
Sign in to add a comment |
||||
Comment 1 by fhorschig@chromium.org
, Jan 25 2017Okay, so this seems to be deeper rooted. The ContentSuggestionsServiceFactory sometimes provides a nullptr as response to: ContentSuggestionsServiceFactory::GetForProfile( ProfileManager::GetLastUsedProfile()); which resulted in this code: // Can maybe be null in some cases? (Incognito profile?) crbug.com/647920 if (!content_suggestions_service) { return nullptr; } The referenced bug crbug.com/647920 introduced this fix but didn't resolve the underlying issue. Before this bug can be fixed, it must be ensured that content_suggestions_service can not be nullptr.