Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
13825003:
Fix uninintialized ResourceBundle in ShowUserDataDirDialog. (Closed)
|
Created:
7 years, 8 months ago by hshi1 Modified:
7 years, 8 months ago CC:
chromium-reviews, markusheintz_ Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionFix uninintialized ResourceBundle in ShowUserDataDirDialog.
Make sure ResourceBundle is initialized when showing the user
data dir dialog. The dialog needs to access string resources.
BUG=230432
TEST=CQ
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=193844
Patch Set 1 #
Total comments: 2
Patch Set 2 : Check if a locale override is specified on the command line. #Patch Set 3 : Fix compile error due to missing switches header include. #
Total comments: 2
Patch Set 4 : Use l10n_util::GetApplicationLocale instead of cmdline switches. #Messages
Total messages: 16 (0 generated)
Please take a look. It seems necessary to initialize ResourceBundle prior to displaying the UserDataDirDialog (windows only), because the dialog accesses string resources. This crash started due to my fix in bug 196301 that moved showing UserDataDirDialog to before initializing LocalState. mnissler@ -- FYI ben@ or thakis@ -- OWNERS
I'm not terribly familiar with resource bundles. tony@ is, and it looks like markusheintz@ did something similar in the past. https://codereview.chromium.org/13825003/diff/1/chrome/browser/user_data_dir_... File chrome/browser/user_data_dir_extractor_win.cc (right): https://codereview.chromium.org/13825003/diff/1/chrome/browser/user_data_dir_... chrome/browser/user_data_dir_extractor_win.cc:53: ResourceBundle::InitSharedInstanceWithLocale("en-US", NULL); Is "en-US" correct? I suppose prefs::kApplicationLocale isn't ready yet, but do you want to use command_line.GetSwitchValueASCII(switches::kLang)?
Yes unfortunately at this moment user prefs is not yet available. Is the switches::kLang typically present in the command line arguments on Windows? Also I wonder if we could just obtain the system UI locale via Win32 API. On 2013/04/11 18:29:29, Nico wrote: > I'm not terribly familiar with resource bundles. tony@ is, and it looks like > markusheintz@ did something similar in the past. > > https://codereview.chromium.org/13825003/diff/1/chrome/browser/user_data_dir_... > File chrome/browser/user_data_dir_extractor_win.cc (right): > > https://codereview.chromium.org/13825003/diff/1/chrome/browser/user_data_dir_... > chrome/browser/user_data_dir_extractor_win.cc:53: > ResourceBundle::InitSharedInstanceWithLocale("en-US", NULL); > Is "en-US" correct? I suppose prefs::kApplicationLocale isn't ready yet, but do > you want to use command_line.GetSwitchValueASCII(switches::kLang)?
On Thu, Apr 11, 2013 at 11:33 AM, <hshi@chromium.org> wrote: > Yes unfortunately at this moment user prefs is not yet available. > > Is the switches::kLang typically present in the command line arguments on > Windows? > I think it's only present if the user explicitly adds it. > Also I wonder if we could just obtain the system UI locale via Win32 API. Do we do this anywhere else? I couldn't find an example. > > > On 2013/04/11 18:29:29, Nico wrote: > >> I'm not terribly familiar with resource bundles. tony@ is, and it looks >> like >> markusheintz@ did something similar in the past. >> > > > https://codereview.chromium.**org/13825003/diff/1/chrome/** > browser/user_data_dir_**extractor_win.cc<https://codereview.chromium.org/13825003/diff/1/chrome/browser/user_data_dir_extractor_win.cc> > >> File chrome/browser/user_data_dir_**extractor_win.cc (right): >> > > > https://codereview.chromium.**org/13825003/diff/1/chrome/** > browser/user_data_dir_**extractor_win.cc#newcode53<https://codereview.chromium.org/13825003/diff/1/chrome/browser/user_data_dir_extractor_win.cc#newcode53> > >> chrome/browser/user_data_dir_**extractor_win.cc:53: >> ResourceBundle::**InitSharedInstanceWithLocale("**en-US", NULL); >> Is "en-US" correct? I suppose prefs::kApplicationLocale isn't ready yet, >> but >> > do > >> you want to use command_line.**GetSwitchValueASCII(switches::**kLang)? >> > > > > https://codereview.chromium.**org/13825003/<https://codereview.chromium.org/1... >
Tony's ooo responder informs me that we should ask Jungshik about the locale question. Jungshik: Can you look at this CL? This code runs before the profile is initialized.
https://codereview.chromium.org/13825003/diff/1/chrome/browser/user_data_dir_... File chrome/browser/user_data_dir_extractor_win.cc (right): https://codereview.chromium.org/13825003/diff/1/chrome/browser/user_data_dir_... chrome/browser/user_data_dir_extractor_win.cc:53: ResourceBundle::InitSharedInstanceWithLocale("en-US", NULL); I've added a check for command line locale switch. Please review the Patch Set #2, thanks. On 2013/04/11 18:29:29, Nico wrote: > Is "en-US" correct? I suppose prefs::kApplicationLocale isn't ready yet, but do > you want to use command_line.GetSwitchValueASCII(switches::kLang)?
https://codereview.chromium.org/13825003/diff/24001/chrome/browser/user_data_... File chrome/browser/user_data_dir_extractor_win.cc (right): https://codereview.chromium.org/13825003/diff/24001/chrome/browser/user_data_... chrome/browser/user_data_dir_extractor_win.cc:60: locale = kDefaultUserDataDirDialogLocale; Can we do better than this? Is this only for Windows? Why does this happen only sometimes but not always? Falling back to en-US is not that great (it's really last last resort). Is it possible to use our UI-language-determination logic here ? Then, I'd use that instead of falling back to en-US. If it's too much for M26 merge, please add TODO to that effect and use that approach in the trunk after the merge.
Jungshik - To give you some background: the ShowUserDataDirDialog is a Windows-specific dialog that is only shown in rare situations where Chrome is either not installed properly, or is run from a standalone folder (for example a canary build), and Chrome somehow is unable to access user's data folder -- this is usually C:\Users\<username>\ or some kind of subdirectory under "My Documents" for earlier Windows versions. The dialog prompts user to select a different folder where the profile and local state will be stored. After selection, chrome will restart with the correct user data dir. Due to the nature of this, none of the user's profile and preferences are available at the point when the dialog is shown. We only have the Operating Systems's UI language and the command-line switches available. On 2013/04/11 21:57:55, Jungshik Shin wrote: > https://codereview.chromium.org/13825003/diff/24001/chrome/browser/user_data_... > File chrome/browser/user_data_dir_extractor_win.cc (right): > > https://codereview.chromium.org/13825003/diff/24001/chrome/browser/user_data_... > chrome/browser/user_data_dir_extractor_win.cc:60: locale = > kDefaultUserDataDirDialogLocale; > Can we do better than this? Is this only for Windows? > Why does this happen only sometimes but not always? > Falling back to en-US is not that great (it's really last last resort). > > Is it possible to use our UI-language-determination logic here ? Then, I'd use > that instead of falling back to en-US. > > If it's too much for M26 merge, please add TODO to that effect and use that > approach in the trunk after the merge.
BTW Junshik - could you point me to where our UI-language-determination logic is? If there is some additional information that can help determine the locale that would certainly be useful.
Sorry that I didn't point you where that is. (and also thank you for the detailed explanation). Can you use GetApplicationLocale in l10n_util.cc? On 2013/04/11 22:17:30, hshi1 wrote: > BTW Junshik - could you point me to where our UI-language-determination logic > is? If there is some additional information that can help determine the locale > that would certainly be useful.
https://codereview.chromium.org/13825003/diff/24001/chrome/browser/user_data_... File chrome/browser/user_data_dir_extractor_win.cc (right): https://codereview.chromium.org/13825003/diff/24001/chrome/browser/user_data_... chrome/browser/user_data_dir_extractor_win.cc:60: locale = kDefaultUserDataDirDialogLocale; Can you review Patch Set #4? Thanks! On 2013/04/11 21:57:55, Jungshik Shin wrote: > Can we do better than this? Is this only for Windows? > Why does this happen only sometimes but not always? > Falling back to en-US is not that great (it's really last last resort). > > Is it possible to use our UI-language-determination logic here ? Then, I'd use > that instead of falling back to en-US. > > If it's too much for M26 merge, please add TODO to that effect and use that > approach in the trunk after the merge.
LGTM thanks. On 2013/04/11 23:05:33, hshi1 wrote: > https://codereview.chromium.org/13825003/diff/24001/chrome/browser/user_data_... > File chrome/browser/user_data_dir_extractor_win.cc (right): > > https://codereview.chromium.org/13825003/diff/24001/chrome/browser/user_data_... > chrome/browser/user_data_dir_extractor_win.cc:60: locale = > kDefaultUserDataDirDialogLocale; > Can you review Patch Set #4? Thanks! > > On 2013/04/11 21:57:55, Jungshik Shin wrote: > > Can we do better than this? Is this only for Windows? > > Why does this happen only sometimes but not always? > > Falling back to en-US is not that great (it's really last last resort). > > > > Is it possible to use our UI-language-determination logic here ? Then, I'd use > > that instead of falling back to en-US. > > > > If it's too much for M26 merge, please add TODO to that effect and use that > > approach in the trunk after the merge.
lgtm too then
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hshi@chromium.org/13825003/31001
Message was sent while issue was closed.
Change committed as 193844
Message was sent while issue was closed.
Jungshik, The crbug.com is currently down so I'm commenting here. Unfortunately this CL doesn't work, we're still observing CHECK failures due to uninitialized ResourceBundle on the latest win-dev builds. Sample crash report: https://crash.corp.google.com/reportdetail?reportid=8a2d767ec481a44a Problem: l10n_util::GetApplicationLocale calls l10n_util::IsLocaleAvailable which relies on ResourceBundle being already initialized. So it is not safe to use this. I suggest to fix this by adding a ResourceBundle::HasSharedInstance() in l10n_util::IsLocaleAvailable and return false if the ResourceBundle is not initialized. |
