Fix crash on openning settings when DCHECK is on.

Fix crash on openning settings when DCHECK is on.

Use randomly generated settings app id in format used for app
and extension identification.

BUG=718987
Test=Manually

Review-Url: https://codereview.chromium.org/2869473002
Cr-Commit-Position: refs/heads/master@{#469881}
Committed: https://chromium.googlesource.com/chromium/src/+/5a006e9c8d2f5049f39f49a9280cc6cebb1a4ec3

[khmel, 2017-05-05 19:44:00]

khmel@chromium.org changed reviewers:
+ msw@chromium.org

[khmel, 2017-05-05 19:44:00]

Hi

PTAL

Thanks!

[msw, 2017-05-05 19:58:51]

Hey, this is interesting, I have a few questions:
1) Where did you get djpioaflnacfdgafjcjjbidfjclmdgnc?
2) Is it okay to assign crx ids to non-crx things?
3) Should we do the same for these other ones I added?
https://codereview.chromium.org/2860503002/diff/220001/chrome/browser/ui/view...
https://codereview.chromium.org/2860503002/diff/220001/ash/shelf/app_list_she...
(perhaps these next two should use a new public static
AppListShelfItemDelegate::kAppListId?)
https://codereview.chromium.org/2860503002/diff/220001/ash/shelf/shelf_model_...
https://codereview.chromium.org/2860503002/diff/220001/chrome/browser/ui/ash/...
4) Should we be checking for valid ids in ash::ShelfID (AppLaunchId) and the
mojom struct traits?

[khmel, 2017-05-05 21:05:43]

On 2017/05/05 19:58:51, msw wrote:
> Hey, this is interesting, I have a few questions:
> 1) Where did you get djpioaflnacfdgafjcjjbidfjclmdgnc?
I used following to generate (just for my convenience):
ArcAppListPrefs::GetAppId("chrome.settings", "chrome.settings.ui")

> 2) Is it okay to assign crx ids to non-crx things?
It is not obvious but this is de-facto standard. A lot of check to validate this
in extension and once ARC shares some functionality with extension and in ARC.
For example:
https://cs.chromium.org/chromium/src/extensions/browser/extension_prefs.cc?q=...


> 3) Should we do the same for these other ones I added?
>
https://codereview.chromium.org/2860503002/diff/220001/chrome/browser/ui/view...
>
https://codereview.chromium.org/2860503002/diff/220001/ash/shelf/app_list_she...
> (perhaps these next two should use a new public static
> AppListShelfItemDelegate::kAppListId?)
>
https://codereview.chromium.org/2860503002/diff/220001/ash/shelf/shelf_model_...
>
https://codereview.chromium.org/2860503002/diff/220001/chrome/browser/ui/ash/...
Yes, thanks for reminding.

> 4) Should we be checking for valid ids in ash::ShelfID (AppLaunchId) and the
> mojom struct traits?
Not sure, it is not easy to add check at AppLaunchId due dependencies to
components. If we add there we have to update several unit tests were "app_id1"
and similar is used.

[khmel, 2017-05-05 21:05:51]

https://codereview.chromium.org/2869473002/diff/20001/chrome/browser/ui/ash/l...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller_unittest.cc
(right):

https://codereview.chromium.org/2869473002/diff/20001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_unittest.cc:129:
constexpr char kAppListId[] = "eiiblffaiacobmjejhcoookmokblahea";
Not sure if we need have ASH_EXPORT app_list_shelf_item_delegate.h to support
this unit test. Included dup

[msw, 2017-05-05 21:45:04]

Thanks for the explanations, I wasn't aware of this.

https://codereview.chromium.org/2869473002/diff/20001/ash/shelf/app_list_shel...
File ash/shelf/app_list_shelf_item_delegate.cc (right):

https://codereview.chromium.org/2869473002/diff/20001/ash/shelf/app_list_shel...
ash/shelf/app_list_shelf_item_delegate.cc:18:
"eiiblffaiacobmjejhcoookmokblahea";
Instead of using an arc-specific formatting for the input via
ArcAppListPrefs::GetAppId("chrome.settings", "chrome.settings.ui"), could you
use crx_file::id_util::GenerateId("org.chromium.settings_ui") or similar for
this and the other new ids, and document what you used in comments with the
generated id literals?

https://codereview.chromium.org/2869473002/diff/20001/ash/shelf/shelf_model_u...
File ash/shelf/shelf_model_unittest.cc (right):

https://codereview.chromium.org/2869473002/diff/20001/ash/shelf/shelf_model_u...
ash/shelf/shelf_model_unittest.cc:72: item.id =
ShelfID(AppListShelfItemDelegate::kAppListId);
nit: include the header for this (or use something like kDummyAppId or define a
string copy here, and make kAppListId private to the cc file again)

https://codereview.chromium.org/2869473002/diff/20001/chrome/browser/ui/ash/l...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller_browsertest.cc
(right):

https://codereview.chromium.org/2869473002/diff/20001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_browsertest.cc:92:
constexpr char kFooShelfId[] = "mjemblieefldeffoklbbmkhihdblgaaa";
Could we use something like kDummyAppId? It seems like we should have a set of
fake extension/app ids somewhere, or use some random valid ids (ie. values from
extensions/common/constants.h or
chrome/common/extensions/extension_constants.h).

https://codereview.chromium.org/2869473002/diff/20001/chrome/browser/ui/ash/l...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller_unittest.cc
(right):

https://codereview.chromium.org/2869473002/diff/20001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_unittest.cc:129:
constexpr char kAppListId[] = "eiiblffaiacobmjejhcoookmokblahea";
On 2017/05/05 21:05:51, khmel wrote:
> Not sure if we need have ASH_EXPORT app_list_shelf_item_delegate.h to support
> this unit test. Included dup

Could we use kDummyAppId? Otherwise, this duplicate string for tests is okay.

[msw, 2017-05-05 21:46:04]

Can you add tests that would have caught these crashes?

[khmel, 2017-05-05 22:45:13]

Thanks for comments.

Also added test coverage.

https://codereview.chromium.org/2869473002/diff/20001/ash/shelf/app_list_shel...
File ash/shelf/app_list_shelf_item_delegate.cc (right):

https://codereview.chromium.org/2869473002/diff/20001/ash/shelf/app_list_shel...
ash/shelf/app_list_shelf_item_delegate.cc:18:
"eiiblffaiacobmjejhcoookmokblahea";
On 2017/05/05 21:45:04, msw wrote:
> Instead of using an arc-specific formatting for the input via
> ArcAppListPrefs::GetAppId("chrome.settings", "chrome.settings.ui"), could you
> use crx_file::id_util::GenerateId("org.chromium.settings_ui") or similar for
> this and the other new ids, and document what you used in comments with the
> generated id literals?

That makes sense.

https://codereview.chromium.org/2869473002/diff/20001/ash/shelf/shelf_model_u...
File ash/shelf/shelf_model_unittest.cc (right):

https://codereview.chromium.org/2869473002/diff/20001/ash/shelf/shelf_model_u...
ash/shelf/shelf_model_unittest.cc:72: item.id =
ShelfID(AppListShelfItemDelegate::kAppListId);
On 2017/05/05 21:45:04, msw wrote:
> nit: include the header for this (or use something like kDummyAppId or define
a
> string copy here, and make kAppListId private to the cc file again)

Done.

https://codereview.chromium.org/2869473002/diff/20001/chrome/browser/ui/ash/l...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller_browsertest.cc
(right):

https://codereview.chromium.org/2869473002/diff/20001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_browsertest.cc:92:
constexpr char kFooShelfId[] = "mjemblieefldeffoklbbmkhihdblgaaa";
On 2017/05/05 21:45:04, msw wrote:
> Could we use something like kDummyAppId? It seems like we should have a set of
> fake extension/app ids somewhere, or use some random valid ids (ie. values
from
> extensions/common/constants.h or
> chrome/common/extensions/extension_constants.h). 

Good point, no need to have special id here.

https://codereview.chromium.org/2869473002/diff/20001/chrome/browser/ui/ash/l...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller_unittest.cc
(right):

https://codereview.chromium.org/2869473002/diff/20001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_unittest.cc:129:
constexpr char kAppListId[] = "eiiblffaiacobmjejhcoookmokblahea";
On 2017/05/05 21:45:04, msw wrote:
> On 2017/05/05 21:05:51, khmel wrote:
> > Not sure if we need have ASH_EXPORT app_list_shelf_item_delegate.h to
support
> > this unit test. Included dup
> 
> Could we use kDummyAppId? Otherwise, this duplicate string for tests is okay.

Hmm, don't want to mix these too because kDummyAppId has specific description.
Let leave here kAppListId

[msw, 2017-05-05 23:54:55]

Nice, lgtm with minor nits; thanks!

https://codereview.chromium.org/2869473002/diff/40001/ash/shelf/shelf_model_u...
File ash/shelf/shelf_model_unittest.cc (right):

https://codereview.chromium.org/2869473002/diff/40001/ash/shelf/shelf_model_u...
ash/shelf/shelf_model_unittest.cc:18: constexpr char kAppListId[] =
"eiiblffaiacobmjejhcoookmokblahea";
nit: use the new jlfapfmkapbjlfbpjedlinehodkccjee

https://codereview.chromium.org/2869473002/diff/40001/chrome/browser/ui/ash/l...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller_unittest.cc
(right):

https://codereview.chromium.org/2869473002/diff/40001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_unittest.cc:129:
constexpr char kAppListId[] = "eiiblffaiacobmjejhcoookmokblahea";
nit: use the new jlfapfmkapbjlfbpjedlinehodkccjee

[khmel, 2017-05-06 01:10:40]

Thank you for review and ideas!

https://codereview.chromium.org/2869473002/diff/40001/ash/shelf/shelf_model_u...
File ash/shelf/shelf_model_unittest.cc (right):

https://codereview.chromium.org/2869473002/diff/40001/ash/shelf/shelf_model_u...
ash/shelf/shelf_model_unittest.cc:18: constexpr char kAppListId[] =
"eiiblffaiacobmjejhcoookmokblahea";
On 2017/05/05 23:54:55, msw wrote:
> nit: use the new jlfapfmkapbjlfbpjedlinehodkccjee

Ohh :)

https://codereview.chromium.org/2869473002/diff/40001/chrome/browser/ui/ash/l...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller_unittest.cc
(right):

https://codereview.chromium.org/2869473002/diff/40001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_unittest.cc:129:
constexpr char kAppListId[] = "eiiblffaiacobmjejhcoookmokblahea";
On 2017/05/05 23:54:55, msw wrote:
> nit: use the new jlfapfmkapbjlfbpjedlinehodkccjee

Done.

[khmel, 2017-05-06 01:10:43]

The CQ bit was checked by khmel@chromium.org

[khmel, 2017-05-06 01:10:44]

The patchset sent to the CQ was uploaded after l-g-t-m from msw@chromium.org
Link to the patchset: https://codereview.chromium.org/2869473002/#ps60001
(title: "use new id for app list")

[commit-bot: I haz the power, 2017-05-06 01:11:16]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-06 19:20:04]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1494033043914750,
"parent_rev": "29de591dcbc520f84e3cfffd519027fec091675f", "commit_rev":
"5a006e9c8d2f5049f39f49a9280cc6cebb1a4ec3"}

[commit-bot: I haz the power, 2017-05-06 19:20:19]

Description was changed from

==========
Fix crash on openning settings when DCHECK is on.

Use randomly generated settings app id in format used for app
and extension identification.

BUG=718987
Test=Manually
==========

to

==========
Fix crash on openning settings when DCHECK is on.

Use randomly generated settings app id in format used for app
and extension identification.

BUG=718987
Test=Manually

Review-Url: https://codereview.chromium.org/2869473002
Cr-Commit-Position: refs/heads/master@{#469881}
Committed:
https://chromium.googlesource.com/chromium/src/+/5a006e9c8d2f5049f39f49a9280c...
==========

[commit-bot: I haz the power, 2017-05-06 19:20:21]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/5a006e9c8d2f5049f39f49a9280c...