I'm reverting for now to fix. Tagging this as security since we'll have to audit and see what IPC changes landed without security review.

dpranke@, I know script like my_reviews.py and my_activity.py can look at what CLs you reviewed, etc. Is there any sort of documented API for Rietveld to help the retroactive investigation? Or should we need to just cobble something together ourselves?

Gah, thanks for catching that, Daniel, and for the repro example.

The problem with the original change looks like it's actually the change to the call to CheckOwners(), which used to apply to all of the files, and now only applies to the default whitelisted set of source files which, possibly incorrectly, doesn't include .mojom's.

So, it's quite possible a lot of owners file checks might not have been happening, but possibly the only ones we really care about were the IPC checks to mojoms.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/tools/depot_tools.git/+/3c86cee33acc7a3641d7c8fdd3944ee44ed507dd

commit 3c86cee33acc7a3641d7c8fdd3944ee44ed507dd
Author: Dirk Pranke <dpranke@chromium.org>
Date: Tue Jan 24 06:02:06 2017

Revert "Fix OWNERS canned check to avoid duplicate output."

This reverts commit 66c50ea1f11a8776355f50800fe7e77e9eea4ce1.

Reason for revert:
Breaks some OWNERS checks. See  https://crbug.com/684270  for details.

Original change's description:
> Fix OWNERS canned check to avoid duplicate output.
>
> 1. Update PanProjectChecks() to supply source_filter to CheckOwners().
>    This prevents files under e.g. third_party/WebKit being checked both
>    by the top-level and sub-directory PRESUBMIT rules.
> 2. Fix CheckOwners() to list missing-OWNERS only for the sub-directory
>    it is invoked on, rather than the whole CL. This prevents files under
>    sub-directories with their own PRESUBMIT rule, within the same repo,
>    from causing OWNERS to be checked for files outside that directory.
>
> BUG=
>
> Reviewed-on: https://chromium-review.googlesource.com/426003

TBR=wez@chromium.org,dcheng@chromium.org
BUG= 684270 

Change-Id: I094818a6fa3e16fbac0d37d2596a40210611ee05
Reviewed-on: https://chromium-review.googlesource.com/431656
Reviewed-by: Dirk Pranke <dpranke@chromium.org>

[modify] https://crrev.com/3c86cee33acc7a3641d7c8fdd3944ee44ed507dd/presubmit_canned_checks.py
[modify] https://crrev.com/3c86cee33acc7a3641d7c8fdd3944ee44ed507dd/tests/presubmit_unittest.py

I've reverted the change.

I think we can get the list of reviewers for a CL from the Rietveld APIs; I'll double-check tomorrow.

Dirk, can you halp w/ the Rietveld API fu, so we can establish the affected CLs and close this out?

Yes, I can help and will try to get to this later today.

Here's a change that will return the list of email addresses of people that approved a CL:

https://codereview.chromium.org/2660133002

Presumably you can integrate that into the appropriate series of git commands to find all changes landed since 1/6 that touched IPC files, extract the issue IDs for them, and then check the reviewers against the list of IPC reviewers? If not, let me know if you'd like help, though my cycles are quite limited at the moment :(.

Thanks Dirk.

OK, I've chosen the following CLs as the start & end points to consider:
Start: df4f7e43c1e7
End: 11da5fe6e7d5

In between those CLs there were modifications to:
64 *.mojom
16 *_messages.h

So I think I can do git log df4f7e43c1e7..11da5fe6e7d5 -- <files> to get the canonical list of at-risk commits.

My Frankenscript is complete, and reports the following:

There are 3570 affected revisions.
There are 66 potentially affected CLs.
http://codereview.chromium.org/2568303006 (approvers:sky@chromium.org)
http://codereview.chromium.org/2611183006 (approvers:ben@chromium.org)
http://codereview.chromium.org/2617403003 (approvers:sky@chromium.org)
http://codereview.chromium.org/2621143002 (approvers:primiano@chromium.org, rockot@chromium.org)
http://codereview.chromium.org/2621843003 (approvers:sky@chromium.org)
http://codereview.chromium.org/2625873004 (approvers:jam@chromium.org)
http://codereview.chromium.org/2629743003 (approvers:dpranke@chromium.org, kmarshall@chromium.org, thakis@chromium.org)
http://codereview.chromium.org/2630443002 (approvers:bauerb@chromium.org)
http://codereview.chromium.org/2633233003 (approvers:sky@chromium.org)
http://codereview.chromium.org/2634073002 (approvers:)
http://codereview.chromium.org/2634493002 (approvers:)
http://codereview.chromium.org/2637393002 (approvers:yzshen@chromium.org)
http://codereview.chromium.org/2637403007 (approvers:jamescook@chromium.org)
http://codereview.chromium.org/2642943002 (approvers:jsbell@chromium.org)
http://codereview.chromium.org/2646033003 (approvers:dgozman@chromium.org)

(The listed CLs both have one or more .mojoms and have a list of approvers that does not include anyone from ipc/SECURITY_OWNERS.)

Frankenscript is uploaded at https://codereview.chromium.org/2675403002/, to record the procedure that was taken.

All the affected CLs have had owners added & messages sent requesting reviews; though many of the reviews are trivial it seemed preferable to let an OWNER make that judgement call.

I only noticed these CLs emailing me by accident, they don't show on my review queue because they are "Closed". I can go through and manually un-close the CLs and re-review but I expect a lot of these will get missed by the reviewers - is there any chance of re-opening all the CLs that have not already been lgtmed by the missing reviewers?

Re-opening the CLs seems dubious since they can be CQ'd again and will
either fail (since they already landed) or succeed (usually as a no-op),
and add spurious extra commit messages.

But yes, it is strightforward for me to determine the affected CLs which
still need LGTM - I could grab the list of authors and email them directly,
say?

Issue 679531 has been merged into this issue.