Bug 29896 - Runtime configurable WebSocket code is detectable when turned off
Summary: Runtime configurable WebSocket code is detectable when turned off
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Fumitoshi Ukai
URL:
Keywords: Regression
Depends on:
Blocks: 30240
  Show dependency treegraph
 
Reported: 2009-09-29 14:08 PDT by Sam Weinig
Modified: 2009-10-20 12:11 PDT (History)
7 users (show)

See Also:


Attachments
WebCore: Enable WebSocket by default if WEB_SOCKETS is enabled at compile time. (7.38 KB, patch)
2009-10-09 00:38 PDT, Fumitoshi Ukai
no flags Details | Formatted Diff | Diff
Change WebSOcket EnableAtRuntime attribute in DOMWindow (8.60 KB, patch)
2009-10-15 23:54 PDT, Fumitoshi Ukai
no flags Details | Formatted Diff | Diff
WebSocket runtime configuration for chromium/v8 (23.63 KB, patch)
2009-10-16 07:03 PDT, Fumitoshi Ukai
no flags Details | Formatted Diff | Diff
WebCore: Set EnabledAtRuntime for WebSocket in DOMWindow (22.17 KB, patch)
2009-10-18 21:22 PDT, Fumitoshi Ukai
no flags Details | Formatted Diff | Diff
WebCore: Set EnabledAtRuntime for WebSocket in DOMWindow (23.21 KB, patch)
2009-10-19 20:07 PDT, Fumitoshi Ukai
levin: review+
eric: commit-queue+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sam Weinig 2009-09-29 14:08:54 PDT
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.
Comment 1 Mark Rowe (bdash) 2009-09-29 15:31:22 PDT
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?
Comment 2 Fumitoshi Ukai 2009-09-29 19:00:40 PDT
(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.
Comment 3 Fumitoshi Ukai 2009-09-29 19:04:24 PDT
(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.
Comment 4 Sam Weinig 2009-10-05 16:59:06 PDT
> 
> 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.
Comment 5 Jeremy Orlow 2009-10-05 20:37:45 PDT
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.
Comment 6 Fumitoshi Ukai 2009-10-09 00:38:33 PDT
Created attachment 40934 [details]
WebCore: Enable WebSocket by default if WEB_SOCKETS is enabled at compile time.
Comment 7 Jeremy Orlow 2009-10-09 00:49:55 PDT
fyi: a similar bug: https://bugs.webkit.org/show_bug.cgi?id=30240
Comment 8 Andrew Wilson 2009-10-09 09:21:10 PDT
(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().
Comment 9 Andrew Wilson 2009-10-09 09:38:30 PDT
(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.
Comment 10 Jeremy Orlow 2009-10-09 09:46:35 PDT
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.
Comment 11 David Levin 2009-10-15 10:07:08 PDT
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.
Comment 12 David Levin 2009-10-15 11:59:22 PDT
This patch http://trac.webkit.org/changeset/49643 will also have to be undone.
Comment 13 Fumitoshi Ukai 2009-10-15 23:54:55 PDT
Created attachment 41274 [details]
Change WebSOcket EnableAtRuntime attribute in DOMWindow
Comment 14 Fumitoshi Ukai 2009-10-16 07:03:37 PDT
Created attachment 41285 [details]
WebSocket runtime configuration for chromium/v8
Comment 15 Andrew Wilson 2009-10-16 09:56:15 PDT
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.
Comment 16 Fumitoshi Ukai 2009-10-18 21:22:10 PDT
Created attachment 41395 [details]
WebCore: Set EnabledAtRuntime for WebSocket in DOMWindow
Comment 17 Andrew Wilson 2009-10-19 10:50:43 PDT
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?
Comment 18 David Levin 2009-10-19 11:10:41 PDT
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.
Comment 19 Fumitoshi Ukai 2009-10-19 20:07:50 PDT
Created attachment 41476 [details]
WebCore: Set EnabledAtRuntime for WebSocket in DOMWindow
Comment 20 David Levin 2009-10-19 22:18:33 PDT
Comment on attachment 41476 [details]
WebCore: Set EnabledAtRuntime for WebSocket in DOMWindow

Thanks Ukai!
Comment 21 WebKit Commit Bot 2009-10-20 00:06:57 PDT
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.
Comment 22 Fumitoshi Ukai 2009-10-20 00:41:21 PDT
Committed r49843: <http://trac.webkit.org/changeset/49843>
Comment 23 Eric Seidel (no email) 2009-10-20 12:11:13 PDT
Comment on attachment 41476 [details]
WebCore: Set EnabledAtRuntime for WebSocket in DOMWindow

Sorry about the commit-queue troubles.