PDF: Add UMA to track embedded PDF triggered drive-by downloads.

PDF: Add UMA to track embedded PDF triggered drive-by downloads.

Adds an entry to an existing histogram to track drive-by downloads
triggered when the user views a page with an embedded PDF but has
no PDF plugin (or it's disabled).

BUG=714917
Review-Url: https://codereview.chromium.org/2901563003
Cr-Commit-Position: refs/heads/master@{#475220}
Committed: https://chromium.googlesource.com/chromium/src/+/c173ef62237c56354fd42882ea0999cc5564ef1c

[tommycli, 2017-05-22 22:35:35]

The CQ bit was checked by tommycli@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-22 22:36:01]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tommycli, 2017-05-22 22:37:47]

tommycli@chromium.org changed reviewers:
+ isherman@chromium.org

[tommycli, 2017-05-22 22:37:48]

isherman: metrics PTAL, thanks!

[tommycli, 2017-05-22 22:38:36]

Description was changed from

==========
PDF: Add UMA to track embedded PDF triggered drive-by downloads.

BUG=714917
==========

to

==========
PDF: Add UMA to track embedded PDF triggered drive-by downloads.

Adds an entry to an existing histogram to track drive-by downloads
triggered when the user views a page with an embedded PDF but has
no PDF plugin (or it's disabled).

BUG=714917
==========

[Ilya Sherman, 2017-05-22 22:54:08]

On 2017/05/22 22:37:48, tommycli wrote:
> isherman: metrics PTAL, thanks!

What did you want me to take a look at?  Any reviewer can review new entries to
enums.xml =)

[tommycli, 2017-05-22 22:56:42]

On 2017/05/22 22:54:08, Ilya Sherman wrote:
> On 2017/05/22 22:37:48, tommycli wrote:
> > isherman: metrics PTAL, thanks!
> 
> What did you want me to take a look at?  Any reviewer can review new entries
to
> enums.xml =)

isherman: Oh, ... whoops! I didn't realize that enums.xml doesn't a metrics
reviewers.

I guess I'd like a general opinion: Is the overall patch sane? (Recording a
chrome-specific metric through the ChromeDownloadManagerDelegate).

[Ilya Sherman, 2017-05-22 23:00:42]

On 2017/05/22 22:56:42, tommycli wrote:
> On 2017/05/22 22:54:08, Ilya Sherman wrote:
> > On 2017/05/22 22:37:48, tommycli wrote:
> > > isherman: metrics PTAL, thanks!
> > 
> > What did you want me to take a look at?  Any reviewer can review new entries
> to
> > enums.xml =)
> 
> isherman: Oh, ... whoops! I didn't realize that enums.xml doesn't a metrics
> reviewers.
> 
> I guess I'd like a general opinion: Is the overall patch sane? (Recording a
> chrome-specific metric through the ChromeDownloadManagerDelegate).

Generally seems fine, though it would probably be better for someone more
familiar with the code to review it.

https://codereview.chromium.org/2901563003/diff/1/chrome/common/pdf_uma.h
File chrome/common/pdf_uma.h (right):

https://codereview.chromium.org/2901563003/diff/1/chrome/common/pdf_uma.h#new...
chrome/common/pdf_uma.h:9: #include "url/gurl.h"
nit: Where are these used?

https://codereview.chromium.org/2901563003/diff/1/chrome/common/pdf_uma.h#new...
chrome/common/pdf_uma.h:13: enum PDFLoadStatus {
Optional nit: I'd make this an enum class

https://codereview.chromium.org/2901563003/diff/1/chrome/common/pdf_uma.h#new...
chrome/common/pdf_uma.h:21: void ReportPDFLoadStatus(PDFLoadStatus status);
Hmm, it looks like you're defining a function in the global namespace.  I think
that's discouraged?  I'm not actually 100% sure on what's preferred though -- a
wrapper class, or a wrapper namespace, or neither.

[tommycli, 2017-05-23 00:32:00]

thanks!

https://codereview.chromium.org/2901563003/diff/1/chrome/common/pdf_uma.h
File chrome/common/pdf_uma.h (right):

https://codereview.chromium.org/2901563003/diff/1/chrome/common/pdf_uma.h#new...
chrome/common/pdf_uma.h:9: #include "url/gurl.h"
On 2017/05/22 23:00:42, Ilya Sherman wrote:
> nit: Where are these used?

Done.

https://codereview.chromium.org/2901563003/diff/1/chrome/common/pdf_uma.h#new...
chrome/common/pdf_uma.h:13: enum PDFLoadStatus {
On 2017/05/22 23:00:42, Ilya Sherman wrote:
> Optional nit: I'd make this an enum class

Done.

https://codereview.chromium.org/2901563003/diff/1/chrome/common/pdf_uma.h#new...
chrome/common/pdf_uma.h:21: void ReportPDFLoadStatus(PDFLoadStatus status);
On 2017/05/22 23:00:42, Ilya Sherman wrote:
> Hmm, it looks like you're defining a function in the global namespace.  I
think
> that's discouraged?  I'm not actually 100% sure on what's preferred though --
a
> wrapper class, or a wrapper namespace, or neither.

Hey, yeah... I thought the same thing... but I didn't have a strong argument
(other than polluting the global namespace) as to why or what I should wrap this
with... I'll leave this for another reviewer to weigh in.

[tommycli, 2017-05-23 00:32:31]

tommycli@chromium.org changed reviewers:
+ asanka@chromium.org

[tommycli, 2017-05-23 00:32:31]

asanka: PTAL to make sure I'm not abusing the downloads code too much. Thanks!

[asanka, 2017-05-23 00:53:04]

On 2017/05/23 00:32:31, tommycli wrote:
> asanka: PTAL to make sure I'm not abusing the downloads code too much. Thanks!

I'd suggest making this an entirely //chrome side thing. You can tell a download
is new by examining download->GetTargetFilePath().empty() in
ChromeDownloadManagerDelegate::DetermineDownloadTarget(). The MIME type and
user-gesture flag are available on DownloadItem.

That said, could you elaborate which PDF downloads you'd like to classify as
drive-by downloads? I ask because it's possible for a download to have an
associated user gesture, but still be a "drive-by" download. E.g. some malicious
ad dropping a download when the user visits a page.

[tommycli, 2017-05-23 16:50:05]

The CQ bit was checked by tommycli@chromium.org to run a CQ dry run

[tommycli, 2017-05-23 16:50:21]

On 2017/05/23 00:53:04, asanka wrote:
> On 2017/05/23 00:32:31, tommycli wrote:
> > asanka: PTAL to make sure I'm not abusing the downloads code too much.
Thanks!
> 
> I'd suggest making this an entirely //chrome side thing. You can tell a
download
> is new by examining download->GetTargetFilePath().empty() in
> ChromeDownloadManagerDelegate::DetermineDownloadTarget(). The MIME type and
> user-gesture flag are available on DownloadItem.
> 
> That said, could you elaborate which PDF downloads you'd like to classify as
> drive-by downloads? I ask because it's possible for a download to have an
> associated user gesture, but still be a "drive-by" download. E.g. some
malicious
> ad dropping a download when the user visits a page.

asanka: Thank you! I tested and I was able to remove the content/ changes
entirely!

[commit-bot: I haz the power, 2017-05-23 16:50:40]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-23 18:20:49]

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

[commit-bot: I haz the power, 2017-05-23 18:20:50]

Dry run: This issue passed the CQ dry run.

[tommycli, 2017-05-24 21:18:32]

tommycli@chromium.org changed reviewers:
+ thestig@chromium.org

[tommycli, 2017-05-24 21:18:32]

PTAL chrome/ review, thanks!

[Lei Zhang, 2017-05-24 22:39:21]

https://codereview.chromium.org/2901563003/diff/80001/chrome/common/BUILD.gn
File chrome/common/BUILD.gn (right):

https://codereview.chromium.org/2901563003/diff/80001/chrome/common/BUILD.gn#...
chrome/common/BUILD.gn:472: if (enable_pdf) {
You are using BUILDFLAG(ENABLE_PLUGINS) in the C++ code. Is that the same as
enable_pdf?

https://codereview.chromium.org/2901563003/diff/80001/chrome/common/pdf_uma.h
File chrome/common/pdf_uma.h (right):

https://codereview.chromium.org/2901563003/diff/80001/chrome/common/pdf_uma.h...
chrome/common/pdf_uma.h:1: // Copyright 2016 The Chromium Authors. All rights
reserved.
It's half way through 2017.

https://codereview.chromium.org/2901563003/diff/80001/chrome/common/pdf_uma.h...
chrome/common/pdf_uma.h:8: #include "base/macros.h"
Are there any macros here?

https://codereview.chromium.org/2901563003/diff/80001/chrome/common/pdf_uma.h...
chrome/common/pdf_uma.h:13: LOADED_FULL_PAGE_PDF_WITH_PDFIUM = 0,
FYI, there was a thread from last month title:

[chromium-dev] PSA: Chromium style no longer prefers FOO over kFoo for enums

[asanka, 2017-05-25 00:47:47]

On 2017/05/23 16:50:21, tommycli wrote:
> On 2017/05/23 00:53:04, asanka wrote:
> > On 2017/05/23 00:32:31, tommycli wrote:
> > > asanka: PTAL to make sure I'm not abusing the downloads code too much.
> Thanks!
> > 
> > I'd suggest making this an entirely //chrome side thing. You can tell a
> download
> > is new by examining download->GetTargetFilePath().empty() in
> > ChromeDownloadManagerDelegate::DetermineDownloadTarget(). The MIME type and
> > user-gesture flag are available on DownloadItem.
> > 
> > That said, could you elaborate which PDF downloads you'd like to classify as
> > drive-by downloads? I ask because it's possible for a download to have an
> > associated user gesture, but still be a "drive-by" download. E.g. some
> malicious
> > ad dropping a download when the user visits a page.
> 
> asanka: Thank you! I tested and I was able to remove the content/ changes
> entirely!

/download/ LGTM, but I removed myself from /download/ OWNERS a couple of days
ago. So you'll need to pick another reviewer :)

My concern about what you are measuring still stands, though. Relying on 'has
user gesture' may not tell you what you want to know.

[tommycli, 2017-05-25 17:31:26]

On 2017/05/25 00:47:47, asanka wrote:
> On 2017/05/23 16:50:21, tommycli wrote:
> > On 2017/05/23 00:53:04, asanka wrote:
> > > On 2017/05/23 00:32:31, tommycli wrote:
> > > > asanka: PTAL to make sure I'm not abusing the downloads code too much.
> > Thanks!
> > > 
> > > I'd suggest making this an entirely //chrome side thing. You can tell a
> > download
> > > is new by examining download->GetTargetFilePath().empty() in
> > > ChromeDownloadManagerDelegate::DetermineDownloadTarget(). The MIME type
and
> > > user-gesture flag are available on DownloadItem.
> > > 
> > > That said, could you elaborate which PDF downloads you'd like to classify
as
> > > drive-by downloads? I ask because it's possible for a download to have an
> > > associated user gesture, but still be a "drive-by" download. E.g. some
> > malicious
> > > ad dropping a download when the user visits a page.
> > 
> > asanka: Thank you! I tested and I was able to remove the content/ changes
> > entirely!
> 
> /download/ LGTM, but I removed myself from /download/ OWNERS a couple of days
> ago. So you'll need to pick another reviewer :)
> 
> My concern about what you are measuring still stands, though. Relying on 'has
> user gesture' may not tell you what you want to know.

Hey asanka: I'm trying to measure drive-by downloads triggered by IFRAMEed PDFs
when there's no PDF plugin installed.

Since those IFRAMEs are loaded without user gesture, I don't think this can have
a user gesture attached.

Does that seem sane to you?

[tommycli, 2017-05-25 18:16:38]

thestig: Thanks! addressd your comments below.

https://codereview.chromium.org/2901563003/diff/80001/chrome/common/BUILD.gn
File chrome/common/BUILD.gn (right):

https://codereview.chromium.org/2901563003/diff/80001/chrome/common/BUILD.gn#...
chrome/common/BUILD.gn:472: if (enable_pdf) {
On 2017/05/24 22:39:21, Lei Zhang wrote:
> You are using BUILDFLAG(ENABLE_PLUGINS) in the C++ code. Is that the same as
> enable_pdf?

Actually i have no idea what i was thinking. i specifically want this enabled
whether or not there's PDF or plugins.

https://codereview.chromium.org/2901563003/diff/80001/chrome/common/pdf_uma.h
File chrome/common/pdf_uma.h (right):

https://codereview.chromium.org/2901563003/diff/80001/chrome/common/pdf_uma.h...
chrome/common/pdf_uma.h:1: // Copyright 2016 The Chromium Authors. All rights
reserved.
On 2017/05/24 22:39:21, Lei Zhang wrote:
> It's half way through 2017.

Done.

https://codereview.chromium.org/2901563003/diff/80001/chrome/common/pdf_uma.h...
chrome/common/pdf_uma.h:8: #include "base/macros.h"
On 2017/05/24 22:39:21, Lei Zhang wrote:
> Are there any macros here?

Done.

https://codereview.chromium.org/2901563003/diff/80001/chrome/common/pdf_uma.h...
chrome/common/pdf_uma.h:13: LOADED_FULL_PAGE_PDF_WITH_PDFIUM = 0,
On 2017/05/24 22:39:21, Lei Zhang wrote:
> FYI, there was a thread from last month title:
> 
> [chromium-dev] PSA: Chromium style no longer prefers FOO over kFoo for enums

Done.

https://codereview.chromium.org/2901563003/diff/100001/chrome/renderer/chrome...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/2901563003/diff/100001/chrome/renderer/chrome...
chrome/renderer/chrome_content_renderer_client.cc:149: #include
"chrome/common/pdf_uma.h"
I'm leaving these usages ENABLE_PLUGINS for now, but will add the android
(NonLoadablePlaceholder) calls in a separate CL, where i'll make this outside of
ENABLE_PLUGINS.

[Lei Zhang, 2017-05-25 21:11:08]

https://codereview.chromium.org/2901563003/diff/100001/chrome/renderer/chrome...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/2901563003/diff/100001/chrome/renderer/chrome...
chrome/renderer/chrome_content_renderer_client.cc:149: #include
"chrome/common/pdf_uma.h"
On 2017/05/25 18:16:38, tommycli wrote:
> I'm leaving these usages ENABLE_PLUGINS for now, but will add the android
> (NonLoadablePlaceholder) calls in a separate CL, where i'll make this outside
of
> ENABLE_PLUGINS.

Is that why pdf_uma* are in the general file list in chrome/common/BUILD.gn, and
not in the if (enable_plugins) section?

[Lei Zhang, 2017-05-25 21:11:49]

lgtm

[tommycli, 2017-05-25 21:20:48]

thestig: thank you!

https://codereview.chromium.org/2901563003/diff/100001/chrome/renderer/chrome...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/2901563003/diff/100001/chrome/renderer/chrome...
chrome/renderer/chrome_content_renderer_client.cc:149: #include
"chrome/common/pdf_uma.h"
On 2017/05/25 21:11:08, Lei Zhang wrote:
> On 2017/05/25 18:16:38, tommycli wrote:
> > I'm leaving these usages ENABLE_PLUGINS for now, but will add the android
> > (NonLoadablePlaceholder) calls in a separate CL, where i'll make this
outside
> of
> > ENABLE_PLUGINS.
> 
> Is that why pdf_uma* are in the general file list in chrome/common/BUILD.gn,
and
> not in the if (enable_plugins) section?

Yes exactly. And in chrome_download_manager_delegate, we use them even if
plugins is off too.

[tommycli, 2017-05-25 21:20:51]

The CQ bit was checked by tommycli@chromium.org

[tommycli, 2017-05-25 21:20:52]

The patchset sent to the CQ was uploaded after l-g-t-m from asanka@chromium.org
Link to the patchset: https://codereview.chromium.org/2901563003/#ps100001
(title: "fix")

[commit-bot: I haz the power, 2017-05-25 21:21:34]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-25 21:34:03]

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

[commit-bot: I haz the power, 2017-05-25 21:34:04]

Try jobs failed on following builders:
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)

[tommycli, 2017-05-26 23:40:30]

The CQ bit was checked by tommycli@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-26 23:40:46]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tommycli, 2017-05-26 23:41:13]

The CQ bit was checked by tommycli@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-26 23:41:35]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tommycli, 2017-05-26 23:41:55]

The CQ bit was unchecked by tommycli@chromium.org

[tommycli, 2017-05-26 23:41:55]

The CQ bit was checked by tommycli@chromium.org

[tommycli, 2017-05-26 23:41:56]

The patchset sent to the CQ was uploaded after l-g-t-m from asanka@chromium.org,
thestig@chromium.org
Link to the patchset: https://codereview.chromium.org/2901563003/#ps120001
(title: "fix")

[commit-bot: I haz the power, 2017-05-26 23:42:13]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-27 00:03:00]

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

[commit-bot: I haz the power, 2017-05-27 00:03:02]

Try jobs failed on following builders:
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)

[Lei Zhang, 2017-05-27 01:40:16]

The CQ bit was checked by thestig@chromium.org

[commit-bot: I haz the power, 2017-05-27 01:40:27]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-27 02:20:03]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1495849216900580,
"parent_rev": "dac60ab4c5c107622c519dfd58aaf0ff417d142d", "commit_rev":
"c173ef62237c56354fd42882ea0999cc5564ef1c"}

[commit-bot: I haz the power, 2017-05-27 02:20:17]

Description was changed from

==========
PDF: Add UMA to track embedded PDF triggered drive-by downloads.

Adds an entry to an existing histogram to track drive-by downloads
triggered when the user views a page with an embedded PDF but has
no PDF plugin (or it's disabled).

BUG=714917
==========

to

==========
PDF: Add UMA to track embedded PDF triggered drive-by downloads.

Adds an entry to an existing histogram to track drive-by downloads
triggered when the user views a page with an embedded PDF but has
no PDF plugin (or it's disabled).

BUG=714917

Review-Url: https://codereview.chromium.org/2901563003
Cr-Commit-Position: refs/heads/master@{#475220}
Committed:
https://chromium.googlesource.com/chromium/src/+/c173ef62237c56354fd42882ea09...
==========

[commit-bot: I haz the power, 2017-05-27 02:20:18]

Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/c173ef62237c56354fd42882ea09...