Disable DOM MutationEvents for platform apps

Disable DOM MutationEvents for platform apps

New apps should use MutationObservers instead, which provide
similar (or better) capabilities without the performance and
stability/security downsides of MutationEvents.

BUG=142648
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=152914

[adamk, 2012-08-22 17:45:24]

How do I test this? A similar change for WebComponents didn't add any tests, but
it seems like some automated test should be possible (e.g., a tiny app that
tries to listen for MutationEvents), but I'm not at all familiar with this area
of the code.

[Mihai Parparita -not on Chrome, 2012-08-22 18:46:48]

I later added a test for Shadow DOM being enable for apps only in
http://crrev.com/145229. You can have a similar browser test for this.

Mihai

On Wed, Aug 22, 2012 at 10:45 AM, <adamk@chromium.org> wrote:

> Reviewers: Mihai Parparita, rafaelw,
>
> Message:
> How do I test this? A similar change for WebComponents didn't add any
> tests, but
> it seems like some automated test should be possible (e.g., a tiny app that
> tries to listen for MutationEvents), but I'm not at all familiar with this
> area
> of the code.
>
> Description:
> Disable DOM MutationEvents for platform apps
>
> New apps should use MutationObservers instead, which provide
> similar (or better) capabilities without the performance and
> stability/security downsides of MutationEvents.
>
> BUG=142648
>
>
> Please review this at
https://chromiumcodereview.**appspot.com/10870013/<https://chromiumcodereview...
>
> SVN Base:
http://git.chromium.org/**chromium/src.git@git-svn<http://git.chromium.org/ch...
>
> Affected files:
>   M chrome/renderer/chrome_render_**view_observer.h
>   M chrome/renderer/chrome_render_**view_observer.cc
>
>
> Index: chrome/renderer/chrome_render_**view_observer.cc
> diff --git a/chrome/renderer/chrome_**render_view_observer.cc
> b/chrome/renderer/chrome_**render_view_observer.cc
> index 8277af961d5fd879d491ed20d543e2**33a0faf6fb..**
> 720a03f2a92f272d57bbdae015b911**8bddb02028 100644
> --- a/chrome/renderer/chrome_**render_view_observer.cc
> +++ b/chrome/renderer/chrome_**render_view_observer.cc
> @@ -498,21 +498,16 @@ bool ChromeRenderViewObserver::**
> allowWriteToClipboard(**WebFrame* frame,
>    return allowed;
>  }
>
> -bool ChromeRenderViewObserver::**HasExtensionPermission(
> -    const WebSecurityOrigin& origin, APIPermission::ID permission) const {
> +const extensions::Extension* ChromeRenderViewObserver::**GetExtension(
> +    const WebSecurityOrigin& origin) const {
>    if (!EqualsASCII(origin.protocol(**), chrome::kExtensionScheme))
> -    return false;
> +    return NULL;
>
>    const std::string extension_id = origin.host().utf8().data();
>    if (!extension_dispatcher_->**IsExtensionActive(extension_**id))
> -    return false;
> -
> -  const extensions::Extension* extension =
> -      extension_dispatcher_->**extensions()->GetByID(**extension_id);
> -  if (!extension)
> -    return false;
> +    return NULL;
>
> -  return extension->HasAPIPermission(**permission);
> +  return extension_dispatcher_->**extensions()->GetByID(**extension_id);
>  }
>
>  bool ChromeRenderViewObserver::**allowWebComponents(const WebDocument&
> document,
> @@ -524,20 +519,38 @@ bool
ChromeRenderViewObserver::**allowWebComponents(const
> WebDocument& document,
>    if (EqualsASCII(origin.protocol()**, chrome::kChromeUIScheme))
>      return true;
>
> -  // The <browser> tag is implemented via Shadow DOM.
> -  if (HasExtensionPermission(**origin, APIPermission::kBrowserTag))
> -    return true;
> +  if (const extensions::Extension* extension = GetExtension(origin)) {
> +    // The <browser> tag is implemented via Shadow DOM.
> +    if (extension->HasAPIPermission(**APIPermission::kBrowserTag))
> +      return true;
>
> -  if (HasExtensionPermission(**origin, APIPermission::kExperimental))
> -    return true;
> +    if (extension->HasAPIPermission(**APIPermission::kExperimental))
> +      return true;
> +  }
>
>    return false;
>  }
>
> +bool ChromeRenderViewObserver::**allowMutationEvents(
> +    const WebDocument& document,
> +    bool default_value) {
> +  WebSecurityOrigin origin = document.securityOrigin();
> +  if (!EqualsASCII(origin.protocol(**), chrome::kExtensionScheme))
> +    return default_value;
> +
> +  if (const extensions::Extension* extension = GetExtension(origin)) {
> +    if (extension->is_platform_app())
> +      return false;
> +  }
> +
> +  return default_value;
> +}
> +
>  bool ChromeRenderViewObserver::**allowHTMLNotifications(
>      const WebDocument& document) {
>    WebSecurityOrigin origin = document.securityOrigin();
> -  return HasExtensionPermission(origin, APIPermission::kNotification);
> +  const extensions::Extension* extension = GetExtension(origin);
> +  return extension && extension->HasAPIPermission(**
> APIPermission::kNotification);
>  }
>
>  static void SendInsecureContentSignal(int signal) {
> Index: chrome/renderer/chrome_render_**view_observer.h
> diff --git a/chrome/renderer/chrome_**render_view_observer.h
> b/chrome/renderer/chrome_**render_view_observer.h
> index 46c158626aa05806b0b6e9e4c1c006**78f32bdd97..**
> e8328ee346368d0bcb3bd6ae9cfbb4**e5a81c58c3 100644
> --- a/chrome/renderer/chrome_**render_view_observer.h
> +++ b/chrome/renderer/chrome_**render_view_observer.h
> @@ -28,6 +28,7 @@ class WebViewAnimatingOverlay;
>
>  namespace extensions {
>  class Dispatcher;
> +class Extension;
>  }
>
>  namespace WebKit {
> @@ -111,6 +112,8 @@ class ChromeRenderViewObserver : public
> content::RenderViewObserver,
>    virtual bool allowWebComponents(const WebKit::WebDocument&, bool)
> OVERRIDE;
>    virtual bool allowHTMLNotifications(
>        const WebKit::WebDocument& document) OVERRIDE;
> +  virtual bool allowMutationEvents(const WebKit::WebDocument&,
> +                                   bool default_value) OVERRIDE;
>    virtual void didNotAllowPlugins(WebKit::**WebFrame* frame) OVERRIDE;
>    virtual void didNotAllowScript(WebKit::**WebFrame* frame) OVERRIDE;
>    virtual bool allowDisplayingInsecureContent**(
> @@ -191,10 +194,10 @@ class ChromeRenderViewObserver : public
> content::RenderViewObserver,
>    // Determines if a host is in the strict security host set.
>    bool IsStrictSecurityHost(const std::string& host);
>
> -  // Checks if |origin| correponds to an installed extension that has been
> -  // granted the |permission|.
> -  bool HasExtensionPermission(const WebKit::WebSecurityOrigin& origin,
> -                              extensions::APIPermission::ID permission)
> const;
> +  // If |origin| corresponds to an installed extension, returns that
> extension.
> +  // Otherwise returns NULL.
> +  const extensions::Extension* GetExtension(
> +      const WebKit::WebSecurityOrigin& origin) const;
>
>    // Save the JavaScript to preload if a ViewMsg_WebUIJavaScript is
> received.
>    scoped_ptr<WebUIJavaScript> webui_javascript_;
>
>
>

[Mihai Parparita -not on Chrome, 2012-08-22 20:04:05]

https://chromiumcodereview.appspot.com/10870013/diff/1/chrome/renderer/chrome...
File chrome/renderer/chrome_render_view_observer.cc (right):

https://chromiumcodereview.appspot.com/10870013/diff/1/chrome/renderer/chrome...
chrome/renderer/chrome_render_view_observer.cc:538: if
(!EqualsASCII(origin.protocol(), chrome::kExtensionScheme))
I don't think you need this check, GetExtension already looks at the scheme, and
returns NULL if it's not the extension scheme, thus putting you on the
default_value path below.

[adamk, 2012-08-22 21:39:53]

I've added a test (and verified that it fails without my patch).

https://chromiumcodereview.appspot.com/10870013/diff/1/chrome/renderer/chrome...
File chrome/renderer/chrome_render_view_observer.cc (right):

https://chromiumcodereview.appspot.com/10870013/diff/1/chrome/renderer/chrome...
chrome/renderer/chrome_render_view_observer.cc:538: if
(!EqualsASCII(origin.protocol(), chrome::kExtensionScheme))
On 2012/08/22 20:04:05, Mihai Parparita wrote:
> I don't think you need this check, GetExtension already looks at the scheme,
and
> returns NULL if it's not the extension scheme, thus putting you on the
> default_value path below.

Thanks, this was left over from a previous version.  Simplified.

[Mihai Parparita -not on Chrome, 2012-08-22 21:51:04]

LGTM

[commit-bot: I haz the power, 2012-08-22 22:01:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/adamk@chromium.org/10870013/4002

[commit-bot: I haz the power, 2012-08-22 22:01:41]

Presubmit check for 10870013-4002 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome

Presubmit checks took 1.2s to calculate.

[adamk, 2012-08-22 22:04:34]

+thakis for chrome OWNERS (randomly-selected).

[Nico, 2012-08-22 22:06:29]

lgtm

[commit-bot: I haz the power, 2012-08-22 22:07:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/adamk@chromium.org/10870013/4002

[commit-bot: I haz the power, 2012-08-22 23:21:03]

Try job failure for 10870013-4002 (retry) on win for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...

[commit-bot: I haz the power, 2012-08-22 23:26:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/adamk@chromium.org/10870013/4002

[commit-bot: I haz the power, 2012-08-23 01:46:47]

Change committed as 152914