New issue
Advanced search Search tips

Issue 661280 link

Starred by 6 users

Issue metadata

Status: Started
Owner:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Android , Windows , Chrome , Mac
Pri: 3
Type: Feature



Sign in to add a comment

Feature Policy: Log to console when disabled features are requested

Project Member Reported by iclell...@chromium.org, Nov 1 2016

Issue description

We should log to the JS console any time a feature which is disabled by policy is accessed, or use of the feature is attempted.

 

Comment 1 by kochi@chromium.org, Nov 4 2016

iclelland@ could you be more specific about what it would be?
And could you also fill "Components" field and avoid "Blink"?

Thanks!
(commenting as a part of my blink bug triage sheriff duty)
Components: -Blink Blink>SecurityFeature
Components: Blink>FeaturePolicy
Components: -Blink>SecurityFeature
Blocking: -623682
This is currently implemented in each feature (fullscreen, vibrate and paymentrequest) independently. There is currently no need for a generic mechanism, but we can revisit that once we have more features.

In any case, the feature code is the correct place to decide when to log, since the actual test could be made in many different places, and not all need to pollute the console.
Labels: Type-Feature
Lighthouse could also surface all these messages nicely if they were emitted uniquely.

If you add a console message with a unique source (https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/inspector/ConsoleTypes.h?q=ConsoleType&sq=package:chromium&l=6) we can collect them on the Lighthouse side. I _think_ this means adding a new MessageSource to that enum, rather than reusing one already present.


I looked at this briefly when working on 'document-write' but that feature throws DOM exceptions. Is there any issues with both throwing and logging in console (for features that also throw exceptions)?

Also should we add custom information for a feature to the log or would a generic format "Fearure X is disabled in document/origin Y" suffice?
Owner: iclell...@chromium.org
Status: Started (was: Available)
Project Member

Comment 10 by bugdroid1@chromium.org, Oct 16

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/6580741bfb5141cbffff0da67f147efbb5cd0690

commit 6580741bfb5141cbffff0da67f147efbb5cd0690
Author: Ian Clelland <iclelland@chromium.org>
Date: Tue Oct 16 16:00:50 2018

Add ConsoleMessage to IsFeatureEnabled and ReportFeaturePolicyViolation

Report all feature policy violations to the inspector console. This
log message is generated at the same time that a report would be sent
to the ReportingAPI.

By default, a generic message is logged, but the methods will accept a
ConsoleMessage object which will be used in its place if the FP check
determines that use of the feature is not allowed.

Bug: 661280
Change-Id: Icf2e89e3e6e582480b809b64edfa5832a836ebac
Reviewed-on: https://chromium-review.googlesource.com/c/1257545
Commit-Queue: Ian Clelland <iclelland@chromium.org>
Reviewed-by: Dmitry Gozman <dgozman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#600004}
[modify] https://crrev.com/6580741bfb5141cbffff0da67f147efbb5cd0690/third_party/WebKit/LayoutTests/fullscreen/full-screen-frameset-expected.txt
[modify] https://crrev.com/6580741bfb5141cbffff0da67f147efbb5cd0690/third_party/blink/renderer/core/dom/document.cc
[modify] https://crrev.com/6580741bfb5141cbffff0da67f147efbb5cd0690/third_party/blink/renderer/core/dom/document.h
[modify] https://crrev.com/6580741bfb5141cbffff0da67f147efbb5cd0690/third_party/blink/renderer/core/execution_context/security_context.cc
[modify] https://crrev.com/6580741bfb5141cbffff0da67f147efbb5cd0690/third_party/blink/renderer/core/execution_context/security_context.h
[modify] https://crrev.com/6580741bfb5141cbffff0da67f147efbb5cd0690/third_party/blink/renderer/core/xmlhttprequest/xml_http_request.cc
[modify] https://crrev.com/6580741bfb5141cbffff0da67f147efbb5cd0690/third_party/blink/renderer/modules/geolocation/geolocation.cc
[modify] https://crrev.com/6580741bfb5141cbffff0da67f147efbb5cd0690/third_party/blink/renderer/modules/webmidi/navigator_web_midi.cc

Should this be working in Canary for vertical-scroll?

https://20181024t224456-dot-chromedevsummit-site.appspot.com/devsummit/schedule uses `vertical-scroll: 'none'`. The page doesn't scroll when you try to scroll it, but nothing gets logged to the console. Looks like a buggy site!
We definitely do not log anything for the vertical scroll policy.

At a technical level, we only sample that policy once at document creation, and then we change the scrolling behavior based on that bit; we don't actually look at the policy at the time that the user's finger is moving on the page.

At a higher level, I don't think that a user initiating a scroll should result in a message -- I think that the primary use case for this policy was to enable embedding of ads which cannot capture scroll. If the user successfully scrolls the main document with a scroll gesture which happens to start on the ad surface, then the policy is working, but there's no need to log any "violation" in that case.

Ehasn and I discussed possible behaviours which could be considered attempted violations: calling preventDefault on a scroll or touch event in the frame, or calling scrollIntoView, but those have not been implemented yet.

Sign in to add a comment