Bug 29896 – Runtime configurable WebSocket code is detectable when turned off

• Status: RESOLVED
• Resolution: FIXED
• Component: Forms

[Sam Weinig]

The runtime switch for WebSockets that was added in https://bugs.webkit.org/show_bug.cgi?id=28941 does not sufficiently hide WebSockets from developer detection techniques.  For instance, ("WebSocket" in window) will return true.  This can be seen as a noticeable regression in LayoutTests/fast/dom/Window/window-properties-expected.txt as it now has a window.WebSocket [undefined] line.

This should either be fixed shortly or the patch that introduced this behavior should be rolled out as we don't want early adopters using the nightly builds trying to use feature detection for WebSockets and getting bad results.

[Mark Rowe (bdash)]

It's also worth noting that the change turned off Web Socket support on Mac and Windows when it was on before.  Was that intentional?

[Fumitoshi Ukai]

(In reply to comment #0)
> The runtime switch for WebSockets that was added in
> https://bugs.webkit.org/show_bug.cgi?id=28941 does not sufficiently hide
> WebSockets from developer detection techniques.  For instance, ("WebSocket" in
> window) will return true.  This can be seen as a noticeable regression in
> LayoutTests/fast/dom/Window/window-properties-expected.txt as it now has a
> window.WebSocket [undefined] line.

Hmm, I'll try to fix it. could you give me a hint what function/method is used for '"WebSocket" in window'?

> This should either be fixed shortly or the patch that introduced this behavior
> should be rolled out as we don't want early adopters using the nightly builds
> trying to use feature detection for WebSockets and getting bad results.

[Fumitoshi Ukai]

(In reply to comment #1)
> It's also worth noting that the change turned off Web Socket support on Mac and
> Windows when it was on before.  Was that intentional?

Actually, we don't have any SocketStreamHandle implementation in any platform yet, so Web Socket is not usable at all right now.  You may call "new WebSocket(url)", but it will always immediately be closed.

[Sam Weinig]

> 
> Hmm, I'll try to fix it. could you give me a hint what function/method is used
> for '"WebSocket" in window'?
> 

This is handled by the getPropertyNames function in JSDOMWindow.

[Jeremy Orlow]

In case anyone's looking at this thread and not following webkit-dev, there's a thread going on right now called "[webkit-dev] Runtime setting for incomplete features" related to this issue.

As for this bug: We need to make it so that if WEB_SOCKETS is enabled at compile time, it's also enabled at run time by default.  Chromium should then override this default based on the presence of a run-time flag.

[Fumitoshi Ukai]

Created attachment 40934 [details]
WebCore: Enable WebSocket by default if WEB_SOCKETS is enabled at compile time.

[Jeremy Orlow]

fyi: a similar bug: https://bugs.webkit.org/show_bug.cgi?id=30240

[Andrew Wilson]

(In reply to comment #7)
> fyi: a similar bug: https://bugs.webkit.org/show_bug.cgi?id=30240

Bug 30240 has a V8 implementation of turning off attributes at runtime, so you should be able to leverage that work.

I do think that it's the wrong approach to have an experimentalWebSocketsEnabled() flag in Settings, because that Settings object is not available in worker context so you can't disable WebSockets in workers. IMO, a better approach is via some kind of global static API ala SharedWorker.isAvailable() or MediaPlayer.isAvailable().

[Andrew Wilson]

(In reply to comment #4)
> This is handled by the getPropertyNames function in JSDOMWindow.

A couple of questions:

1) If Chrome is the only platform that does runtime enabling, is it sufficient to only address this in the V8 bindings, not JSC?

2) If we do need to deal with this in the JS bindings, is it similarly sufficient to set the DontEnum flag on the appropriate entries in JSDOMWindowTableValues? This was not sufficient in V8, but if JSC handles this correctly, that might be an easy fix for this.

[Jeremy Orlow]

Note there are 2 VERY DISTINCT issues being tracked in 30240 and this bug.

This bug is about the fact that the run-time configuration should match the compile time configuration.  The patch posted above solves this, should be r+'ed, and checked in.

30240 is about making the run time state 100% hidden from JavaScript when disabled at run time.  To date, not a single JSC user has said they plan to ship features behind command line flags, so at best it's a low priority and at worst we shouldn't do it.

I'm sorry I wasn't more clear when I posted the link to the other thread.

[David Levin]

Comment on attachment 40934 [details]
WebCore: Enable WebSocket by default if WEB_SOCKETS is enabled at compile time.

Summary: The right fix is to remove change done in https://bugs.webkit.org/show_bug.cgi?id=28941 and replace it with the mechanism from https://bugs.webkit.org/show_bug.cgi?id=30240

Details:
The change done in 28941 was to allow for runtime enabling of web sockets, but it is fundamentally broken because it allows for the feature to be detected even when it isn't enabled which breaks websites. This patch attempts to repair 28941 by enabling web sockets by default, but it does not address the core issue that this approach is broken.

There is now a better way to do what is desired: https://bugs.webkit.org/show_bug.cgi?id=30240

So instead of trying to patch code that will still be broken, it should be ripped out and replaced with a proper solution.

[David Levin]

This patch http://trac.webkit.org/changeset/49643 will also have to be undone.

[Fumitoshi Ukai]

Created attachment 41274 [details]
Change WebSOcket EnableAtRuntime attribute in DOMWindow

[Fumitoshi Ukai]

Created attachment 41285 [details]
WebSocket runtime configuration for chromium/v8

[Andrew Wilson]

Comment on attachment 41285 [details]
WebSocket runtime configuration for chromium/v8


> diff --git a/WebCore/page/Settings.cpp b/WebCore/page/Settings.cpp
>      // A Frame may not have been created yet, so we initialize the AtomicString 
>      // hash before trying to use it.
> @@ -535,7 +533,12 @@ void Settings::setWebGLEnabled(bool enabled)
>  #if ENABLE(WEB_SOCKETS)
>  void Settings::setExperimentalWebSocketsEnabled(bool enabled)
>  {
> -    m_experimentalWebSocketsEnabled = enabled;
> +    WebSocket::setIsAvailable(enabled);
> +}
> +

Is it possible to entirely remove Settings::setExperimentalWebSocketsEnabled() and Settings::experimentalWebSocketsEnabled(), in favor of just using WebSocket::isAvailable() directly?


> +#if USE(V8)
> +bool WebSocket::m_available = false;
> +
> +void WebSocket::setIsAvailable(bool available)
> +{
> +    m_available = available;
> +}
> +
> +bool WebSocket::isAvailable()
> +{
> +    return m_available;
> +}
> +#else
> +void WebSocket::setIsAvailable(bool)
> +{
> +}
> +
> +bool WebSocket::isAvailable()
> +{
> +    return true;
> +}
> +#endif
> +

I don't really like #if USE(V8) usually, but it's probably OK here. However, in this case when not using V8 you're left with a setIsAvailable() function that does nothing which seems bad.

I'd prefer only defining setIsAvailable() for chromium/V8 for now - that way if someone ends up trying to use setIsAvailable() on an unsupported (non-V8) platform, they'll get a compile error.

And if you ever have platform-specific support for WebSockets, you could move WebSocket::isAvailable() into a chromium specific file, and get rid of setIsAvailable() entirely.

[Fumitoshi Ukai]

Created attachment 41395 [details]
WebCore: Set EnabledAtRuntime for WebSocket in DOMWindow

[Andrew Wilson]

I might not be fully understanding your latest change - I see that you define isAvailable() in WebSocket.h, but the patch doesn't include an implementation anywhere (i.e. no changes to WebSocket.cpp). Where is the implementation defined?

[David Levin]

Comment on attachment 41395 [details]
WebCore: Set EnabledAtRuntime for WebSocket in DOMWindow

Where is WebSocket::isAvailable() implemented? (And why isn't inside of some v8 guard? I don't think there needs to be an implementation for jsc.)

> diff --git a/WebCore/bindings/v8/custom/V8DOMWindowCustom.cpp b/WebCore/bindings/v8/custom/V8DOMWindowCustom.cpp
>  #include "Settings.h"
>  #include "SharedWorkerRepository.h"
>  #include "WindowFeatures.h"
> +#include "WebSocket.h"

Out of alphabetical order.

[Fumitoshi Ukai]

Created attachment 41476 [details]
WebCore: Set EnabledAtRuntime for WebSocket in DOMWindow

[David Levin]

Comment on attachment 41476 [details]
WebCore: Set EnabledAtRuntime for WebSocket in DOMWindow

Thanks Ukai!

[WebKit Commit Bot]

Comment on attachment 41476 [details]
WebCore: Set EnabledAtRuntime for WebSocket in DOMWindow

Rejecting patch 41476 from commit-queue.

Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--quiet', '--exit-after-n-failures=1']" exit_code: 60
Last 500 characters of output:
ages while\n\trunning as root.  There are known race conditions that\n\twill allow any local user to read any file on the system.\n\tIf you still desire to serve pages as root then\n\tadd -DBIG_SECURITY_HOLE to the CFLAGS env variable\n\tand then rebuild the server.\n\tIt is strongly suggested that you instead modify the User\n\tdirective in your httpd.conf file to list a non-root\n\tuser.\n
Timed out waiting for httpd to start at WebKitTools/Scripts/run-webkit-tests line 1359, <IN> line 30183.

[Fumitoshi Ukai]

Committed r49843: <http://trac.webkit.org/changeset/49843>

[Eric Seidel (no email)]

Comment on attachment 41476 [details]
WebCore: Set EnabledAtRuntime for WebSocket in DOMWindow

Sorry about the commit-queue troubles.