Add interstitial info to certificate reports

Add interstitial info to certificate reports

This change adds additional information about the SSL interstitial (what
type it was, whether the user clicked through, and whether it was
overridable) to SSL certificate reports.

BUG=462713, 461588
Committed: https://crrev.com/6473c592af545f354f53b65251623cc509063c02
Cr-Commit-Position: refs/heads/master@{#330605}

[estark, 2015-04-10 16:26:27]

estark@chromium.org changed reviewers:
+ felt@chromium.org, rsleevi@chromium.org

[estark, 2015-04-10 16:26:27]

This isn't ready for a full review yet (lacking tests, comments, and some other
clean-up), but Learning on the Loo told me that it could be a good idea to get
some high-level design guidance.

My goal here is to add information about the interstitial shown to the user into
the certificate reports (what type of interstitial it was, whether the user
clicked through or not, etc.) In this CL I've done that by having the
SSLBlockingPage build the actual report, and then passing around a
CertLoggerRequest (the protobuf) instead of passing around all the individual
bits of data that go into a report.

What I like about this is that it avoids SafeBrowsingUIManager and PingManager
having to know about what goes in a report and having to know a bunch of details
about interstitials (like what the different types of interstitials are).

What I don't like about this:
- I don't know if it's weird to #include a protobuf everywhere. Would it be
better to use a CertReport object that wraps the protobuf rather than passing
around a protobuf?
- SSLBlockingPage now needs to know about (and call a static method on)
CertificateErrorReporter directly, which it didn't before.
- //chrome/browser/net is now concerned with details about interstitials, but I
don't know if that's avoidable because the protobuf is defined there and if we
want the data, it's gotta go into the protobuf.

Any thoughts? Thanks and sorry for the essay!

[estark, 2015-04-14 21:24:14]

ping :)

[felt, 2015-04-14 23:40:49]

Ryan probably has super-strong opinions, so I hesitate to ask you to change
things only to have Ryan then tell you to change it another way. :) So Ryan, you
have the honors...

[Ryan Sleevi, 2015-04-16 01:44:10]

Adrienne: Feel free to drive the comments; usually helps to parallelize and to
see where I'm unreasonable ;)

https://codereview.chromium.org/1076273002/diff/1/chrome/browser/net/certific...
File chrome/browser/net/certificate_error_reporter.cc (right):

https://codereview.chromium.org/1076273002/diff/1/chrome/browser/net/certific...
chrome/browser/net/certificate_error_reporter.cc:48: // TODO(estark): Encrypt
the report if not sending over HTTPS
FWIW, a tracking bug for this is handy, even if it doesn't have a milestone for
it.

https://codereview.chromium.org/1076273002/diff/1/chrome/browser/net/certific...
chrome/browser/net/certificate_error_reporter.cc:139: }
Dead code? Broken?

https://codereview.chromium.org/1076273002/diff/1/chrome/browser/net/certific...
File chrome/browser/net/certificate_error_reporter.h (right):

https://codereview.chromium.org/1076273002/diff/1/chrome/browser/net/certific...
chrome/browser/net/certificate_error_reporter.h:44: enum Overridable {
OVERRIDABLE, NOT_OVERRIDABLE };
First scan: This seems to introduce awareness of interstitials into the
CertificateErrorReporter, which doesn't seem ideal.

I'm not sure what to suggest, but it doesn't feel right.

Does anything else in chrome/browser/net know about interstitials? I don't think
it does....

(This is somewhat the same problem we had with /ssl/ and /interstitials/ and
breaking that dependency)

https://codereview.chromium.org/1076273002/diff/1/chrome/browser/net/certific...
chrome/browser/net/certificate_error_reporter.h:64: virtual void
SendReport(ReportType type, const CertLoggerRequest& report);
Virtuals + overloads = special place in hell when you try to subclass (override
helps a little, but then you can end up shadow-hiding an override on the
subclass unless you import. Confused? Like I said, special place in hell...)

Generally, prefer
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Function_Over...

https://codereview.chromium.org/1076273002/diff/1/chrome/browser/net/certific...
chrome/browser/net/certificate_error_reporter.h:73: // Populate the
CertLoggerRequest for a report.
By making this public, this is beginning to violate the "Single Responsibility
Principle" of having one responsibility and doing it well.

1) I'm not a big fan of static methods on classes (I know, i'm a hater)
2) Does it make sense to factor out a report builder?

https://codereview.chromium.org/1076273002/diff/1/chrome/browser/net/certific...
chrome/browser/net/certificate_error_reporter.h:77: static void
BuildReport(const std::string& hostname,
ditto overloads again

https://codereview.chromium.org/1076273002/diff/1/chrome/browser/ssl/ssl_bloc...
File chrome/browser/ssl/ssl_blocking_page.cc (right):

https://codereview.chromium.org/1076273002/diff/1/chrome/browser/ssl/ssl_bloc...
chrome/browser/ssl/ssl_blocking_page.cc:590: FinishCertCollection(true);
Suggestion: Reuse the enum (SecurityInterstitialMetricsHelper seems public - is
it? If not, perhaps a new enum?); it's not clear what the "true" / "false" are
here

Alternatively (less ideally, but depending on scope, may be cleaner)
FinishCertCollection(true /*proceeded*/);
FinishCertCollection(false /*proceeded*/);

But can you can see, "false proceeded" isn't exactly clear

[estark, 2015-04-16 02:09:31]

Thanks Ryan. Brainstorming inline about how to avoid chrome/browser/net knowing
about interstitials (sort of).

https://codereview.chromium.org/1076273002/diff/1/chrome/browser/net/certific...
File chrome/browser/net/certificate_error_reporter.h (right):

https://codereview.chromium.org/1076273002/diff/1/chrome/browser/net/certific...
chrome/browser/net/certificate_error_reporter.h:44: enum Overridable {
OVERRIDABLE, NOT_OVERRIDABLE };
On 2015/04/16 01:44:10, Ryan Sleevi wrote:
> First scan: This seems to introduce awareness of interstitials into the
> CertificateErrorReporter, which doesn't seem ideal.
> 
> I'm not sure what to suggest, but it doesn't feel right.
> 
> Does anything else in chrome/browser/net know about interstitials? I don't
think
> it does....
> 
> (This is somewhat the same problem we had with /ssl/ and /interstitials/ and
> breaking that dependency)

Agreed. As I mentioned, I wasn't sure how to avoid this problem because the
protobuf (which must surely know about all the data it sends, including
interstitial info) is defined in chrome/browser/net.*

If we're willing to accept the protobuf knowing about interstitials, then here
is an alternative proposal that at least avoids CertificateErrorReporter knowing
about interstitials: chrome/browser/net defines a CertificateErrorReport
interface, and a CertificateErrorReport is what gets passed to |SendReport|.
chrome/browser/ssl implements that interface to build protobufs and populate the
interstitial fields. chrome/browser/net also has an implementation of the
interface that builds the protobuf without the interstitial fields; this is what
would be used by ChromeFraudulentCertificateReporter.

*I don't suppose there's some magical protobufs feature that I don't know about
that would solve this problem?

[estark, 2015-04-17 23:02:52]

On 2015/04/16 02:09:31, estark wrote:
> Thanks Ryan. Brainstorming inline about how to avoid chrome/browser/net
knowing
> about interstitials (sort of).
> 
>
https://codereview.chromium.org/1076273002/diff/1/chrome/browser/net/certific...
> File chrome/browser/net/certificate_error_reporter.h (right):
> 
>
https://codereview.chromium.org/1076273002/diff/1/chrome/browser/net/certific...
> chrome/browser/net/certificate_error_reporter.h:44: enum Overridable {
> OVERRIDABLE, NOT_OVERRIDABLE };
> On 2015/04/16 01:44:10, Ryan Sleevi wrote:
> > First scan: This seems to introduce awareness of interstitials into the
> > CertificateErrorReporter, which doesn't seem ideal.
> > 
> > I'm not sure what to suggest, but it doesn't feel right.
> > 
> > Does anything else in chrome/browser/net know about interstitials? I don't
> think
> > it does....
> > 
> > (This is somewhat the same problem we had with /ssl/ and /interstitials/ and
> > breaking that dependency)
> 
> Agreed. As I mentioned, I wasn't sure how to avoid this problem because the
> protobuf (which must surely know about all the data it sends, including
> interstitial info) is defined in chrome/browser/net.*
> 
> If we're willing to accept the protobuf knowing about interstitials, then here
> is an alternative proposal that at least avoids CertificateErrorReporter
knowing
> about interstitials: chrome/browser/net defines a CertificateErrorReport
> interface, and a CertificateErrorReport is what gets passed to |SendReport|.
> chrome/browser/ssl implements that interface to build protobufs and populate
the
> interstitial fields. chrome/browser/net also has an implementation of the
> interface that builds the protobuf without the interstitial fields; this is
what
> would be used by ChromeFraudulentCertificateReporter.
> 
> *I don't suppose there's some magical protobufs feature that I don't know
about
> that would solve this problem?

Ping -- any thoughts about the above proposal, Ryan? (making the protobuf the
only thing in //chrome/browser/net that knows about interstitials)

[Ryan Sleevi, 2015-04-17 23:20:08]

rsleevi@chromium.org changed reviewers:
+ davidben@chromium.org

[Ryan Sleevi, 2015-04-17 23:20:08]

Roping in David because my review latency is about to spike next week, and I
don't want to drop this. Thanks for the ping - I haven't had a chance to really
sit down and think on this, but David might have either some spare cycles or
someone to route it through.

[estark, 2015-04-20 17:58:46]

On 2015/04/17 23:20:08, Ryan Sleevi wrote:
> Roping in David because my review latency is about to spike next week, and I
> don't want to drop this. Thanks for the ping - I haven't had a chance to
really
> sit down and think on this, but David might have either some spare cycles or
> someone to route it through.

davidben, here's the tl;dr. In //chrome/browser/net, I have a protobuf and a
class (CertificateErrorReporter) that constructs and sends the protobuf over the
network. I want to include some data in the reports that //c/b/net really has no
business knowing about (related to interstitials). Any ideas on how to do this?

My only idea so far has been to have a report building interface
(CertificateReport) in //c/b/net with an implementation of it in //c/b/ssl, so
that the CertificateErrorReporter method would be |SendReport(report)| rather
than |(SendReport(hostname, ssl_info, a bunch of interstitial data)|. The
protobuf would still know about the interstitial-related data, but it would be
the only thing in //c/b/net that does.

[estark, 2015-04-28 00:26:30]

On 2015/04/20 17:58:46, estark wrote:
> On 2015/04/17 23:20:08, Ryan Sleevi wrote:
> > Roping in David because my review latency is about to spike next week, and I
> > don't want to drop this. Thanks for the ping - I haven't had a chance to
> really
> > sit down and think on this, but David might have either some spare cycles or
> > someone to route it through.
> 
> davidben, here's the tl;dr. In //chrome/browser/net, I have a protobuf and a
> class (CertificateErrorReporter) that constructs and sends the protobuf over
the
> network. I want to include some data in the reports that //c/b/net really has
no
> business knowing about (related to interstitials). Any ideas on how to do
this?
> 
> My only idea so far has been to have a report building interface
> (CertificateReport) in //c/b/net with an implementation of it in //c/b/ssl, so
> that the CertificateErrorReporter method would be |SendReport(report)| rather
> than |(SendReport(hostname, ssl_info, a bunch of interstitial data)|. The
> protobuf would still know about the interstitial-related data, but it would be
> the only thing in //c/b/net that does.

friendly ping -- davidben, rsleevi?

[Ryan Sleevi, 2015-04-28 22:53:39]

On 2015/04/28 00:26:30, estark wrote:
> On 2015/04/20 17:58:46, estark wrote:
> > My only idea so far has been to have a report building interface
> > (CertificateReport) in //c/b/net with an implementation of it in //c/b/ssl,
so
> > that the CertificateErrorReporter method would be |SendReport(report)|
rather
> > than |(SendReport(hostname, ssl_info, a bunch of interstitial data)|. The
> > protobuf would still know about the interstitial-related data, but it would
be
> > the only thing in //c/b/net that does.
> 
> friendly ping -- davidben, rsleevi?

I keep hoping David would have a better idea here, in part because I'm not sure
what the layering is *supposed* to be, only what I've observed.

As far as ideas go, I think encapsulating it in the protobuf is slightly better
than baking it hard into the API, or at least, feels nicer, although it might
mean more complexity for those having to build reports (for example, can they
botch the protobuf structures).

I suspect we're going to continue to run into these sorts of layering concerns
by having a single reporter reporting two different types of reports, but
without being able to give concrete suggestions as to the resolution of these,
nor who to recommend (beyond David), it feels wrong to block this CL.

So your suggestion how to resolve the tension sounds right to me, and unless
David chimes in within the day with a way that solves all our problems, go with
it.

[davidben, 2015-04-28 22:54:47]

On 2015/04/28 22:53:39, Ryan Sleevi wrote:
> On 2015/04/28 00:26:30, estark wrote:
> > On 2015/04/20 17:58:46, estark wrote:
> > > My only idea so far has been to have a report building interface
> > > (CertificateReport) in //c/b/net with an implementation of it in
//c/b/ssl,
> so
> > > that the CertificateErrorReporter method would be |SendReport(report)|
> rather
> > > than |(SendReport(hostname, ssl_info, a bunch of interstitial data)|. The
> > > protobuf would still know about the interstitial-related data, but it
would
> be
> > > the only thing in //c/b/net that does.
> > 
> > friendly ping -- davidben, rsleevi?
> 
> I keep hoping David would have a better idea here, in part because I'm not
sure
> what the layering is *supposed* to be, only what I've observed.
> 
> As far as ideas go, I think encapsulating it in the protobuf is slightly
better
> than baking it hard into the API, or at least, feels nicer, although it might
> mean more complexity for those having to build reports (for example, can they
> botch the protobuf structures).
> 
> I suspect we're going to continue to run into these sorts of layering concerns
> by having a single reporter reporting two different types of reports, but
> without being able to give concrete suggestions as to the resolution of these,
> nor who to recommend (beyond David), it feels wrong to block this CL.
> 
> So your suggestion how to resolve the tension sounds right to me, and unless
> David chimes in within the day with a way that solves all our problems, go
with
> it.

Sorry, a lot of things fell through the cracks while I was sick and I'm still
catching up things. I'll look at this first thing tomorrow.

[davidben, 2015-04-29 18:38:45]

What if we move the four chrome/browser/net files here to chrome/browser/ssl.
That would be cert_logger.proto, certificate_error_reporter.{h,cc}, and
chrome_fraudulent_certificate_reporter.cc. It seems all three of these would
naturally live in SSL and they'd be able to know about interstitial stuff.

Since CertificateErrorReporter and CertLoggerRequest are the interface to some
web service whose API inherently knows about interstitials, it makes sense that
they live somewhere that knows about interstitial state or above. That seems to
be chrome/browser/ssl based on SSLBlockingPage and stuff living there.

I see two signals that end up at CertificateErrorReporter. One's in net for
pinning failures. It routes up through an interface to
ChromeFradulentCertificateReporter which necessarily depends on
CertificateErrorReporter. Moving ChromeFradulentCertificateReporter to
chrome/browser/ssl seems pretty natural and so it'd be
ChromeFradulentCertificateReporter's job to assemble the //net signal (which
can't depend on CertLoggerRequest) into a CertLoggerRequest or whatever
appropriate format and then call CertificateErrorReporter.

The other signal seems to start from SSLBlockingPage somewhere. Right now it
seems to route up through another interface to SafeBrowsing and back down to
CertificateErrorReporter. chrome/browser/safe_browser doesn't currently depend
on chrome/browser/ssl, but I'm actually confused why SafeBrowsing is involved at
all, actually.

Right now we do SSLBlockingPage --> ChromeContentBrowserClient --> SafeBrowsing
stuff -> CertificateErrorReporter. If CertificateErrorReporter gets moved to
chrome/browser/ssl, we're routing from chrome/browser/ssl, up a layer, down to a
sibling layer, and then back over to chrome/browser/ssl. Why not keep that logic
entirely within chrome/browser/ssl? It seems the only thing SafeBrowsing
contributes is a toggle that we don't report interstitial stuff is SafeBrowsing
is disabled? If that's it, perhaps it should just query
prefs->GetBoolean(prefs::kSafeBrowsingEnabled) on the UI thread sometime before
popping to IO to fire the request.

[estark, 2015-04-29 19:22:36]

Thanks David.

On 2015/04/29 18:38:45, David Benjamin wrote:
> What if we move the four chrome/browser/net files here to chrome/browser/ssl.
> That would be cert_logger.proto, certificate_error_reporter.{h,cc}, and
> chrome_fraudulent_certificate_reporter.cc. It seems all three of these would
> naturally live in SSL and they'd be able to know about interstitial stuff.
> 
> Since CertificateErrorReporter and CertLoggerRequest are the interface to some
> web service whose API inherently knows about interstitials, it makes sense
that
> they live somewhere that knows about interstitial state or above. That seems
to
> be chrome/browser/ssl based on SSLBlockingPage and stuff living there.
> 
> I see two signals that end up at CertificateErrorReporter. One's in net for
> pinning failures. It routes up through an interface to
> ChromeFradulentCertificateReporter which necessarily depends on
> CertificateErrorReporter. Moving ChromeFradulentCertificateReporter to
> chrome/browser/ssl seems pretty natural and so it'd be
> ChromeFradulentCertificateReporter's job to assemble the //net signal (which
> can't depend on CertLoggerRequest) into a CertLoggerRequest or whatever
> appropriate format and then call CertificateErrorReporter.

That means that ProfileIOData (who constructs the
ChromeFraudulentCertificateReporter) would have to depend on //c/b/ssl. Is that
kosher? I guess there are existing //c/b/ssl dependencies in //c/b/profiles so
it's probably okay. One other question is if it's appropriate for
ChromeFraudulentCertificateReporter to be in //c/b/ssl given that it implements
net::FraudulentCertificateReporter. (I was sort of operating on the assumption
that things that implement //net interfaces belong in //c/b/net, but maybe I
completely fabricated that rule.)

> 
> The other signal seems to start from SSLBlockingPage somewhere. Right now it
> seems to route up through another interface to SafeBrowsing and back down to
> CertificateErrorReporter. chrome/browser/safe_browser doesn't currently depend
> on chrome/browser/ssl, but I'm actually confused why SafeBrowsing is involved
at
> all, actually.
> 
> Right now we do SSLBlockingPage --> ChromeContentBrowserClient -->
SafeBrowsing
> stuff -> CertificateErrorReporter. If CertificateErrorReporter gets moved to
> chrome/browser/ssl, we're routing from chrome/browser/ssl, up a layer, down to
a
> sibling layer, and then back over to chrome/browser/ssl. Why not keep that
logic
> entirely within chrome/browser/ssl? It seems the only thing SafeBrowsing
> contributes is a toggle that we don't report interstitial stuff is
SafeBrowsing
> is disabled? If that's it, perhaps it should just query
> prefs->GetBoolean(prefs::kSafeBrowsingEnabled) on the UI thread sometime
before
> popping to IO to fire the request.

Well, the motivation for involving SafeBrowsing was because we wanted to use the
SB URLRequestContext to send these requests, since they're going to safebrowsing
servers. (Though honestly I'm not sure what the concrete requirement is that is
solved by using the safebrowsing URLRequestContext.)

Even if we are able to keep all the CertificateErrorReporter stuff entirely
within //c/b/ssl while still using the SB URLRequestContext somehow, there's
also the question of owns the CertificateErrorReporter. The SSLBlockingPage
can't own it because it has to outlive the interstitial (the request might not
be sent until after the blocking page is destroyed). In an earlier iteration
(before a SB person suggested putting it on the SB PingManager), the certificate
error reporter was living on ProfileIOData, but I'm not sure if that's correct.

[estark, 2015-04-29 19:42:05]

On 2015/04/29 19:22:36, estark wrote:
> Thanks David.
> 
> On 2015/04/29 18:38:45, David Benjamin wrote:
> > What if we move the four chrome/browser/net files here to
chrome/browser/ssl.
> > That would be cert_logger.proto, certificate_error_reporter.{h,cc}, and
> > chrome_fraudulent_certificate_reporter.cc. It seems all three of these would
> > naturally live in SSL and they'd be able to know about interstitial stuff.
> > 
> > Since CertificateErrorReporter and CertLoggerRequest are the interface to
some
> > web service whose API inherently knows about interstitials, it makes sense
> that
> > they live somewhere that knows about interstitial state or above. That seems
> to
> > be chrome/browser/ssl based on SSLBlockingPage and stuff living there.
> > 
> > I see two signals that end up at CertificateErrorReporter. One's in net for
> > pinning failures. It routes up through an interface to
> > ChromeFradulentCertificateReporter which necessarily depends on
> > CertificateErrorReporter. Moving ChromeFradulentCertificateReporter to
> > chrome/browser/ssl seems pretty natural and so it'd be
> > ChromeFradulentCertificateReporter's job to assemble the //net signal (which
> > can't depend on CertLoggerRequest) into a CertLoggerRequest or whatever
> > appropriate format and then call CertificateErrorReporter.
> 
> That means that ProfileIOData (who constructs the
> ChromeFraudulentCertificateReporter) would have to depend on //c/b/ssl. Is
that
> kosher? I guess there are existing //c/b/ssl dependencies in //c/b/profiles so
> it's probably okay. One other question is if it's appropriate for
> ChromeFraudulentCertificateReporter to be in //c/b/ssl given that it
implements
> net::FraudulentCertificateReporter. (I was sort of operating on the assumption
> that things that implement //net interfaces belong in //c/b/net, but maybe I
> completely fabricated that rule.)
> 
> > 
> > The other signal seems to start from SSLBlockingPage somewhere. Right now it
> > seems to route up through another interface to SafeBrowsing and back down to
> > CertificateErrorReporter. chrome/browser/safe_browser doesn't currently
depend
> > on chrome/browser/ssl, but I'm actually confused why SafeBrowsing is
involved
> at
> > all, actually.
> > 
> > Right now we do SSLBlockingPage --> ChromeContentBrowserClient -->
> SafeBrowsing
> > stuff -> CertificateErrorReporter. If CertificateErrorReporter gets moved to
> > chrome/browser/ssl, we're routing from chrome/browser/ssl, up a layer, down
to
> a
> > sibling layer, and then back over to chrome/browser/ssl. Why not keep that
> logic
> > entirely within chrome/browser/ssl? It seems the only thing SafeBrowsing
> > contributes is a toggle that we don't report interstitial stuff is
> SafeBrowsing
> > is disabled? If that's it, perhaps it should just query
> > prefs->GetBoolean(prefs::kSafeBrowsingEnabled) on the UI thread sometime
> before
> > popping to IO to fire the request.
> 
> Well, the motivation for involving SafeBrowsing was because we wanted to use
the
> SB URLRequestContext to send these requests, since they're going to
safebrowsing
> servers. (Though honestly I'm not sure what the concrete requirement is that
is
> solved by using the safebrowsing URLRequestContext.)
> 
> Even if we are able to keep all the CertificateErrorReporter stuff entirely
> within //c/b/ssl while still using the SB URLRequestContext somehow, there's
> also the question of owns the CertificateErrorReporter. The SSLBlockingPage
> can't own it because it has to outlive the interstitial (the request might not
> be sent until after the blocking page is destroyed). In an earlier iteration
> (before a SB person suggested putting it on the SB PingManager), the
certificate
> error reporter was living on ProfileIOData, but I'm not sure if that's
correct.

(Also, if we put the CertificateErrorrReporter on ProfileIOData, I'm not sure if
we can guarantee that the SafeBrowsingService is initialized before the
CertificateErrorReporter)

[davidben, 2015-04-29 19:44:18]

On 2015/04/29 19:22:36, estark wrote:
> Thanks David.
> 
> On 2015/04/29 18:38:45, David Benjamin wrote:
> > What if we move the four chrome/browser/net files here to
chrome/browser/ssl.
> > That would be cert_logger.proto, certificate_error_reporter.{h,cc}, and
> > chrome_fraudulent_certificate_reporter.cc. It seems all three of these would
> > naturally live in SSL and they'd be able to know about interstitial stuff.
> > 
> > Since CertificateErrorReporter and CertLoggerRequest are the interface to
some
> > web service whose API inherently knows about interstitials, it makes sense
> that
> > they live somewhere that knows about interstitial state or above. That seems
> to
> > be chrome/browser/ssl based on SSLBlockingPage and stuff living there.
> > 
> > I see two signals that end up at CertificateErrorReporter. One's in net for
> > pinning failures. It routes up through an interface to
> > ChromeFradulentCertificateReporter which necessarily depends on
> > CertificateErrorReporter. Moving ChromeFradulentCertificateReporter to
> > chrome/browser/ssl seems pretty natural and so it'd be
> > ChromeFradulentCertificateReporter's job to assemble the //net signal (which
> > can't depend on CertLoggerRequest) into a CertLoggerRequest or whatever
> > appropriate format and then call CertificateErrorReporter.
> 
> That means that ProfileIOData (who constructs the
> ChromeFraudulentCertificateReporter) would have to depend on //c/b/ssl. Is
that
> kosher? I guess there are existing //c/b/ssl dependencies in //c/b/profiles so
> it's probably okay. One other question is if it's appropriate for
> ChromeFraudulentCertificateReporter to be in //c/b/ssl given that it
implements
> net::FraudulentCertificateReporter. (I was sort of operating on the assumption
> that things that implement //net interfaces belong in //c/b/net, but maybe I
> completely fabricated that rule.)

Yeah, I think that's fine. ProfileIOData's the IO-side counterpart to Profile
which is the top-level monster thing which encompasses everything that is scoped
to a BrowserContext. (In fact, Profile already depends on c/b/ssl.)

TBH, I'm not really sure what c/b/net's concrete role is. I guess it's a place
to drop Chrome-specific net-related stuff that doesn't depend on UI or
higher-level subsystems? I don't think it's necessary that net interfaces only
be implemented there. If the certificate error-reporting stuff is to be enriched
with info from high-level bits, it seems kosher to lift it up a bit.

> > 
> > The other signal seems to start from SSLBlockingPage somewhere. Right now it
> > seems to route up through another interface to SafeBrowsing and back down to
> > CertificateErrorReporter. chrome/browser/safe_browser doesn't currently
depend
> > on chrome/browser/ssl, but I'm actually confused why SafeBrowsing is
involved
> at
> > all, actually.
> > 
> > Right now we do SSLBlockingPage --> ChromeContentBrowserClient -->
> SafeBrowsing
> > stuff -> CertificateErrorReporter. If CertificateErrorReporter gets moved to
> > chrome/browser/ssl, we're routing from chrome/browser/ssl, up a layer, down
to
> a
> > sibling layer, and then back over to chrome/browser/ssl. Why not keep that
> logic
> > entirely within chrome/browser/ssl? It seems the only thing SafeBrowsing
> > contributes is a toggle that we don't report interstitial stuff is
> SafeBrowsing
> > is disabled? If that's it, perhaps it should just query
> > prefs->GetBoolean(prefs::kSafeBrowsingEnabled) on the UI thread sometime
> before
> > popping to IO to fire the request.
> 
> Well, the motivation for involving SafeBrowsing was because we wanted to use
the
> SB URLRequestContext to send these requests, since they're going to
safebrowsing
> servers. (Though honestly I'm not sure what the concrete requirement is that
is
> solved by using the safebrowsing URLRequestContext.)

Ah, okay. I'm actually not that familiar either with all the different
URLRequestContexts we have and what the motivations are for each. Normally I'd
wander over to mmenke's desk and ask him, but I think he's OOO for the week.

(ChromeFraudulentCertificateReporter just uses the main one it seems.)

> Even if we are able to keep all the CertificateErrorReporter stuff entirely
> within //c/b/ssl while still using the SB URLRequestContext somehow, there's
> also the question of owns the CertificateErrorReporter. The SSLBlockingPage
> can't own it because it has to outlive the interstitial (the request might not
> be sent until after the blocking page is destroyed). In an earlier iteration
> (before a SB person suggested putting it on the SB PingManager), the
certificate
> error reporter was living on ProfileIOData, but I'm not sure if that's
correct.

ProfileIOData SGTM. ChromeFraudulentCertificateReporter lives there too.
Admittedly, it's sort of a dumping ground for BrowserContext-scoped IO-thread
stuff, but it should hang off (possibly something else hanging off of) that or
the Profile anyway; you're something shared between everything on a profile and
so should have your lifetime bound by the profile in some way.

(We used to have everything similarly live on Profile in the UI thread. And then
that got unwieldy, hence all this BrowserContextKeyedServiceSomething
machinery.)

[estark, 2015-04-30 01:31:58]

On 2015/04/29 19:44:18, David Benjamin wrote:
> On 2015/04/29 19:22:36, estark wrote:
> > Thanks David.
> > 
> > On 2015/04/29 18:38:45, David Benjamin wrote:
> > > What if we move the four chrome/browser/net files here to
> chrome/browser/ssl.
> > > That would be cert_logger.proto, certificate_error_reporter.{h,cc}, and
> > > chrome_fraudulent_certificate_reporter.cc. It seems all three of these
would
> > > naturally live in SSL and they'd be able to know about interstitial stuff.
> > > 
> > > Since CertificateErrorReporter and CertLoggerRequest are the interface to
> some
> > > web service whose API inherently knows about interstitials, it makes sense
> > that
> > > they live somewhere that knows about interstitial state or above. That
seems
> > to
> > > be chrome/browser/ssl based on SSLBlockingPage and stuff living there.
> > > 
> > > I see two signals that end up at CertificateErrorReporter. One's in net
for
> > > pinning failures. It routes up through an interface to
> > > ChromeFradulentCertificateReporter which necessarily depends on
> > > CertificateErrorReporter. Moving ChromeFradulentCertificateReporter to
> > > chrome/browser/ssl seems pretty natural and so it'd be
> > > ChromeFradulentCertificateReporter's job to assemble the //net signal
(which
> > > can't depend on CertLoggerRequest) into a CertLoggerRequest or whatever
> > > appropriate format and then call CertificateErrorReporter.
> > 
> > That means that ProfileIOData (who constructs the
> > ChromeFraudulentCertificateReporter) would have to depend on //c/b/ssl. Is
> that
> > kosher? I guess there are existing //c/b/ssl dependencies in //c/b/profiles
so
> > it's probably okay. One other question is if it's appropriate for
> > ChromeFraudulentCertificateReporter to be in //c/b/ssl given that it
> implements
> > net::FraudulentCertificateReporter. (I was sort of operating on the
assumption
> > that things that implement //net interfaces belong in //c/b/net, but maybe I
> > completely fabricated that rule.)
> 
> Yeah, I think that's fine. ProfileIOData's the IO-side counterpart to Profile
> which is the top-level monster thing which encompasses everything that is
scoped
> to a BrowserContext. (In fact, Profile already depends on c/b/ssl.)
> 
> TBH, I'm not really sure what c/b/net's concrete role is. I guess it's a place
> to drop Chrome-specific net-related stuff that doesn't depend on UI or
> higher-level subsystems? I don't think it's necessary that net interfaces only
> be implemented there. If the certificate error-reporting stuff is to be
enriched
> with info from high-level bits, it seems kosher to lift it up a bit.
> 
> > > 
> > > The other signal seems to start from SSLBlockingPage somewhere. Right now
it
> > > seems to route up through another interface to SafeBrowsing and back down
to
> > > CertificateErrorReporter. chrome/browser/safe_browser doesn't currently
> depend
> > > on chrome/browser/ssl, but I'm actually confused why SafeBrowsing is
> involved
> > at
> > > all, actually.
> > > 
> > > Right now we do SSLBlockingPage --> ChromeContentBrowserClient -->
> > SafeBrowsing
> > > stuff -> CertificateErrorReporter. If CertificateErrorReporter gets moved
to
> > > chrome/browser/ssl, we're routing from chrome/browser/ssl, up a layer,
down
> to
> > a
> > > sibling layer, and then back over to chrome/browser/ssl. Why not keep that
> > logic
> > > entirely within chrome/browser/ssl? It seems the only thing SafeBrowsing
> > > contributes is a toggle that we don't report interstitial stuff is
> > SafeBrowsing
> > > is disabled? If that's it, perhaps it should just query
> > > prefs->GetBoolean(prefs::kSafeBrowsingEnabled) on the UI thread sometime
> > before
> > > popping to IO to fire the request.
> > 
> > Well, the motivation for involving SafeBrowsing was because we wanted to use
> the
> > SB URLRequestContext to send these requests, since they're going to
> safebrowsing
> > servers. (Though honestly I'm not sure what the concrete requirement is that
> is
> > solved by using the safebrowsing URLRequestContext.)
> 
> Ah, okay. I'm actually not that familiar either with all the different
> URLRequestContexts we have and what the motivations are for each. Normally I'd
> wander over to mmenke's desk and ask him, but I think he's OOO for the week.
> 
> (ChromeFraudulentCertificateReporter just uses the main one it seems.)
> 
> > Even if we are able to keep all the CertificateErrorReporter stuff entirely
> > within //c/b/ssl while still using the SB URLRequestContext somehow, there's
> > also the question of owns the CertificateErrorReporter. The SSLBlockingPage
> > can't own it because it has to outlive the interstitial (the request might
not
> > be sent until after the blocking page is destroyed). In an earlier iteration
> > (before a SB person suggested putting it on the SB PingManager), the
> certificate
> > error reporter was living on ProfileIOData, but I'm not sure if that's
> correct.
> 
> ProfileIOData SGTM. ChromeFraudulentCertificateReporter lives there too.
> Admittedly, it's sort of a dumping ground for BrowserContext-scoped IO-thread
> stuff, but it should hang off (possibly something else hanging off of) that or
> the Profile anyway; you're something shared between everything on a profile
and
> so should have your lifetime bound by the profile in some way.
> 
> (We used to have everything similarly live on Profile in the UI thread. And
then
> that got unwieldy, hence all this BrowserContextKeyedServiceSomething
> machinery.)

David and I chatted more about this out-of-band. Here is a new Frankenstein
proposal combining a bunch of ideas that David, Ryan, and I have floated. (Or
maybe this was Ryan's idea that I heard through David? I don't remember
anymore.) Overview is to place report building-and-serializing code in
//c/b/ssl, but leave report sending code in //c/b/net.

* Move ChromeFraudulentCertificateReporter and the cert report protobuf to
//c/b/ssl. Leave CertificateErrorReporter where it is in //c/b/net, but modify
the interface to take a string (a serialized report) to send.

* Introduce a new //c/b/ssl class called CertificateErrorReport. This class
fills in the protobuf fields and returns a serialized report.

* Modify ChromeFraudulentCertificateReporter to build a report with
CertificateErrorReport and send it with CertificateErrorReporter, instead of
building-and-sending with CertificateErrorReporter as it does now.

* Modify the Safe Browsing code to accept a string (a serialized report) instead
of pieces of data that go in the report. Safe Browsing code will use
CertificateErrorReporter (in //c/b/net, as it is now) to actually send the
report.

* Modify the SSLCertReporter interface and its implementation to plumb a string
from SSLBlockingPage to the Safe Browsing code, instead of plumbing through
pieces of data that go into a report.

Here are my selling points for this proposal:
* The logic for actually sending reports out over the network stays in
//c/b/net, which to me makes sense, and we can continue to reuse the same
request-sending logic for both invalid cert reports and
ChromeFraudulentCertificateReporter reports. (e.g. if we add retry-on-failure,
both types of reports benefit. We can also leave the encryption logic in the
CertificateErrorReporter class -- in fact, we must leave it there since the
decision about whether to encrypt depends on the scheme of the endpoint we're
sending to -- and so we can eventually apply the encryption logic to pinning
violation reports if we want to.)
* All knowledge of interstitials is in //c/b/ssl, none in //c/b/net.
* No ssl <-> s_b dependencies yay

Thoughts?

[estark, 2015-05-01 21:45:56]

On 2015/04/30 01:31:58, estark wrote:
> On 2015/04/29 19:44:18, David Benjamin wrote:
> > On 2015/04/29 19:22:36, estark wrote:
> > > Thanks David.
> > > 
> > > On 2015/04/29 18:38:45, David Benjamin wrote:
> > > > What if we move the four chrome/browser/net files here to
> > chrome/browser/ssl.
> > > > That would be cert_logger.proto, certificate_error_reporter.{h,cc}, and
> > > > chrome_fraudulent_certificate_reporter.cc. It seems all three of these
> would
> > > > naturally live in SSL and they'd be able to know about interstitial
stuff.
> > > > 
> > > > Since CertificateErrorReporter and CertLoggerRequest are the interface
to
> > some
> > > > web service whose API inherently knows about interstitials, it makes
sense
> > > that
> > > > they live somewhere that knows about interstitial state or above. That
> seems
> > > to
> > > > be chrome/browser/ssl based on SSLBlockingPage and stuff living there.
> > > > 
> > > > I see two signals that end up at CertificateErrorReporter. One's in net
> for
> > > > pinning failures. It routes up through an interface to
> > > > ChromeFradulentCertificateReporter which necessarily depends on
> > > > CertificateErrorReporter. Moving ChromeFradulentCertificateReporter to
> > > > chrome/browser/ssl seems pretty natural and so it'd be
> > > > ChromeFradulentCertificateReporter's job to assemble the //net signal
> (which
> > > > can't depend on CertLoggerRequest) into a CertLoggerRequest or whatever
> > > > appropriate format and then call CertificateErrorReporter.
> > > 
> > > That means that ProfileIOData (who constructs the
> > > ChromeFraudulentCertificateReporter) would have to depend on //c/b/ssl. Is
> > that
> > > kosher? I guess there are existing //c/b/ssl dependencies in
//c/b/profiles
> so
> > > it's probably okay. One other question is if it's appropriate for
> > > ChromeFraudulentCertificateReporter to be in //c/b/ssl given that it
> > implements
> > > net::FraudulentCertificateReporter. (I was sort of operating on the
> assumption
> > > that things that implement //net interfaces belong in //c/b/net, but maybe
I
> > > completely fabricated that rule.)
> > 
> > Yeah, I think that's fine. ProfileIOData's the IO-side counterpart to
Profile
> > which is the top-level monster thing which encompasses everything that is
> scoped
> > to a BrowserContext. (In fact, Profile already depends on c/b/ssl.)
> > 
> > TBH, I'm not really sure what c/b/net's concrete role is. I guess it's a
place
> > to drop Chrome-specific net-related stuff that doesn't depend on UI or
> > higher-level subsystems? I don't think it's necessary that net interfaces
only
> > be implemented there. If the certificate error-reporting stuff is to be
> enriched
> > with info from high-level bits, it seems kosher to lift it up a bit.
> > 
> > > > 
> > > > The other signal seems to start from SSLBlockingPage somewhere. Right
now
> it
> > > > seems to route up through another interface to SafeBrowsing and back
down
> to
> > > > CertificateErrorReporter. chrome/browser/safe_browser doesn't currently
> > depend
> > > > on chrome/browser/ssl, but I'm actually confused why SafeBrowsing is
> > involved
> > > at
> > > > all, actually.
> > > > 
> > > > Right now we do SSLBlockingPage --> ChromeContentBrowserClient -->
> > > SafeBrowsing
> > > > stuff -> CertificateErrorReporter. If CertificateErrorReporter gets
moved
> to
> > > > chrome/browser/ssl, we're routing from chrome/browser/ssl, up a layer,
> down
> > to
> > > a
> > > > sibling layer, and then back over to chrome/browser/ssl. Why not keep
that
> > > logic
> > > > entirely within chrome/browser/ssl? It seems the only thing SafeBrowsing
> > > > contributes is a toggle that we don't report interstitial stuff is
> > > SafeBrowsing
> > > > is disabled? If that's it, perhaps it should just query
> > > > prefs->GetBoolean(prefs::kSafeBrowsingEnabled) on the UI thread sometime
> > > before
> > > > popping to IO to fire the request.
> > > 
> > > Well, the motivation for involving SafeBrowsing was because we wanted to
use
> > the
> > > SB URLRequestContext to send these requests, since they're going to
> > safebrowsing
> > > servers. (Though honestly I'm not sure what the concrete requirement is
that
> > is
> > > solved by using the safebrowsing URLRequestContext.)
> > 
> > Ah, okay. I'm actually not that familiar either with all the different
> > URLRequestContexts we have and what the motivations are for each. Normally
I'd
> > wander over to mmenke's desk and ask him, but I think he's OOO for the week.
> > 
> > (ChromeFraudulentCertificateReporter just uses the main one it seems.)
> > 
> > > Even if we are able to keep all the CertificateErrorReporter stuff
entirely
> > > within //c/b/ssl while still using the SB URLRequestContext somehow,
there's
> > > also the question of owns the CertificateErrorReporter. The
SSLBlockingPage
> > > can't own it because it has to outlive the interstitial (the request might
> not
> > > be sent until after the blocking page is destroyed). In an earlier
iteration
> > > (before a SB person suggested putting it on the SB PingManager), the
> > certificate
> > > error reporter was living on ProfileIOData, but I'm not sure if that's
> > correct.
> > 
> > ProfileIOData SGTM. ChromeFraudulentCertificateReporter lives there too.
> > Admittedly, it's sort of a dumping ground for BrowserContext-scoped
IO-thread
> > stuff, but it should hang off (possibly something else hanging off of) that
or
> > the Profile anyway; you're something shared between everything on a profile
> and
> > so should have your lifetime bound by the profile in some way.
> > 
> > (We used to have everything similarly live on Profile in the UI thread. And
> then
> > that got unwieldy, hence all this BrowserContextKeyedServiceSomething
> > machinery.)
> 
> David and I chatted more about this out-of-band. Here is a new Frankenstein
> proposal combining a bunch of ideas that David, Ryan, and I have floated. (Or
> maybe this was Ryan's idea that I heard through David? I don't remember
> anymore.) Overview is to place report building-and-serializing code in
> //c/b/ssl, but leave report sending code in //c/b/net.
> 
> * Move ChromeFraudulentCertificateReporter and the cert report protobuf to
> //c/b/ssl. Leave CertificateErrorReporter where it is in //c/b/net, but modify
> the interface to take a string (a serialized report) to send.
> 
> * Introduce a new //c/b/ssl class called CertificateErrorReport. This class
> fills in the protobuf fields and returns a serialized report.
> 
> * Modify ChromeFraudulentCertificateReporter to build a report with
> CertificateErrorReport and send it with CertificateErrorReporter, instead of
> building-and-sending with CertificateErrorReporter as it does now.
> 
> * Modify the Safe Browsing code to accept a string (a serialized report)
instead
> of pieces of data that go in the report. Safe Browsing code will use
> CertificateErrorReporter (in //c/b/net, as it is now) to actually send the
> report.
> 
> * Modify the SSLCertReporter interface and its implementation to plumb a
string
> from SSLBlockingPage to the Safe Browsing code, instead of plumbing through
> pieces of data that go into a report.
> 
> Here are my selling points for this proposal:
> * The logic for actually sending reports out over the network stays in
> //c/b/net, which to me makes sense, and we can continue to reuse the same
> request-sending logic for both invalid cert reports and
> ChromeFraudulentCertificateReporter reports. (e.g. if we add retry-on-failure,
> both types of reports benefit. We can also leave the encryption logic in the
> CertificateErrorReporter class -- in fact, we must leave it there since the
> decision about whether to encrypt depends on the scheme of the endpoint we're
> sending to -- and so we can eventually apply the encryption logic to pinning
> violation reports if we want to.)
> * All knowledge of interstitials is in //c/b/ssl, none in //c/b/net.
> * No ssl <-> s_b dependencies yay
> 
> Thoughts?

Ping -- how does the above proposal sound? (certificate report building and
serializing logic to //c/b/ssl, report sending stays in in //c/b/net)

[Ryan Sleevi, 2015-05-01 22:30:59]

On 2015/05/01 21:45:56, estark wrote:
> Ping -- how does the above proposal sound? (certificate report building and
> serializing logic to //c/b/ssl, report sending stays in in //c/b/net)

Who is that ping for? :) felt? David? me? 

I think most of that sounds fine until the last bit (which is "Modify the
SSLCertReporter interface and its implementation to plumb a string from
SSLBlockingPage") - since it sounds like now SSLBlockingPage will be responsible
for packaging the report?

The rest sounds right and about what David and I had chatted about :)

[estark, 2015-05-02 00:19:32]

On 2015/05/01 22:30:59, Ryan Sleevi wrote:
> On 2015/05/01 21:45:56, estark wrote:
> > Ping -- how does the above proposal sound? (certificate report building and
> > serializing logic to //c/b/ssl, report sending stays in in //c/b/net)
> 
> Who is that ping for? :) felt? David? me? 

Oops, sorry... You and David.

> 
> I think most of that sounds fine until the last bit (which is "Modify the
> SSLCertReporter interface and its implementation to plumb a string from
> SSLBlockingPage") - since it sounds like now SSLBlockingPage will be
responsible
> for packaging the report?

I was imagining that SSLBlockingPage would do something like this:

  std::string serialized_report;
  CertificateErrorReport report(hostname, ssl_info, interstitial_info);
  report.serialize(&serialized_report);
  ssl_cert_reporter_->SendSerializedReport(report);

Is that what you had in mind? Or something else?

> 
> The rest sounds right and about what David and I had chatted about :)

[Ryan Sleevi, 2015-05-02 00:20:25]

On 2015/05/02 00:19:32, estark wrote:
> I was imagining that SSLBlockingPage would do something like this:
> 
>   std::string serialized_report;
>   CertificateErrorReport report(hostname, ssl_info, interstitial_info);
>   report.serialize(&serialized_report);
>   ssl_cert_reporter_->SendSerializedReport(report);
> 
> Is that what you had in mind? Or something else?

Sounds reasonable then :)

[estark, 2015-05-02 01:25:58]

On 2015/05/02 00:20:25, Ryan Sleevi wrote:
> On 2015/05/02 00:19:32, estark wrote:
> > I was imagining that SSLBlockingPage would do something like this:
> > 
> >   std::string serialized_report;
> >   CertificateErrorReport report(hostname, ssl_info, interstitial_info);
> >   report.serialize(&serialized_report);
> >   ssl_cert_reporter_->SendSerializedReport(report);
> > 
> > Is that what you had in mind? Or something else?
> 
> Sounds reasonable then :)

Cool. I'll give David the weekend and part of Monday to object and then update
this CL. :) Thanks both for your help with this!

[estark, 2015-05-14 04:25:52]

estark@chromium.org changed reviewers:
- davidben@chromium.org, rsleevi@chromium.org

[estark, 2015-05-14 04:25:53]

I'm back, with a much smaller patchset. (This change is much simpler after
crrev.com/1117173004)

I've pared down the reviewers list to felt and moved rsleevi and davidben to CC,
but all feel free to review if you'd like to.

[Ryan Sleevi, 2015-05-14 04:31:03]

rsleevi@chromium.org changed reviewers:
+ rsleevi@chromium.org

[Ryan Sleevi, 2015-05-14 04:31:03]

Design and code LGTM. But good to make sure felt is good, especially with the
.protos and whatever style requirements we have :)

https://codereview.chromium.org/1076273002/diff/20001/chrome/browser/ssl/ssl_...
File chrome/browser/ssl/ssl_blocking_page.h (right):

https://codereview.chromium.org/1076273002/diff/20001/chrome/browser/ssl/ssl_...
chrome/browser/ssl/ssl_blocking_page.h:106: void FinishCertCollection(
Update comment :)

[estark, 2015-05-14 13:50:29]

Thanks Ryan.

https://codereview.chromium.org/1076273002/diff/20001/chrome/browser/ssl/ssl_...
File chrome/browser/ssl/ssl_blocking_page.h (right):

https://codereview.chromium.org/1076273002/diff/20001/chrome/browser/ssl/ssl_...
chrome/browser/ssl/ssl_blocking_page.h:106: void FinishCertCollection(
On 2015/05/14 04:31:03, Ryan Sleevi wrote:
> Update comment :)

Done.

[estark, 2015-05-18 23:52:50]

On 2015/05/14 13:50:29, estark wrote:
> Thanks Ryan.
> 
>
https://codereview.chromium.org/1076273002/diff/20001/chrome/browser/ssl/ssl_...
> File chrome/browser/ssl/ssl_blocking_page.h (right):
> 
>
https://codereview.chromium.org/1076273002/diff/20001/chrome/browser/ssl/ssl_...
> chrome/browser/ssl/ssl_blocking_page.h:106: void FinishCertCollection(
> On 2015/05/14 04:31:03, Ryan Sleevi wrote:
> > Update comment :)
> 
> Done.

friendly ping to felt

[felt, 2015-05-19 19:32:05]

lgtm: but can you be specific in the description about what info is added in
this CL? (eg don't just have "etc.") since the neterror is not in this one

[estark, 2015-05-19 20:09:26]

On 2015/05/19 19:32:05, felt wrote:
> lgtm: but can you be specific in the description about what info is added in
> this CL? (eg don't just have "etc.") since the neterror is not in this one

Done, thanks felt.

[estark, 2015-05-19 20:09:31]

The CQ bit was checked by estark@chromium.org

[estark, 2015-05-19 20:09:32]

The patchset sent to the CQ was uploaded after l-g-t-m from rsleevi@chromium.org
Link to the patchset: https://codereview.chromium.org/1076273002/#ps40001
(title: "update FinishCertCollection comment")

[commit-bot: I haz the power, 2015-05-19 20:10:22]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1076273002/40001

[commit-bot: I haz the power, 2015-05-19 21:06:32]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-05-19 21:07:25]

Patchset 3 (id:??) landed as
https://crrev.com/6473c592af545f354f53b65251623cc509063c02
Cr-Commit-Position: refs/heads/master@{#330605}