Clusterfuzz crashes in ash due to later shelf / status area initialization |
|||||
Issue descriptionWe have a slow trickle of clusterfuzz crashes in ash due to pieces of the shelf and/or status area being uninitialized/null during the tests. See blocking issues. The clusterfuzz tests all use gestures. Clusterfuzz in general skips the login screen (similar to browser tests). I suspect what is happening is that the shelf and status area are created slightly later now than they used to be, probably due to mojo-ification of shelf creation. I think clusterfuzz is jamming input events into the first couple spins of the message loop so the events are processed before things are fully built. It's not clear to me that these crashes happen in production. I don't think they do -- I've never gotten a report from the field. Thus far we've added lots of little null-check band-aids. I think we need a more comprehensive solution -- either: * initialize the shelf earlier (like always create it during shelf init, see issue 674773 ), * have a stub shelf during startup, or * synchronously create it during clusterfuzz tests. Ideas, guys? Anybody want to dig into this?
,
May 4 2017
Another possible case. I think it's time to look at this. jdufault, are you actively working on shelf stuff right now? If so we should coordinate so I don't step on your changes.
,
May 4 2017
Native shelf needs to happen by m62, but I haven't started working on it yet. I'll probably start on it in a few weeks.
,
May 4 2017
msw, I'm thinking of making the shelf widget/view always get created during ash::Shell initialization. It can be hidden until the session becomes active. The icons can then be filled in later, after the profile loads. I think this will be a useful step toward native shelf. Do you forsee problems with that approach?
,
May 4 2017
Sounds good, I can't think of any problems off the top of my head, hopefully anything that does come up will be obvious and straightforward to solve :) Lmk if you want help, or just want to pass this to me.
,
May 18 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/788b4fcb1909904cda962cac4a637ffecfa90c1a commit 788b4fcb1909904cda962cac4a637ffecfa90c1a Author: jamescook <jamescook@chromium.org> Date: Thu May 18 16:16:06 2017 chromeos: Refactor shelf to create ShelfView earlier in startup Currently ShelfView is created in the middle of login, after the profile loads, whereas the ShelfWidget and StatusAreaWidget are created on display initialization. We also have complex logic around showing and hiding the shelf based on login state, supervised user creation flows, etc. This makes it difficult to reason about the state of the shelf. Instead, always create the ShelfView when the widget is created. The view stays hidden at login because its window container is hidden. This makes it easier to reason about ShelfView (it's always there). We also want this because we want to replace the Web UI "fake shelf" during OOBE and login with a views-based shelf. It may help with Clusterfuzz crashes we're seeing due to shelf not being fully initialized during the tests. This is a refactor and should not cause behavior changes. BUG= 717559 , 674773 TEST=ash_unittests, browser_tests, manually testing login, ChromeVox, and adding/removing displays TBR=tsepez@chromium.org for rename Review-Url: https://codereview.chromium.org/2889673002 Cr-Commit-Position: refs/heads/master@{#472832} [modify] https://crrev.com/788b4fcb1909904cda962cac4a637ffecfa90c1a/ash/public/interfaces/shelf.mojom [modify] https://crrev.com/788b4fcb1909904cda962cac4a637ffecfa90c1a/ash/root_window_controller.cc [modify] https://crrev.com/788b4fcb1909904cda962cac4a637ffecfa90c1a/ash/root_window_controller.h [modify] https://crrev.com/788b4fcb1909904cda962cac4a637ffecfa90c1a/ash/shelf/shelf_controller.cc [modify] https://crrev.com/788b4fcb1909904cda962cac4a637ffecfa90c1a/ash/shelf/shelf_controller.h [modify] https://crrev.com/788b4fcb1909904cda962cac4a637ffecfa90c1a/ash/shelf/shelf_layout_manager.cc [modify] https://crrev.com/788b4fcb1909904cda962cac4a637ffecfa90c1a/ash/shelf/shelf_locking_manager.cc [modify] https://crrev.com/788b4fcb1909904cda962cac4a637ffecfa90c1a/ash/shelf/shelf_locking_manager.h [modify] https://crrev.com/788b4fcb1909904cda962cac4a637ffecfa90c1a/ash/shelf/shelf_locking_manager_unittest.cc [modify] https://crrev.com/788b4fcb1909904cda962cac4a637ffecfa90c1a/ash/shelf/shelf_widget.cc [modify] https://crrev.com/788b4fcb1909904cda962cac4a637ffecfa90c1a/ash/shelf/shelf_widget.h [modify] https://crrev.com/788b4fcb1909904cda962cac4a637ffecfa90c1a/ash/shelf/shelf_widget_unittest.cc [modify] https://crrev.com/788b4fcb1909904cda962cac4a637ffecfa90c1a/ash/shelf/wm_shelf.cc [modify] https://crrev.com/788b4fcb1909904cda962cac4a637ffecfa90c1a/ash/shelf/wm_shelf.h [modify] https://crrev.com/788b4fcb1909904cda962cac4a637ffecfa90c1a/ash/shell.cc [modify] https://crrev.com/788b4fcb1909904cda962cac4a637ffecfa90c1a/ash/shell.h [modify] https://crrev.com/788b4fcb1909904cda962cac4a637ffecfa90c1a/ash/wm/workspace/workspace_layout_manager.cc [modify] https://crrev.com/788b4fcb1909904cda962cac4a637ffecfa90c1a/ash/wm/workspace_controller.cc [modify] https://crrev.com/788b4fcb1909904cda962cac4a637ffecfa90c1a/chrome/browser/chromeos/accessibility/accessibility_manager.cc [modify] https://crrev.com/788b4fcb1909904cda962cac4a637ffecfa90c1a/chrome/browser/chromeos/accessibility/chromevox_panel.cc [modify] https://crrev.com/788b4fcb1909904cda962cac4a637ffecfa90c1a/chrome/browser/chromeos/accessibility/chromevox_panel.h [modify] https://crrev.com/788b4fcb1909904cda962cac4a637ffecfa90c1a/chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc [modify] https://crrev.com/788b4fcb1909904cda962cac4a637ffecfa90c1a/chrome/browser/ui/ash/launcher/chrome_launcher_controller.h
,
May 18 2017
msw, when is the app list button created? For this issue it might help to have the button created on ShelfView init (or ShelfModel init) so it exists from startup. It won't be visible until after login, which is what we want.
,
May 18 2017
CreateAppListItemAndDelegate is called from the ShelfController ctor: https://cs.chromium.org/chromium/src/ash/shelf/shelf_controller.cc?rcl=d25bb92682c7fb325d1f2b5608832a195fb0dd27&l=35 That should happen in the Shell ctor, which is pretty early on. https://cs.chromium.org/chromium/src/ash/shell.cc?rcl=d25bb92682c7fb325d1f2b5608832a195fb0dd27&l=569
,
May 18 2017
msw, WDYT of moving the applist item creation to the ShelfModel constructor? It's a trivial change, but it might make it a touch clearer.
,
May 18 2017
Hmm, it'll slightly complicate the impending two-model approach needed for mash, but I guess I can try to work around it...
,
May 18 2017
Hrm, I don't want to complicate things. In the two-model shelf, will the app list item still be synchronously created during ash startup (like when Shell is initialized?). If so, that should be sufficient for this bug.
,
May 18 2017
Yes, the app list item should still be synchronously created in Ash's ShelfModel on Shell creation, as it is now.
,
May 22 2017
I'm calling this done. If anyone here sees more early-startup clusterfuzz crashes in shelf/status area, please ping me.
,
Aug 1 2017
,
Jan 22 2018
|
|||||
►
Sign in to add a comment |
|||||
Comment 1 by xiy...@chromium.org
, May 2 2017