New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 613359 link

Starred by 3 users

Issue metadata

Status: Verified
Owner:
Closed: May 2016
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 1
Type: Bug

Blocking:
issue 568954



Sign in to add a comment

Offline Pages should check for existence of the model before casting to a bridge.

Project Member Reported by dewittj@chromium.org, May 19 2016

Issue description

Since we switched to being unavailable in incognito mode, model might be null for some profiles.

We should check for null pointers in different places now.
 
One example:

https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/android/offline_pages/offline_page_bridge.cc&l=167

  Profile* profile = ProfileAndroid::FromProfileAndroid(j_profile);
  OfflinePageModel* offline_page_model =
      OfflinePageModelFactory::GetForBrowserContext(profile);

  OfflinePageBridge* bridge = static_cast<OfflinePageBridge*>(
      offline_page_model->GetUserData(kOfflinePageBridgeKey));
Blocking: 568954
Cc: bauerb@chromium.org treib@chromium.org petewil@chromium.org
Components: UI>Browser>Offline
+cc interested parties.

Since we moved to offline page cache (instead of offline bookmarks) we no longer operate at all in incognito, and so the model might be null.

Tester report:

Was able to repro with Chrome canary build '52.0.2741.0', Please see crash ID: 58ee5d1c00000000

http://go/chrome-androidlogs1/6/568954-1
Steps to repro:
1. Launch Chrome canary
2. Open incognito tab
3. Visit https site such as 'https://bofa.com'
4. Click on site information button.
Chrome canary will crash.

Added file:
CrashOnIncognito.log

Project Member

Comment 4 by bugdroid1@chromium.org, May 20 2016

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

commit 9a12ce07eb8c36407ae27beba7b2082229cf5678
Author: dewittj <dewittj@chromium.org>
Date: Fri May 20 16:42:25 2016

New Tab Page: Work around potentially long delays if model is not loaded.

This avoids querying for offline pages if the model is not loaded, so
that the maximum possible delay is minimal (once the model is loaded,
all pages are stored in memory).

Additionally checks for null when obtaining the offline page model.

BUG= 607573 , 613359 

Review-Url: https://codereview.chromium.org/1982483002
Cr-Commit-Position: refs/heads/master@{#395096}

[modify] https://crrev.com/9a12ce07eb8c36407ae27beba7b2082229cf5678/chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java

Labels: Stability-Crash ReleaseBlock-Stable M-52
Labels: Merge-Request-52
Status: Started (was: Untriaged)

Comment 7 by tin...@google.com, May 23 2016

Labels: -Merge-Request-52 Merge-Approved-52 Hotlist-Merge-Approved
Your change meets the bar and is auto-approved for M52 (branch: 2743)
Project Member

Comment 8 by bugdroid1@chromium.org, May 23 2016

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

commit 261c816b0900eec6c3b41277288ccd4020c23f5d
Author: dewittj <dewittj@chromium.org>
Date: Mon May 23 21:44:20 2016

Offline Pages: Add NULL-checks for when the model is NULL (incognito mode)

Recently offline pages was changed to be completely disabled in
incognito profiles, since it is no longer associated with bookmarks, so
some consumers failed to check for null.

BUG= 613359 

Review-Url: https://codereview.chromium.org/2004473002
Cr-Commit-Position: refs/heads/master@{#395422}

[modify] https://crrev.com/261c816b0900eec6c3b41277288ccd4020c23f5d/chrome/browser/android/offline_pages/offline_page_bridge.cc

Project Member

Comment 9 by bugdroid1@chromium.org, May 24 2016

Labels: -merge-approved-52 merge-merged-2743
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/fac0bea283906de6dffd7e9fe7af6ba5872949fe

commit fac0bea283906de6dffd7e9fe7af6ba5872949fe
Author: Justin DeWitt <dewittj@chromium.org>
Date: Tue May 24 17:54:20 2016

New Tab Page: Work around potentially long delays if model is not loaded.

This avoids querying for offline pages if the model is not loaded, so
that the maximum possible delay is minimal (once the model is loaded,
all pages are stored in memory).

Additionally checks for null when obtaining the offline page model.

BUG= 607573 , 613359 

Review-Url: https://codereview.chromium.org/1982483002
Cr-Commit-Position: refs/heads/master@{#395096}
(cherry picked from commit 9a12ce07eb8c36407ae27beba7b2082229cf5678)

Review URL: https://codereview.chromium.org/2007563005 .

Cr-Commit-Position: refs/branch-heads/2743@{#38}
Cr-Branched-From: 2b3ae3b8090361f8af5a611712fc1a5ab2de53cb-refs/heads/master@{#394939}

[modify] https://crrev.com/fac0bea283906de6dffd7e9fe7af6ba5872949fe/chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java

Labels: Merge-Request-52
Sorry about the double request but I'd also like to merge the revision in #8 - crrev.com/395422


Comment 11 by tin...@google.com, May 24 2016

Labels: -Merge-Request-52 Merge-Approved-52
Your change meets the bar and is auto-approved for M52 (branch: 2743)
Issue 614601 has been merged into this issue.
Labels: ReleaseBlock-Dev
Adding ReleaseBlock-Dev since that tag was on Issue 614601
Project Member

Comment 14 by bugdroid1@chromium.org, May 25 2016

Labels: -merge-approved-52
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/2f02d09e1696e7f4df14a0f73712f51397a8f5db

commit 2f02d09e1696e7f4df14a0f73712f51397a8f5db
Author: Justin DeWitt <dewittj@chromium.org>
Date: Wed May 25 15:59:45 2016

Offline Pages: Add NULL-checks for when the model is NULL (incognito mode)

Recently offline pages was changed to be completely disabled in
incognito profiles, since it is no longer associated with bookmarks, so
some consumers failed to check for null.

BUG= 613359 

Review-Url: https://codereview.chromium.org/2004473002
Cr-Commit-Position: refs/heads/master@{#395422}
(cherry picked from commit 261c816b0900eec6c3b41277288ccd4020c23f5d)

Review URL: https://codereview.chromium.org/2007113007 .

Cr-Commit-Position: refs/branch-heads/2743@{#52}
Cr-Branched-From: 2b3ae3b8090361f8af5a611712fc1a5ab2de53cb-refs/heads/master@{#394939}

[modify] https://crrev.com/2f02d09e1696e7f4df14a0f73712f51397a8f5db/chrome/browser/android/offline_pages/offline_page_bridge.cc

Status: Fixed (was: Started)
Status: Verified (was: Fixed)
Verified in 52.0.2743.8 build

Sign in to add a comment