Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2605463003:
Fix the Crash in the launcher's start page on Chrome OS. (Closed)
DescriptionFix the Crash in the launcher's start page on Chrome OS.
In Chrome OS launcher's start page, we recreate the app tile views
if we need to display different numbers of apps compared with the
previous time (e.g., install a new app or uninstall an existing app).
We need to make sure the number of the displayed apps is properly
updated. Otherwise, it might cause browser crash because of retrieving
a stale pointer in StartPageView::StartPageTilesContainer::GetTileItemView().
This simple CL is a temporary fix that meant to merge back to M56. It
will be reverted later on Tot and a better but more complicated fix
will be landed.
BUG=675150
Committed: https://crrev.com/49ff293042830e095777651dc4058772af41fdda
Cr-Commit-Position: refs/heads/master@{#441275}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Address xiyuan@'s comments. #
Total comments: 4
Patch Set 3 : . #Patch Set 4 : Address xiyuan@'s offline comment. #Patch Set 5 : nit fix. #Messages
Total messages: 22 (10 generated)
xdai@chromium.org changed reviewers: + xiyuan@chromium.org
xiyuan@, could you help review this CL please? Thanks!
The CQ bit was checked by xdai@chromium.org to run a CQ dry run
Dry run: 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
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2605463003/diff/1/ui/app_list/views/start_pag... File ui/app_list/views/start_page_view.cc (right): https://codereview.chromium.org/2605463003/diff/1/ui/app_list/views/start_pag... ui/app_list/views/start_page_view.cc:195: ScheduleUpdate(); Is this necessary? We are in Update(), which is called from SearchResultContainerView::DoUpdate as a result of a scheduled update. num_results_ should be updated when we return later at line 210. I suspect the problem is because we sometime ignore updates in line 177-182 and we might keep an old num_results(). Maybe we should ScheduleUpdate() there.
xiyuan@, please take another look. Thanks! https://codereview.chromium.org/2605463003/diff/1/ui/app_list/views/start_pag... File ui/app_list/views/start_page_view.cc (right): https://codereview.chromium.org/2605463003/diff/1/ui/app_list/views/start_pag... ui/app_list/views/start_page_view.cc:195: ScheduleUpdate(); On 2016/12/23 05:56:22, xiyuan (ooo Jan 3) wrote: > Is this necessary? We are in Update(), which is called from > SearchResultContainerView::DoUpdate as a result of a scheduled update. > num_results_ should be updated when we return later at line 210. > > I suspect the problem is because we sometime ignore updates in line 177-182 and > we might keep an old num_results(). Maybe we should ScheduleUpdate() there. I think you're right. Modified as you suggested. Thanks!
https://codereview.chromium.org/2605463003/diff/20001/ui/app_list/views/start... File ui/app_list/views/start_page_view.cc (left): https://codereview.chromium.org/2605463003/diff/20001/ui/app_list/views/start... ui/app_list/views/start_page_view.cc:193: CreateAppsGrid(std::min(kNumStartPageTiles, display_results.size())); Would this change have side effect that we might display too many items on start page? https://codereview.chromium.org/2605463003/diff/20001/ui/app_list/views/start... File ui/app_list/views/start_page_view.cc (right): https://codereview.chromium.org/2605463003/diff/20001/ui/app_list/views/start... ui/app_list/views/start_page_view.cc:187: return display_results.size() + 1; Not sure whether this is the right thing to do. We will update num_results() when this returns. However, |search_result_tile_views_| is not changed and GetTileItemView probably will not work properly because of that, e.g. display_results.size() + 1 is greater than old num_results and GetTileItemView tries to access beyond the old num_results.
xiyuan@, thanks for the review. Please take another look, thanks! https://codereview.chromium.org/2605463003/diff/20001/ui/app_list/views/start... File ui/app_list/views/start_page_view.cc (left): https://codereview.chromium.org/2605463003/diff/20001/ui/app_list/views/start... ui/app_list/views/start_page_view.cc:193: CreateAppsGrid(std::min(kNumStartPageTiles, display_results.size())); On 2017/01/03 17:05:14, xiyuan wrote: > Would this change have side effect that we might display too many items on start > page? The maximum number of |display_results| is kNumStartPageTiles, so we should be fine here. See line 184-186 in this patch. https://codereview.chromium.org/2605463003/diff/20001/ui/app_list/views/start... File ui/app_list/views/start_page_view.cc (right): https://codereview.chromium.org/2605463003/diff/20001/ui/app_list/views/start... ui/app_list/views/start_page_view.cc:187: return display_results.size() + 1; On 2017/01/03 17:05:13, xiyuan wrote: > Not sure whether this is the right thing to do. We will update num_results() > when this returns. However, |search_result_tile_views_| is not changed and > GetTileItemView probably will not work properly because of that, e.g. > display_results.size() + 1 is greater than old num_results and GetTileItemView > tries to access beyond the old num_results. Emmm... In this case the approach in the first patch might be better? StartPageTilesContainer::Update() is not only called by SearchResultContainerView::DoUpdate() but also can be called by StartPageView::OnShown() (the state is AppListModel::STATE_START), which I think is the case here. From the bug's crash stack, the crash is caused by the keyboard navigation gesture StartPageView::OnKeyPressed() which is guaranteed to be called after StartPageView::OnShown(). Thus if we call ScheduleUpdate() in StartPageTilesContainer::Update(), num_results() and |search_result_tile_views_| are forced to be updated to prevent the crash. Or maybe we can expose a set_num_results() function and call it in StartPageView::OnShown()? Suggestions?
Description was changed from ========== Fix the Crash in the launcher's start page on Chrome OS. In Chrome OS launcher's start page, we recreate the app tile views if we need to display different numbers of apps compared with the previous time (e.g., install a new app or uninstall an existing app). We need to make sure the number of the displayed apps is properly updated. Otherwise, it might cause browser crash because of retrieving a stale pointer in StartPageView::StartPageTilesContainer::GetTileItemView(). BUG=675150 ========== to ========== Fix the Crash in the launcher's start page on Chrome OS. In Chrome OS launcher's start page, we recreate the app tile views if we need to display different numbers of apps compared with the previous time (e.g., install a new app or uninstall an existing app). We need to make sure the number of the displayed apps is properly updated. Otherwise, it might cause browser crash because of retrieving a stale pointer in StartPageView::StartPageTilesContainer::GetTileItemView(). This simple CL is a temporary fix that meant to merge back to M56. It will be reverted later on Tot and a better but more complicated fix will be landed. BUG=675150 ==========
xiyuan@, as we discussed offline, please take a look at the temporary fix. I will revert it after merge and work on the better fix. Thanks!
lgtm
xiyuan@, please take another look at this CL to see if it still looks good to you, thanks! In StartPageView::OnShown() function, I changed the calling order of ClearSelectedIndex() and set_num_results() so that the previous selected index can be properly cleared.
On 2017/01/03 23:36:56, xdai1 wrote: > xiyuan@, please take another look at this CL to see if it still looks good to > you, thanks! > > In StartPageView::OnShown() function, I changed the calling order of > ClearSelectedIndex() and set_num_results() so that the previous selected index > can be properly cleared. think it is fine. lgtm
The CQ bit was checked by xdai@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": 80001, "attempt_start_ts": 1483489854566380,
"parent_rev": "549cb02325d66aa3e5444bd5dc5eba3c3b1392be", "commit_rev":
"dcbd3d9041935a274a9e57361b984f5fae4ef6d7"}
Message was sent while issue was closed.
Description was changed from ========== Fix the Crash in the launcher's start page on Chrome OS. In Chrome OS launcher's start page, we recreate the app tile views if we need to display different numbers of apps compared with the previous time (e.g., install a new app or uninstall an existing app). We need to make sure the number of the displayed apps is properly updated. Otherwise, it might cause browser crash because of retrieving a stale pointer in StartPageView::StartPageTilesContainer::GetTileItemView(). This simple CL is a temporary fix that meant to merge back to M56. It will be reverted later on Tot and a better but more complicated fix will be landed. BUG=675150 ========== to ========== Fix the Crash in the launcher's start page on Chrome OS. In Chrome OS launcher's start page, we recreate the app tile views if we need to display different numbers of apps compared with the previous time (e.g., install a new app or uninstall an existing app). We need to make sure the number of the displayed apps is properly updated. Otherwise, it might cause browser crash because of retrieving a stale pointer in StartPageView::StartPageTilesContainer::GetTileItemView(). This simple CL is a temporary fix that meant to merge back to M56. It will be reverted later on Tot and a better but more complicated fix will be landed. BUG=675150 Review-Url: https://codereview.chromium.org/2605463003 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Fix the Crash in the launcher's start page on Chrome OS. In Chrome OS launcher's start page, we recreate the app tile views if we need to display different numbers of apps compared with the previous time (e.g., install a new app or uninstall an existing app). We need to make sure the number of the displayed apps is properly updated. Otherwise, it might cause browser crash because of retrieving a stale pointer in StartPageView::StartPageTilesContainer::GetTileItemView(). This simple CL is a temporary fix that meant to merge back to M56. It will be reverted later on Tot and a better but more complicated fix will be landed. BUG=675150 Review-Url: https://codereview.chromium.org/2605463003 ========== to ========== Fix the Crash in the launcher's start page on Chrome OS. In Chrome OS launcher's start page, we recreate the app tile views if we need to display different numbers of apps compared with the previous time (e.g., install a new app or uninstall an existing app). We need to make sure the number of the displayed apps is properly updated. Otherwise, it might cause browser crash because of retrieving a stale pointer in StartPageView::StartPageTilesContainer::GetTileItemView(). This simple CL is a temporary fix that meant to merge back to M56. It will be reverted later on Tot and a better but more complicated fix will be landed. BUG=675150 Committed: https://crrev.com/49ff293042830e095777651dc4058772af41fdda Cr-Commit-Position: refs/heads/master@{#441275} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/49ff293042830e095777651dc4058772af41fdda Cr-Commit-Position: refs/heads/master@{#441275} |
