+kmarshall

This seems to be similar to bug 729716 which was probably fixed by crrev.com/523983 which got reverted two weeks ago.
 
Details:
The symbol in #3 is actually something like

void base::internal::Invoker<base::internal::BindState<
    void (safe_browsing::V4Database::*)(scoped_refptr<base::SingleThreadTaskRunner> const&, base::Callback<void (std::vector<safe_browsing::ListIdentifier, .. > const&), ..>),
    base::WeakPtr<safe_browsing::V4Database>, ...>::RunImpl<..>(...)

so it seems that it's invoking a callback which takes a vector<ListIdentifier>. The only such callback I could find was the safe_browsing::DatabaseReadyForUpdatesCallback. So it seems that that callback is invoked from a different thread than the one where it was created by base::Bind (i.e. where the weakptr was acquired).

The only callback with such a signature I could find is safe_browsing::DatabaseReadyForUpdatesCallback passed into V4Database::VerifyChecksum.

The code around that currently does:
db_task_runner_->PostTask(
    FROM_HERE,
    base::Bind(&V4Database::VerifyChecksumOnTaskRunner,
               weak_factory_on_io_.GetWeakPtr(), callback_task_runner,
               db_ready_for_updates_callback));

which seems weird to me (running on db_task_runner_ with a weakptr acquited by weak_factory_on_io_), but I might be missing something.

Sorry for the broken CL link, it should be:
https://chromium-review.googlesource.com/523983

Another failure:
https://luci-milo.appspot.com/buildbot/tryserver.chromium.linux/linux_chromium_chromeos_ozone_rel_ng/428822

 Issue 742976  has been merged into this issue.

This test is failing on a significant number of tryjobs and causing CLs to be rejected from the CQ. linux_chromium_chromeos_ozone_rel_ng is unacceptably flaky because of this; see  Issue 742966  for another problem, and the attached screenshot.

I don't see an easy way to just disable the one parameterized version PolicyPrefIndicatorTestInstance/PolicyPrefIndicatorTest.CheckPolicyIndicators/14 .

Fixing these CQ flakes is urgent. I would be inclined to disable the test entirely but am concerned about allowing broken CLs to land in the product. Who can take this?

Deleted: Screen Shot 2017-07-14 at 10.30.37 AM.png 736 KB

	Screen Shot 2017-07-14 at 10.30.37 AM.png 736 KB View Download

Note that per  Issue 742976  (which I duplicated into this one), chromium-try-flakes found this flaky on both the regular and Ozone bots (which dpranke@ just folded together, BTW).

https://chromium-try-flakes.appspot.com/all_flake_occurrences?key=ahVzfmNocm9taXVtLXRyeS1mbGFrZXNyWwsSBUZsYWtlIlBQb2xpY3lQcmVmSW5kaWNhdG9yVGVzdEluc3RhbmNlL1BvbGljeVByZWZJbmRpY2F0b3JUZXN0LkNoZWNrUG9saWN5SW5kaWNhdG9ycy8xNAw.

We should re-land kmarshall's patch 523983 ASAP - unfortunately it was reverted because of a KitKat breakage, which we seem to be missing try-bot coverage for - any suggestions for how we can address that?

I don't know that we have any K trybots; maybe jbudorick@ has suggestions.

Downgrading to P1 since I think the tests are off the CQ for the moment.

linux_android_rel_ng is K, though it's K phone and iirc kmarshall's patch only broke K tablet?

@jbudorick - that's possible (I'm not that involved in this). Any suggestions for testing on a K tablet, then?

Outside of local testing (which is likely easier if you can get your hands on an N7v2), it's possible to launch tests from local builds into the K tablet swarming bots, but we have very limited capacity there. I have a doc containing the instructions for doing so that I'll send over.

Is it just me or are there no logs of the failure in https://bugs.chromium.org/p/chromium/issues/detail?id=736924 (due to which kmarshall's CL was reverted)?

I *do* have a N7 kicking around. It's not on Kitkat but I can flash it.

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

commit e6af5cab1a0d4fde0e721bcdb5de52254679b27f
Author: Kevin Marshall <kmarshall@chromium.org>
Date: Fri Jul 14 23:52:07 2017

Reland "Fix thread safety issues with safe browsing DB."

This is a reland of 0ac8d89ee822c431ea66e0a7da762bb8c28b1877
Original change's description:
> Fix thread safety issues with safe browsing DB.
> 
> Fix thread safety issues with safe browsing DB.
> The Safe Browsing DB has some sequence consistency issues, which
> is caught by the stricter WeakPtr semantics detailed on CL 2908073007.
> 
> * Delete SafeBrowsingDatabaseManager on IO thread.
> * Remove illegal dereferencing of IO-thread WeakPtr from DB thread in
>   V4Database::VerifyChecksumOnTaskRunner
> * Remove non-threadsafe accesses of |io_thread_| resident members from
>   the DB thread in V4LocalDatabaseManager.
> * Add IO-thread runloops to unit tests to handle database teardown.
> 
> 
> R=nparker@chromium.org
> CC=wez@chromium.org
> 
> Bug: 729716
> Change-Id: I1bc620d42aca2f1cc99e482b7776a628d783d390
> Reviewed-on: https://chromium-review.googlesource.com/523983
> Commit-Queue: Kevin Marshall <kmarshall@chromium.org>
> Reviewed-by: Charlie Harrison <csharrison@chromium.org>
> Reviewed-by: Greg Thompson <grt@chromium.org>
> Reviewed-by: Varun Khaneja <vakh@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#481735}

TBR=csharrison@chromium.org, grt@chromium.org, vakh@chromium.org

Bug: 729716,  742596 
Change-Id: I4d67aa201c54be7a1e75a7375ac9861a29f1d87f
Reviewed-on: https://chromium-review.googlesource.com/571684
Reviewed-by: Kevin Marshall <kmarshall@chromium.org>
Commit-Queue: Kevin Marshall <kmarshall@chromium.org>
Cr-Commit-Position: refs/heads/master@{#486946}
[modify] https://crrev.com/e6af5cab1a0d4fde0e721bcdb5de52254679b27f/chrome/browser/safe_browsing/incident_reporting/resource_request_detector_unittest.cc
[modify] https://crrev.com/e6af5cab1a0d4fde0e721bcdb5de52254679b27f/components/safe_browsing_db/BUILD.gn
[modify] https://crrev.com/e6af5cab1a0d4fde0e721bcdb5de52254679b27f/components/safe_browsing_db/database_manager.cc
[modify] https://crrev.com/e6af5cab1a0d4fde0e721bcdb5de52254679b27f/components/safe_browsing_db/database_manager.h
[modify] https://crrev.com/e6af5cab1a0d4fde0e721bcdb5de52254679b27f/components/safe_browsing_db/database_manager_unittest.cc
[modify] https://crrev.com/e6af5cab1a0d4fde0e721bcdb5de52254679b27f/components/safe_browsing_db/remote_database_manager_unittest.cc
[modify] https://crrev.com/e6af5cab1a0d4fde0e721bcdb5de52254679b27f/components/safe_browsing_db/v4_database.cc
[modify] https://crrev.com/e6af5cab1a0d4fde0e721bcdb5de52254679b27f/components/safe_browsing_db/v4_database.h
[modify] https://crrev.com/e6af5cab1a0d4fde0e721bcdb5de52254679b27f/components/safe_browsing_db/v4_local_database_manager.h
[modify] https://crrev.com/e6af5cab1a0d4fde0e721bcdb5de52254679b27f/components/safe_browsing_db/whitelist_checker_client_unittest.cc
[modify] https://crrev.com/e6af5cab1a0d4fde0e721bcdb5de52254679b27f/components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle_unittest.cc

OK, relanded the fix - please verify?

Thank you!
According to the chromium-try-flakes[1], the last failures were 2017-07-14 23:01:42 UTC. The CL landed Jul 14 23:52:07 2017.

The flakiness dashboard[2] also looks fine IMO.

[1] https://chromium-try-flakes.appspot.com/all_flake_occurrences?key=ahVzfmNocm9taXVtLXRyeS1mbGFrZXNyWwsSBUZsYWtlIlBQb2xpY3lQcmVmSW5kaWNhdG9yVGVzdEluc3RhbmNlL1BvbGljeVByZWZJbmRpY2F0b3JUZXN0LkNoZWNrUG9saWN5SW5kaWNhdG9ycy8xNAw

[2] https://test-results.appspot.com/dashboards/flakiness_dashboard.html#testType=browser_tests (with patch)&tests=PolicyPrefIndicatorTestInstance/PolicyPrefIndicatorTest.CheckPolicyIndicators/14