Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
5939002:
Error handling added (Closed)
|
Created:
10 years ago by glotov Modified:
9 years, 7 months ago CC:
chromium-reviews, nkostylev+cc_chromium.org, davemoore+watch_chromium.org, pam+watch_chromium.org, brettw-cc_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionError handling added
BUG=chromium-os:10033
TEST=none
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=69953
Patch Set 1 #
Total comments: 2
Patch Set 2 : Comment update #Patch Set 3 : InitSharedResource() and ReloadSharedResource() return code is checked #
Total comments: 12
Patch Set 4 : return code CHECK fixed #
Total comments: 16
Patch Set 5 : Nit comments fixed #Patch Set 6 : Sync up #
Messages
Total messages: 17 (0 generated)
LGTM http://codereview.chromium.org/5939002/diff/1/chrome/browser/chromeos/login/l... File chrome/browser/chromeos/login/language_switch_menu.cc (right): http://codereview.chromium.org/5939002/diff/1/chrome/browser/chromeos/login/l... chrome/browser/chromeos/login/language_switch_menu.cc:72: // If locale cannot be found (probably user edited prefs file I'm not sure that in the original crash users were able to actually edit prefs file manually. Would be helpful to understand why it's caused. This could be a bug here and we're hiding it now. http://codereview.chromium.org/5939002/diff/1/chrome/browser/chromeos/login/l... chrome/browser/chromeos/login/language_switch_menu.cc:74: index = 0; Default locale (0) is always en-US?
> Would be helpful to understand why it's caused. This could be a bug here and > we're hiding it now. so you do not have any example of what this string locale looked like? I suggest VLOG(ERROR) locale string in case it is not found.
Yes, I added logging, but is is unfortunately a little help because crash dumps contain no logs.
I added CHECK()-ing return codes for InitSharedResource() and ReloadSharedResource() so, if resource .pak is not found for the given locale, we will CHECK and prevent propagating strange SEGSEGVs. The other (and probably smaller) fix to this would be to CHECK() within InitSharedResource() and ReloadSharedResource() themselves. But I did not want to change existing behavior (and break tests). Or should have I?
The repeated crashes issue is 100% reproducible on my L13. Doesn't it reproducible on your machine? I can test your patch tomorrow. On 2010/12/20 13:36:38, glotov wrote: > I added CHECK()-ing return codes for InitSharedResource() and > ReloadSharedResource() so, if resource .pak is not found for the given locale, > we will CHECK and prevent propagating strange SEGSEGVs. > > The other (and probably smaller) fix to this would be to CHECK() within > InitSharedResource() and ReloadSharedResource() themselves. But I did not want > to change existing behavior (and break tests). Or should have I?
I ran on Mario and have not seen any crash like that. How do you make them happen? I'll try. On Mon, Dec 20, 2010 at 5:38 PM, <satorux@chromium.org> wrote: > The repeated crashes issue is 100% reproducible on my L13. Doesn't it > reproducible on your machine? > > I can test your patch tomorrow. > > > On 2010/12/20 13:36:38, glotov wrote: > >> I added CHECK()-ing return codes for InitSharedResource() and >> ReloadSharedResource() so, if resource .pak is not found for the given >> locale, >> we will CHECK and prevent propagating strange SEGSEGVs. >> > > The other (and probably smaller) fix to this would be to CHECK() within >> InitSharedResource() and ReloadSharedResource() themselves. But I did not >> want >> to change existing behavior (and break tests). Or should have I? >> > > > > http://codereview.chromium.org/5939002/ > -- Thank you, Denis
Note on my comments below: While in theory it's safe to do work in CHECK because it's not compiled out, it's bad practice because it's easy to break later (e.g. a reader believes the statement has no side effects and removes the CHECK entirely). To be safe, treat CHECK and DCHECK the same. http://codereview.chromium.org/5939002/diff/10001/chrome/app/chrome_main.cc File chrome/app/chrome_main.cc (right): http://codereview.chromium.org/5939002/diff/10001/chrome/app/chrome_main.cc#n... chrome/app/chrome_main.cc:852: CHECK(ResourceBundle::InitSharedInstance(locale) == locale) Never perform real work inside a CHECK or DCHECK. http://codereview.chromium.org/5939002/diff/10001/chrome/browser/browser_main.cc File chrome/browser/browser_main.cc (right): http://codereview.chromium.org/5939002/diff/10001/chrome/browser/browser_main... chrome/browser/browser_main.cc:1155: CHECK(app_locale == locale) << "Locale could not be found for " << locale; Nit: CHECK_EQ http://codereview.chromium.org/5939002/diff/10001/chrome/browser/browser_main... File chrome/browser/browser_main_mac.mm (right): http://codereview.chromium.org/5939002/diff/10001/chrome/browser/browser_main... chrome/browser/browser_main_mac.mm:106: CHECK(ResourceBundle::InitSharedInstance(pref_locale) == pref_locale) Never do real work inside a CHECK or DCHECK. http://codereview.chromium.org/5939002/diff/10001/chrome/browser/chromeos/log... File chrome/browser/chromeos/login/language_switch_menu.cc (right): http://codereview.chromium.org/5939002/diff/10001/chrome/browser/chromeos/log... chrome/browser/chromeos/login/language_switch_menu.cc:71: if (index == -1) { I don't think we should "handle" this error. IMO we should CHECK or DCHECK instead. http://codereview.chromium.org/5939002/diff/10001/chrome/browser/chromeos/log... chrome/browser/chromeos/login/language_switch_menu.cc:102: CHECK(ResourceBundle::ReloadSharedInstance(locale) == locale) Never do real work inside a CHECK or DCHECK. http://codereview.chromium.org/5939002/diff/10001/chrome/browser/chromeos/log... File chrome/browser/chromeos/login/wizard_controller.cc (right): http://codereview.chromium.org/5939002/diff/10001/chrome/browser/chromeos/log... chrome/browser/chromeos/login/wizard_controller.cc:1010: CHECK(ResourceBundle::ReloadSharedInstance(locale) == locale) Never do real work inside a CHECK or DCHECK.
Nothing special. Just boot chromium os on L13. I just tested your patch, I'll write the results on the bug 10033. On 2010/12/20 15:18:35, glotov wrote: > I ran on Mario and have not seen any crash like that. How do you make them > happen? I'll try. > > On Mon, Dec 20, 2010 at 5:38 PM, <mailto:satorux@chromium.org> wrote: > > > The repeated crashes issue is 100% reproducible on my L13. Doesn't it > > reproducible on your machine? > > > > I can test your patch tomorrow. > > > > > > On 2010/12/20 13:36:38, glotov wrote: > > > >> I added CHECK()-ing return codes for InitSharedResource() and > >> ReloadSharedResource() so, if resource .pak is not found for the given > >> locale, > >> we will CHECK and prevent propagating strange SEGSEGVs. > >> > > > > The other (and probably smaller) fix to this would be to CHECK() within > >> InitSharedResource() and ReloadSharedResource() themselves. But I did not > >> want > >> to change existing behavior (and break tests). Or should have I? > >> > > > > > > > > http://codereview.chromium.org/5939002/ > > > > > > -- > Thank you, > Denis
I meant crosbug.com/10437. Note that booting was not sufficient. You have to login first, then logout. You'll see repeating crashes. On 2010/12/21 02:33:20, satorux1 wrote: > Nothing special. Just boot chromium os on L13. > I just tested your patch, I'll write the results on the bug 10033. > > On 2010/12/20 15:18:35, glotov wrote: > > I ran on Mario and have not seen any crash like that. How do you make them > > happen? I'll try. > > > > On Mon, Dec 20, 2010 at 5:38 PM, <mailto:satorux@chromium.org> wrote: > > > > > The repeated crashes issue is 100% reproducible on my L13. Doesn't it > > > reproducible on your machine? > > > > > > I can test your patch tomorrow. > > > > > > > > > On 2010/12/20 13:36:38, glotov wrote: > > > > > >> I added CHECK()-ing return codes for InitSharedResource() and > > >> ReloadSharedResource() so, if resource .pak is not found for the given > > >> locale, > > >> we will CHECK and prevent propagating strange SEGSEGVs. > > >> > > > > > > The other (and probably smaller) fix to this would be to CHECK() within > > >> InitSharedResource() and ReloadSharedResource() themselves. But I did not > > >> want > > >> to change existing behavior (and break tests). Or should have I? > > >> > > > > > > > > > > > > http://codereview.chromium.org/5939002/ > > > > > > > > > > > -- > > Thank you, > > Denis
My bad. I was completely confused. The patch is nothing to do with the repeated crashes issue crosbug.com/10437. I was thinking about a different bug. Please forget about my comments about repeated crashes in the code reviews... On 2010/12/21 02:37:02, satorux1 wrote: > I meant crosbug.com/10437. > > Note that booting was not sufficient. You have to login first, then logout. > You'll see repeating crashes. > > On 2010/12/21 02:33:20, satorux1 wrote: > > Nothing special. Just boot chromium os on L13. > > I just tested your patch, I'll write the results on the bug 10033. > > > > On 2010/12/20 15:18:35, glotov wrote: > > > I ran on Mario and have not seen any crash like that. How do you make them > > > happen? I'll try. > > > > > > On Mon, Dec 20, 2010 at 5:38 PM, <mailto:satorux@chromium.org> wrote: > > > > > > > The repeated crashes issue is 100% reproducible on my L13. Doesn't it > > > > reproducible on your machine? > > > > > > > > I can test your patch tomorrow. > > > > > > > > > > > > On 2010/12/20 13:36:38, glotov wrote: > > > > > > > >> I added CHECK()-ing return codes for InitSharedResource() and > > > >> ReloadSharedResource() so, if resource .pak is not found for the given > > > >> locale, > > > >> we will CHECK and prevent propagating strange SEGSEGVs. > > > >> > > > > > > > > The other (and probably smaller) fix to this would be to CHECK() within > > > >> InitSharedResource() and ReloadSharedResource() themselves. But I did not > > > >> want > > > >> to change existing behavior (and break tests). Or should have I? > > > >> > > > > > > > > > > > > > > > > http://codereview.chromium.org/5939002/ > > > > > > > > > > > > > > > > -- > > > Thank you, > > > Denis
Anyway, I tested this patch with L13, and here's the result: Chrome didn't boot and crashed at: [1:1:13868622:FATAL:chrome/app/chrome_main.cc(852)] Check failed: ResourceBundle::InitSharedInstance(locale) == locale. Locale could not be found for On 2010/12/21 03:33:34, satorux1 wrote: > My bad. I was completely confused. The patch is nothing to do with the repeated > crashes issue crosbug.com/10437. I was thinking about a different bug. Please > forget about my comments about repeated crashes in the code reviews... > > On 2010/12/21 02:37:02, satorux1 wrote: > > I meant crosbug.com/10437. > > > > Note that booting was not sufficient. You have to login first, then logout. > > You'll see repeating crashes. > > > > On 2010/12/21 02:33:20, satorux1 wrote: > > > Nothing special. Just boot chromium os on L13. > > > I just tested your patch, I'll write the results on the bug 10033. > > > > > > On 2010/12/20 15:18:35, glotov wrote: > > > > I ran on Mario and have not seen any crash like that. How do you make them > > > > happen? I'll try. > > > > > > > > On Mon, Dec 20, 2010 at 5:38 PM, <mailto:satorux@chromium.org> wrote: > > > > > > > > > The repeated crashes issue is 100% reproducible on my L13. Doesn't it > > > > > reproducible on your machine? > > > > > > > > > > I can test your patch tomorrow. > > > > > > > > > > > > > > > On 2010/12/20 13:36:38, glotov wrote: > > > > > > > > > >> I added CHECK()-ing return codes for InitSharedResource() and > > > > >> ReloadSharedResource() so, if resource .pak is not found for the given > > > > >> locale, > > > > >> we will CHECK and prevent propagating strange SEGSEGVs. > > > > >> > > > > > > > > > > The other (and probably smaller) fix to this would be to CHECK() within > > > > >> InitSharedResource() and ReloadSharedResource() themselves. But I did > not > > > > >> want > > > > >> to change existing behavior (and break tests). Or should have I? > > > > >> > > > > > > > > > > > > > > > > > > > > http://codereview.chromium.org/5939002/ > > > > > > > > > > > > > > > > > > > > > -- > > > > Thank you, > > > > Denis
Changed to CHECK_EQ. |locale| appears to be empty, and InitSharedInstance(locale) returns "en-US". [1:1:12298789:FATAL:chrome/app/chrome_main.cc(852)] Check failed: ResourceBundle::InitSharedInstance(locale) == locale (en-US vs. )Locale could not be found for On 2010/12/21 03:46:51, satorux1 wrote: > Anyway, I tested this patch with L13, and here's the result: > > Chrome didn't boot and crashed at: > > [1:1:13868622:FATAL:chrome/app/chrome_main.cc(852)] Check failed: > ResourceBundle::InitSharedInstance(locale) == locale. Locale could not be found > for > > > On 2010/12/21 03:33:34, satorux1 wrote: > > My bad. I was completely confused. The patch is nothing to do with the > repeated > > crashes issue crosbug.com/10437. I was thinking about a different bug. Please > > forget about my comments about repeated crashes in the code reviews... > > > > On 2010/12/21 02:37:02, satorux1 wrote: > > > I meant crosbug.com/10437. > > > > > > Note that booting was not sufficient. You have to login first, then logout. > > > You'll see repeating crashes. > > > > > > On 2010/12/21 02:33:20, satorux1 wrote: > > > > Nothing special. Just boot chromium os on L13. > > > > I just tested your patch, I'll write the results on the bug 10033. > > > > > > > > On 2010/12/20 15:18:35, glotov wrote: > > > > > I ran on Mario and have not seen any crash like that. How do you make > them > > > > > happen? I'll try. > > > > > > > > > > On Mon, Dec 20, 2010 at 5:38 PM, <mailto:satorux@chromium.org> wrote: > > > > > > > > > > > The repeated crashes issue is 100% reproducible on my L13. Doesn't it > > > > > > reproducible on your machine? > > > > > > > > > > > > I can test your patch tomorrow. > > > > > > > > > > > > > > > > > > On 2010/12/20 13:36:38, glotov wrote: > > > > > > > > > > > >> I added CHECK()-ing return codes for InitSharedResource() and > > > > > >> ReloadSharedResource() so, if resource .pak is not found for the > given > > > > > >> locale, > > > > > >> we will CHECK and prevent propagating strange SEGSEGVs. > > > > > >> > > > > > > > > > > > > The other (and probably smaller) fix to this would be to CHECK() > within > > > > > >> InitSharedResource() and ReloadSharedResource() themselves. But I did > > not > > > > > >> want > > > > > >> to change existing behavior (and break tests). Or should have I? > > > > > >> > > > > > > > > > > > > > > > > > > > > > > > > http://codereview.chromium.org/5939002/ > > > > > > > > > > > > > > > > > > > > > > > > > > -- > > > > > Thank you, > > > > > Denis
I fixed the comments, please have a look now. http://codereview.chromium.org/5939002/diff/10001/chrome/app/chrome_main.cc File chrome/app/chrome_main.cc (right): http://codereview.chromium.org/5939002/diff/10001/chrome/app/chrome_main.cc#n... chrome/app/chrome_main.cc:852: CHECK(ResourceBundle::InitSharedInstance(locale) == locale) On 2010/12/20 17:18:48, Peter Kasting wrote: > Never perform real work inside a CHECK or DCHECK. Done. http://codereview.chromium.org/5939002/diff/10001/chrome/browser/browser_main.cc File chrome/browser/browser_main.cc (right): http://codereview.chromium.org/5939002/diff/10001/chrome/browser/browser_main... chrome/browser/browser_main.cc:1155: CHECK(app_locale == locale) << "Locale could not be found for " << locale; On 2010/12/20 17:18:48, Peter Kasting wrote: > Nit: CHECK_EQ Done. http://codereview.chromium.org/5939002/diff/10001/chrome/browser/browser_main... File chrome/browser/browser_main_mac.mm (right): http://codereview.chromium.org/5939002/diff/10001/chrome/browser/browser_main... chrome/browser/browser_main_mac.mm:106: CHECK(ResourceBundle::InitSharedInstance(pref_locale) == pref_locale) On 2010/12/20 17:18:48, Peter Kasting wrote: > Never do real work inside a CHECK or DCHECK. Done. http://codereview.chromium.org/5939002/diff/10001/chrome/browser/chromeos/log... File chrome/browser/chromeos/login/language_switch_menu.cc (right): http://codereview.chromium.org/5939002/diff/10001/chrome/browser/chromeos/log... chrome/browser/chromeos/login/language_switch_menu.cc:71: if (index == -1) { Agree. That was my second option but I wanted someone's explicit comment. Thank you for that :) http://codereview.chromium.org/5939002/diff/10001/chrome/browser/chromeos/log... chrome/browser/chromeos/login/language_switch_menu.cc:102: CHECK(ResourceBundle::ReloadSharedInstance(locale) == locale) On 2010/12/20 17:18:48, Peter Kasting wrote: > Never do real work inside a CHECK or DCHECK. Done. http://codereview.chromium.org/5939002/diff/10001/chrome/browser/chromeos/log... File chrome/browser/chromeos/login/wizard_controller.cc (right): http://codereview.chromium.org/5939002/diff/10001/chrome/browser/chromeos/log... chrome/browser/chromeos/login/wizard_controller.cc:1010: CHECK(ResourceBundle::ReloadSharedInstance(locale) == locale) On 2010/12/20 17:18:48, Peter Kasting wrote: > Never do real work inside a CHECK or DCHECK. Done.
http://codereview.chromium.org/5939002/diff/25001/chrome/app/chrome_main.cc File chrome/app/chrome_main.cc (right): http://codereview.chromium.org/5939002/diff/25001/chrome/app/chrome_main.cc#n... chrome/app/chrome_main.cc:852: const std::string res = ResourceBundle::InitSharedInstance(locale); Nit: |res| is a poor name; consider |loaded_locale|. http://codereview.chromium.org/5939002/diff/25001/chrome/app/chrome_main.cc#n... chrome/app/chrome_main.cc:853: CHECK(!res.empty()) << "Locale could not be found for " << locale; Nit: Shouldn't we be checking that these are equal? http://codereview.chromium.org/5939002/diff/25001/chrome/browser/browser_main.cc File chrome/browser/browser_main.cc (right): http://codereview.chromium.org/5939002/diff/25001/chrome/browser/browser_main... chrome/browser/browser_main.cc:1155: const std::string res = ResourceBundle::InitSharedInstance(locale); Nit: Same nits http://codereview.chromium.org/5939002/diff/25001/chrome/browser/browser_main... File chrome/browser/browser_main_mac.mm (right): http://codereview.chromium.org/5939002/diff/25001/chrome/browser/browser_main... chrome/browser/browser_main_mac.mm:106: std::string locale = ResourceBundle::InitSharedInstance(pref_locale); Is this going to do the right thing? We've never set |pref_locale|. It seems like we should, like we do on other platforms? http://codereview.chromium.org/5939002/diff/25001/chrome/browser/browser_main... chrome/browser/browser_main_mac.mm:107: CHECK(!locale.empty()) << "Locale could not be found for " << pref_locale; Nit: Same nit http://codereview.chromium.org/5939002/diff/25001/chrome/browser/chromeos/log... File chrome/browser/chromeos/login/language_switch_menu.cc (right): http://codereview.chromium.org/5939002/diff/25001/chrome/browser/chromeos/log... chrome/browser/chromeos/login/language_switch_menu.cc:71: CHECK_NE(index, -1) << "Unknown locale: " << locale; Nit: (expected, actual) http://codereview.chromium.org/5939002/diff/25001/chrome/browser/chromeos/log... chrome/browser/chromeos/login/language_switch_menu.cc:97: const std::string res = ResourceBundle::ReloadSharedInstance(locale); Nit: Same nits http://codereview.chromium.org/5939002/diff/25001/chrome/browser/chromeos/log... File chrome/browser/chromeos/login/wizard_controller.cc (right): http://codereview.chromium.org/5939002/diff/25001/chrome/browser/chromeos/log... chrome/browser/chromeos/login/wizard_controller.cc:1010: const std::string res = ResourceBundle::ReloadSharedInstance(locale); Nit: Same nits
http://codereview.chromium.org/5939002/diff/25001/chrome/app/chrome_main.cc File chrome/app/chrome_main.cc (right): http://codereview.chromium.org/5939002/diff/25001/chrome/app/chrome_main.cc#n... chrome/app/chrome_main.cc:852: const std::string res = ResourceBundle::InitSharedInstance(locale); Agree, just wanted a short name to fit the line. Moreover it is very local. But since you noticed... Done. http://codereview.chromium.org/5939002/diff/25001/chrome/app/chrome_main.cc#n... chrome/app/chrome_main.cc:853: CHECK(!res.empty()) << "Locale could not be found for " << locale; That was my mistake that time. No, we shouldn't. Because usually |locale| is empty, but InitSharedInstance("") will load the default locale then (and return its name). http://codereview.chromium.org/5939002/diff/25001/chrome/browser/browser_main.cc File chrome/browser/browser_main.cc (right): http://codereview.chromium.org/5939002/diff/25001/chrome/browser/browser_main... chrome/browser/browser_main.cc:1155: const std::string res = ResourceBundle::InitSharedInstance(locale); On 2010/12/21 18:35:58, Peter Kasting wrote: > Nit: Same nits Done. http://codereview.chromium.org/5939002/diff/25001/chrome/browser/browser_main... File chrome/browser/browser_main_mac.mm (right): http://codereview.chromium.org/5939002/diff/25001/chrome/browser/browser_main... chrome/browser/browser_main_mac.mm:106: std::string locale = ResourceBundle::InitSharedInstance(pref_locale); Yes, looks confusing, I rewrote it. As I mentioned above, InitSharedInstance() will choose the default locale then. http://codereview.chromium.org/5939002/diff/25001/chrome/browser/browser_main... chrome/browser/browser_main_mac.mm:107: CHECK(!locale.empty()) << "Locale could not be found for " << pref_locale; On 2010/12/21 18:35:58, Peter Kasting wrote: > Nit: Same nit Done. http://codereview.chromium.org/5939002/diff/25001/chrome/browser/chromeos/log... File chrome/browser/chromeos/login/language_switch_menu.cc (right): http://codereview.chromium.org/5939002/diff/25001/chrome/browser/chromeos/log... chrome/browser/chromeos/login/language_switch_menu.cc:71: CHECK_NE(index, -1) << "Unknown locale: " << locale; On 2010/12/21 18:35:58, Peter Kasting wrote: > Nit: (expected, actual) Done. http://codereview.chromium.org/5939002/diff/25001/chrome/browser/chromeos/log... chrome/browser/chromeos/login/language_switch_menu.cc:97: const std::string res = ResourceBundle::ReloadSharedInstance(locale); On 2010/12/21 18:35:58, Peter Kasting wrote: > Nit: Same nits Done. http://codereview.chromium.org/5939002/diff/25001/chrome/browser/chromeos/log... File chrome/browser/chromeos/login/wizard_controller.cc (right): http://codereview.chromium.org/5939002/diff/25001/chrome/browser/chromeos/log... chrome/browser/chromeos/login/wizard_controller.cc:1010: const std::string res = ResourceBundle::ReloadSharedInstance(locale); On 2010/12/21 18:35:58, Peter Kasting wrote: > Nit: Same nits Done.
On 2010/12/21 20:33:34, glotov wrote:
> That was my mistake that time. No, we shouldn't. Because usually |locale| is
> empty, but InitSharedInstance("") will load the default locale then (and
return
> its name).
I see. In that case, you might want your CHECK()s to print something like
CHECK(!loaded_locale.empty()) << (locale.empty() ? std::string("Default locale")
: ("Locale \"" + locale + "\"")) << " not found";
LGTM
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
