[CategoryRanker] Chrome crashes when a local category is removed |
|||||
Issue descriptionWhen you remove a local category from KnownCategories and start chrome again, it crashes: I 11.045s Main [FATAL:category.cc(27)] Check failed: id < static_cast<int>(KnownCategories::LOCAL_CATEGORIES_COUNT) || id > static_cast<int>(KnownCategories::REMOTE_CATEGORIES_OFFSET). I 11.047s Main 0014fb4b ntp_snippets::Category::FromIDValue(int) /usr/local/google/code/clankium/src/components/ntp_snippets/category.cc:26 I 11.047s Main 001e0e73 ntp_snippets::ClickBasedCategoryRanker::ReadOrderFromPrefs(std::__ndk1::vector<ntp_snippets::ClickBasedCategoryRanker::RankedCategory, std::__ndk1::allocator<ntp_snippets::ClickBasedCategoryRanker::RankedCategory> >*) const /usr/local/google/code/clankium/src/components/ntp_snippets/category_rankers/click_based_category_ranker.cc:317 I 11.047s Main 001e0d85 ClickBasedCategoryRanker /usr/local/google/code/clankium/src/components/ntp_snippets/category_rankers/click_based_category_ranker.cc:80 This is actually according to the specification, no category should be removed from KnownCategories. Still, it is a bit frustrating for the development. If there is a new category in one of your branches, then you need to uninstall / clear data when switching out of this branch.
,
Jan 25 2017
,
Jan 25 2017
,
Feb 2 2017
yeah, this also seems like a risk if we ever decide to remove a category and don't pay attention to details.
,
Feb 2 2017
However, this check is not ranker related at all. It is in Category itself. The only ranker's problem here is that it stores all the categories and hits this check while parsing them. I don't think that there is any workaround for this on ranker's side.
,
Feb 2 2017
I do not care on which side this is fixed. I still think this should be fixed (probably touching both Category and the Ranker).
,
Feb 2 2017
The category enum values are persisted, so removing an existing one is always wrong. (We have many such "must-not-be-changed" enums in Chromium.) This might be slightly annoying for development, but you can resolve it by clearing the app data, so it's not a huge deal. OTOH, IMO it's wrong to write code for a situation that MUST NEVER HAPPEN.
,
Feb 2 2017
Well, I get your point about removing categories is wrong. Still, I do not see that much difference between persisting remote categories and local ones. We currently enforce (by the DCHECK) that a local category is never removed. We have no power over remote categories. Is the partial guarantee worth the hassle for development? Anyway, I do not want to fight too strongly, I am also fine with WontFix.
,
Feb 2 2017
The difference between local/remote is that for remote categories, we have to handle unknown categories anyway; that's an explicit feature. For local, however, all categories should always be known. Let's get more concrete: What exact change are you proposing? I'm a bit worried that by removing DCHECKs etc, we make it easier to "accidentally" remove a local category, breaking various things in subtle ways.
,
Feb 2 2017
Yep, the proposed change is to remove that guarantee (and DCHECK). I am not sure where we persist category ids and if we ever persist local category ids only. - If we persist on some places local-only category ids, removing the guarantee would make the code more complex, it is not worth it. - If not, if in all places we persist local ids along with remote ids (or maybe remote only), we need to handle categories disappearing anyway. Then removing the guarantee would actually streamline reasoning about the code, IMO. Another (opposite) solution is to make sure we introduce guarantees on the server that also no remote category is ever removed. This way, we may persist remote ids and never care about removing persisted data for categories that are not available at the moment. This would probably result in even simpler code, overall. WDYT?
,
Feb 2 2017
I know of two places that persist category-related stuff: 1) RemoteSuggestionsProviderImpl stores CategoryInfo (title, and a few other things), for remote categories only (including Articles though). 2) ClickBasedCategoryRanker, both local and remote. I still think the DCHECK in category.cc is valid and justified. (Though maybe the whole Category thing needs a re-design. Contact me for details ;) ) I don't think I follow the "opposite" solution. The server can't possibly guarantee that every category that ever existed will be included in every response.
,
Feb 6 2017
After an offline suggestion with treib@, I suggest to only expand the comment before the category enum a bit.
,
Feb 6 2017
It is not obvious to me what you suggest to add to the comment, could you elaborate?
,
Feb 6 2017
From offline discussion with vitaliii@: - put a warning before the KnownCategories enum (or maybe even expanding the comment "// INSERT NEW LOCAL CATEGORIES HERE!") that you may remove a category unintentionally (by switching branch) and Chrome will crash. - make the DCHECK error a bit more helpful
,
Feb 8 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/2d864d4d394055b7e20606037f7d4465d3928bd2 commit 2d864d4d394055b7e20606037f7d4465d3928bd2 Author: jkrcal <jkrcal@chromium.org> Date: Wed Feb 08 11:44:39 2017 [Suggestions category] Warn more about removing categories. This CL adds a more verbose DLOG warning if a local category is removed. Furthermore, a big comment warns you in advance when you add a new local category. BUG= 682184 Review-Url: https://codereview.chromium.org/2678343004 Cr-Commit-Position: refs/heads/master@{#448958} [modify] https://crrev.com/2d864d4d394055b7e20606037f7d4465d3928bd2/components/ntp_snippets/category.cc [modify] https://crrev.com/2d864d4d394055b7e20606037f7d4465d3928bd2/components/ntp_snippets/category.h
,
Mar 3 2017
|
|||||
►
Sign in to add a comment |
|||||
Comment 1 by vitaliii@chromium.org
, Jan 18 2017