Logging accesses to URL-properties of DOM elements, e.g. src.

Logging accesses to URL-properties of DOM elements, e.g., src.
Logging calls to submit method of Form elements.

This CL allows UMA logging of the following extension activities:
- an extension sets a url property on a DOM element within a page, e.g., 
the extension's content script set src property of an image element.
- an extension's content script calls submit method on a Form element 
within a page. 

BUG=313901, 313878
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=161238

[pmarch, 2013-10-31 21:17:11]

Adrienne,

Please, have a look at this CL.

[felt, 2013-10-31 21:31:35]

lgtm petr. Can you file a bug that lists all of the properties you are adding?

[pmarch, 2013-10-31 22:33:13]

Hi Adam,

Could you please review this CL. We want this CL to land before the branch point
if possible which is Mon, Nov 4.

Thanks.

[abarth-chromium, 2013-11-01 16:59:53]

I'm sorry, I'm not interested in reviewing these CLs.  Perhaps someone else is
interested in reviewing them.  You should be aware that many of these logging
points are trivial to circumvent using other DOM APIs.

[pmarch, 2013-11-01 17:14:39]

Hi Dimitri,

Would you have time to review this CL; we need it to get it in before branch
point this Monday.

Thanks.

[dglazkov, 2013-11-01 17:27:31]

Petr, you really should consider abarth's response as a not LGTM. I don't have
any context on what's going on here.

[felt, 2013-11-01 17:30:35]

On 2013/11/01 17:27:31, Dimitri Glazkov wrote:
> Petr, you really should consider abarth's response as a not LGTM. I don't have
> any context on what's going on here.

There is similar logging already in place for other properties; e.g.,
https://codereview.chromium.org/13799007/ and
https://codereview.chromium.org/19697008/. This extends the existing logging to
the src attribute.

This information is used to support the chrome.activityLogPrivate API, which has
several uses (e.g., collecting UMA stats on the types of DOM manipulations
performed by extensions, and the Watchdog app that shows information about
extension behavior to end users).

[pmarch, 2013-11-01 18:17:29]

Kentaro

Could you please review this CL as you reviewed some other CLs that introduce
the same feature to other DOM elements

https://codereview.chromium.org/13799007/
https://codereview.chromium.org/19697008/

We have a time constraint as we want to get it in before the branch point. So,
if it is possible, please, review it before Monday.

Thanks

[haraken, 2013-11-02 02:17:28]

I'm also concerned that the logging points in this CL won't give you accurate
results.

Would you elaborate on why you need to log these DOM APIs? As adam mentioned,
it's possible for content scripts to circumvent DOM attribute setters to set
URLs. For example, if a content script statically writes <form
action='http://some_malicious_url'> in HTML, you cannot catch it just by logging
DOM attribute setters for HTMLElement.formAction.

In short, a binding/ layer wouldn't be the right place to log these kind of
things. You'll need to add some mechanism to log in a core/ layer if you want to
measure them accurately.

[pmarch, 2013-11-02 03:09:22]

On 2013/11/02 02:17:28, haraken wrote:
> I'm also concerned that the logging points in this CL won't give you accurate
> results.
> 
> Would you elaborate on why you need to log these DOM APIs? As adam mentioned,
> it's possible for content scripts to circumvent DOM attribute setters to set
> URLs. For example, if a content script statically writes <form
> action='http://some_malicious_url'> in HTML, you cannot catch it just by
logging
> DOM attribute setters for HTMLElement.formAction.
> 
> In short, a binding/ layer wouldn't be the right place to log these kind of
> things. You'll need to add some mechanism to log in a core/ layer if you want
to
> measure them accurately.

It is true that setting these attributes can be avoided via other DOM API calls,
but we are not trying to log *all* the paths that can be used for setting these
attributes. We will discuss this issue via email as we do not want to advertise
the feature that we are building ahead of time.

[haraken, 2013-11-02 04:21:45]

Discussed offline with petr and adam.

Given that this is just an experiment to see how successfully the approach goes,
it sounds reasonable to add ActivityLogging to URL attributes for now.

My points are:

- These attributes won't be performance-sensitive, so I don't have any
performance concern about the CL.

- However, the logging points won't give you accurate results. There should be a
better way for logging. Specifically, I think you should consider adding logging
points to core/ instead of binding/. binding/ wouldn't be a right layer to log
what you want to log.

- However, it will take a substantial amount of work to introduce a new logging
mechanism to core/. So it would be OK for now to add a hack to binding/ and see
how successfully the experiment goes. Then we can decide whether it's worth
implementing a more robust logging system or not.

LGTM therefore. But please consider a better way for logging. Given that
PerWorldLogging code increases binary size, we don't want to have
ActivityLogging in a lot of places just for collecting inaccurate logs.

[philipj_slow, 2013-11-02 06:10:32]

Not my review, bit why dö we want to log all URL properties? And why is
PerWorldBindings addee everywhere? Makes the IDL less readable, I think.

[eseidel, 2013-11-02 06:48:18]

What is "ActivityLogging".  It sounds scary to the casual observer. :(

[eseidel, 2013-11-02 06:59:37]

Searching on Google it appears to be related to understanding what extensions
are doing to a page and auditing such.  I've emailed blink-dev in hopes of
clarifying to a larger audience (since I suspect I won't be the only one
confused by these notes in the IDL... or maybe I'm just the only one who missed
the memo). :)

[eseidel, 2013-11-02 06:59:37]

Searching on Google it appears to be related to understanding what extensions
are doing to a page and auditing such.  I've emailed blink-dev in hopes of
clarifying to a larger audience (since I suspect I won't be the only one
confused by these notes in the IDL... or maybe I'm just the only one who missed
the memo). :)

[dglazkov, 2013-11-02 17:01:55]

On 2013/11/02 04:21:45, haraken wrote:
> Discussed offline with petr and adam.
> 
> Given that this is just an experiment to see how successfully the approach
goes,
> it sounds reasonable to add ActivityLogging to URL attributes for now.

I guess I am having a hard time understanding why an experimental approach needs
to land before the branch point.

[felt, 2013-11-02 18:38:25]

On 2013/11/02 17:01:55, Dimitri Glazkov wrote:
> On 2013/11/02 04:21:45, haraken wrote:
> > Discussed offline with petr and adam.
> > 
> > Given that this is just an experiment to see how successfully the approach
> goes,
> > it sounds reasonable to add ActivityLogging to URL attributes for now.
> 
> I guess I am having a hard time understanding why an experimental approach
needs
> to land before the branch point.

The timeline is up to Petr, but to give some visibility into what we're doing:

In Q4/Q1, we'll be internally dogfooding a version of the Watchdog app that
offers some extra security features that rely on the logging API. If the
internal experiment goes well, at that point we'll start talking about whether
and how to build it into a real Chrome security feature. It doesn't need to be
robust to circumvention right now because we don't think anyone wants to
circumvent a pre-release feature that may not be worth building out into a real
Chrome feature. Just a very early stage experiment.

[pmarch2, 2013-11-02 21:34:16]

In order to evaluate our behavior approach on the client machines, we need
this change to propagate to Chrome stable version, so we want to land it in
m32 which will become stable in Jan.


On Sat, Nov 2, 2013 at 1:01 PM, <dglazkov@chromium.org> wrote:

> On 2013/11/02 04:21:45, haraken wrote:
>
>> Discussed offline with petr and adam.
>>
>
>  Given that this is just an experiment to see how successfully the approach
>>
> goes,
>
>> it sounds reasonable to add ActivityLogging to URL attributes for now.
>>
>
> I guess I am having a hard time understanding why an experimental approach
> needs
> to land before the branch point.
>
>
> https://codereview.chromium.org/55313003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[pmarch, 2013-11-02 21:39:54]

On 2013/11/02 06:10:32, philipj wrote:
> Not my review, bit why dö we want to log all URL properties? And why is
> PerWorldBindings addee everywhere? Makes the IDL less readable, I think.

PerWorldBindings allows us not to add any new code into bindings for script
operating within pages. Thus, there is no overhead for page scripts caused by
this change. However, this approach requires larger code base and introduces
some overhead to extensions' content scripts operating on pages.

[haraken, 2013-11-03 00:51:16]

We should have added more explanations about this change. I want to apologize.

I'm OK with landing this CL before the branch point, given:

- This CL is just adding a kind of UMA. There would be no harm in performance
and stability.
- Landing the CL before the branch point is necessary for them to unblock their
work.
- The feature has passed security & privacy reviews.

Probably we should wait to land the CL for several hours to give folks a chance
to add comments.

[pmarch, 2013-11-03 04:06:00]

On 2013/11/03 00:51:16, haraken wrote:
> We should have added more explanations about this change. I want to apologize.
> 
> I'm OK with landing this CL before the branch point, given:
> 
> - This CL is just adding a kind of UMA. There would be no harm in performance
> and stability.
> - Landing the CL before the branch point is necessary for them to unblock
their
> work.
> - The feature has passed security & privacy reviews.
> 
> Probably we should wait to land the CL for several hours to give folks a
chance
> to add comments.

Added more description to this CL.

[eseidel, 2013-11-03 19:22:52]

It sounds like you all have a tight deadline to hit.  From the sounds of things
here and blink-dev I suspect some wider understanding of where we're going here
is needed, but this particular CL is just continuation of the existing work and
shouldn't be impeded by that discussion.  Sounds like you have your r+ and
should land at will.

Unclear to me if peppering idls with these annotations is the right solution to
this problem, but I don't feel particularly well versed in this problem and very
much trust that smart folks have considered the options.

[commit-bot: I haz the power, 2013-11-04 00:26:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pmarch@chromium.org/55313003/110001

[commit-bot: I haz the power, 2013-11-04 01:12:16]

Change committed as 161238