Return a NULL histogram pointer on construction error

Return a NULL histogram pointer on construction error

Check the pointer returned for the metricsPrivate API to keep from crashing Chrome with bad data.

BUG=334135
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=256822

[elijahtaylor1, 2014-01-17 01:27:15]

PTAL.  It's not exactly what we discussed, this just discards both Log and
Linear histogram data if the construction arguments don't match.

I think this is a better solution than trying to extend Linear histograms.  It
is a rare event and didn't seem worth the complexity of restructuring an
existing histogram.

[Alexei Svitkine (slow), 2014-01-17 15:40:13]

This approach makes sense to me. Some comments below.

https://codereview.chromium.org/141393002/diff/190001/base/metrics/histogram.h
File base/metrics/histogram.h (right):

https://codereview.chromium.org/141393002/diff/190001/base/metrics/histogram....
base/metrics/histogram.h:683: class BASE_EXPORT DummyHistogram : public
Histogram {
I would actually define this class in an anon namespace in histogram.cc.

https://codereview.chromium.org/141393002/diff/190001/chrome/browser/extensio...
File chrome/browser/extensions/api/metrics_private/metrics_apitest.cc (right):

https://codereview.chromium.org/141393002/diff/190001/chrome/browser/extensio...
chrome/browser/extensions/api/metrics_private/metrics_apitest.cc:47:
{"test.bucketchange.linear", base::LINEAR_HISTOGRAM, 1, 100, 10, 2},
Can you also add tests for this to histogram_unittest.cc?

[elijahtaylor1, 2014-01-17 20:04:15]

PTAL

https://codereview.chromium.org/141393002/diff/190001/base/metrics/histogram.h
File base/metrics/histogram.h (right):

https://codereview.chromium.org/141393002/diff/190001/base/metrics/histogram....
base/metrics/histogram.h:683: class BASE_EXPORT DummyHistogram : public
Histogram {
On 2014/01/17 15:40:13, Alexei Svitkine wrote:
> I would actually define this class in an anon namespace in histogram.cc.

Done.

https://codereview.chromium.org/141393002/diff/190001/chrome/browser/extensio...
File chrome/browser/extensions/api/metrics_private/metrics_apitest.cc (right):

https://codereview.chromium.org/141393002/diff/190001/chrome/browser/extensio...
chrome/browser/extensions/api/metrics_private/metrics_apitest.cc:47:
{"test.bucketchange.linear", base::LINEAR_HISTOGRAM, 1, 100, 10, 2},
On 2014/01/17 15:40:13, Alexei Svitkine wrote:
> Can you also add tests for this to histogram_unittest.cc?

Done.

[Alexei Svitkine (slow), 2014-01-17 20:06:30]

Thanks, LGTM.

+jar for base owners

https://codereview.chromium.org/141393002/diff/430001/base/metrics/histogram.cc
File base/metrics/histogram.cc (right):

https://codereview.chromium.org/141393002/diff/430001/base/metrics/histogram....
base/metrics/histogram.cc:94: bool
DummyHistogram::AddSamplesFromPickle(PickleIterator* iter) {
Nit: You can probably define these inline above.

[elijahtaylor1, 2014-01-17 20:25:34]

https://codereview.chromium.org/141393002/diff/430001/base/metrics/histogram.cc
File base/metrics/histogram.cc (right):

https://codereview.chromium.org/141393002/diff/430001/base/metrics/histogram....
base/metrics/histogram.cc:94: bool
DummyHistogram::AddSamplesFromPickle(PickleIterator* iter) {
On 2014/01/17 20:06:31, Alexei Svitkine wrote:
> Nit: You can probably define these inline above.

I got an error on some windows trybots before for inlining non-empty functions. 
Is that only in header files?

[Alexei Svitkine (slow), 2014-01-17 22:14:27]

https://codereview.chromium.org/141393002/diff/430001/base/metrics/histogram.cc
File base/metrics/histogram.cc (right):

https://codereview.chromium.org/141393002/diff/430001/base/metrics/histogram....
base/metrics/histogram.cc:94: bool
DummyHistogram::AddSamplesFromPickle(PickleIterator* iter) {
On 2014/01/17 20:25:34, elijahtaylor1 wrote:
> On 2014/01/17 20:06:31, Alexei Svitkine wrote:
> > Nit: You can probably define these inline above.
> 
> I got an error on some windows trybots before for inlining non-empty
functions. 
> Is that only in header files?

I think it should work - there's code elsewhere that does this. What's the
error?

[elijahtaylor1, 2014-01-17 22:18:52]

On 2014/01/17 22:14:27, Alexei Svitkine wrote:
>
https://codereview.chromium.org/141393002/diff/430001/base/metrics/histogram.cc
> File base/metrics/histogram.cc (right):
> 
>
https://codereview.chromium.org/141393002/diff/430001/base/metrics/histogram....
> base/metrics/histogram.cc:94: bool
> DummyHistogram::AddSamplesFromPickle(PickleIterator* iter) {
> On 2014/01/17 20:25:34, elijahtaylor1 wrote:
> > On 2014/01/17 20:06:31, Alexei Svitkine wrote:
> > > Nit: You can probably define these inline above.
> > 
> > I got an error on some windows trybots before for inlining non-empty
> functions. 
> > Is that only in header files?
> 
> I think it should work - there's code elsewhere that does this. What's the
> error?

Sorry, it was mac, not windows:
http://build.chromium.org/p/tryserver.chromium/builders/mac/builds/116370/ste...

I'll upload and try on that bot with the impl in .cc and see if that works.

[elijahtaylor1, 2014-01-23 01:57:38]

On 2014/01/17 22:18:52, elijahtaylor1 wrote:
> On 2014/01/17 22:14:27, Alexei Svitkine wrote:
> >
>
https://codereview.chromium.org/141393002/diff/430001/base/metrics/histogram.cc
> > File base/metrics/histogram.cc (right):
> > 
> >
>
https://codereview.chromium.org/141393002/diff/430001/base/metrics/histogram....
> > base/metrics/histogram.cc:94: bool
> > DummyHistogram::AddSamplesFromPickle(PickleIterator* iter) {
> > On 2014/01/17 20:25:34, elijahtaylor1 wrote:
> > > On 2014/01/17 20:06:31, Alexei Svitkine wrote:
> > > > Nit: You can probably define these inline above.
> > > 
> > > I got an error on some windows trybots before for inlining non-empty
> > functions. 
> > > Is that only in header files?
> > 
> > I think it should work - there's code elsewhere that does this. What's the
> > error?
> 
> Sorry, it was mac, not windows:
>
http://build.chromium.org/p/tryserver.chromium/builders/mac/builds/116370/ste...
> 
> I'll upload and try on that bot with the impl in .cc and see if that works.


Ok, so it seems less picky about inline implementations in source files instead
of headers.

jar@: ping for OWNERS review

[jar (doing other things), 2014-01-23 02:16:17]

https://codereview.chromium.org/141393002/diff/510001/base/metrics/histogram.cc
File base/metrics/histogram.cc (left):

https://codereview.chromium.org/141393002/diff/510001/base/metrics/histogram....
base/metrics/histogram.cc:570:
CHECK(histogram->HasConstructionArguments(minimum, maximum, bucket_count));
Same issue... I really wish we could preserve this... and maybe ignore *only*
the case where a plugin provides the change.

Can to adjust things to catch this further up the stack, closer to your use
case??  ... and avoid calling this far in?

Your use case involves a slow find anyway (and no use of a static), so it is
probably plausible to call to check for the histogram's presence, and get its
args, before going further (but it has been years since I've looked at this
code).

https://codereview.chromium.org/141393002/diff/510001/base/metrics/histogram.cc
File base/metrics/histogram.cc (right):

https://codereview.chromium.org/141393002/diff/510001/base/metrics/histogram....
base/metrics/histogram.cc:130: if (!histogram->HasConstructionArguments(minimum,
maximum, bucket_count)) {
This is a sad thing... as it means we can't catch folks that try to construct a
histogram with different args.

Is there any way to preserve this CHECK?

[Alexei Svitkine (slow), 2014-01-23 15:28:22]

https://codereview.chromium.org/141393002/diff/510001/base/metrics/histogram.cc
File base/metrics/histogram.cc (left):

https://codereview.chromium.org/141393002/diff/510001/base/metrics/histogram....
base/metrics/histogram.cc:570:
CHECK(histogram->HasConstructionArguments(minimum, maximum, bucket_count));
On 2014/01/23 02:16:18, jar wrote:
> Same issue... I really wish we could preserve this... and maybe ignore *only*
> the case where a plugin provides the change.
> 
> Can to adjust things to catch this further up the stack, closer to your use
> case??  ... and avoid calling this far in?
> 
> Your use case involves a slow find anyway (and no use of a static), so it is
> probably plausible to call to check for the histogram's presence, and get its
> args, before going further (but it has been years since I've looked at this
> code).

Personally, I think this is a reasonable place to have this check because this
is the codepath of the "slow find". Meaning that for regular macro'd histograms,
they'd only hit this check once, and from then on cache a pointer to the
histogram, whereas for dynamic users of the API, they'd do the check every time.
This seems like a reasonable trade off.

Also note that this shouldn't add any extra performance loss, since the check
was being done already (just in a CHECK() rather than an if).

I do somewhat agree that it would also be nice to preserve the CHECK at the same
time, but I'm not sure of a good way to do that. Hmm, perhaps the factory API
could take a parameter specifying what the failure mode is (i.e. whether a dummy
object is returned vs. a check is hit) and the default being the CHECK? WDYT?

[elijahtaylor1, 2014-01-24 01:27:26]

PTAL

https://codereview.chromium.org/141393002/diff/510001/base/metrics/histogram.cc
File base/metrics/histogram.cc (left):

https://codereview.chromium.org/141393002/diff/510001/base/metrics/histogram....
base/metrics/histogram.cc:570:
CHECK(histogram->HasConstructionArguments(minimum, maximum, bucket_count));
On 2014/01/23 15:28:22, Alexei Svitkine wrote:

> time, but I'm not sure of a good way to do that. Hmm, perhaps the factory API
> could take a parameter specifying what the failure mode is (i.e. whether a
dummy
> object is returned vs. a check is hit) and the default being the CHECK? WDYT?


I've implemented this suggestion to see how ugly it might be.  It means adding
optional parameters to 5 different function and propagating that parameter
across all of them, but not too bad.

I'm not in love with the names I chose for the new parameter, if either of you
have suggestions to improve them, please offer them.

[jar (doing other things), 2014-01-24 16:15:07]

https://codereview.chromium.org/141393002/diff/810001/base/metrics/histogram.h
File base/metrics/histogram.h (right):

https://codereview.chromium.org/141393002/diff/810001/base/metrics/histogram....
base/metrics/histogram.h:394: int32 construction_behavior =
kDoNotAllowBadConstruction);
Sadly, this is a violation of the style guide.

Perhaps instead of using a CHECK() in the method, we can return a NULL pointer
when there is a violation.  That would then effectively CHECK (crash) if the
caller was not prepared for "bad construction".  If the caller could tolerate
bad construction, and wanted the flow to continue, then they could replace the
NULL pointer with the planned "dummy_histogram".

[elijahtaylor1, 2014-01-24 20:37:19]

https://codereview.chromium.org/141393002/diff/810001/base/metrics/histogram.h
File base/metrics/histogram.h (right):

https://codereview.chromium.org/141393002/diff/810001/base/metrics/histogram....
base/metrics/histogram.h:394: int32 construction_behavior =
kDoNotAllowBadConstruction);
On 2014/01/24 16:15:09, jar wrote:
> Sadly, this is a violation of the style guide.
> 
> Perhaps instead of using a CHECK() in the method, we can return a NULL pointer
> when there is a violation.  That would then effectively CHECK (crash) if the
> caller was not prepared for "bad construction".  If the caller could tolerate
> bad construction, and wanted the flow to continue, then they could replace the
> NULL pointer with the planned "dummy_histogram".

If allowing NULL dereference is ok for what we're considering bad code in
Chrome, this approach makes everything much simpler.  Both the extension and
Pepper API do a FactoryGet then immediately Add, they don't keep the object
around, so we can just guard against NULL dereference there and everything
should be good.

[Alexei Svitkine (slow), 2014-01-24 20:39:39]

I'm fine with this approach too. Can you update the NaCL callsites as well with
the NULL check?

https://codereview.chromium.org/141393002/diff/1130001/chrome/browser/extensi...
File chrome/browser/extensions/api/metrics_private/metrics_private_api.cc
(right):

https://codereview.chromium.org/141393002/diff/1130001/chrome/browser/extensi...
chrome/browser/extensions/api/metrics_private/metrics_private_api.cc:142: if
(counter)
Add a comment.

[elijahtaylor1, 2014-01-24 23:10:36]

Added checks to the Pepper API portion of this since that change just landed.
+bbudge for OWNERS check on those files.

PTAL

https://codereview.chromium.org/141393002/diff/1130001/chrome/browser/extensi...
File chrome/browser/extensions/api/metrics_private/metrics_private_api.cc
(right):

https://codereview.chromium.org/141393002/diff/1130001/chrome/browser/extensi...
chrome/browser/extensions/api/metrics_private/metrics_private_api.cc:142: if
(counter)
On 2014/01/24 20:39:40, Alexei Svitkine wrote:
> Add a comment.

Done.

[bbudge, 2014-01-24 23:20:04]

Pepper code LGTM

[Alexei Svitkine (slow), 2014-03-03 18:22:00]

Ping, what's the status of this?

[Alexei Svitkine (slow), 2014-03-03 18:23:46]

Latest version LGTM, fwiw.

[elijahtaylor1, 2014-03-03 18:34:18]

I think I still need an LGTM from jar@ for base/metrics OWNER.  I will also
likely need to rebase this now since it's been so long.

On 2014/03/03 18:23:46, Alexei Svitkine wrote:
> Latest version LGTM, fwiw.

[jar (doing other things), 2014-03-06 02:36:34]

Patch set 7 LGTM

[elijahtaylor1, 2014-03-13 01:19:56]

The CQ bit was checked by elijahtaylor@chromium.org

[commit-bot: I haz the power, 2014-03-13 01:20:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/elijahtaylor@chromium.org/141393002/15...

[commit-bot: I haz the power, 2014-03-13 03:59:47]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-13 03:59:48]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_chromeos_clang_dbg

[Alexei Svitkine (slow), 2014-03-13 13:43:54]

The CQ bit was checked by asvitkine@chromium.org

[commit-bot: I haz the power, 2014-03-13 13:44:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/elijahtaylor@chromium.org/141393002/15...

[commit-bot: I haz the power, 2014-03-13 14:08:24]

Change committed as 256822