New issue
Advanced search Search tips

Issue 839883 link

Starred by 1 user
Status: Verified
Owner:
rohitrao@chromium.org
Closed: May 2018
Cc:
sczs@chromium.org
ios-bugs-priority@chromium.org
ios-bugs@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: iOS
Pri: 1
Type: Bug



Sign in to add a comment

Crash when opening Bookmarks before the model has loaded

Project Member Reported by rohitrao@chromium.org, May 4 2018 
(I don't think this requires the UIRefresh flag to be enabled, but it's much easier to repro with the flag because there's easy NTP access to Bookmarks.)

1) Set up Chrome so that the NTP will be visible on a cold start.
2) Cold start Chrome.
3) As soon as the NTP is visible, immediately hit the Bookmarks button.
4) Bookmarks will launch as a blank screen.  Eventually the app will crash.

I believe that Bookmarks is launching before the model is loaded.  We have code to handle this, but it might have a use-after-free or some other garbage pointer in it, because we seem to crash as the model finishes loading.

#1  0x0000000107923dce in bookmark_utils_ios::TitleForBookmarkNode(bookmarks::BookmarkNode const*) at ios/chrome/browser/ui/bookmarks/bookmark_utils_ios.mm:78
#2  0x0000000107900c21 in ::-[BookmarkHomeViewController setupNavigationBar]() at ios/chrome/browser/ui/bookmarks/bookmark_home_view_controller.mm:742
#3  0x00000001078fa516 in ::-[BookmarkHomeViewController loadBookmarkViews]() at ios/chrome/browser/ui/bookmarks/bookmark_home_view_controller.mm:260
#4  0x00000001078ff16d in ::__49-[BookmarkHomeViewController bookmarkModelLoaded]_block_invoke() at ios/chrome/browser/ui/bookmarks/bookmark_home_view_controller.mm:616
#5  0x000000010790ce7d in ::-[BookmarkHomeWaitingView activityIndicatorAnimationDidFinish:](MDCActivityIndicator *) at ios/chrome/browser/ui/bookmarks/bookmark_home_waiting_view.mm:81
#6  0x000000010758c038 in __34-[MDCActivityIndicator animateOut]_block_invoke at ios/third_party/material_components_ios/src/components/ActivityIndicator/src/MDCActivityIndicator.m:892

Comment 1 by rohitrao@chromium.org, May 4 2018 

e349e963a7d1a4d8674673fdbd8d174360b9e76a is the first bad commit
commit e349e963a7d1a4d8674673fdbd8d174360b9e76a
Author: Scott Violet <sky@chromium.org>
Date:   Thu May 3 20:45:23 2018 +0000

    bookmarks: moves creation of various state to BookmarkLoadDetails
    
    In particular the permenent node creation moves to
    BookmarkLoadDetails, and LoadBookmarks() becomes a bare function.
    
    BUG= 680698 
    TEST=covered by existing tests
    
    Change-Id: Ifdc7ef84ee73ac7b59c122976a37cb85d5d27b4f
    Reviewed-on: https://chromium-review.googlesource.com/1041575
    Commit-Queue: Scott Violet <sky@chromium.org>
    Reviewed-by: Jay Civelli <jcivelli@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#555859}
Project Member

Comment 2 by bugdroid1@chromium.org, May 4 2018 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/56a1674641d7434b8ced8020b6790b441ae89633

commit 56a1674641d7434b8ced8020b6790b441ae89633
Author: Rohit Rao <rohitrao@chromium.org>
Date: Fri May 04 17:33:37 2018

[ios] Fixes a crash in Bookmarks.

If the bookmarks UI was displayed before the BookmarkModel was done
loading, we would incorrectly cache a pointer to a BookmarkNode that had
been destroyed.  Instead, do not set the root node of the
BookmarkHomeViewController until after the model has finished loading.

BUG= 839883 
TEST=See repro steps in bug. Displaying the Bookmarks UI immediately upon startup should not crash.

Cq-Include-Trybots: master.tryserver.chromium.mac:ios-simulator-cronet;master.tryserver.chromium.mac:ios-simulator-full-configs
Change-Id: I062171c6d588da6be666b92e0c961abb90f1af17
Reviewed-on: https://chromium-review.googlesource.com/1044353
Reviewed-by: Sergio Collazos <sczs@chromium.org>
Commit-Queue: Rohit Rao <rohitrao@chromium.org>
Cr-Commit-Position: refs/heads/master@{#556101}
[modify] https://crrev.com/56a1674641d7434b8ced8020b6790b441ae89633/ios/chrome/browser/ui/bookmarks/bookmark_home_view_controller.mm
[modify] https://crrev.com/56a1674641d7434b8ced8020b6790b441ae89633/ios/chrome/browser/ui/bookmarks/bookmark_interaction_controller.mm

Comment 3 by rohitrao@chromium.org, May 4 2018

Status: Fixed (was: Started)

Comment 4 by rohitrao@chromium.org, May 4 2018

Labels: M-68

Comment 5 by shbarezer@chromium.org, May 15 2018

Status: Verified (was: Fixed)
Verified in 68.0.3431.0 Canary, iPhone 6 iOS 10.3.3, iPhone 7 iOS11.4 beta 3
Looks good.
►

Sign in to add a comment