Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2806613002:
Check IME state in ImeController (Closed)
|
Created:
3 years, 8 months ago by Wenzhao (Colin) Zang Modified:
3 years, 7 months ago CC:
chromium-reviews, kalyank, sadrul, rkc, tbarzic, xiyuan Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionCheck IME state in ImeController
ImeController::CanCycleIme() and ImeController::CanSwitchIme() should first
check if IME state is NULL and if it is, ignore the cycle / switch action
because it does not make sense before IME initialization.
BUG=703510
Review-Url: https://codereview.chromium.org/2806613002
Cr-Commit-Position: refs/heads/master@{#467804}
Committed: https://chromium.googlesource.com/chromium/src/+/f8c865910c21c379df39eb057530e13630d2f00d
Patch Set 1 #Patch Set 2 : Postpone message box until profile is initialized #
Total comments: 2
Patch Set 3 : Add active user check #
Total comments: 2
Patch Set 4 : Add comments #
Total comments: 4
Patch Set 5 : Change according to comments #
Total comments: 1
Patch Set 6 : Delete unnecessary files #
Total comments: 2
Patch Set 7 : Check IME state #
Total comments: 4
Patch Set 8 : Change according to comments #Messages
Total messages: 41 (18 generated)
Description was changed from ========== Add CanCycleIme NULL pointer checking BUG=703510 ========== to ========== Add CanCycleIme NULL pointer checking Before calling CanCycleInputMethod(), one needs to make sure that IME state has been set, which is not done until the profile initialization is finished, so if a user tries to cycle through IMEs before that it'll crash. BUG=703510 ==========
wzang@chromium.org changed reviewers: + alemate@chromium.org
The value of the IME state remains NULL until it's set in PostProfileInit(), so if a user tries to cycle through IMEs using 'ctrl + space' shortcut before profile is initialized, it'll crash. Please see: https://cs.chromium.org/chromium/src/chrome/browser/chromeos/chrome_browser_m... I don't think chrome::ShowWarningMessageBox() is the problem though. The message box itself does not prompt users to cycle through IMEs, so it seems the crash will only happen if the user randomly use the shortcut when the message box is present. In addition, I don't really think the bug is reproducible in a real user case, because when one extension fails loading, it won't block others, so by the time the user sees the message box, IME should have been loaded. There won't be a freezing point, as the fuzzer might think.
On 2017/04/06 22:11:44, Wenzhao (Colin) Zang wrote: > The value of the IME state remains NULL until it's set in PostProfileInit(), so > if a user tries to cycle through IMEs using 'ctrl + space' shortcut before > profile is initialized, it'll crash. Please see: > https://cs.chromium.org/chromium/src/chrome/browser/chromeos/chrome_browser_m... > > I don't think chrome::ShowWarningMessageBox() is the problem though. The message > box itself does not prompt users to cycle through IMEs, so it seems the crash > will only happen if the user randomly use the shortcut when the message box is > present. In addition, I don't really think the bug is reproducible in a real > user case, because when one extension fails loading, it won't block others, so > by the time the user sees the message box, IME should have been loaded. There > won't be a freezing point, as the fuzzer might think. To me the problem sounds like "input event is delivered to a wrong user". I.e. there must be no input events until extensions are initialized. During signin, we first initialize "Signin user", then show OOBE/signin UI, then user logs in, new profile is initialized and then we show UI. You cannot have WebUI without a valid profile. So I don't agree with this fix.
Description was changed from ========== Add CanCycleIme NULL pointer checking Before calling CanCycleInputMethod(), one needs to make sure that IME state has been set, which is not done until the profile initialization is finished, so if a user tries to cycle through IMEs before that it'll crash. BUG=703510 ========== to ========== Add CanCycleIme NULL pointer checking We should not allow displaying messages before profile initialization is finished. All the noisy alert boxes for extension loading errors should then be postponed. We also need to make sure they are shown after IME is initialized, otherwise IME state is NULL and might cause crashes such as this bug. BUG=703510 ==========
Description was changed from ========== Add CanCycleIme NULL pointer checking We should not allow displaying messages before profile initialization is finished. All the noisy alert boxes for extension loading errors should then be postponed. We also need to make sure they are shown after IME is initialized, otherwise IME state is NULL and might cause crashes such as this bug. BUG=703510 ========== to ========== Add CanCycleIme NULL pointer checking We should not allow displaying messages before profile initialization is finished. All the noisy alert boxes for extension loading errors should then be postponed. We also need to make sure they are shown after IME is initialized, otherwise IME state is NULL and might cause crashes such as this bug. BUG=703510 ==========
Description was changed from ========== Add CanCycleIme NULL pointer checking We should not allow displaying messages before profile initialization is finished. All the noisy alert boxes for extension loading errors should then be postponed. We also need to make sure they are shown after IME is initialized, otherwise IME state is NULL and might cause crashes such as this bug. BUG=703510 ========== to ========== Add CanCycleIme NULL pointer checking We should not allow displaying messages before profile initialization is finished. All the noisy alert boxes for extension loading errors should then be postponed. We also need to make sure they are shown after IME is initialized, otherwise IME state is NULL and might cause crashes such as this bug. BUG=703510 ==========
Description was changed from ========== Add CanCycleIme NULL pointer checking We should not allow displaying messages before profile initialization is finished. All the noisy alert boxes for extension loading errors should then be postponed. We also need to make sure they are shown after IME is initialized, otherwise IME state is NULL and might cause crashes such as this bug. BUG=703510 ========== to ========== Postpone extension loading error message box until profile is initialized We should not allow displaying messages before profile initialization is finished. All the noisy alert boxes for extension loading errors should then be postponed. We also need to make sure they are shown after IME is initialized, otherwise IME state is NULL and might cause crashes such as this bug. BUG=703510 ==========
Description was changed from ========== Postpone extension loading error message box until profile is initialized We should not allow displaying messages before profile initialization is finished. All the noisy alert boxes for extension loading errors should then be postponed. We also need to make sure they are shown after IME is initialized, otherwise IME state is NULL and might cause crashes such as this bug. BUG=703510 ========== to ========== Postpone message box until profile is initialized We should not allow displaying messages before profile initialization is finished. All the noisy alert boxes for extension loading errors should then be postponed. We also need to make sure they are shown after IME is initialized, otherwise IME state is NULL and might cause crashes such as this bug. BUG=703510 ==========
At first I wanted to add ExtensionErrorReporter as an observer for ProfileManager, but I took the current approach instead because ExtensionErrorReporter has a singleton object which is available in ChromeBrowserMainPartsChromeos, in addition we need to wait until IME state is set in order to solve the crash. So it was added in ChromeBrowserMainPartsChromeos::PostProfileInit().
alemate@, friendly ping. Thanks.
https://codereview.chromium.org/2806613002/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right): https://codereview.chromium.org/2806613002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/chrome_browser_main_chromeos.cc:714: ExtensionErrorReporter::GetInstance()->OnProfileInitialized(); You need to pass profile() here and later check that this is a correct profile. We may be restarting after crash in multiprofile mode so that several profiles are initialized asynchronously.
https://codereview.chromium.org/2806613002/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right): https://codereview.chromium.org/2806613002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/chrome_browser_main_chromeos.cc:714: ExtensionErrorReporter::GetInstance()->OnProfileInitialized(); On 2017/04/19 21:53:32, Alexander Alekseev wrote: > You need to pass profile() here and later check that this is a correct profile. > We may be restarting after crash in multiprofile mode so that several profiles > are initialized asynchronously. Done.
lgtm
https://codereview.chromium.org/2806613002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right): https://codereview.chromium.org/2806613002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/chrome_browser_main_chromeos.cc:754: if (ProfileHelper::Get()->GetUserByProfile(profile())->is_active()) Maybe we should add more details to this comment. --------------- This works because ExtensionErrors are always reported in context of active user, even if they refer to another profile. So if we are in the process of restart after crash in multiprofile mode, primary user profile will be initialized first, so it is safe to display error balloons after first user pfofile is initialized.
wzang@chromium.org changed reviewers: + tbarzic@chromium.org, xiyuan@chromium.org
xiyuan@, would you please review the small changes in chrome/browser/chromeos/chrome_browser_main_chromeos.cc? Thanks a lot. tbarzic@, would you please review the small changes under chrome/browser/extensions/? Thanks a lot.
https://codereview.chromium.org/2806613002/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right): https://codereview.chromium.org/2806613002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/chrome_browser_main_chromeos.cc:756: if (ProfileHelper::Get()->GetUserByProfile(profile())->is_active()) Most of the time (i.e. NOT the crash-n-restart case), profile() is the sign-in profile. There is no user associated with it. So, this line would probably crash. https://codereview.chromium.org/2806613002/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/extension_error_reporter.h (right): https://codereview.chromium.org/2806613002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/extension_error_reporter.h:90: bool should_postpone_messages_ = true; Think ExtensionErrorReporter code also runs for desktop chrome (i.e. non-chromeos) and who resets this flag on those platforms?
The signin profile check is added. Thanks. https://codereview.chromium.org/2806613002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right): https://codereview.chromium.org/2806613002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/chrome_browser_main_chromeos.cc:754: if (ProfileHelper::Get()->GetUserByProfile(profile())->is_active()) On 2017/04/24 11:43:27, Alexander Alekseev wrote: > Maybe we should add more details to this comment. > --------------- > This works because ExtensionErrors are always reported in context of active > user, even if they refer to another profile. > > So if we are in the process of restart after crash in multiprofile mode, primary > user profile will be initialized first, so it is safe to display error balloons > after first user pfofile is initialized. Done. https://codereview.chromium.org/2806613002/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right): https://codereview.chromium.org/2806613002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/chrome_browser_main_chromeos.cc:756: if (ProfileHelper::Get()->GetUserByProfile(profile())->is_active()) On 2017/04/24 18:21:51, xiyuan wrote: > Most of the time (i.e. NOT the crash-n-restart case), profile() is the sign-in > profile. There is no user associated with it. So, this line would probably > crash. Done.
https://codereview.chromium.org/2806613002/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right): https://codereview.chromium.org/2806613002/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/chrome_browser_main_chromeos.cc:756: if (ProfileHelper::IsSigninProfile(profile()) || Isn't this code run only for default profile (which will be sign-in profile in the normal circumstances)? Should extension error reporting be disabled for user profiles as well (I imagine the same issue that is being fixed here applies to user profiles)? https://codereview.chromium.org/2806613002/diff/100001/chrome/browser/extensi... File chrome/browser/extensions/extension_error_reporter.cc (right): https://codereview.chromium.org/2806613002/diff/100001/chrome/browser/extensi... chrome/browser/extensions/extension_error_reporter.cc:93: chrome::ShowWarningMessageBox( it would be nice to condense multiple messages into a single warning, but I guess even before this we'd show a message box per error, so it's not really making things worse.
https://codereview.chromium.org/2806613002/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/extension_error_reporter.h (right): https://codereview.chromium.org/2806613002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/extension_error_reporter.h:90: bool should_postpone_messages_ = true; On 2017/04/24 18:21:51, xiyuan wrote: > Think ExtensionErrorReporter code also runs for desktop chrome (i.e. > non-chromeos) and who resets this flag on those platforms? This would essentially disable extension error reporting on non-chromeos platform. Are you sure this is intended? https://codereview.chromium.org/2806613002/diff/80001/mojo/public/interfaces/... File mojo/public/interfaces/bindings/tests/data/validation/conformance_mthd8_good.expected~HEAD (right): https://codereview.chromium.org/2806613002/diff/80001/mojo/public/interfaces/... mojo/public/interfaces/bindings/tests/data/validation/conformance_mthd8_good.expected~HEAD:1: PASS What is this file?
Description was changed from ========== Postpone message box until profile is initialized We should not allow displaying messages before profile initialization is finished. All the noisy alert boxes for extension loading errors should then be postponed. We also need to make sure they are shown after IME is initialized, otherwise IME state is NULL and might cause crashes such as this bug. BUG=703510 ========== to ========== Check IME state in ImeController ImeController::CanCycleIme() and ImeController::CanSwitchIme() should first check if IME state is NULL and if it is, ignore the cycle / switch action because it does not make sense before IME initialization. BUG=703510 ==========
wzang@chromium.org changed reviewers: - tbarzic@chromium.org, xiyuan@chromium.org
wzang@chromium.org changed reviewers: + stevenjb@chromium.org
alemate@, stevenjb@: please take a look at this small CL. Thanks so much. tbarzic@, xiyuan@: I'm using a different approach which does not make changes to those files. Thanks for the previous comments.
Actually Colin persuaded me that the first version was the best one in this complicate case. So we probably should land it. lgtm.
xiyuan@chromium.org changed reviewers: + xiyuan@chromium.org
lgtm with nits https://codereview.chromium.org/2806613002/diff/120001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/ime_controller_chromeos.cc (right): https://codereview.chromium.org/2806613002/diff/120001/chrome/browser/ui/ash/... chrome/browser/ui/ash/ime_controller_chromeos.cc:14: if (manager->GetActiveIMEState() == NULL) { nit: NULL -> nullptr of if (!manager->GetActiveIMEState()) { https://codereview.chromium.org/2806613002/diff/120001/chrome/browser/ui/ash/... chrome/browser/ui/ash/ime_controller_chromeos.cc:38: if (manager->GetActiveIMEState() == NULL) { nit: ditto
https://codereview.chromium.org/2806613002/diff/120001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/ime_controller_chromeos.cc (right): https://codereview.chromium.org/2806613002/diff/120001/chrome/browser/ui/ash/... chrome/browser/ui/ash/ime_controller_chromeos.cc:14: if (manager->GetActiveIMEState() == NULL) { On 2017/04/27 03:16:41, xiyuan wrote: > nit: NULL -> nullptr > > of if (!manager->GetActiveIMEState()) { Done. https://codereview.chromium.org/2806613002/diff/120001/chrome/browser/ui/ash/... chrome/browser/ui/ash/ime_controller_chromeos.cc:38: if (manager->GetActiveIMEState() == NULL) { On 2017/04/27 03:16:41, xiyuan wrote: > nit: ditto Done.
As long as the WARNING does not show up under normal operation (on a device or when running on linux, e.g. for browser tests), this seems fine. lgtm
The CQ bit was checked by wzang@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from alemate@chromium.org, xiyuan@chromium.org Link to the patchset: https://codereview.chromium.org/2806613002/#ps140001 (title: "Change according to comments")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by wzang@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 140001, "attempt_start_ts": 1493328392689780,
"parent_rev": "19c7980bb940e1fe442c13e430ac9f7e2f35a220", "commit_rev":
"f8c865910c21c379df39eb057530e13630d2f00d"}
Message was sent while issue was closed.
Description was changed from ========== Check IME state in ImeController ImeController::CanCycleIme() and ImeController::CanSwitchIme() should first check if IME state is NULL and if it is, ignore the cycle / switch action because it does not make sense before IME initialization. BUG=703510 ========== to ========== Check IME state in ImeController ImeController::CanCycleIme() and ImeController::CanSwitchIme() should first check if IME state is NULL and if it is, ignore the cycle / switch action because it does not make sense before IME initialization. BUG=703510 Review-Url: https://codereview.chromium.org/2806613002 Cr-Commit-Position: refs/heads/master@{#467804} Committed: https://chromium.googlesource.com/chromium/src/+/f8c865910c21c379df39eb057530... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://chromium.googlesource.com/chromium/src/+/f8c865910c21c379df39eb057530... |
