Bug 30240 – window attributes (like localStorage) that are disabled at runtime are still visible

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore JavaScript

[Andrew Wilson]

Chrome enables some DOMWindow attributes (like localStorage) at runtime based on command line flags.

However, even when these attributes are supposed to be disabled, they still appear if the client code does something like:

if ('localStorage' in window) {
   // yay, we have localStorage
}

This screws up capabilities detection.

[Andrew Wilson]

Created attachment 40922 [details]
proposed patch

This just contains a single example of how to disable an attribute at runtime (using localStorage).

If this approach is acceptable, we can follow up with a second patch to address the other runtime-enabled APIs.

The changes to CodeGeneratorV8.pm are not as large as they appear - I refactored the guts of GenerateBatchedAttributeData into GenerateSingleBatchedAttribute, which necessitated changing the indentation level, which confused the heck out of diff. But I did not make any significant changes to that code.

[Evan Martin]

One thing that helps reviews it to write one patch that is just the refactor/whitespace, then a second one that makes structural changes.

[Andrew Wilson]

One other thing that occurs to me is I don't think I need a mutex controlling the static V8Proxy settings cache, because I think it's only accessed from within a single thread within a given process. Can anyone confirm/refute this?

[Darin Fisher (:fishd, Google)]

I believe that the Settings object is definitely not intended to be accessed from background threads.  I am curious if we will need some locking though for cases where features are exposed to both the main thread and workers.  LocalStorage is not a good test case ;-)

Ideally, we should just have a copy of the booleans living on each worker thread so that no locking is required.

[Andrew Wilson]

In reply to comment #4)
> I believe that the Settings object is definitely not intended to be accessed
> from background threads.  I am curious if we will need some locking though for
> cases where features are exposed to both the main thread and workers. 
> LocalStorage is not a good test case ;-)
> 
> Ideally, we should just have a copy of the booleans living on each worker
> thread so that no locking is required.

There's currently only one runtime-enabled API for workers (notifications) and I'm not sure it's actually disabled currently (I don't see any code that would disable it). I'll follow up with johnnyg about this.

I think the simplest solution for Chrome is just to have a static global set of booleans that are initialized at process startup. Since these flags don't/can't change after process startup anyway, that's probably a better solution than keeping multiple copies around. 

It occurs to me that some of these flags (like localStorageEnabled()) need to stay in WebCore::Settings, since they are exposed outside of WebCore (e.g. WebKit/mac/WebView). I'm not sure what the right fix is there.

It sounds like exposing command line flags/settings in a way that they can be accessed from any thread within WebKit should be the subject of another patch, as it's outside the scope of what I'm trying to accomplish (demonstrate the correct way to disable an attribute at runtime via the V8 bindings).

Since people seem to like my approach to updating the V8 bindings, I'm going to change this patch to:

1) Get rid of the static cache of Settings in V8Proxy, since it sounds like that's the wrong approach (isn't thread safe)
2) Remove EnabledAtRuntime from DOMWindow.localStorage
3) Add EnabledAtRuntime to DOMWindow.audio
4) Modify the V8 bindings appropriately to disable DOMWindow.audio if MediaPlayer::isAvailable() returns false.

This gets my V8 change into the tree, and gives everyone an example of how to properly disable a V8 API at runtime in a way that doesn't depend on whatever solution we come up with for exposing command line flags. 

I can log a separate bug for that issue, as we need to address that before things like WebSockets and Databases can be properly disabled.

[Andrew Wilson]

Created attachment 40972 [details]
Pulled the refactoring of GenerateBatchedAttributeData into a separate patch

As requested, pulled the refactoring of GenerateBatchedAttributeData into two functions into its own patch, to make the whole patch simpler to review. Tragically, diff is still confused by this patch, so it's not obvious that 95% of this is just a whitespace change caused by changing the indentation level.

[Andrew Wilson]

Created attachment 40973 [details]
Added support for EnabledAtRuntime attribute, and set this attr on the DOMWindow.Audio attribute.

I didn't want to open up the "how to we pull localStorage/etc out of WebCore::Settings" can of worms in this patch, so I instead used DOMWindow.Audio as my example, since it has a cleaner, thread-safe, static interface I can use to determine whether window.Audio should be enabled or not.

[Jeremy Orlow]

The non-refactoring patch LGTM minus the following comments:

> diff --git a/WebCore/bindings/scripts/CodeGeneratorV8.pm b/WebCore/bindings/scripts/CodeGeneratorV8.pm
> index c2d3bab..0ec9252 100644
> --- a/WebCore/bindings/scripts/CodeGeneratorV8.pm
> +++ b/WebCore/bindings/scripts/CodeGeneratorV8.pm
> @@ -1185,17 +1185,21 @@ sub GenerateImplementation
>      # For the DOMWindow interface we partition the attributes into the
>      # ones that disallows shadowing and the rest.
>      my @disallows_shadowing;
> +    # Also separate out attributes that are enabled at runtime so we can process them specially.
> +    my @enabled_at_runtime;
>      my @normal;
> -    if ($interfaceName eq "DOMWindow") {
> -        foreach my $attribute (@$attributes) {
> -            if ($attribute->signature->extendedAttributes->{"V8DisallowShadowing"}) {
> -                push(@disallows_shadowing, $attribute);
> -            } else {
> -                push(@normal, $attribute);
> -            }
> +    foreach my $attribute (@$attributes) {
> +        if ($interfaceName eq "DOMWindow" && $attribute->signature->extendedAttributes->{"V8DisallowShadowing"}) {
> +            push(@disallows_shadowing, $attribute);
> +        } elsif ($attribute->signature->extendedAttributes->{"EnabledAtRuntime"}) {

Is it an error if both EnabledAtRuntime and V8DisallowShadowing are enabled?


> diff --git a/WebCore/bindings/v8/custom/V8CustomBinding.h b/WebCore/bindings/v8/custom/V8CustomBinding.h
> index 2ff4ea9..88d2dc1 100644
> --- a/WebCore/bindings/v8/custom/V8CustomBinding.h
> +++ b/WebCore/bindings/v8/custom/V8CustomBinding.h
> @@ -78,6 +78,9 @@ struct NPObject;
>      bool V8Custom::v8##NAME##IndexedSecurityCheck(v8::Local<v8::Object> host, \
>          uint32_t index, v8::AccessType type, v8::Local<v8::Value> data)
>  
> +#define ACCESSOR_RUNTIME_ENABLER(NAME) \
> +    bool V8Custom::v8##NAME##Enabled()
> +

This could probably just be on one line.

>  namespace WebCore {
>  
>      class DOMWindow;
> @@ -232,6 +235,9 @@ namespace WebCore {
>      static bool v8##NAME##IndexedSecurityCheck(v8::Local<v8::Object> host, \
>          uint32_t index, v8::AccessType type, v8::Local<v8::Value> data)
>  
> +#define DECLARE_ACCESSOR_RUNTIME_ENABLER(NAME) \
> +    static bool v8##NAME##Enabled()
> +

This could probably just be on one line.

[Andrew Wilson]

(In reply to comment #8)

> 
> Is it an error if both EnabledAtRuntime and V8DisallowShadowing are enabled?
> 

Technically yes.

In reality, the "disallowShadowing" attributes are a small fixed set (location, top, window, opener) that aren't going to change in the future so people aren't going to add both.

If there's some way to generate an error within the code generator, I'd be happy to add an error here - I couldn't see any good way to do it, and there are plenty of other fringe cases in the codegen that aren't treated as errors, so I think this is good enough.

I'll clean up the other two nits you mentioned (combine them into one line).

[Andrew Wilson]

Created attachment 40976 [details]
Addressed jorlow's style suggestions.

[Andrew Wilson]

Accidentally closed the wrong bug - reopening.

[Dimitri Glazkov (Google)]

Comment on attachment 40976 [details]
Addressed jorlow's style suggestions.

Overall, this looks good, couple of nits:

>      my @disallows_shadowing;
> +    # Also separate out attributes that are enabled at runtime so we can process them specially.
> +    my @enabled_at_runtime;

both should be @disallowsShadowing and @enabledAtRuntime. Sorry for making you refactor old code, but those are style-boogers we shouldn't have, so it's a good thing :)

>  
> +    # Setup the enable-at-runtime attrs if we have them
> +    foreach my $runtime_attr (@enabled_at_runtime) {
> +        $enable_function = $interfaceName . $codeGenerator->WK_ucfirst($runtime_attr->signature->name);

Ditto.

> +        my $conditionalString = GenerateConditionalString($runtime_attr->signature);
> +        push(@implContent, "\n#if ${conditionalString}\n") if $conditionalString;
> +        push(@implContent, "  if (V8Custom::v8${enable_function}Enabled()) {\n");

4 spaces even in generated code :)

> +#define ACCESSOR_RUNTIME_ENABLER(NAME) bool V8Custom::v8##NAME##Enabled()

I was envisioning something of a one-place structure, which can be queried for a feature, but I guess this is ok, too.

[Andrew Wilson]

(In reply to comment #12)
> 
> > +#define ACCESSOR_RUNTIME_ENABLER(NAME) bool V8Custom::v8##NAME##Enabled()
> 
> I was envisioning something of a one-place structure, which can be queried for
> a feature, but I guess this is ok, too.

I did it this way because some attributes that we may want to hide (like window.Audio)  may not map directly to a static boolean (for example, window.Audio depends on MediaPlayer.isAvailable() which does a runtime check for installed codecs).

Given that not many attributes are likely to be runtime enabled (just stuff that's under development), the extra flexibility seemed worth the cost. If you feel strongly about this, I don't mind updating the codegen again once we have a global structure for these settings.

[Dimitri Glazkov (Google)]

No, I don't feel strongly about it.

[Andrew Wilson]

Created attachment 41081 [details]
patch addressing dglazkov's comments

OK, updated the whitespace (since GenerateSingleBatchedAttribute needed to support two different levels of indentation depending on its calling context, it now takes an indent parameter). The generated file now seems to adhere to the WebKit style guidelines (at least the parts generated by my new code :).

[Dimitri Glazkov (Google)]

Comment on attachment 41081 [details]
patch addressing dglazkov's comments

> diff --git a/WebCore/ChangeLog b/WebCore/ChangeLog
> +        window attributes (like localStorage) that are disabled at runtime are still visible

"DOMWindow properties" sounds a bit better than "window attributes".

r=me otherwise.

[Andrew Wilson]

Created attachment 41120 [details]
Combined two patches into a single one.

Re-combined the two patches into a single one for ease of landing/reviewing.

[Dimitri Glazkov (Google)]

Comment on attachment 41120 [details]
Combined two patches into a single one.

r=me. Godspeed.

[Andrew Wilson]

Landed as r49510.