New issue
Advanced search Search tips

Issue 911439 link

Starred by 1 user
Status: Fixed
Owner:
wzang@chromium.org
Closed: Dec 6
Cc:
ios-bugs-priority@chromium.org
ios-bugs@chromium.org
EstimatedDays: ----
NextAction: ----
OS: iOS
Pri: 1
Type: Bug



Sign in to add a comment

Reverting cros: Fix DCHECK for rlz_lib::UpdateExistingAccessPointRlz

Project Member Reported by jlebel@chromium.org, Dec 4 
Sorry I had to revert:
https://chromium-review.googlesource.com/c/chromium/src/+/1353990

It broke Chrome iOS with:
../../ios/chrome/browser/ios_chrome_main_parts.mm:175:56: error: allocating an object of abstract class type 'RLZTrackerDelegateImpl'
  rlz::RLZTracker::SetRlzDelegate(base::WrapUnique(new RLZTrackerDelegateImpl));

This patch was reverted with:
https://chromium-review.googlesource.com/c/chromium/src/+/1360274

For more information:
https://uberchromegw.corp.google.com/i/internal.bling.main/builders/canary-device/builds/1686
Project Member

Comment 1 by bugdroid1@chromium.org, Dec 6 

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

commit 0d2a5a88a940d3df25c6a074824adb6a901e099f
Author: Jérôme Lebel <jlebel@chromium.org>
Date: Thu Dec 06 10:13:37 2018

Reland "cros: Fix DCHECK for rlz_lib::UpdateExistingAccessPointRlz"

This is a reland of 1d497322e9fd8d9ab1da00767e30c618186151c6

This patch also includes implementation for:
RLZTrackerDelegateImpl::ShouldUpdateExistingAccessPointRlz()

Original change's description:
> cros: Fix DCHECK for rlz_lib::UpdateExistingAccessPointRlz
>
> In addition to fixing the DCHECK, this CL contains a speculative fix
> for flaky test ( crbug.com/907379 ). The guess for the flakiness is
> (not able to repro the failure locally though):
>
> |RecordProductEvent| may fail to acquire the |ScopedRlzValueStoreLock|
> when recording the CAI event, because the lock is being taken by
> |UpdateExistingAccessPointRlz|. In tests, the delay for
> |ScheduleDelayedInit| is set to zero, so it's possible for this race
> condition to happen. But in production, the delay is non-zero.
>
> I don't think we're losing test coverage by disabling
> |UpdateExistingAccessPointRlz| here because the call is a no-op in
> tests, and another test (RlzLibTest.UpdateExistingAccessPointRlz) is
> testing its implementation.
>
> Bug:  906293 ,  907379 
> Change-Id: I208919f22edc09fc1a739a1efe1909a71e5a2730
> Reviewed-on: https://chromium-review.googlesource.com/c/1353990
> Reviewed-by: Roger Tawa <rogerta@chromium.org>
> Commit-Queue: Wenzhao (Colin) Zang <wzang@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#613172}

Bug:  906293 ,  907379 ,  911439 
Change-Id: Ic1d9f85bc3714f890d2dc7fcc1d070d0711baec0
Reviewed-on: https://chromium-review.googlesource.com/c/1360692
Commit-Queue: Jérôme Lebel <jlebel@chromium.org>
Reviewed-by: Wenzhao (Colin) Zang <wzang@chromium.org>
Reviewed-by: Rohit Rao <rohitrao@chromium.org>
Reviewed-by: Roger Tawa <rogerta@chromium.org>
Reviewed-by: Jérôme Lebel <jlebel@chromium.org>
Cr-Commit-Position: refs/heads/master@{#614303}
[modify] https://crrev.com/0d2a5a88a940d3df25c6a074824adb6a901e099f/chrome/browser/rlz/chrome_rlz_tracker_delegate.cc
[modify] https://crrev.com/0d2a5a88a940d3df25c6a074824adb6a901e099f/chrome/browser/rlz/chrome_rlz_tracker_delegate.h
[modify] https://crrev.com/0d2a5a88a940d3df25c6a074824adb6a901e099f/components/rlz/rlz_tracker.cc
[modify] https://crrev.com/0d2a5a88a940d3df25c6a074824adb6a901e099f/components/rlz/rlz_tracker_delegate.h
[modify] https://crrev.com/0d2a5a88a940d3df25c6a074824adb6a901e099f/components/rlz/rlz_tracker_unittest.cc
[modify] https://crrev.com/0d2a5a88a940d3df25c6a074824adb6a901e099f/ios/chrome/browser/rlz/rlz_tracker_delegate_impl.cc
[modify] https://crrev.com/0d2a5a88a940d3df25c6a074824adb6a901e099f/ios/chrome/browser/rlz/rlz_tracker_delegate_impl.h

Comment 2 by jlebel@chromium.org, Dec 6

Status: Fixed (was: Untriaged)
►

Sign in to add a comment