Segfault in AppListModel::RemoveItemFromFolder after rapidly uninstalling multiple Linux apps |
||||||||
Issue description
Chrome Version: ToT 12/14/2018
OS: ChromeOS
What steps will reproduce the problem?
1. Enable development feature CrostiniAppUninstallGui in /etc/chrome_dev.conf
2. Install Linux from settings menu
3. In Linux terminal:
a. sudo apt-get install emacs eclipse gimp --yes
b. sudo touch /usr/share/applications/eclipse.desktop
4. Rearrange icons so that terminal and Emacs are out of folder (see attached image)
5. Open Launcher fully. Right click on GIMP. Select Uninstall. Do *not* answer the "Uninstall / Cancel" prompt yet
6. Re-open Launcher fully. Right-click on Eclipse. Select Uninstall. There should now be 2 "Uninstall / Cancel" windows stacked on top of each other.
8. Answer "Uninstall" two times.
What is the expected result?
Apps uninstalled, no crash
What happens instead?
Segfault in app_list::AppListModel::RemoveItemFromFolder
Debug logging shows we are remove the GIMP application twice. On the second remove, it doesn't have a containing folder (the first remove removed the now-empty folder):
[11414:11414:1127/121832.981761:ERROR:app_list_model.cc(260)] iby: FindFolderItem(ddolnhmblagmcagkedkbfejapapdimlk) returned 0x3a8929b64870
[11414:11414:1127/121832.981794:ERROR:app_list_model.cc(371)] iby: RemoveItemFromFolder(0x3a8929b64870, 0x3a892b513960)
[11414:11414:1127/121832.981812:ERROR:app_list_model.cc(372)] iby: Folder: ddolnhmb '' [zqgm]
[11414:11414:1127/121832.981828:ERROR:app_list_model.cc(373)] iby: Item: pbfempnh 'Eclipse' [nzzzg]
[11414:11414:1127/121832.981844:ERROR:app_list_model.cc(375)] iby: folder_id ddolnhmblagmcagkedkbfejapapdimlk
[11414:11414:1127/121832.981858:ERROR:app_list_model.cc(377)] iby: item_id pbfempnhjiikpcenodcklacopbdhldlh
[11414:11414:1127/121832.982589:ERROR:app_list_model.cc(385)] iby: Done, returning
[11414:11414:1127/121832.982630:ERROR:app_list_model.cc(371)] iby: RemoveItemFromFolder(0x3a8929b64870, 0x3a8927f5ff60)
[11414:11414:1127/121832.982642:ERROR:app_list_model.cc(372)] iby: Folder: ddolnhmb '' [zqgm]
[11414:11414:1127/121832.982652:ERROR:app_list_model.cc(373)] iby: Item: nddljkoc 'GNU Image Manipulation Program' [nzzzg]
[11414:11414:1127/121832.982662:ERROR:app_list_model.cc(375)] iby: folder_id ddolnhmblagmcagkedkbfejapapdimlk
[11414:11414:1127/121832.982676:ERROR:app_list_model.cc(377)] iby: item_id nddljkocfpafnehjhaonajkhgpakcogn
[11414:11414:1127/121832.982703:ERROR:app_list_model.cc(382)] iby: Deleting empty folder: ddolnhmb '' [zqgm]
[11414:11414:1127/121832.983111:ERROR:app_list_model.cc(385)] iby: Done, returning
[11414:11414:1127/121836.317827:ERROR:app_list_model.cc(260)] iby: FindFolderItem(ddolnhmblagmcagkedkbfejapapdimlk) returned (nil)
[11414:11414:1127/121836.317857:ERROR:app_list_model.cc(262)] iby: Invalid item was nddljkoc 'GNU Image Manipulation Program' [nzzzg]
[11414:11414:1127/121836.317874:ERROR:app_list_model.cc(371)] iby: RemoveItemFromFolder((nil), 0x3a8927f5ff60)
Callstack:
#0 app_list::AppListModel::RemoveItemFromFolder(app_list::AppListFolderItem*, app_list::AppListItem*) ()
at ../../build/cros_cache/chrome-sdk/tarballs/eve+11244.0.0+target_toolchain/usr/bin/../include/c++/v1/memory:2607
#1 0x000059bfa8e593fa in app_list::AppListModel::DeleteItem(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&) ()
at ../../ash/app_list/model/app_list_model.cc:261
#2 0x000059bfa8e59fe3 in app_list::AppListModel::DeleteUninstalledItem(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&) () at ../../ash/app_list/model/app_list_model.cc:275
#3 0x000059bfa53a231e in ash::mojom::AppListControllerStubDispatch::Accept(ash::mojom::AppListController*, mojo::Message*) ()
at gen/ash/public/interfaces/app_list.mojom.cc:2246
#4 0x000059bfa6a58937 in mojo::InterfaceEndpointClient::HandleValidatedMessage(mojo::Message*) ()
at ../../mojo/public/cpp/bindings/lib/interface_endpoint_client.cc:423
#5 0x000059bfa6a5d084 in mojo::internal::MultiplexRouter::ProcessIncomingMessage(mojo::internal::MultiplexRouter::MessageWrapper*, mojo::internal::MultiplexRouter::ClientCallBehavior, base::SequencedTaskRunner*) ()
at ../../mojo/public/cpp/bindings/lib/multiplex_router.cc:869
#6 0x000059bfa6a5c867 in mojo::internal::MultiplexRouter::Accept(mojo::Message*) () at ../../mojo/public/cpp/bindings/lib/multiplex_router.cc:590
#7 0x000059bfa6a5763a in mojo::Connector::ReadSingleMessage(unsigned int*) ()
at ../../mojo/public/cpp/bindings/lib/connector.cc:476
,
Nov 28
,
Nov 28
I may be off base here: Is it possible that a pref update was not yet committed (whatever that means), so our next access read the old value (app still present recorded as, needs deleting) on the next app list update?
,
Nov 28
Repo with more logging in CrostiniRegistryService::UpdateApplicationList It looks like one call to CrostiniRegistryService::UpdateApplicationList is trying to *update* GIMP, but it ends up having AppListModel::RemoveItemFromFolder remove the item and the folder. Then later, when GIMP is removed, the folder doesn't exist anymore. log attached
,
Nov 28
https://codesearch.chromium.org/chromium/src/chrome/browser/ui/app_list/crostini/crostini_app_model_builder.cc?sq=package:chromium&g=0&l=76 Perhaps the app registration was NoDisplay? That would lead to removal at line 76
,
Nov 28
Have you enabled DCHECK (cros chrome-sdk ... --gn-extra-args='dcheck_always_on=trye')? I suspect that would trigger this: https://cs.chromium.org/chromium/src/ash/app_list/model/app_list_model.cc?type=cs&q=RemoveItemFromFolder&sq=package:chromium&g=0&l=260 It looks like something may be setting the folder id back to the deleted folder after it was removed. The simplest "fix" would be to replace the DCHECK with an extra test that the folder exists at like 249, but I would encourage tracking down how/why the folder id is set for the item after it was (presumably) removed from the folder. It's possibly that this is an edge case that has always existed, but never exposed before because we didn't used to have a confirmation dialog. +hejq@, +weidongg@ who have been in this code more recently.
,
Nov 28
> Have you enabled DCHECK (cros chrome-sdk ... --gn-extra-args='dcheck_always_on=trye') No, there are currently too many DCHECKs during login to make it practical. But yes, the DCHECK would be triggered. The second-to-the-last-line in the log file is it getting nullptr back from FindFolderItem. I'll add yet more logging and repo to try and get a clearer picture of the sequence of events.
,
Nov 28
I'm only aware of one that was recently reverted, and one that is arm specific. Did you make sure bugs are filed for those? It is easy enough to comment out a DCHECK for testing in the meanwhile.
,
Nov 28
Here's an even more complete list of what's going on, including the calls in crostini_app_model_builder.cc. The actual uninstall starts with the line [11220:11220:1128/144532.423276:ERROR:crostini_registry_service.cc(632)] iby: CrostiniRegistryService::UpdateApplicationList() start everything above that is the setup.
,
Nov 28
#8: I was running into https://bugs.chromium.org/p/chromium/issues/detail?id=907590 as well.
,
Nov 29
<triage>Ian, if somebody else doesn't pick this one up, please assign to yourself and investigate</triage>
,
Nov 29
Hey iby, I had a look at your log. To me it looks like the major problem is that the folder gets deleted when the second-last item gets taken out of it, and then deleted again when the last item is taken out, at which point it is null. You might be able to make a simpler test case. What happens if you do 1. Install Linux from settings menu 2. In Linux terminal: a. sudo apt-get install emacs eclipse gimp --yes b. sudo touch /usr/share/applications/eclipse.desktop 3. Rearrange icons so that terminal and Emacs are out of folder (see attached image) 4. Open Launcher fully. Right click on GIMP. Select Uninstall. Do *not* answer the "Uninstall / Cancel" prompt yet 5. Re-open Launcher fully. Right-click on Eclipse. Select Uninstall. There should now be two "Uninstall / Cancel" windows stacked on top of each other. 6. Answer "Uninstall" two times. 7. Close both notification popups, in order, before first one finishes.
,
Nov 29
rjwright: Yes, just uninstalling GIMP and Eclipse when they are the only things in the folder will crash it. Same log output. Also, step #7 (Close both notification popups) is unnecessary; Chrome will crash even if the notifications are open.
,
Nov 30
Thank you. I want to repro this bug and try to fix it but I'm blocked on SSH problems at the moment.
,
Nov 30
Reading the implementation of AppListModel::DeleteItem, it should only call FindFolderItem when item->IsInFolder returns true. We know this should be false for GIMP in this case (it was at the top level), but why? It's intended behavior for the folder to be deleted when the second last element is removed. (See AppListModel::DeleteUninstalledItem). I don't know why. What's wrong with a folder containing one item?
,
Nov 30
IIRC, that was by design. A single item folder isn't very interesting and looks odd. This is also consistent with Android behavior where single item folders are removed. It sounds like perhaps we are not correctly clearing the folder id from the remaining item when the folder is removed in that case?
,
Dec 3
By Crostini's design, there can now be a folder with one item, namely Linux Apps. Right after installing Crostini, it contains just Terminal. Other newly installed apps are added to it. You can cause its deletion by moving apps out. It even gets recreated when you install a new app (again with just one item). I'm not sure how else we could have this Linux Apps folder exist otherwise. If it can only exist after adding a second app, that's a bit weird. Terminal (always present) would then jump into the folder and confuse users who saw it previously at the top level.
,
Dec 3
Behavior regarding when to create and erase folders is unavoidable. Folders are designed to disallow any special casing (for obvious reasons). There is a special case for OEM but it's plumbed all the way through the UI arch & is pretty gross. Steven I think you are correct that the folder id isn't being cleared in a timely manner. I'll repro this w/ Ian's changes and try to fix it.
,
Dec 3
If UX wants special behavior for a 'Linux Apps' folder, you will need to implement something along the lines of what was done for the OEM folder. Possibly now that there are two examples we could do this in a more maintainable way (e.g. specify a separate folder type).
,
Dec 4
#18: Note that for ToT, you'll now need to enable feature CrostiniAppUninstallGui but do not need https://chromium-review.googlesource.com/c/chromium/src/+/1213519 which is merged. Will update description.
,
Dec 4
,
Dec 4
Steven (#19), yes, that's right. We would need to get the OK from the Ash people and it would be a fairly large job, but do-able. Thanks Ian. I was confused about that.
,
Dec 14
Note that https://chromium-review.googlesource.com/c/chromium/src/+/1275292 is now merged, so ToT from today (PST) will contain everything you need to reproduce this. Still need the feature flag, though.
,
Dec 14
,
Dec 15
|
||||||||
►
Sign in to add a comment |
||||||||
Comment 1 by iby@chromium.org
, Nov 28