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

Issue 612160 link

Starred by 2 users
Status: Fixed
Owner:
dewittj@chromium.org
Closed: Jul 2016
Cc:
dewittj@chromium.org
█ sfiera@chromium.org
treib@chromium.org
█ ramyan@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 1
Type: Bug



Sign in to add a comment

Java tile types and native suggestions can get out of Sync

Project Member Reported by bauerb@chromium.org, May 16 2016 
NewTabPageView.onMostVisitedURLsAvailable() receives Most Visited URLs from the native side, but first calls NewTabPage.getUrlsAvailableOffline(), which _may_ call back asynchronously. When that happens and all the load tasks are finished, NewTabPageView will record tile type metrics, but at that point the suggestions on the native side might already have changed, so when we call back into native code, the DCHECK in MostVisitedSites::RecordTileTypeMetrics() will fail.

Example logcat / stack trace:
  signal 6 (SIGABRT), code -6 (SI_TKILL), fault addr --------
  [FATAL:most_visited_sites.cc(321)] Check failed: current_suggestions_.size() == tile_types.size() (8 vs. 0)

  Stack Trace:
    RELADDR   FUNCTION                                                                                                                                                 FILE:LINE
    0007ec5f  ~LogMessage                                                                                                                                              /usr/local/google_ssd/build/clankium/src/base/logging.cc:524
    0032d58f  MostVisitedSites::RecordTileTypeMetrics(std::__1::vector<int, std::__1::allocator<int> > const&)                                                         /usr/local/google_ssd/build/clankium/src/chrome/browser/android/ntp/most_visited_sites.cc:321
    0032f879  MostVisitedSitesBridge::RecordTileTypeMetrics(_JNIEnv*, base::android::JavaParamRef<_jobject*> const&, base::android::JavaParamRef<_jintArray*> const&)  /usr/local/google_ssd/build/clankium/src/chrome/browser/android/ntp/most_visited_sites_bridge.cc:221
    0032f841  Java_org_chromium_chrome_browser_profiles_MostVisitedSites_nativeRecordTileTypeMetrics                                                                   /usr/local/google_ssd/build/clankium/src/out_gn/Debug/gen/chrome/browser/jni_headers/chrome/jni/MostVisitedSites_jni.h:121

  -----------------------------------------------------

      r0 00000000  r1 00001985  r2 00000006  r3 b6ff1b7c
      r4 b6ff1b84  r5 b6ff1b34  r6 0000000b  r7 0000010c
      r8 bef75eac  r9 95e21084  sl 12c97d40  fp 00000000
      ip 00000006  sp bef759b0  lr b6d60b61  pc b6d62f50

  Stack Trace:
    RELADDR   FUNCTION                                                                                                                                                                                                         FILE:LINE
    00041f50  tgkill+12                                                                                                                                                                                                        /system/lib/libc.so
    0003fb5d  pthread_kill+32                                                                                                                                                                                                  /system/lib/libc.so
    0001c30f  raise+10                                                                                                                                                                                                         /system/lib/libc.so
    000194c1  __libc_android_abort+34                                                                                                                                                                                          /system/lib/libc.so
    000174ac  abort+4                                                                                                                                                                                                          /system/lib/libc.so
    v------>  base::debug::(anonymous namespace)::DebugBreak()                                                                                                                                                                 /usr/local/google_ssd/build/clankium/src/base/debug/debugger_posix.cc:219
    0006ead1  base::debug::BreakDebugger()                                                                                                                                                                                     /usr/local/google_ssd/build/clankium/src/base/debug/debugger_posix.cc:249
    0007ee65  ~LogMessage                                                                                                                                                                                                      /usr/local/google_ssd/build/clankium/src/base/logging.cc:740
    0032d58d  MostVisitedSites::RecordTileTypeMetrics(std::__1::vector<int, std::__1::allocator<int> > const&)                                                                                                                 /usr/local/google_ssd/build/clankium/src/chrome/browser/android/ntp/most_visited_sites.cc:321
    0032f877  MostVisitedSitesBridge::RecordTileTypeMetrics(_JNIEnv*, base::android::JavaParamRef<_jobject*> const&, base::android::JavaParamRef<_jintArray*> const&)                                                          /usr/local/google_ssd/build/clankium/src/chrome/browser/android/ntp/most_visited_sites_bridge.cc:221
    0032f83f  Java_org_chromium_chrome_browser_profiles_MostVisitedSites_nativeRecordTileTypeMetrics                                                                                                                           /usr/local/google_ssd/build/clankium/src/out_gn/Debug/gen/chrome/browser/jni_headers/chrome/jni/MostVisitedSites_jni.h:121
    01b85c45  offset 0xcd9000) (void org.chromium.chrome.browser.profiles.MostVisitedSites.nativeRecordTileTypeMetrics(long, int[])+120                                                                                        /data/app/com.google.android.apps.chrome-1/oat/arm/base.odex
    01b860dd  offset 0xcd9000) (void org.chromium.chrome.browser.profiles.MostVisitedSites.recordTileTypeMetrics(int[])+88                                                                                                     /data/app/com.google.android.apps.chrome-1/oat/arm/base.odex
    01a9ff2d  offset 0xcd9000) (void org.chromium.chrome.browser.ntp.NewTabPage$2.onLoadingComplete(org.chromium.chrome.browser.ntp.MostVisitedItem[])+1168                                                                    /data/app/com.google.android.apps.chrome-1/oat/arm/base.odex
    01aab041  offset 0xcd9000) (void org.chromium.chrome.browser.ntp.NewTabPageView.loadTaskCompleted()+396                                                                                                                    /data/app/com.google.android.apps.chrome-1/oat/arm/base.odex
    01aab6df  offset 0xcd9000) (void org.chromium.chrome.browser.ntp.NewTabPageView.onOfflineUrlsAvailable(java.lang.String[], java.lang.String[], java.lang.String[], java.util.Set)+1610                                     /data/app/com.google.android.apps.chrome-1/oat/arm/base.odex
    01aaa101  offset 0xcd9000) (void org.chromium.chrome.browser.ntp.NewTabPageView.access$1300(org.chromium.chrome.browser.ntp.NewTabPageView, java.lang.String[], java.lang.String[], java.lang.String[], java.util.Set)+84  /data/app/com.google.android.apps.chrome-1/oat/arm/base.odex
    01aa6c33  offset 0xcd9000) (void org.chromium.chrome.browser.ntp.NewTabPageView$15.onResult(java.util.Set)+150                                                                                                             /data/app/com.google.android.apps.chrome-1/oat/arm/base.odex
    01aa6b51  offset 0xcd9000) (void org.chromium.chrome.browser.ntp.NewTabPageView$15.onResult(java.lang.Object)+92                                                                                                           /data/app/com.google.android.apps.chrome-1/oat/arm/base.odex
    01a9b66f  offset 0xcd9000) (void org.chromium.chrome.browser.ntp.NewTabPage$2$1.run()+74                                                                                                                                   /data/app/com.google.android.apps.chrome-1/oat/arm/base.odex

(The bottom part of the stack is from a local build where I modified NewTabPage.getUrlsAvailableOffline() to always call back asynchronously, so this wouldn't happen like that at the moment, but it might once offline pages are reenabled.)

Comment 1 by dewittj@chromium.org, May 16 2016 

OfflinePageBridge#checkPagesExistOffline() should always call back asynchronously.  So once NewTabPage#getUrlsAvailableOffline is async, then the call should be reliably async.

Comment 2 by bauerb@chromium.org, May 16 2016 

Okay, so just to be clear: that will make this issue worse, not better ;-)

It still probably won't fail deterministically, because it depends on the timing for the suggestions to change on the native side (loading Popular Sites from disk seems to do the trick at the moment).

Comment 3 by dewittj@chromium.org, May 16 2016 

OK, so to do this completely async we would need to thread through Suggestion.source and Suggestion.provider_index so that DCHECK_EQ is unnecessary, right?

Comment 4 by bauerb@chromium.org, May 16 2016 

Yup, exactly. We could probably pass them to Java when we construct the MostVisitedItem array.

Comment 5 by nepper@chromium.org, May 17 2016 

To help me triage this: what is the user perceivable product impact of the current behavior, and what would be the ideal product behavior?

Thanks

Comment 6 by bauerb@chromium.org, May 17 2016 

The immediate impact as described in the bug is a DCHECK, which will not run in a Release build. It could result in a crash due to an out-of-bounds memory access though, and even if it didn't, it would record incorrect UMA data.

Comment 7 by treib@chromium.org, May 17 2016

Cc: sfiera@chromium.org
Yup, seems like we need to pass the source and provider to Java. At that point, we should also just record the histograms from Java, no point in passing everything back to native code.

Comment 8 by treib@chromium.org, May 17 2016

Labels: -Pri-3 Pri-1
Also, increasing to P1, since we should at the very least fix the crash immediately.

Comment 9 by treib@chromium.org, May 17 2016

Status: Available (was: Untriaged)

Comment 10 by treib@chromium.org, Jul 8 2016

Owner: dewittj@chromium.org
Status: Started (was: Available)
Project Member

Comment 11 by bugdroid1@chromium.org, Jul 13 2016 

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

commit e14e21a2eb9ef970a6fc34f41290afe5e1015fc2
Author: dewittj <dewittj@chromium.org>
Date: Wed Jul 13 08:25:37 2016

NTP: Fix metrics recording crash by plumbing the necessary data to Java.

This has been broken since offline pages caused the refresh of the NTP
MostVisitedSites to be asynchronous.

BUG= 612160 

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

[modify] https://crrev.com/e14e21a2eb9ef970a6fc34f41290afe5e1015fc2/chrome/android/java/src/org/chromium/chrome/browser/ntp/MostVisitedItem.java
[modify] https://crrev.com/e14e21a2eb9ef970a6fc34f41290afe5e1015fc2/chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java
[modify] https://crrev.com/e14e21a2eb9ef970a6fc34f41290afe5e1015fc2/chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java
[modify] https://crrev.com/e14e21a2eb9ef970a6fc34f41290afe5e1015fc2/chrome/android/java/src/org/chromium/chrome/browser/profiles/MostVisitedSites.java
[modify] https://crrev.com/e14e21a2eb9ef970a6fc34f41290afe5e1015fc2/chrome/android/javatests/src/org/chromium/chrome/browser/ntp/FakeMostVisitedSites.java
[modify] https://crrev.com/e14e21a2eb9ef970a6fc34f41290afe5e1015fc2/chrome/android/javatests/src/org/chromium/chrome/browser/ntp/NewTabPageTest.java
[modify] https://crrev.com/e14e21a2eb9ef970a6fc34f41290afe5e1015fc2/chrome/browser/android/ntp/most_visited_sites_bridge.cc
[modify] https://crrev.com/e14e21a2eb9ef970a6fc34f41290afe5e1015fc2/chrome/browser/android/ntp/most_visited_sites_bridge.h
[modify] https://crrev.com/e14e21a2eb9ef970a6fc34f41290afe5e1015fc2/components/ntp_tiles.gypi
[modify] https://crrev.com/e14e21a2eb9ef970a6fc34f41290afe5e1015fc2/components/ntp_tiles/BUILD.gn
[modify] https://crrev.com/e14e21a2eb9ef970a6fc34f41290afe5e1015fc2/components/ntp_tiles/most_visited_sites.cc
[modify] https://crrev.com/e14e21a2eb9ef970a6fc34f41290afe5e1015fc2/components/ntp_tiles/most_visited_sites.h
Project Member

Comment 12 by bugdroid1@chromium.org, Jul 13 2016 

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

commit 87a0e130422e6427c19a69dd70f6511b95b0dac7
Author: treib <treib@chromium.org>
Date: Wed Jul 13 15:34:07 2016

NTP: Fix metrics recording race condition

The UI calls MostVisitedSites::RecordOpenedMostVisitedItem when an item is clicked. Before this CL, MostVisitedSites would get some info from its |current_suggestions_|, but those might have changed in the meantime. Now, all required info is passed into RecordOpenedMostVisitedItem instead.

BUG= 612160 

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

[modify] https://crrev.com/87a0e130422e6427c19a69dd70f6511b95b0dac7/chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java
[modify] https://crrev.com/87a0e130422e6427c19a69dd70f6511b95b0dac7/chrome/android/java/src/org/chromium/chrome/browser/profiles/MostVisitedSites.java
[modify] https://crrev.com/87a0e130422e6427c19a69dd70f6511b95b0dac7/chrome/android/javatests/src/org/chromium/chrome/browser/ntp/FakeMostVisitedSites.java
[modify] https://crrev.com/87a0e130422e6427c19a69dd70f6511b95b0dac7/chrome/browser/android/ntp/most_visited_sites_bridge.cc
[modify] https://crrev.com/87a0e130422e6427c19a69dd70f6511b95b0dac7/chrome/browser/android/ntp/most_visited_sites_bridge.h
[modify] https://crrev.com/87a0e130422e6427c19a69dd70f6511b95b0dac7/components/ntp_tiles/most_visited_sites.cc
[modify] https://crrev.com/87a0e130422e6427c19a69dd70f6511b95b0dac7/components/ntp_tiles/most_visited_sites.h

Comment 13 by treib@chromium.org, Jul 13 2016

Status: Fixed (was: Started)
Should be fixed with the previous two CLs
Project Member

Comment 14 by bugdroid1@chromium.org, Jul 13 2016

Labels: merge-merged-2795
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e14e21a2eb9ef970a6fc34f41290afe5e1015fc2

commit e14e21a2eb9ef970a6fc34f41290afe5e1015fc2
Author: dewittj <dewittj@chromium.org>
Date: Wed Jul 13 08:25:37 2016

NTP: Fix metrics recording crash by plumbing the necessary data to Java.

This has been broken since offline pages caused the refresh of the NTP
MostVisitedSites to be asynchronous.

BUG= 612160 

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

[modify] https://crrev.com/e14e21a2eb9ef970a6fc34f41290afe5e1015fc2/chrome/android/java/src/org/chromium/chrome/browser/ntp/MostVisitedItem.java
[modify] https://crrev.com/e14e21a2eb9ef970a6fc34f41290afe5e1015fc2/chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java
[modify] https://crrev.com/e14e21a2eb9ef970a6fc34f41290afe5e1015fc2/chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java
[modify] https://crrev.com/e14e21a2eb9ef970a6fc34f41290afe5e1015fc2/chrome/android/java/src/org/chromium/chrome/browser/profiles/MostVisitedSites.java
[modify] https://crrev.com/e14e21a2eb9ef970a6fc34f41290afe5e1015fc2/chrome/android/javatests/src/org/chromium/chrome/browser/ntp/FakeMostVisitedSites.java
[modify] https://crrev.com/e14e21a2eb9ef970a6fc34f41290afe5e1015fc2/chrome/android/javatests/src/org/chromium/chrome/browser/ntp/NewTabPageTest.java
[modify] https://crrev.com/e14e21a2eb9ef970a6fc34f41290afe5e1015fc2/chrome/browser/android/ntp/most_visited_sites_bridge.cc
[modify] https://crrev.com/e14e21a2eb9ef970a6fc34f41290afe5e1015fc2/chrome/browser/android/ntp/most_visited_sites_bridge.h
[modify] https://crrev.com/e14e21a2eb9ef970a6fc34f41290afe5e1015fc2/components/ntp_tiles.gypi
[modify] https://crrev.com/e14e21a2eb9ef970a6fc34f41290afe5e1015fc2/components/ntp_tiles/BUILD.gn
[modify] https://crrev.com/e14e21a2eb9ef970a6fc34f41290afe5e1015fc2/components/ntp_tiles/most_visited_sites.cc
[modify] https://crrev.com/e14e21a2eb9ef970a6fc34f41290afe5e1015fc2/components/ntp_tiles/most_visited_sites.h
Project Member

Comment 15 by bugdroid1@chromium.org, Jul 13 2016 

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

commit 87a0e130422e6427c19a69dd70f6511b95b0dac7
Author: treib <treib@chromium.org>
Date: Wed Jul 13 15:34:07 2016

NTP: Fix metrics recording race condition

The UI calls MostVisitedSites::RecordOpenedMostVisitedItem when an item is clicked. Before this CL, MostVisitedSites would get some info from its |current_suggestions_|, but those might have changed in the meantime. Now, all required info is passed into RecordOpenedMostVisitedItem instead.

BUG= 612160 

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

[modify] https://crrev.com/87a0e130422e6427c19a69dd70f6511b95b0dac7/chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java
[modify] https://crrev.com/87a0e130422e6427c19a69dd70f6511b95b0dac7/chrome/android/java/src/org/chromium/chrome/browser/profiles/MostVisitedSites.java
[modify] https://crrev.com/87a0e130422e6427c19a69dd70f6511b95b0dac7/chrome/android/javatests/src/org/chromium/chrome/browser/ntp/FakeMostVisitedSites.java
[modify] https://crrev.com/87a0e130422e6427c19a69dd70f6511b95b0dac7/chrome/browser/android/ntp/most_visited_sites_bridge.cc
[modify] https://crrev.com/87a0e130422e6427c19a69dd70f6511b95b0dac7/chrome/browser/android/ntp/most_visited_sites_bridge.h
[modify] https://crrev.com/87a0e130422e6427c19a69dd70f6511b95b0dac7/components/ntp_tiles/most_visited_sites.cc
[modify] https://crrev.com/87a0e130422e6427c19a69dd70f6511b95b0dac7/components/ntp_tiles/most_visited_sites.h

Comment 16 by dewittj@chromium.org, Jul 14 2016 

Marc, are you interested in merging the two CLs in to 53?

Comment 17 by treib@chromium.org, Jul 15 2016 

The first one should get merged, yes. The second one wouldn't apply cleanly, because a bunch of other stuff has landed in between, but that problem has been around for a long time, so there's no particular need to get it into M53.

I will handle the merge of the first one. (Thanks for reminding me!)

Comment 18 by treib@chromium.org, Jul 15 2016

Labels: Merge-Request-53
Requesting to merge crrev.com/405079 (the commit from comment 14) to M53

Comment 19 by dimu@google.com, Jul 15 2016

Labels: -Merge-Request-53 Merge-Approved-53 Hotlist-Merge-Approved
Your change meets the bar and is auto-approved for M53 (branch: 2785)
Project Member

Comment 20 by bugdroid1@chromium.org, Jul 15 2016

Labels: -merge-approved-53 merge-merged-2785
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/5da565407f9804fca8b16a29de7bf8eabba79925

commit 5da565407f9804fca8b16a29de7bf8eabba79925
Author: Marc Treib <treib@chromium.org>
Date: Fri Jul 15 11:58:08 2016

NTP: Fix metrics recording crash by plumbing the necessary data to Java.

This has been broken since offline pages caused the refresh of the NTP
MostVisitedSites to be asynchronous.

BUG= 612160 

Review-Url: https://codereview.chromium.org/2105933002
Cr-Commit-Position: refs/heads/master@{#405079}
(cherry picked from commit e14e21a2eb9ef970a6fc34f41290afe5e1015fc2)

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

Cr-Commit-Position: refs/branch-heads/2785@{#148}
Cr-Branched-From: 68623971be0cfc492a2cb0427d7f478e7b214c24-refs/heads/master@{#403382}

[modify] https://crrev.com/5da565407f9804fca8b16a29de7bf8eabba79925/chrome/android/java/src/org/chromium/chrome/browser/ntp/MostVisitedItem.java
[modify] https://crrev.com/5da565407f9804fca8b16a29de7bf8eabba79925/chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java
[modify] https://crrev.com/5da565407f9804fca8b16a29de7bf8eabba79925/chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java
[modify] https://crrev.com/5da565407f9804fca8b16a29de7bf8eabba79925/chrome/android/java/src/org/chromium/chrome/browser/profiles/MostVisitedSites.java
[modify] https://crrev.com/5da565407f9804fca8b16a29de7bf8eabba79925/chrome/android/javatests/src/org/chromium/chrome/browser/ntp/FakeMostVisitedSites.java
[modify] https://crrev.com/5da565407f9804fca8b16a29de7bf8eabba79925/chrome/android/javatests/src/org/chromium/chrome/browser/ntp/NewTabPageTest.java
[modify] https://crrev.com/5da565407f9804fca8b16a29de7bf8eabba79925/chrome/browser/android/ntp/most_visited_sites_bridge.cc
[modify] https://crrev.com/5da565407f9804fca8b16a29de7bf8eabba79925/chrome/browser/android/ntp/most_visited_sites_bridge.h
[modify] https://crrev.com/5da565407f9804fca8b16a29de7bf8eabba79925/components/ntp_tiles.gypi
[modify] https://crrev.com/5da565407f9804fca8b16a29de7bf8eabba79925/components/ntp_tiles/BUILD.gn
[modify] https://crrev.com/5da565407f9804fca8b16a29de7bf8eabba79925/components/ntp_tiles/most_visited_sites.cc
[modify] https://crrev.com/5da565407f9804fca8b16a29de7bf8eabba79925/components/ntp_tiles/most_visited_sites.h

Comment 21 by bauerb@chromium.org, Sep 8 2016 

Issue 585716 has been merged into this issue.
►

Sign in to add a comment