Bug 28037 – SocketStreamHandle interface for WebSocket API

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Fumitoshi Ukai]

SocketStreamHandle will be used for backend of WebSocket API.
These will be added in WebCore/platform/network

[Fumitoshi Ukai]

Created attachment 34545 [details]
Add SocketStreamHandle interface


---
 13 files changed, 736 insertions(+), 1 deletions(-)

[David Levin]

Comment on attachment 34545 [details]
Add SocketStreamHandle interface

Simply a style pass. I looked at this and saw a few style issues to clean up.

> diff --git a/WebCore/platform/network/SocketStreamErrorBase.h b/WebCore/platform/network/SocketStreamErrorBase.h
> +    class SocketStreamErrorBase {
> +      public:

public:, private:, protected: should be aligned with "class". There are multiple instances but I'm only commenting on this one.

> +        SocketStreamErrorBase()
> +                : m_errorCode(0)
> +                , m_isNull(true)

These should only be indented 4 spaces (not 8). There are multiple instances but I'm only commenting on this one.


> diff --git a/WebCore/platform/network/SocketStreamHandle.h b/WebCore/platform/network/SocketStreamHandle.h
> +        virtual int platformSend(const char*, int);

Please include parameter names for this method.
Parameter names are omitted when they don't add information.  In this case, they would add information. 


> diff --git a/WebCore/platform/network/SocketStreamHandleBase.cpp b/WebCore/platform/network/SocketStreamHandleBase.cpp
> +bool SocketStreamHandleBase::send(const char* buf, int size)
s/buf/buffer/
Use full words for variables.

> +    int num_written = 0;

s/num_written/bytesWritten/

bad casing.

> +void SocketStreamHandleBase::setClient(SocketStreamHandleClient* client)
> +{

Consider adding asserts here.
Can this be called in any state?
Can it be called after a client has already been set?


> +bool SocketStreamHandleBase::sendPendingData()
> +{
> +    if (m_state != Open)
> +        return false;
> +    if (m_buffer.isEmpty())
> +        return false;
> +    int num_written = platformSend(m_buffer.data(), m_buffer.size());

s/num_written/bytesWritten/

bad casing.

> +    Vector<char> remaining_data;

s/remaining_data/remainingData/

bad casing.

> +    remaining_data.append(m_buffer.data() + num_written,
> +                           m_buffer.size() - num_written);

Indentation mistake.


> diff --git a/WebCore/platform/network/SocketStreamHandleBase.h b/WebCore/platform/network/SocketStreamHandleBase.h
> +        virtual ~SocketStreamHandleBase() {}
s/{}/{ }/
Add a space in the {}


> +        bool send(const char*, int);

Please include parameter names for this method.

> +        virtual int platformSend(const char*, int) = 0;

Please include parameter names for this method.

> diff --git a/WebCore/platform/network/SocketStreamHandleClient.h b/WebCore/platform/network/SocketStreamHandleClient.h
> +        virtual void willSendData(SocketStreamHandle*, const char*, int) { }

Please include parameter names for the char* and int.

> +        virtual void didReceivedData(SocketStreamHandle*, const char*, int) { }

Please include parameter names for the char* and int.


> diff --git a/WebCore/platform/network/mac/SocketStreamError.h b/WebCore/platform/network/mac/SocketStreamError.h
> +        SocketStreamError() {}
s/{}/{ }/

> diff --git a/WebCore/platform/network/mac/SocketStreamHandleMac.mm b/WebCore/platform/network/mac/SocketStreamHandleMac.mm
> +    SocketStreamHandleInternal() {}
> +    ~SocketStreamHandleInternal() {}

s/{}/{ }/


> diff --git a/WebCore/platform/network/soup/SocketStreamHandleSoup.cpp b/WebCore/platform/network/soup/SocketStreamHandleSoup.cpp
> +class SocketStreamHandleInternal {
> +public:
> +    SocketStreamHandleInternal() {}
> +    ~SocketStreamHandleInternal() {}

s/{}/{ }/

[David Levin]

Comment on attachment 34545 [details]
Add SocketStreamHandle interface

Two more style comments:

> + /* static */
WebKit doesn't do this.

> + int SocketStreamHandle::platformSend(const char *, int)
* goes near the type

[Alexey Proskuryakov]

Comment on attachment 34545 [details]
Add SocketStreamHandle interface

> +    if (num_written < size)
> +        m_buffer.append(buf + num_written, size - num_written);

I think that we need a (large) upper limit on buffer size - it's too easy to send data faster than network permits, exhausting system memory and making the system unresponsive. Certainly, there are many other ways to exhaust memory via JavaScript, so it would be just a safety net, not a real security measure.

> +    remaining_data.append(m_buffer.data() + num_written,
> +                           m_buffer.size() - num_written);

As David pointed out, this is an indentation mistake - but there's no need to split the line anyway, it's quite short.

Looks good to me in general. I'm not sure if I'm not stepping on Dave's toes here, but r- to address the comments.

[David Levin]

>  I'm not sure if I'm not stepping on Dave's toes...
Not at all.  I simply noticed some styling issues and thought I'd point them out (in case he was able to get to them before the "real" review).  I should have made this more clear.

[Fumitoshi Ukai]

Created attachment 34645 [details]
Add SocketStreamHandle interface


---
 13 files changed, 748 insertions(+), 1 deletions(-)

[Fumitoshi Ukai]

Thanks for review.
I believe I fixed all comments.
Could you take a look again, please?
Thanks.

[Fumitoshi Ukai]

Created attachment 34646 [details]
Add SocketStreamHandle interface


---
 13 files changed, 748 insertions(+), 1 deletions(-)

[Alexey Proskuryakov]

Comment on attachment 34646 [details]
Add SocketStreamHandle interface

+        No new tests. (OOPS!)

Please don't say OOPS in ChangeLog - pre-commit script will block such a patch from landing. I think it would be better to say something positive anyway, like "tests will be landed once this code is complete and functional).

 	WebCore/bindings/js/JSWebSocketCustom.cpp \
 	WebCore/websockets/WebSocket.cpp \
-	WebCore/websockets/WebSocket.h
+	WebCore/websockets/WebSocket.h \
+	WebCore/platform/network/SocketStreamErrorBase.cpp \

Looks like this list is supposed to be sorted.

 			children = (
+				510D4A391031665F0049EA54 /* SocketStreamHandleMac.mm */,
+				510D4A2D103165EE0049EA54 /* SocketStreamErrorBase.cpp */,
+				510D4A2E103165EE0049EA54 /* SocketStreamErrorBase.h */,
+				510D4A2F103165EE0049EA54 /* SocketStreamHandle.h */,
+				510D4A30103165EE0049EA54 /* SocketStreamHandleBase.cpp */,
+				510D4A31103165EE0049EA54 /* SocketStreamHandleBase.h */,
+				510D4A32103165EE0049EA54 /* SocketStreamHandleClient.h */,
 				B2F34FE70E82F81700F627CD /* cf */,
 				656B84E70AEA1DAE00A095B4 /* mac */,

Shouldn't SocketStreamHandleMac be in mac folder? And sorry for not realizing this before, but I would actually prefer to have "cf" stubs for SocketStream, not mac ones.

+ * Copyright (C) 2009 Google Inc. All rights reserved.
<...>
+ * THIS SOFTWARE IS PROVIDED BY APPLE COMPUTER, INC. ``AS IS'' AND ANY
+ * EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE

I'm not sure what template is right for files originally added by Google, but "Apple Computer, Inc." is not even the correct name for Apple any more. You've previously used a different header in WebSocket.h.

+const int maxBufferAmount = 524288;

I've been thinking of a much larger buffer, something like 100MB (for desktop platforms at least). The idea is to prevent the browser from bringing the whole system to its knees too easily, not to close the connection as soon as it seems that data is sent too quickly.

"Buffer amount" doesn't sound like proper English to me - should be either "buffer size" or "maximum buffered amount".

+            // FIXME: set error code?

There is no error code in SocketStreamHandleBase, so I don't understand what this FIXME is about. Could you re-phrase the comment to make it clearer what the concern/bug is?

Perhaps an error should just be logged to console via a client didFail () callback.

+        int bufferedAmount() const { return m_buffer.size(); }
+
+        SocketStreamHandleClient* client() const;

It's surprising that one is inline, and the other is not. Not really a problem but was there a reason for this difference?

> diff --git a/WebCore/platform/network/SocketStreamHandle.h b/WebCore/platform/network/SocketStreamHandle.h

Why is SocketStreamHandle.h in cross-platform directory? The model that e.g. ResourceRequest.h uses seems superior to me - cross-platform parts go into Base, and platforms have both headers and cpp files of their own.

[Fumitoshi Ukai]

Created attachment 35115 [details]
Add SocketStreamHandle interface


---
 14 files changed, 870 insertions(+), 0 deletions(-)

[Fumitoshi Ukai]

Thanks for review. 
Sorry for late response since I was on vacation.
I've tried to address all of your comments.
Could you take a look again, please?

[Alexey Proskuryakov]

Comment on attachment 35115 [details]
Add SocketStreamHandle interface

> diff --git a/WebCore/platform/network/cf/SocketStreamHandle.h b/WebCore/platform/network/cf/SocketStreamHandle.h
<...>
> +        KURL m_url;
> +
> +        // platform specific data
> +        friend class SocketStreamHandleInternal;
> +        OwnPtr<SocketStreamHandleInternal> internal;

All data in this file is platform specific, cross-platform data goes to SocketStreamHandleBase. So, there is no need for SocketStreamHandleInternal any more.

Everything else looks good to me, but this is something that's best to fix before landing, so r-.

[Fumitoshi Ukai]

Created attachment 35185 [details]
Add SocketStreamHandle interface


---
 14 files changed, 844 insertions(+), 0 deletions(-)

[Alexey Proskuryakov]

Comment on attachment 35185 [details]
Add SocketStreamHandle interface

Bugzilla has experienced amnesia today, please upload the latest patch again.

[Fumitoshi Ukai]

Created attachment 38464 [details]
Add SocketStreamHandle interface


---
 14 files changed, 839 insertions(+), 0 deletions(-)

[Alexey Proskuryakov]

Comment on attachment 38464 [details]
Add SocketStreamHandle interface

r=me

[Eric Seidel (no email)]

Comment on attachment 38464 [details]
Add SocketStreamHandle interface

Fumitoshi doesn't have commit bit, so marking cq+

[Eric Seidel (no email)]

Comment on attachment 38464 [details]
Add SocketStreamHandle interface

Rejecting patch 38464 from commit-queue.  This patch will require manual commit.

WebKitTools/Scripts/build-webkit failed with exit code 1

[Eric Seidel (no email)]

Since this fails release build on the Mac, the patch will likely need more modification.

[Fumitoshi Ukai]

Created attachment 38520 [details]
Add SocketStreamHandle interface


---
 14 files changed, 839 insertions(+), 0 deletions(-)

[Fumitoshi Ukai]

(In reply to comment #19)
> Since this fails release build on the Mac, the patch will likely need more
> modification.

Fixed build failure on the Mac.
Comment unused parameters, and make bufferSize unsigned.

[Alexey Proskuryakov]

Comment on attachment 38520 [details]
Add SocketStreamHandle interface

r=me

[Fumitoshi Ukai]

(In reply to comment #22)
> (From update of attachment 38520 [details])
> r=me

Thanks for review.
Anyway, do I need to fix typo didReceived* here as well that you mentioned in bug 28038?

[Fumitoshi Ukai]

Created attachment 38527 [details]
Add SocketStreamHandle interface


---
 14 files changed, 839 insertions(+), 0 deletions(-)

[Eric Seidel (no email)]

Comment on attachment 38527 [details]
Add SocketStreamHandle interface

Rejecting patch 38527 from commit-queue.  This patch will require manual commit.

['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--quiet', '--exit-after-n-failures=1'] failed with exit code 1

[Eric Seidel (no email)]

media/video-played-ranges-1.html -> failed

is the failure.  I assume that it's just a flakey media test.  I'll add this back to the commit queue.

[Eric Seidel (no email)]

Comment on attachment 38527 [details]
Add SocketStreamHandle interface

Rejecting patch 38527 from commit-queue.  This patch will require manual commit.

Failed to run "['git', 'svn', 'dcommit']"  exit_code: 1  cwd: None

[Eric Seidel (no email)]

Comment on attachment 38527 [details]
Add SocketStreamHandle interface

Sigh.  commit-queue bug.
Merge conflict during commit: Your file or directory 'WebCore/ChangeLog' is probably out-of-date: resource out of date; try updating at /usr/local/libexec/git-core//git-svn line 469

I'll try again, if this fails I'll just do it manually.

[Eric Seidel (no email)]

Comment on attachment 38527 [details]
Add SocketStreamHandle interface

Rejecting patch 38527 from commit-queue.  This patch will require manual commit.

Patch https://bugs.webkit.org/attachment.cgi?id=38527 from bug 28037 failed to download and apply.

[Eric Seidel (no email)]

patching file WebCore/WebCore.xcodeproj/project.pbxproj
Hunk #1 succeeded at 893 (offset 61 lines).
Hunk #2 succeeded at 981 (offset 61 lines).
Hunk #3 succeeded at 6028 (offset 121 lines).
Hunk #4 succeeded at 6123 (offset 121 lines).
Hunk #5 succeeded at 10668 (offset 154 lines).
Hunk #6 succeeded at 13876 (offset 178 lines).
Hunk #7 FAILED at 17532.
Hunk #8 FAILED at 19605.
2 out of 8 hunks FAILED -- saving rejects to file WebCore/WebCore.xcodeproj/project.pbxproj.rej
patch -p0 "WebCore/WebCore.xcodeproj/project.pbxproj" returned 1.  Pass --force to ignore patch failures.

[Eric Seidel (no email)]

I attempted to land this manually.  However svn-apply bails out after the first failure, so it left only a partial patch applied.

I tried using "curl --location 'https://bugs.webkit.org/attachment.cgi?id=38527' | patch -p1" and that failed to apply the ChangeLogs it seems.

So I think we're left with you updating this patch.  Alternatively someone could sync to the revision you made the patch from, apply it there, and then update.

[Fumitoshi Ukai]

Created attachment 38588 [details]
Add SocketStreamHandle interface


---
 14 files changed, 839 insertions(+), 0 deletions(-)

[Fumitoshi Ukai]

(In reply to comment #31)
> I attempted to land this manually.  However svn-apply bails out after the first
> failure, so it left only a partial patch applied.
> 
> I tried using "curl --location
> 'https://bugs.webkit.org/attachment.cgi?id=38527' | patch -p1" and that failed
> to apply the ChangeLogs it seems.
> 
> So I think we're left with you updating this patch.  Alternatively someone
> could sync to the revision you made the patch from, apply it there, and then
> update.

Update the patch from revision 47769.

[Alexey Proskuryakov]

Comment on attachment 38588 [details]
Add SocketStreamHandle interface

> I attempted to land this manually.  However svn-apply bails out after the first
> failure, so it left only a partial patch applied.

Is this a recent regression? Sounds like a serious problem to me.

[Eric Seidel (no email)]

Comment on attachment 38588 [details]
Add SocketStreamHandle interface

Rejecting patch 38588 from commit-queue.  This patch will require manual commit.

['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--quiet', '--exit-after-n-failures=1'] failed with exit code 1

[Eric Seidel (no email)]

Comment on attachment 38588 [details]
Add SocketStreamHandle interface

accessibility/nochildren-elements.html -> crashed

Seems unrelated.  Maybe it's a flakey test.

[Eric Seidel (no email)]

Comment on attachment 38588 [details]
Add SocketStreamHandle interface

Clearing flags on attachment: 38588

Committed r47788: <http://trac.webkit.org/changeset/47788>

[Eric Seidel (no email)]

All reviewed patches have been landed.  Closing bug.

[Eric Seidel (no email)]

(In reply to comment #26)
> media/video-played-ranges-1.html -> failed
> 
> is the failure.  I assume that it's just a flakey media test.  I'll add this
> back to the commit queue.

This was likely one of the first instances of bug 28845.