Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1304943006:
ProfileChooserVIew heap-use-after-free fix (Closed)
DescriptionDo not show avatar_bubble if we can't show avatar_menu
When profile path is outside user_data_dir, the profile
manager does not show an avatar_menu. This CL disables
showing an avatar window if there is no avatar menu.
BUG=527505
Committed: https://crrev.com/80acb6c51d9897918d73cf8b7a272d3878c93ddd
Cr-Commit-Position: refs/heads/master@{#349191}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Set the parent window of profile_chooser_view #Patch Set 3 : small fix #
Total comments: 3
Patch Set 4 : Do not show avatar_bubble if we can't show avatar_menu #
Total comments: 1
Patch Set 5 : update comments #
Total comments: 7
Patch Set 6 : re comments #Patch Set 7 : remove un-needed import #
Messages
Total messages: 30 (9 generated)
estade@chromium.org changed reviewers: + estade@chromium.org
https://codereview.chromium.org/1304943006/diff/1/chrome/browser/ui/views/pro... File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right): https://codereview.chromium.org/1304943006/diff/1/chrome/browser/ui/views/pro... chrome/browser/ui/views/profiles/profile_chooser_view.cc:706: browser_ = NULL; There are many places in this file that reference browser_ without null checking, so this may fix this one problem but still allow us to crash in other cases. ~Browser as well as BrowserList::RemoveBrowser is in the call stack already so it seems like it's almost working as intended (this view is automatically cleaned up when the browser is destroyed). I don't understand what the problem is actually. browser_ should still be valid (it's being destructed, not already destructed). Is |browser_| different from the browser currently being destroyed? Perhaps due to injecting some mock browser object for testing? also nit: s/NULL/nullptr
On 2015/09/03 17:38:39, Evan Stade wrote: > https://codereview.chromium.org/1304943006/diff/1/chrome/browser/ui/views/pro... > File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right): > > https://codereview.chromium.org/1304943006/diff/1/chrome/browser/ui/views/pro... > chrome/browser/ui/views/profiles/profile_chooser_view.cc:706: browser_ = NULL; > There are many places in this file that reference browser_ without null > checking, so this may fix this one problem but still allow us to crash in other > cases. Right, having the assumption that ProfileChooserView could exist with |browser_| being NULL complicates the whole flow in this file. > ~Browser as well as BrowserList::RemoveBrowser is in the call stack already so > it seems like it's almost working as intended (this view is automatically > cleaned up when the browser is destroyed). I don't understand what the problem > is actually. browser_ should still be valid (it's being destructed, not already > destructed). Is |browser_| different from the browser currently being destroyed? > Perhaps due to injecting some mock browser object for testing? Yes, the problem is that |browser_| is different than the browser currently being destroyed. As far as I can tell, that is because the view was only destroyed when the "last" browser is destroyed. That is done when browserList removes the last browser: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/... I am not sure why the view wasn't destroyed at the time |browser_| was destroyed. It could be due to how CloseAllBrowsers() works. All the browsers in the test are created as new Browser() objects here: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/syn...
https://codereview.chromium.org/1304943006/diff/1/chrome/browser/ui/views/pro... File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right): https://codereview.chromium.org/1304943006/diff/1/chrome/browser/ui/views/pro... chrome/browser/ui/views/profiles/profile_chooser_view.cc:615: BrowserList::AddObserver(this); So if we're not closing the bubble when its browser goes away, that sounds like a bug and this just papers over it. What if you call set_parent_window() as is done for the bookmark bubble[1]? I suspect that would tie the bubble's lifetime to the browser. [1] https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...
shadi@chromium.org changed reviewers: + msw@chromium.org - estade@chromium.org
msw@: This is actually a follow up on https://codereview.chromium.org/1311123003/ There the fix for the tests was to hide the view after it was up (which is not a real fix to root problem). This fix is more stable and generic. PTAL. I was able to repro the failures and created a browser test for it. This solution is similar to what is done in bookmark bubble[1] as estade@ suggested. [1] https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...
https://codereview.chromium.org/1304943006/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right): https://codereview.chromium.org/1304943006/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/profiles/profile_chooser_view.cc:568: BrowserView* browser_view = static_cast<BrowserView*>(browser->window()); nit: use BrowserView::GetBrowserViewForBrowser https://codereview.chromium.org/1304943006/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/profiles/profile_chooser_view.cc:570: browser_view->GetActiveWebContents()->GetNativeView()); Setting the parent of the profile bubble to the active web contents' window doesn't make sense... Shouldn't it use the associated *browser* window? AFAICT, that should already be done just by passing |anchor_view| (unless that's null for fullscreen, etc.). When is the |browser| being destroyed in relation to this crash? The anchor should be reset when the Browser widget's destruction calls OnWidgetDestroying, and (more pertinently) I think that parent window destruction should trigger child window destruction, but I can't easily find that code path. So, I think the defect here is that in step 4 on the bug the deletion of Browser B doesn't trigger bubble destruction. https://codereview.chromium.org/1304943006/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/profiles/profile_chooser_view_browsertest.cc (right): https://codereview.chromium.org/1304943006/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/profiles/profile_chooser_view_browsertest.cc:211: // deleted. Even if the profile is not in user_data_dir. Why does the profile's location matter for this test?
Patchset #4 (id:60001) has been deleted
As mentioned in the bug, the problem was actually that the anchor_view was null in some special cases. The anchor view was supposed to be the avatar menu button. My last patchset disables showing an avatar bubble if the avatar menu does not exist. PTAL
https://codereview.chromium.org/1304943006/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/frame/browser_view.cc (right): https://codereview.chromium.org/1304943006/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/frame/browser_view.cc:2525: // Do not show avatar bubble if there is no avatar menu button. Instead of a no-op, just use the chrome menu button as the anchor here to see if that fixes the defect. In https://codereview.chromium.org/264303006 I worked to enable the avatar bubble (for the CTRL+M accelerator) in fullscreen mode and in single user profile cases (where the button doesn't appear). In that CL I anchored the bubble to (1) the chrome menu button for single-profile restored windows or (2) the fullscreen bubble itself in fullscreen. If we really don't want to support those cases, you should remove some of my earlier work (and likely restore the profile manager observation to enable/disable the bubble command when the profile count changes from 1<->2).
On 2015/09/14 17:12:04, msw wrote: > https://codereview.chromium.org/1304943006/diff/80001/chrome/browser/ui/views... > File chrome/browser/ui/views/frame/browser_view.cc (right): > > https://codereview.chromium.org/1304943006/diff/80001/chrome/browser/ui/views... > chrome/browser/ui/views/frame/browser_view.cc:2525: // Do not show avatar bubble > if there is no avatar menu button. > Instead of a no-op, just use the chrome menu button as the anchor here to see if > that fixes the defect. In https://codereview.chromium.org/264303006 I worked to > enable the avatar bubble (for the CTRL+M accelerator) in fullscreen mode and in > single user profile cases (where the button doesn't appear). In that CL I > anchored the bubble to (1) the chrome menu button for single-profile restored > windows or (2) the fullscreen bubble itself in fullscreen. > > If we really don't want to support those cases, you should remove some of my > earlier work (and likely restore the profile manager observation to > enable/disable the bubble command when the profile count changes from 1<->2). Does the avatar bubble actually do anything useful when GetIndexOfProfileWithPath() returns npos? Your earlier work addressed actual end-user use cases, whereas right now we're just trying to support a weird configuration only used for testing.
On 2015/09/15 17:55:30, Evan Stade (ooo) wrote: > On 2015/09/14 17:12:04, msw wrote: > > > https://codereview.chromium.org/1304943006/diff/80001/chrome/browser/ui/views... > > File chrome/browser/ui/views/frame/browser_view.cc (right): > > > > > https://codereview.chromium.org/1304943006/diff/80001/chrome/browser/ui/views... > > chrome/browser/ui/views/frame/browser_view.cc:2525: // Do not show avatar > bubble > > if there is no avatar menu button. > > Instead of a no-op, just use the chrome menu button as the anchor here to see > if > > that fixes the defect. In https://codereview.chromium.org/264303006 I worked > to > > enable the avatar bubble (for the CTRL+M accelerator) in fullscreen mode and > in > > single user profile cases (where the button doesn't appear). In that CL I > > anchored the bubble to (1) the chrome menu button for single-profile restored > > windows or (2) the fullscreen bubble itself in fullscreen. > > > > If we really don't want to support those cases, you should remove some of my > > earlier work (and likely restore the profile manager observation to > > enable/disable the bubble command when the profile count changes from 1<->2). > > Does the avatar bubble actually do anything useful when > GetIndexOfProfileWithPath() returns npos? > > Your earlier work addressed actual end-user use cases, whereas right now we're > just trying to support a weird configuration only used for testing. Yeah, I just don't want this change to break any cases that users encounter, if that case really only happens in tests, then it might be fine to stifle the bubble then. Please check edge cases to ensure this doesn't break fullscreen, single-profile, etc.
Patchset #5 (id:100001) has been deleted
Patchset #5 (id:120001) has been deleted
On 2015/09/15 18:25:10, msw wrote: > On 2015/09/15 17:55:30, Evan Stade (ooo) wrote: > > On 2015/09/14 17:12:04, msw wrote: > > > > > > https://codereview.chromium.org/1304943006/diff/80001/chrome/browser/ui/views... > > > File chrome/browser/ui/views/frame/browser_view.cc (right): > > > > > > > > > https://codereview.chromium.org/1304943006/diff/80001/chrome/browser/ui/views... > > > chrome/browser/ui/views/frame/browser_view.cc:2525: // Do not show avatar > > bubble > > > if there is no avatar menu button. > > > Instead of a no-op, just use the chrome menu button as the anchor here to > see > > if > > > that fixes the defect. In https://codereview.chromium.org/264303006 I worked > > to > > > enable the avatar bubble (for the CTRL+M accelerator) in fullscreen mode and > > in > > > single user profile cases (where the button doesn't appear). In that CL I > > > anchored the bubble to (1) the chrome menu button for single-profile > restored > > > windows or (2) the fullscreen bubble itself in fullscreen. > > > > > > If we really don't want to support those cases, you should remove some of my > > > earlier work (and likely restore the profile manager observation to > > > enable/disable the bubble command when the profile count changes from > 1<->2). > > > > Does the avatar bubble actually do anything useful when > > GetIndexOfProfileWithPath() returns npos? > > > > Your earlier work addressed actual end-user use cases, whereas right now we're > > just trying to support a weird configuration only used for testing. > > Yeah, I just don't want this change to break any cases that users encounter, if > that case really only happens in tests, then it might be fine to stifle the > bubble then. Please check edge cases to ensure this doesn't break fullscreen, > single-profile, etc. As Evan said, this should not happen in real user cases. With a normal profile, everything is still good. Even though this surfaced through a special case test, I think it is worth fixing anyway. Since you guys agreed on doing a no-op when avatar menu is not available, then PTAL at the CL :) Thanks!
If this does indeed work in full-screen, single-profile, and other user-accessible edge cases; and if we really don't care about users manually setting profile directories outside the user data dir; then I suppose the general approach is okay and I just have some minor comments. https://codereview.chromium.org/1304943006/diff/140001/chrome/browser/ui/view... File chrome/browser/ui/views/profiles/profile_chooser_view_browsertest.cc (right): https://codereview.chromium.org/1304943006/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view_browsertest.cc:36: Profile* CreateTestingProfile(const std::string& profile_name) { Why doesn't this use chrome/test/base/testing_profile[_manager]? https://codereview.chromium.org/1304943006/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view_browsertest.cc:60: Profile* profile = Should this be a TestingProfile instance? https://codereview.chromium.org/1304943006/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view_browsertest.cc:66: void CloseBrowser(Browser* browser) { As an ExtensionBrowserTest, which is an InProcessBrowserTest, call InProcessBrowserTest::CloseBrowserSynchronously in the test fixture, instead of duplicating its code here.
shadi@google.com changed reviewers: + shadi@google.com
https://codereview.chromium.org/1304943006/diff/140001/chrome/browser/ui/view... File chrome/browser/ui/views/profiles/profile_chooser_view_browsertest.cc (right): https://codereview.chromium.org/1304943006/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view_browsertest.cc:36: Profile* CreateTestingProfile(const std::string& profile_name) { On 2015/09/15 19:44:35, msw wrote: > Why doesn't this use chrome/test/base/testing_profile[_manager]? I don't know. I think using TestingProfileManager::CreateTestingProfile(..) should be a good replacement for this. I'll take a look. https://codereview.chromium.org/1304943006/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view_browsertest.cc:60: Profile* profile = On 2015/09/15 19:44:36, msw wrote: > Should this be a TestingProfile instance? Not sure what you mean by that? TestingProfileManager::CreateTestingProfile() does not have an option to create a profile with path outside user data dir. Since this is a special case test, I think having this function here is fair. https://codereview.chromium.org/1304943006/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view_browsertest.cc:66: void CloseBrowser(Browser* browser) { On 2015/09/15 19:44:36, msw wrote: > As an ExtensionBrowserTest, which is an InProcessBrowserTest, call > InProcessBrowserTest::CloseBrowserSynchronously in the test fixture, instead of > duplicating its code here. Cool, thanks for pointing that function out.
https://codereview.chromium.org/1304943006/diff/140001/chrome/browser/ui/view... File chrome/browser/ui/views/profiles/profile_chooser_view_browsertest.cc (right): https://codereview.chromium.org/1304943006/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view_browsertest.cc:60: Profile* profile = On 2015/09/15 20:47:43, shadi1 wrote: > On 2015/09/15 19:44:36, msw wrote: > > Should this be a TestingProfile instance? > > Not sure what you mean by that? > > TestingProfileManager::CreateTestingProfile() does not have an option to > create a profile with path outside user data dir. Since this is a special case > test, I think having this function here is fair. If it's a profile used for testing, perhaps it should be a TestingProfile instance. Check out its 3 ctors that takes more args (ie. a |path| arg): https://code.google.com/p/chromium/codesearch#chromium/src/chrome/test/base/t...
On 2015/09/15 21:40:46, msw wrote: > https://codereview.chromium.org/1304943006/diff/140001/chrome/browser/ui/view... > File chrome/browser/ui/views/profiles/profile_chooser_view_browsertest.cc > (right): > > https://codereview.chromium.org/1304943006/diff/140001/chrome/browser/ui/view... > chrome/browser/ui/views/profiles/profile_chooser_view_browsertest.cc:60: > Profile* profile = > On 2015/09/15 20:47:43, shadi1 wrote: > > On 2015/09/15 19:44:36, msw wrote: > > > Should this be a TestingProfile instance? > > > > Not sure what you mean by that? > > > > TestingProfileManager::CreateTestingProfile() does not have an option to > > create a profile with path outside user data dir. Since this is a special case > > test, I think having this function here is fair. > > If it's a profile used for testing, perhaps it should be a TestingProfile > instance. Check out its 3 ctors that takes more args (ie. a |path| arg): > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/test/base/t... I think the reason TestingProfileManager was not used is because it does not work with InProcessBrowserTests. All its current use cases are in unit_tests. Doing a simple setup I got these failures: ../../chrome/test/base/scoped_testing_local_state.cc:15: Failure Value of: browser_process->local_state() Actual: true Expected: false ../../chrome/test/base/testing_profile_manager.cc:234: Failure Value of: browser_process_->profile_manager() Actual: true Expected: false ProfileManager already exists ../../chrome/browser/ui/views/profiles/profile_chooser_view_browsertest.cc:83: Failure Value of: profile_manager_->SetUp() Actual: false Expected: true followed by a heap-buffer-overflow... If you think expanding TestingProfileManager to InProcessBrowserTests is valuable we should open a bug for it.
On 2015/09/16 01:39:05, shadi wrote: > On 2015/09/15 21:40:46, msw wrote: > > > https://codereview.chromium.org/1304943006/diff/140001/chrome/browser/ui/view... > > File chrome/browser/ui/views/profiles/profile_chooser_view_browsertest.cc > > (right): > > > > > https://codereview.chromium.org/1304943006/diff/140001/chrome/browser/ui/view... > > chrome/browser/ui/views/profiles/profile_chooser_view_browsertest.cc:60: > > Profile* profile = > > On 2015/09/15 20:47:43, shadi1 wrote: > > > On 2015/09/15 19:44:36, msw wrote: > > > > Should this be a TestingProfile instance? > > > > > > Not sure what you mean by that? > > > > > > TestingProfileManager::CreateTestingProfile() does not have an option to > > > create a profile with path outside user data dir. Since this is a special > case > > > test, I think having this function here is fair. > > > > If it's a profile used for testing, perhaps it should be a TestingProfile > > instance. Check out its 3 ctors that takes more args (ie. a |path| arg): > > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/test/base/t... > > I think the reason TestingProfileManager was not used is because it does not > work with InProcessBrowserTests. All its current use cases are in unit_tests. > > Doing a simple setup I got these failures: > ../../chrome/test/base/scoped_testing_local_state.cc:15: Failure > Value of: browser_process->local_state() > Actual: true > Expected: false > ../../chrome/test/base/testing_profile_manager.cc:234: Failure > Value of: browser_process_->profile_manager() > Actual: true > Expected: false > ProfileManager already exists > ../../chrome/browser/ui/views/profiles/profile_chooser_view_browsertest.cc:83: > Failure > Value of: profile_manager_->SetUp() > Actual: false > Expected: true > > followed by a heap-buffer-overflow... > > If you think expanding TestingProfileManager to InProcessBrowserTests > is valuable we should open a bug for it. Ah, if TestingProfile doesn't support this test type, ignore my related suggestions.
shadi@chromium.org changed reviewers: - shadi@google.com
PTAL
lgtm
The CQ bit was checked by shadi@chromium.org
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/1304943006/#ps180001 (title: "remove un-needed import")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1304943006/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1304943006/180001
Message was sent while issue was closed.
Committed patchset #7 (id:180001)
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/80acb6c51d9897918d73cf8b7a272d3878c93ddd Cr-Commit-Position: refs/heads/master@{#349191}
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/80acb6c51d9897918d73cf8b7a272d3878c93ddd Cr-Commit-Position: refs/heads/master@{#349191} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
