Add secure sockets to dart:io

Add a TlsSocket class to dart:io, that implements secure client sockets using TLS (SSL).

This uses the NSS (Network Security Services) libraries from Mozilla, which is a cross-platform library for SSL and TLS sockets.  We use the patched version from the Chromium repository, which is checked into third_party by the DEPS file.

Committed: https://code.google.com/p/dart/source/detail?r=14893

[Bill Hesse, 2012-09-27 13:39:21]

No hurry - this is working, but not cleaned up yet.  Feel free to comment,
though.

[Søren Gjesse, 2012-09-28 09:17:30]

Some initial build related comments.

http://codereview.chromium.org/10916081/diff/17001/runtime/bin/bin.gypi
File runtime/bin/bin.gypi (right):

http://codereview.chromium.org/10916081/diff/17001/runtime/bin/bin.gypi#newcode7
runtime/bin/bin.gypi:7: 'use_chromium_nss': 1,
Please add a comment here on the different NSSes that can be used.

http://codereview.chromium.org/10916081/diff/17001/runtime/bin/bin.gypi#newco...
runtime/bin/bin.gypi:233: '/usr/include/nspr',
Shouldn't the /usr/include/... paths only be there if use_chromium_nss is 0?

http://codereview.chromium.org/10916081/diff/17001/runtime/bin/bin.gypi#newco...
runtime/bin/bin.gypi:235: '../nspr',
Shouldn't the ../... paths be ../third_party/...?

http://codereview.chromium.org/10916081/diff/17001/runtime/bin/bin.gypi#newco...
runtime/bin/bin.gypi:254: 'NSS_USE_STATIC_LIBS=1',
Should this define go with the rest of the NSS configuration?

http://codereview.chromium.org/10916081/diff/17001/runtime/bin/bin.gypi#newco...
runtime/bin/bin.gypi:291: # Should be NSS Distribution include directory.
Should we use a GYP variable to specify the NSS distribution location? That goes
for all platforms.

http://codereview.chromium.org/10916081/diff/17001/runtime/bin/dartutils.cc
File runtime/bin/dartutils.cc (right):

http://codereview.chromium.org/10916081/diff/17001/runtime/bin/dartutils.cc#n...
runtime/bin/dartutils.cc:527: "IllegalArgumentException",
This is now ArgumentError.

http://codereview.chromium.org/10916081/diff/17001/runtime/bin/net/nss_memio.h
File runtime/bin/net/nss_memio.h (right):

http://codereview.chromium.org/10916081/diff/17001/runtime/bin/net/nss_memio....
runtime/bin/net/nss_memio.h:1: // Copyright (c) 2011 The Chromium Authors. All
rights reserved.
Why do we need to add this? Is it not part of NSS?

http://codereview.chromium.org/10916081/diff/17001/runtime/bin/tls_socket.cc
File runtime/bin/tls_socket.cc (right):

http://codereview.chromium.org/10916081/diff/17001/runtime/bin/tls_socket.cc#...
runtime/bin/tls_socket.cc:207: #ifdef CHROMIUM_NSS
We should avoid #ifdefs like this. How about having platform_nss_chromium.cc and
platform_nss_mozilla.cc to handle the differences and having GYP handle which to
use?

http://codereview.chromium.org/10916081/diff/17001/tools/gyp/configurations_x...
File tools/gyp/configurations_xcode.gypi (right):

http://codereview.chromium.org/10916081/diff/17001/tools/gyp/configurations_x...
tools/gyp/configurations_xcode.gypi:21: # 'GCC_VERSION': '4.2',
Why these changes? If compiling NSS require other Xcode flags these should be
set only for compiling NSS.

http://codereview.chromium.org/10916081/diff/17001/tools/gyp/xcode.gypi
File tools/gyp/xcode.gypi (right):

http://codereview.chromium.org/10916081/diff/17001/tools/gyp/xcode.gypi#newco...
tools/gyp/xcode.gypi:21: # Mountain Lion is 10.7.
Is there an issue with supporting 10.5?

[Mads Ager (google), 2012-09-28 12:03:56]

I couple of initial C++ cleanup comments. Ping on the issue again when this has
been cleaned up some more and I will give it a more thorough review.

http://codereview.chromium.org/10916081/diff/17001/runtime/bin/dartutils.cc
File runtime/bin/dartutils.cc (right):

http://codereview.chromium.org/10916081/diff/17001/runtime/bin/dartutils.cc#n...
runtime/bin/dartutils.cc:492: return
Dart_GetClass(Dart_LookupLibrary(Dart_NewString(library_url)),
What is the state of the current Dart API. I forget - does it always return an
error handle when one of the arguments is an error handle? If now, we might have
to split this out:

Dart_Handle lib = Dart_LookupLibrary(Dart_NewString(library_url));
if (Dart_IsError(lib)) return lib;
return Dart_GetClass(lib, Dart_NewString(class_name));

http://codereview.chromium.org/10916081/diff/17001/runtime/bin/tls_socket.cc
File runtime/bin/tls_socket.cc (right):

http://codereview.chromium.org/10916081/diff/17001/runtime/bin/tls_socket.cc#...
runtime/bin/tls_socket.cc:32: static TlsFilter* NativePeer(Dart_NativeArguments
args) {
Maybe call this GetTlsFiler?

http://codereview.chromium.org/10916081/diff/17001/runtime/bin/tls_socket.cc#...
runtime/bin/tls_socket.cc:37: HandleError(Dart_GetNativeInstanceField(dart_this,
0,
Introduce a named constant for the 0. 

static const int kTlsFilterNativeField = 0;

maybe?

http://codereview.chromium.org/10916081/diff/17001/runtime/bin/tls_socket.cc#...
runtime/bin/tls_socket.cc:43: static void SetNativePeer(Dart_NativeArguments
args, TlsFilter* native_peer) {
SetTlsFilter?

http://codereview.chromium.org/10916081/diff/17001/runtime/bin/tls_socket.cc#...
runtime/bin/tls_socket.cc:44: Dart_Handle dart_this =
HandleError(Dart_GetNativeArgument(args, 0));
Ditto on HandleError.

http://codereview.chromium.org/10916081/diff/17001/runtime/bin/tls_socket.cc#...
runtime/bin/tls_socket.cc:53: TlsFilter* native_peer = new TlsFilter;
native_peer -> filter?

http://codereview.chromium.org/10916081/diff/17001/runtime/bin/tls_socket.cc#...
runtime/bin/tls_socket.cc:62: Dart_SetReturnValue(args, Dart_Null());
This is the default behavior so you do not need this line. Similarly for the
methods below, you can drop the SetReturnValue when it is with Dart_Null().

http://codereview.chromium.org/10916081/diff/17001/runtime/bin/tls_socket.cc#...
runtime/bin/tls_socket.cc:153: Dart_Handle tlsExternalBuffer_class =
HandleError(
Please use C++ naming convention: tls_external_buffer_class

http://codereview.chromium.org/10916081/diff/17001/runtime/bin/tls_socket.cc#...
runtime/bin/tls_socket.cc:206: printf("%s\n", pkcert_database);
Please remove debug printing.

http://codereview.chromium.org/10916081/diff/17001/runtime/bin/tls_socket.cc#...
runtime/bin/tls_socket.cc:207: #ifdef CHROMIUM_NSS
On 2012/09/28 09:17:31, Søren Gjesse wrote:
> We should avoid #ifdefs like this. How about having platform_nss_chromium.cc
and
> platform_nss_mozilla.cc to handle the differences and having GYP handle which
to
> use?

I'm not convinced that our build should have to support both. We are going to
build NSS ourselves and we know which version we are using. Just remove the
#ifdef?

http://codereview.chromium.org/10916081/diff/17001/runtime/bin/tls_socket.cc#...
runtime/bin/tls_socket.cc:216: printf("Successful NSS_Init call\n");
Please remove debug printing. You should probably report back somehow that the
initialization failed here and deal with it by throwing a dart exception? So in
the failure cases, maybe you need to UnlockInitMutex and return false? Maybe it
would be a good idea to create a scoped object: LockInitMutexScope. That will
make sure that you unlock correctly.

http://codereview.chromium.org/10916081/diff/17001/runtime/bin/tls_socket.cc#...
runtime/bin/tls_socket.cc:239: data_->memio = memio_CreateIOLayer(30000);
Maybe we can create a named constant for this value. Is this a size in bytes or
something?

http://codereview.chromium.org/10916081/diff/17001/runtime/bin/tls_socket.cc#...
runtime/bin/tls_socket.cc:248: bool SECURITY = true;
Please remove.

http://codereview.chromium.org/10916081/diff/17001/runtime/bin/tls_socket.cc#...
runtime/bin/tls_socket.cc:264: printf("SSL_ImportFD failed.\n");
Similarly here. We should report this error back to the user.

http://codereview.chromium.org/10916081/diff/17001/runtime/bin/tls_socket.cc#...
runtime/bin/tls_socket.cc:281: #ifdef CHROMIUM_NSS
Since we always use CHROMIUM_NSS we can get rid of this #ifdef.

http://codereview.chromium.org/10916081/diff/17001/runtime/bin/tls_socket.cc#...
runtime/bin/tls_socket.cc:358: // TODO(whesse): Destroy OpenSSL objects here.
OpenSSL?

http://codereview.chromium.org/10916081/diff/17001/runtime/bin/tls_socket.dart
File runtime/bin/tls_socket.dart (right):

http://codereview.chromium.org/10916081/diff/17001/runtime/bin/tls_socket.dar...
runtime/bin/tls_socket.dart:5: abstract class TlsSocket implements Socket,
Hashable {
Hashable is gone when you rebase. You should just remove that from the
implements list.

http://codereview.chromium.org/10916081/diff/17001/runtime/bin/tls_socket.h
File runtime/bin/tls_socket.h (right):

http://codereview.chromium.org/10916081/diff/17001/runtime/bin/tls_socket.h#n...
runtime/bin/tls_socket.h:18: static Dart_Handle HandleError(Dart_Handle handle)
{
As you know, I'm on the fence on this one. If we want it we should put it in
dart_utils and use it consistently throughout. It might be a good idea to do so.
Currently, we have asserts all over that !Dart_IsError(something) where the
right thing would be to propagate the error.

Could you move this to dart_utils.h and file a bug report that propagation of
errors are missing for most of the dart:io C++ code.

Maybe we can call this something that more closely matches the API function it
is using: PropagateError / PropagateErrorIfNeeded / PropagateIfError? Not sure.

http://codereview.chromium.org/10916081/diff/17001/runtime/bin/tls_socket_imp...
File runtime/bin/tls_socket_impl.dart (right):

http://codereview.chromium.org/10916081/diff/17001/runtime/bin/tls_socket_imp...
runtime/bin/tls_socket_impl.dart:22: _TlsSocket(String host,
I would move host and port onto the same line here.

http://codereview.chromium.org/10916081/diff/17001/tests/standalone/io/tls_so...
File tests/standalone/io/tls_socket_test.dart (right):

http://codereview.chromium.org/10916081/diff/17001/tests/standalone/io/tls_so...
tests/standalone/io/tls_socket_test.dart:43: void foo1() {}
Remove these.

http://codereview.chromium.org/10916081/diff/17001/tools/gyp/configurations_x...
File tools/gyp/configurations_xcode.gypi (right):

http://codereview.chromium.org/10916081/diff/17001/tools/gyp/configurations_x...
tools/gyp/configurations_xcode.gypi:21: # 'GCC_VERSION': '4.2',
On 2012/09/28 09:17:31, Søren Gjesse wrote:
> Why these changes? If compiling NSS require other Xcode flags these should be
> set only for compiling NSS.

My guess: Bill probably updated to xcode 4?

http://codereview.chromium.org/10916081/diff/17001/tools/gyp/xcode.gypi
File tools/gyp/xcode.gypi (right):

http://codereview.chromium.org/10916081/diff/17001/tools/gyp/xcode.gypi#newco...
tools/gyp/xcode.gypi:21: # Mountain Lion is 10.7.
On 2012/09/28 09:17:31, Søren Gjesse wrote:
> Is there an issue with supporting 10.5?

Similarly, Bill probably updated to xcode 4. Bill, you should make sure to
revert these changes before commit to not break everyone else.

[Bill Hesse, 2012-10-31 16:33:28]

Comments addressed, much cleanup and refactoring done.  Most of TlsSocket's
implementation moved to lib/io.  Ready for review.

http://codereview.chromium.org/10916081/diff/17001/runtime/bin/bin.gypi
File runtime/bin/bin.gypi (right):

http://codereview.chromium.org/10916081/diff/17001/runtime/bin/bin.gypi#newco...
runtime/bin/bin.gypi:235: '../nspr',
All removed.  Replaced by direct-dependent includes in nss.gyp.

On 2012/09/28 09:17:31, Søren Gjesse wrote:
> Shouldn't the ../... paths be ../third_party/...?

http://codereview.chromium.org/10916081/diff/17001/runtime/bin/bin.gypi#newco...
runtime/bin/bin.gypi:254: 'NSS_USE_STATIC_LIBS=1',
Yes.  Done.

On 2012/09/28 09:17:31, Søren Gjesse wrote:
> Should this define go with the rest of the NSS configuration?

http://codereview.chromium.org/10916081/diff/17001/runtime/bin/bin.gypi#newco...
runtime/bin/bin.gypi:291: # Should be NSS Distribution include directory.
Removed.

On 2012/09/28 09:17:31, Søren Gjesse wrote:
> Should we use a GYP variable to specify the NSS distribution location? That
goes
> for all platforms.

http://codereview.chromium.org/10916081/diff/17001/runtime/bin/dartutils.cc
File runtime/bin/dartutils.cc (right):

http://codereview.chromium.org/10916081/diff/17001/runtime/bin/dartutils.cc#n...
runtime/bin/dartutils.cc:492: return
Dart_GetClass(Dart_LookupLibrary(Dart_NewString(library_url)),
The Dart API has been fixed to always return the error handle that is passed in
as an argument, if the return value is a handle, exactly so this sort of code
can be written.

On 2012/09/28 12:03:56, Mads Ager wrote:
> What is the state of the current Dart API. I forget - does it always return an
> error handle when one of the arguments is an error handle? If now, we might
have
> to split this out:
> 
> Dart_Handle lib = Dart_LookupLibrary(Dart_NewString(library_url));
> if (Dart_IsError(lib)) return lib;
> return Dart_GetClass(lib, Dart_NewString(class_name));

http://codereview.chromium.org/10916081/diff/17001/runtime/bin/dartutils.cc#n...
runtime/bin/dartutils.cc:527: "IllegalArgumentException",
Fixed.

On 2012/09/28 09:17:31, Søren Gjesse wrote:
> This is now ArgumentError.

http://codereview.chromium.org/10916081/diff/17001/runtime/bin/net/nss_memio.h
File runtime/bin/net/nss_memio.h (right):

http://codereview.chromium.org/10916081/diff/17001/runtime/bin/net/nss_memio....
runtime/bin/net/nss_memio.h:1: // Copyright (c) 2011 The Chromium Authors. All
rights reserved.
This is a pure Chromium file, not part of NSS, and not part of the
third_party/nss directories.

Should it be placed in a third_party/chromium directory, since it has a
different license than the pure Dart files?  Just the authors are different.


On 2012/09/28 09:17:31, Søren Gjesse wrote:
> Why do we need to add this? Is it not part of NSS?

http://codereview.chromium.org/10916081/diff/17001/runtime/bin/tls_socket.cc
File runtime/bin/tls_socket.cc (right):

http://codereview.chromium.org/10916081/diff/17001/runtime/bin/tls_socket.cc#...
runtime/bin/tls_socket.cc:32: static TlsFilter* NativePeer(Dart_NativeArguments
args) {
On 2012/09/28 12:03:56, Mads Ager wrote:
> Maybe call this GetTlsFiler?

Done.

http://codereview.chromium.org/10916081/diff/17001/runtime/bin/tls_socket.cc#...
runtime/bin/tls_socket.cc:37: HandleError(Dart_GetNativeInstanceField(dart_this,
0,
On 2012/09/28 12:03:56, Mads Ager wrote:
> Introduce a named constant for the 0. 
> 
> static const int kTlsFilterNativeField = 0;
> 
> maybe?

Done.

http://codereview.chromium.org/10916081/diff/17001/runtime/bin/tls_socket.cc#...
runtime/bin/tls_socket.cc:43: static void SetNativePeer(Dart_NativeArguments
args, TlsFilter* native_peer) {
On 2012/09/28 12:03:56, Mads Ager wrote:
> SetTlsFilter?

Done.

http://codereview.chromium.org/10916081/diff/17001/runtime/bin/tls_socket.cc#...
runtime/bin/tls_socket.cc:53: TlsFilter* native_peer = new TlsFilter;
On 2012/09/28 12:03:57, Mads Ager wrote:
> native_peer -> filter?

Done.

http://codereview.chromium.org/10916081/diff/17001/runtime/bin/tls_socket.cc#...
runtime/bin/tls_socket.cc:62: Dart_SetReturnValue(args, Dart_Null());
Removed everywhere.

On 2012/09/28 12:03:57, Mads Ager wrote:
> This is the default behavior so you do not need this line. Similarly for the
> methods below, you can drop the SetReturnValue when it is with Dart_Null().

http://codereview.chromium.org/10916081/diff/17001/runtime/bin/tls_socket.cc#...
runtime/bin/tls_socket.cc:153: Dart_Handle tlsExternalBuffer_class =
HandleError(
On 2012/09/28 12:03:57, Mads Ager wrote:
> Please use C++ naming convention: tls_external_buffer_class

Done.

http://codereview.chromium.org/10916081/diff/17001/runtime/bin/tls_socket.cc#...
runtime/bin/tls_socket.cc:207: #ifdef CHROMIUM_NSS
All #ifdefs removed, only Chromium NSS supported, read-write bug is fixed.

On 2012/09/28 12:03:57, Mads Ager wrote:
> On 2012/09/28 09:17:31, Søren Gjesse wrote:
> > We should avoid #ifdefs like this. How about having platform_nss_chromium.cc
> and
> > platform_nss_mozilla.cc to handle the differences and having GYP handle
which
> to
> > use?
> 
> I'm not convinced that our build should have to support both. We are going to
> build NSS ourselves and we know which version we are using. Just remove the
> #ifdef?

http://codereview.chromium.org/10916081/diff/17001/runtime/bin/tls_socket.h
File runtime/bin/tls_socket.h (right):

http://codereview.chromium.org/10916081/diff/17001/runtime/bin/tls_socket.h#n...
runtime/bin/tls_socket.h:18: static Dart_Handle HandleError(Dart_Handle handle)
{
Done.

On 2012/09/28 12:03:57, Mads Ager wrote:
> As you know, I'm on the fence on this one. If we want it we should put it in
> dart_utils and use it consistently throughout. It might be a good idea to do
so.
> Currently, we have asserts all over that !Dart_IsError(something) where the
> right thing would be to propagate the error.
> 
> Could you move this to dart_utils.h and file a bug report that propagation of
> errors are missing for most of the dart:io C++ code.
> 
> Maybe we can call this something that more closely matches the API function it
> is using: PropagateError / PropagateErrorIfNeeded / PropagateIfError? Not
sure.
>

http://codereview.chromium.org/10916081/diff/17001/runtime/bin/tls_socket_imp...
File runtime/bin/tls_socket_impl.dart (right):

http://codereview.chromium.org/10916081/diff/17001/runtime/bin/tls_socket_imp...
runtime/bin/tls_socket_impl.dart:22: _TlsSocket(String host,
On 2012/09/28 12:03:57, Mads Ager wrote:
> I would move host and port onto the same line here.

Done.

http://codereview.chromium.org/10916081/diff/17001/tests/standalone/io/tls_so...
File tests/standalone/io/tls_socket_test.dart (right):

http://codereview.chromium.org/10916081/diff/17001/tests/standalone/io/tls_so...
tests/standalone/io/tls_socket_test.dart:43: void foo1() {}
On 2012/09/28 12:03:57, Mads Ager wrote:
> Remove these.

Done.

http://codereview.chromium.org/10916081/diff/17001/tools/gyp/configurations_x...
File tools/gyp/configurations_xcode.gypi (right):

http://codereview.chromium.org/10916081/diff/17001/tools/gyp/configurations_x...
tools/gyp/configurations_xcode.gypi:21: # 'GCC_VERSION': '4.2',
Removed.

On 2012/09/28 09:17:31, Søren Gjesse wrote:
> Why these changes? If compiling NSS require other Xcode flags these should be
> set only for compiling NSS.

http://codereview.chromium.org/10916081/diff/17001/tools/gyp/xcode.gypi
File tools/gyp/xcode.gypi (right):

http://codereview.chromium.org/10916081/diff/17001/tools/gyp/xcode.gypi#newco...
tools/gyp/xcode.gypi:21: # Mountain Lion is 10.7.
Removed.

On 2012/09/28 09:17:31, Søren Gjesse wrote:
> Is there an issue with supporting 10.5?

http://codereview.chromium.org/10916081/diff/37001/runtime/bin/dartutils.cc
File runtime/bin/dartutils.cc (right):

http://codereview.chromium.org/10916081/diff/37001/runtime/bin/dartutils.cc#n...
runtime/bin/dartutils.cc:520: Dart_Handle
DartUtils::NewDartIllegalArgumentException(const char* message) {
Changed to ArgumentError

http://codereview.chromium.org/10916081/diff/37001/runtime/bin/dartutils.h
File runtime/bin/dartutils.h (right):

http://codereview.chromium.org/10916081/diff/37001/runtime/bin/dartutils.h#ne...
runtime/bin/dartutils.h:117: static Dart_Handle
NewDartIllegalArgumentException(const char* message);
Changed to ArgumentError

http://codereview.chromium.org/10916081/diff/39001/runtime/bin/bin.gypi
File runtime/bin/bin.gypi (right):

http://codereview.chromium.org/10916081/diff/39001/runtime/bin/bin.gypi#newco...
runtime/bin/bin.gypi:267: 'bin/net/ssl.gyp:libssl',
Changed to "net" target in bin.gypi.

http://codereview.chromium.org/10916081/diff/39001/runtime/bin/bin.gypi#newco...
runtime/bin/bin.gypi:285: 'NSS_USE_STATIC_LIBS=1',
Move to nss.gyp (in)direct dependent settings?  Because nss.gyp is included in
net, which is included in libdart_builtin, it is not directly dependent.

http://codereview.chromium.org/10916081/diff/39001/runtime/bin/bin.gypi#newco...
runtime/bin/bin.gypi:362: 'libraries': [ '-lws2_32.lib', '-lRpcrt4.lib',
'-lwinmm.lib' ],
Move to nss.gyp direct dependent settings?

http://codereview.chromium.org/10916081/diff/39001/runtime/bin/dartutils.cc
File runtime/bin/dartutils.cc (right):

http://codereview.chromium.org/10916081/diff/39001/runtime/bin/dartutils.cc#n...
runtime/bin/dartutils.cc:521: Dart_Handle
DartUtils::NewDartIllegalArgumentException(const char* message) {
Changed to ArgumentError.

[Anders Johnsen, 2012-11-01 07:27:21]

There is quite a bit of debug info there still. First very small round of
comments, but let me know when you have the debug info removed :)

http://codereview.chromium.org/10916081/diff/43003/lib/io/tls_socket.dart
File lib/io/tls_socket.dart (right):

http://codereview.chromium.org/10916081/diff/43003/lib/io/tls_socket.dart#new...
lib/io/tls_socket.dart:22: class _TlsSocket implements TlsSocket {
A higher level question: Would it make sense to expose the TlsFilter as a filter
anyone can use to 'secure' any sequence of data?

http://codereview.chromium.org/10916081/diff/43003/lib/io/tls_socket.dart#new...
lib/io/tls_socket.dart:225: print("Entering readEncryptedData");
Debug info.

http://codereview.chromium.org/10916081/diff/43003/lib/io/tls_socket.dart#new...
lib/io/tls_socket.dart:228: print("    buffer.length: ${buffer.length}");
Debug info.

http://codereview.chromium.org/10916081/diff/43003/lib/io/tls_socket.dart#new...
lib/io/tls_socket.dart:240: print("    bytes read from socket: $bytes");
Debug info.

http://codereview.chromium.org/10916081/diff/43003/lib/io/tls_socket.dart#new...
lib/io/tls_socket.dart:253: print("top of while loop: buffer.length =
${buffer.length}");
Debug info

http://codereview.chromium.org/10916081/diff/43003/lib/io/tls_socket.dart#new...
lib/io/tls_socket.dart:263: print("foo writeEncrypted processBuffer returned
$bytes");
Debug info.

http://codereview.chromium.org/10916081/diff/43003/lib/io/tls_socket.dart#new...
lib/io/tls_socket.dart:299: int get free() => kSize - start - length;
remove ().

http://codereview.chromium.org/10916081/diff/43003/lib/io/tls_socket.dart#new...
lib/io/tls_socket.dart:299: int get free() => kSize - start - length;
Please add (...) to explicit express precedence for better readability (and to
express the intent of this method).

http://codereview.chromium.org/10916081/diff/43003/lib/io/tls_socket.dart#new...
lib/io/tls_socket.dart:315: class _TlsFilter extends NativeFieldWrapperClass1 {
AFAIK, this is VM specific, I think this should be in runtime/bin/

[Mads Ager (google), 2012-11-01 10:09:01]

First round of comments.

http://codereview.chromium.org/10916081/diff/43003/lib/io/tls_socket.dart
File lib/io/tls_socket.dart (right):

http://codereview.chromium.org/10916081/diff/43003/lib/io/tls_socket.dart#new...
lib/io/tls_socket.dart:23: static final int _BUFFER_SIZE = 2048;
The name here is using an '_' in from of it and none of the others are. Users
cannot access these anyway because they are static and you cannot access
_TlsSocket. It should therefore be safe to remove the '_' here.

http://codereview.chromium.org/10916081/diff/43003/lib/io/tls_socket.dart#new...
lib/io/tls_socket.dart:32: static final int kReadPlaintext = 0;
This is using a different naming style than the other constants.

http://codereview.chromium.org/10916081/diff/43003/lib/io/tls_socket.dart#new...
lib/io/tls_socket.dart:149: print("Entering readList");
Debug printing.

http://codereview.chromium.org/10916081/diff/43003/runtime/bin/tls_socket.cc
File runtime/bin/tls_socket.cc (right):

http://codereview.chromium.org/10916081/diff/43003/runtime/bin/tls_socket.cc#...
runtime/bin/tls_socket.cc:36:
ThrowIfError(Dart_GetNativeInstanceField(dart_this,
Please move the first argument down:

LongLineThatWithAMethodCall(
    arg0,
    arg1);

or 

LongLineThatWithAMethodCall(arg0,
                            arg1);

http://codereview.chromium.org/10916081/diff/43003/runtime/bin/tls_socket.cc#...
runtime/bin/tls_socket.cc:205: NSS_Init(pkcert_database);
This will fit on one line.

http://codereview.chromium.org/10916081/diff/43003/runtime/bin/tls_socket.cc#...
runtime/bin/tls_socket.cc:215: ReportError("Called TlsFilter::InitializeLibrary
more than once", 0);
Indentation.

http://codereview.chromium.org/10916081/diff/43003/runtime/bin/tls_socket.cc#...
runtime/bin/tls_socket.cc:237: TlsLog("Entering InitSocket");
InitSocket?

http://codereview.chromium.org/10916081/diff/43003/runtime/bin/tls_socket.cc#...
runtime/bin/tls_socket.cc:239: my_socket = data_->memio;
Indentation is off here.

http://codereview.chromium.org/10916081/diff/43003/runtime/bin/tls_socket.cc#...
runtime/bin/tls_socket.cc:246: if (SSL_SetURL(my_socket, "www.google.dk") == -1)
{
What is this hardcoded www.google.dk stuff?

http://codereview.chromium.org/10916081/diff/43003/runtime/bin/tls_socket.cc#...
runtime/bin/tls_socket.cc:259: PRStatus rv = PR_GetHostByName("www.google.dk",
host_entry_buffer,
Here as well???

http://codereview.chromium.org/10916081/diff/43003/runtime/bin/tls_socket.cc#...
runtime/bin/tls_socket.cc:288: TlsLogInt("Unsuccessful SSL_ForceHandshake call",
PR_GetError());
This needs handling of unsuccessful handshakes when in_handsake_. At least put
in TODOs for the parts that are missing. Currently we are only handling
successful connections and have no handling of unauthorized ones?

http://codereview.chromium.org/10916081/diff/43003/runtime/bin/tls_socket.cc#...
runtime/bin/tls_socket.cc:291: // int writable = ProcessBuffer(kWriteEncrypted);
Code in comments.

http://codereview.chromium.org/10916081/diff/43003/runtime/bin/tls_socket.cc#...
runtime/bin/tls_socket.cc:302: // TODO(whesse): Destroy OpenSSL objects here.
OpenSSL?

http://codereview.chromium.org/10916081/diff/43003/runtime/bin/tls_socket.cc#...
runtime/bin/tls_socket.cc:367: // NEW
Remove comment.

http://codereview.chromium.org/10916081/diff/43003/runtime/bin/tls_socket.h
File runtime/bin/tls_socket.h (right):

http://codereview.chromium.org/10916081/diff/43003/runtime/bin/tls_socket.h#n...
runtime/bin/tls_socket.h:20: // TODO(): Throw SocketIOException here.  The
error_code can go in its
Fix this.

http://codereview.chromium.org/10916081/diff/43003/runtime/bin/tls_socket.h#n...
runtime/bin/tls_socket.h:60: //  static const char* bufferNames_[kNumBuffers];
Code in comment.

http://codereview.chromium.org/10916081/diff/43003/tests/standalone/io/tls_so...
File tests/standalone/io/tls_socket_test.dart (right):

http://codereview.chromium.org/10916081/diff/43003/tests/standalone/io/tls_so...
tests/standalone/io/tls_socket_test.dart:15: new Path.fromNative(new
Options().script).directoryPath.append('pkcert/');
I do not see a pkcert directory in this change?

We should have a test where we use a certificate database based on which we
cannot authorize the request as well. I don't see any handling of that in the
current APIs. Is that ignored in this first changelist?

[Søren Gjesse, 2012-11-01 11:49:11]

https://codereview.chromium.org/10916081/diff/43001/runtime/bin/dartutils.cc
File runtime/bin/dartutils.cc (right):

https://codereview.chromium.org/10916081/diff/43001/runtime/bin/dartutils.cc#...
runtime/bin/dartutils.cc:488: return
Dart_GetClass(Dart_LookupLibrary(Dart_NewString(library_url)),
Does this work correctly with error handles if the library is not found?

https://codereview.chromium.org/10916081/diff/43001/runtime/bin/tls_socket.cc
File runtime/bin/tls_socket.cc (right):

https://codereview.chromium.org/10916081/diff/43001/runtime/bin/tls_socket.cc...
runtime/bin/tls_socket.cc:52: void
FUNCTION_NAME(TlsSocket_Init)(Dart_NativeArguments args) {
Maybe assert that filter is not already set.

https://codereview.chromium.org/10916081/diff/43001/runtime/bin/tls_socket.cc...
runtime/bin/tls_socket.cc:269: if (host_address.inet.family == PR_AF_INET) {
Debug code.

https://codereview.chromium.org/10916081/diff/43001/runtime/bin/tls_socket.cc...
runtime/bin/tls_socket.cc:276: }
Indentation.

https://codereview.chromium.org/10916081/diff/43001/runtime/bin/tls_socket.cc...
runtime/bin/tls_socket.cc:281: TlsLog("Successful SSL_ForceHandshake call");
Please remove logging.

https://codereview.chromium.org/10916081/diff/43001/runtime/bin/tls_socket.cc...
runtime/bin/tls_socket.cc:291: // int writable = ProcessBuffer(kWriteEncrypted);
Code in comments.

https://codereview.chromium.org/10916081/diff/43001/runtime/bin/tls_socket.cc...
runtime/bin/tls_socket.cc:312: int64_t unsafe_start =
DartUtils::GetIntegerValue(start_object);
Maybe change the type to intptr_t here and do the cast here.

https://codereview.chromium.org/10916081/diff/43001/runtime/bin/tls_socket.cc...
runtime/bin/tls_socket.cc:322: int bytes_sent = 0;
Maybe rename bytes_sent to bytes_processed, as this can both read and write.
Also nothing is actually sent in this method.

https://codereview.chromium.org/10916081/diff/43001/runtime/bin/tls_socket.cc...
runtime/bin/tls_socket.cc:367: // NEW
Strange comment.

https://codereview.chromium.org/10916081/diff/43001/runtime/bin/tls_socket.cc...
runtime/bin/tls_socket.cc:391: if (bytes_sent < 0) bytes_sent = 0;
Negative bytes_sent here is not an error?

https://codereview.chromium.org/10916081/diff/43001/runtime/bin/tls_socket.h
File runtime/bin/tls_socket.h (right):

https://codereview.chromium.org/10916081/diff/43001/runtime/bin/tls_socket.h#...
runtime/bin/tls_socket.h:30: static const bool kTlsLogging = true;
This logging should be turned off before committing.

https://codereview.chromium.org/10916081/diff/43001/runtime/bin/tls_socket_im...
File runtime/bin/tls_socket_impl.dart (right):

https://codereview.chromium.org/10916081/diff/43001/runtime/bin/tls_socket_im...
runtime/bin/tls_socket_impl.dart:58: _tlsFilter.connect();
Please explain why you call _tlsFilter.connect() both here and in
_tlsWriteHandler and _tlsDataHandler.

https://codereview.chromium.org/10916081/diff/43001/runtime/bin/tls_socket_im...
runtime/bin/tls_socket_impl.dart:111: void _tlsHandshakeFinishHandler() {
In this case the handshake will always have succeeded?

https://codereview.chromium.org/10916081/diff/43001/runtime/bin/tls_socket_im...
runtime/bin/tls_socket_impl.dart:137: _readEncryptedData();
Do you need to call _readEncryptedData here? Shouldn't it be sufficient to call
it in the _tlsDataHandler?

https://codereview.chromium.org/10916081/diff/43001/runtime/bin/tls_socket_im...
runtime/bin/tls_socket_impl.dart:153: print("Before
processBuffer(readPlaintext)");
Debug print more below.

https://codereview.chromium.org/10916081/diff/43001/tests/standalone/io/tls_s...
File tests/standalone/io/tls_socket_test.dart (right):

https://codereview.chromium.org/10916081/diff/43001/tests/standalone/io/tls_s...
tests/standalone/io/tls_socket_test.dart:19: // var tls = new
Socket("www.google.dk", 80);
Code in comments.

https://codereview.chromium.org/10916081/diff/43001/tests/standalone/io/tls_s...
tests/standalone/io/tls_socket_test.dart:23: var get_list =
get_list -> get_bytes?

https://codereview.chromium.org/10916081/diff/43001/tests/standalone/io/tls_s...
tests/standalone/io/tls_socket_test.dart:25: tls.writeList(get_list, 0, 20);
Maybe extend the test to performing this several times with a different constant
than 20 for each iteration.

https://codereview.chromium.org/10916081/diff/43001/tests/standalone/io/tls_s...
tests/standalone/io/tls_socket_test.dart:43: void foo1() {}
What is the purpose of these fooX functions?

[Bill Hesse, 2012-11-11 22:34:34]

Cleaned up and read over.  PTAL.

http://codereview.chromium.org/10916081/diff/43001/runtime/bin/dartutils.cc
File runtime/bin/dartutils.cc (right):

http://codereview.chromium.org/10916081/diff/43001/runtime/bin/dartutils.cc#n...
runtime/bin/dartutils.cc:488: return
Dart_GetClass(Dart_LookupLibrary(Dart_NewString(library_url)),
On 2012/11/01 11:49:11, Søren Gjesse wrote:
> Does this work correctly with error handles if the library is not found?

Yes.  All the functions return an error handle if it is passed in as an
argument.

http://codereview.chromium.org/10916081/diff/43001/runtime/bin/tls_socket.cc
File runtime/bin/tls_socket.cc (right):

http://codereview.chromium.org/10916081/diff/43001/runtime/bin/tls_socket.cc#...
runtime/bin/tls_socket.cc:52: void
FUNCTION_NAME(TlsSocket_Init)(Dart_NativeArguments args) {
On 2012/11/01 11:49:11, Søren Gjesse wrote:
> Maybe assert that filter is not already set.

This is not easy.  Filter is set by putting it in a native instance field of the
Dart object, and to get this field before setting it requires a call through the
Dart API, in the assert.  This function is only called through a constructor, so
that guarantees it is only called once.

http://codereview.chromium.org/10916081/diff/43001/runtime/bin/tls_socket.cc#...
runtime/bin/tls_socket.cc:281: TlsLog("Successful SSL_ForceHandshake call");
On 2012/11/01 11:49:11, Søren Gjesse wrote:
> Please remove logging.

Removed - it is in a separate private patch, because I will need it while
extending the implementation.

http://codereview.chromium.org/10916081/diff/43001/runtime/bin/tls_socket.cc#...
runtime/bin/tls_socket.cc:312: int64_t unsafe_start =
DartUtils::GetIntegerValue(start_object);
On 2012/11/01 11:49:11, Søren Gjesse wrote:
> Maybe change the type to intptr_t here and do the cast here.

I prefer doing the range check on unsafe_length before casting to intptr_t,
because the Dart object can hold larger integers, and the Dart API legitimately
returns them, so are doing a safer check on the status of the Dart object this
way.

http://codereview.chromium.org/10916081/diff/43001/runtime/bin/tls_socket.cc#...
runtime/bin/tls_socket.cc:322: int bytes_sent = 0;
On 2012/11/01 11:49:11, Søren Gjesse wrote:
> Maybe rename bytes_sent to bytes_processed, as this can both read and write.
> Also nothing is actually sent in this method.

Done.

http://codereview.chromium.org/10916081/diff/43001/runtime/bin/tls_socket.cc#...
runtime/bin/tls_socket.cc:367: // NEW
On 2012/11/01 11:49:11, Søren Gjesse wrote:
> Strange comment.

Done.

http://codereview.chromium.org/10916081/diff/43001/runtime/bin/tls_socket.cc#...
runtime/bin/tls_socket.cc:391: if (bytes_sent < 0) bytes_sent = 0;
On 2012/11/01 11:49:11, Søren Gjesse wrote:
> Negative bytes_sent here is not an error?

We should only normally get a blocking error due to buffers full.  Other errors
should be detected elsewhere.  But we can assert that only blocking errors get
here.

http://codereview.chromium.org/10916081/diff/43001/runtime/bin/tls_socket.h
File runtime/bin/tls_socket.h (right):

http://codereview.chromium.org/10916081/diff/43001/runtime/bin/tls_socket.h#n...
runtime/bin/tls_socket.h:30: static const bool kTlsLogging = true;
Removed.
On 2012/11/01 11:49:11, Søren Gjesse wrote:
> This logging should be turned off before committing.

http://codereview.chromium.org/10916081/diff/43001/runtime/bin/tls_socket_imp...
File runtime/bin/tls_socket_impl.dart (right):

http://codereview.chromium.org/10916081/diff/43001/runtime/bin/tls_socket_imp...
runtime/bin/tls_socket_impl.dart:58: _tlsFilter.connect();
_tlsFilter.connect() was both the initial connection setup, and handshake
processing.  Handshake processing and communication is called multiple times, to
send and receive handshake messages.  

This is now split into connect(host_name), which is called once, and
handshake(), which is repeatedly called when socket events happen, until the
handshake is completed.

On 2012/11/01 11:49:11, Søren Gjesse wrote:
> Please explain why you call _tlsFilter.connect() both here and in
> _tlsWriteHandler and _tlsDataHandler.

http://codereview.chromium.org/10916081/diff/43001/runtime/bin/tls_socket_imp...
runtime/bin/tls_socket_impl.dart:111: void _tlsHandshakeFinishHandler() {
On 2012/11/01 11:49:11, Søren Gjesse wrote:
> In this case the handshake will always have succeeded?

Yes.  Terminating the connection on a failed handshake is a TODO - currently it
terminates by timing out on the server end.

http://codereview.chromium.org/10916081/diff/43001/runtime/bin/tls_socket_imp...
runtime/bin/tls_socket_impl.dart:137: _readEncryptedData();
We need to call _readEncryptedData here because it both moved dart from the
socket to the Dart buffer, and from the Dart buffer to the filter.  If the
filter and buffer were full, then we may not have read from the socket since the
last data event on the socket.  But we need to call this to empty the data from
the buffer to the filter.

We also need the postcondition that if we read 0 bytes in this call, then all of
the pipeline is empty.

This all can and should be looked at again, though.

On 2012/11/01 11:49:11, Søren Gjesse wrote:
> Do you need to call _readEncryptedData here? Shouldn't it be sufficient to
call
> it in the _tlsDataHandler?

http://codereview.chromium.org/10916081/diff/43001/runtime/bin/tls_socket_imp...
runtime/bin/tls_socket_impl.dart:153: print("Before
processBuffer(readPlaintext)");
Removed.
On 2012/11/01 11:49:11, Søren Gjesse wrote:
> Debug print more below.

http://codereview.chromium.org/10916081/diff/43001/tests/standalone/io/tls_so...
File tests/standalone/io/tls_socket_test.dart (right):

http://codereview.chromium.org/10916081/diff/43001/tests/standalone/io/tls_so...
tests/standalone/io/tls_socket_test.dart:19: // var tls = new
Socket("www.google.dk", 80);
On 2012/11/01 11:49:11, Søren Gjesse wrote:
> Code in comments.

Done.

http://codereview.chromium.org/10916081/diff/43001/tests/standalone/io/tls_so...
tests/standalone/io/tls_socket_test.dart:23: var get_list =
request_bytes is even better.
On 2012/11/01 11:49:11, Søren Gjesse wrote:
> get_list -> get_bytes?

http://codereview.chromium.org/10916081/diff/43001/tests/standalone/io/tls_so...
tests/standalone/io/tls_socket_test.dart:25: tls.writeList(get_list, 0, 20);
On 2012/11/01 11:49:11, Søren Gjesse wrote:
> Maybe extend the test to performing this several times with a different
constant
> than 20 for each iteration.

We will be modifying it to use a local server, and can do it then.

http://codereview.chromium.org/10916081/diff/43001/tests/standalone/io/tls_so...
tests/standalone/io/tls_socket_test.dart:43: void foo1() {}
Removed.
On 2012/11/01 11:49:11, Søren Gjesse wrote:
> What is the purpose of these fooX functions?

http://codereview.chromium.org/10916081/diff/43003/lib/io/tls_socket.dart
File lib/io/tls_socket.dart (right):

http://codereview.chromium.org/10916081/diff/43003/lib/io/tls_socket.dart#new...
lib/io/tls_socket.dart:22: class _TlsSocket implements TlsSocket {
I can't see a way anyone could really use it, because it requires a handshake. 
But to keep the design clean, the _TlsFilter class does expose a clean
interface, and could be made public.  There is a hidden dependency, that the
static method TlsSocket.setCertificateDatabase needs to be called before a
filter is created.  Perhaps the method should be moved to TlsFilter, and called
from TlsSocket.



On 2012/11/01 07:27:21, ajohnsen wrote:
> A higher level question: Would it make sense to expose the TlsFilter as a
filter
> anyone can use to 'secure' any sequence of data?

http://codereview.chromium.org/10916081/diff/43003/lib/io/tls_socket.dart#new...
lib/io/tls_socket.dart:23: static final int _BUFFER_SIZE = 2048;
Constant removed.
On 2012/11/01 10:09:01, Mads Ager wrote:
> The name here is using an '_' in from of it and none of the others are. Users
> cannot access these anyway because they are static and you cannot access
> _TlsSocket. It should therefore be safe to remove the '_' here.

http://codereview.chromium.org/10916081/diff/43003/lib/io/tls_socket.dart#new...
lib/io/tls_socket.dart:32: static final int kReadPlaintext = 0;
Should we be consistent?  Which do you prefer?
It does seem like the states are like enum constants, while the k-constants are
actual values.

On 2012/11/01 10:09:01, Mads Ager wrote:
> This is using a different naming style than the other constants.

http://codereview.chromium.org/10916081/diff/43003/lib/io/tls_socket.dart#new...
lib/io/tls_socket.dart:149: print("Entering readList");
On 2012/11/01 10:09:01, Mads Ager wrote:
> Debug printing.

Done.

http://codereview.chromium.org/10916081/diff/43003/lib/io/tls_socket.dart#new...
lib/io/tls_socket.dart:299: int get free() => kSize - start - length;
On 2012/11/01 07:27:21, ajohnsen wrote:
> remove ().

Done.

http://codereview.chromium.org/10916081/diff/43003/lib/io/tls_socket.dart#new...
lib/io/tls_socket.dart:315: class _TlsFilter extends NativeFieldWrapperClass1 {
On 2012/11/01 07:27:21, ajohnsen wrote:
> AFAIK, this is VM specific, I think this should be in runtime/bin/

Done.

We have created an abstract interface TlsFilter.- looking at it, I don't like
the typeless "var _tlsFilter".

http://codereview.chromium.org/10916081/diff/50034/sdk/lib/io/tls_socket.dart
File sdk/lib/io/tls_socket.dart (right):

http://codereview.chromium.org/10916081/diff/50034/sdk/lib/io/tls_socket.dart...
sdk/lib/io/tls_socket.dart:25: static final int NOT_CONNECTED = 200;
The form of these constants seems right to me, but if they should all have the
same form, then which one?

[Mads Ager (google), 2012-11-12 11:39:07]

http://codereview.chromium.org/10916081/diff/43003/lib/io/tls_socket.dart
File lib/io/tls_socket.dart (right):

http://codereview.chromium.org/10916081/diff/43003/lib/io/tls_socket.dart#new...
lib/io/tls_socket.dart:32: static final int kReadPlaintext = 0;
> On 2012/11/01 10:09:01, Mads Ager wrote:
> > This is using a different naming style than the other constants.
> >
On 2012/11/11 22:34:34, Bill Hesse wrote:
> Should we be consistent?  Which do you prefer?
> It does seem like the states are like enum constants, while the k-constants
are
> actual values.

Of course we should be consistent!

The are all just 'static final int' and we have a naming convention for those.
In the rest of dart:io we have been using UPPER_CASE for all of these. Please do
so here as well. These are all constants and therefore use the same naming
convention.

http://codereview.chromium.org/10916081/diff/50034/sdk/lib/io/tls_socket.dart
File sdk/lib/io/tls_socket.dart (right):

http://codereview.chromium.org/10916081/diff/50034/sdk/lib/io/tls_socket.dart...
sdk/lib/io/tls_socket.dart:25: static final int NOT_CONNECTED = 200;
On 2012/11/11 22:34:34, Bill Hesse wrote:
> The form of these constants seems right to me, but if they should all have the
> same form, then which one?

We should use this format for now. That is what is used in dart:io at this point
and we should stay consistent with that. If dart:io uses another format
somewhere, that is a style bug.

http://codereview.chromium.org/10916081/diff/51003/runtime/bin/tls_socket.cc
File runtime/bin/tls_socket.cc (right):

http://codereview.chromium.org/10916081/diff/51003/runtime/bin/tls_socket.cc#...
runtime/bin/tls_socket.cc:16: #include <nss.h>
Can we list this in alphabetic order or are there dependencies on the order
here?

http://codereview.chromium.org/10916081/diff/51003/runtime/bin/tls_socket.cc#...
runtime/bin/tls_socket.cc:23: #include "bin/net/nss_memio.h"
Ditto?

http://codereview.chromium.org/10916081/diff/51003/runtime/bin/tls_socket.cc#...
runtime/bin/tls_socket.cc:33: 
I would delete this empty line.

http://codereview.chromium.org/10916081/diff/51003/runtime/bin/tls_socket.cc#...
runtime/bin/tls_socket.cc:36:
ThrowIfError(Dart_GetNativeInstanceField(dart_this,
Please move the first argument to the next line as well when using the one
argument per line style.

http://codereview.chromium.org/10916081/diff/51003/runtime/bin/tls_socket.cc#...
runtime/bin/tls_socket.cc:46:
ThrowIfError(Dart_SetNativeInstanceField(dart_this,
Ditto

http://codereview.chromium.org/10916081/diff/51003/runtime/bin/tls_socket.cc#...
runtime/bin/tls_socket.cc:178: buffer_size_, NULL, NULL));
Might fit on one line? If not, either move more to first line or use
one-argument per line?

http://codereview.chromium.org/10916081/diff/51003/runtime/bin/tls_socket.cc#...
runtime/bin/tls_socket.cc:188: handshake_start_ =
ThrowIfError(Dart_NewPersistentHandle(start));
Where are these persistent handles destroyed? Is this only called once? Can we
either assert that the handles are null or make sure to destroy them before we
allocated new ones?

http://codereview.chromium.org/10916081/diff/51003/runtime/bin/tls_socket.cc#...
runtime/bin/tls_socket.cc:197: // TODO: Allow read-write opening of
pkcert_database using
Let's remove this TODO for now? We are not currently planning to allow writing
to the database.

http://codereview.chromium.org/10916081/diff/51003/runtime/bin/tls_socket.cc#...
runtime/bin/tls_socket.cc:200: NSS_Init(pkcert_database);
Fits on one line.

http://codereview.chromium.org/10916081/diff/51003/runtime/bin/tls_socket.cc#...
runtime/bin/tls_socket.cc:202: ReportError("Unsuccessful NSS_Init call.\n",
PR_GetError());
Remove the \n from the strings here and let ReportError deal with that?

http://codereview.chromium.org/10916081/diff/51003/runtime/bin/tls_socket.cc#...
runtime/bin/tls_socket.cc:210: ReportError("Called TlsFilter::InitializeLibrary
more than once", 0);
Indentation is off.

http://codereview.chromium.org/10916081/diff/51003/runtime/bin/tls_socket.cc#...
runtime/bin/tls_socket.cc:256: void TlsFilter::Handshake() {
This looks a little convoluted to me.

Don't we always want the handshake_start_ and handshake_finish_ callbacks? How
about not relying on SSL_ForceHandshake returning PR_WOULD_BLOCK for that?

void TlsFilter::Handshake() {
  if (!in_handshake_) {
    // Notify dart code that handshake is starting.
    ThrowIfError(Dart_InvokeClosure(handleshake_start_, 0, NULL);
    in_handshake_ = true;
  }
  SECStatus status = SSL_Forcehandshake(memio_);
  if (status == SECSuccess) {
    // Notify dart code that handshake is complete.
    ThrowIfError(Dart_InvokeClosure(handshake_finish, ...));
  } else if (error != PR_WOULD_BLOCK_ERROR) {
    ReportError("...", ...)
  }
  // Forcehandshake failed with PR_WOULD_BLOCK_ERROR.
  // TlsFilter::Handshake will be called again when more
  // data has arrived on the socket.
}

Writing this, do we need the start callback at all? Can't the dart code setup
the handler, then call this function? Then we only need to call the
handshake_done callback?

http://codereview.chromium.org/10916081/diff/51003/runtime/bin/tls_socket.cc#...
runtime/bin/tls_socket.cc:283: // TODO(whesse): Destroy OpenSSL objects here.
OpenSSL?!?

I made this comment in my last round of comments as well.

http://codereview.chromium.org/10916081/diff/51003/runtime/bin/tls_socket.cc#...
runtime/bin/tls_socket.cc:295: if (unsafe_start < 0 || unsafe_start >=
buffer_size_ ||
What happens here if we get a bigint? Can that happen? Is the length
user-supplied here? I think GetIntegerValue will return 0 in that case in
release mode (it just asserts that it could get the value). You could use
GetInt64Value here which returns a boolean and checks that the integer fits in
64 bits.

However, we are in control of the sizes here aren't we? The Dart code should not
allow something that is larger than buffer_size_. So maybe an ASSERT here is
actually enough. This is a "can't happen" situation, right?

http://codereview.chromium.org/10916081/diff/51003/runtime/bin/tls_socket.cc#...
runtime/bin/tls_socket.cc:308: buffers_[buffer_index] + start + length,
Indentation is off.

http://codereview.chromium.org/10916081/diff/51003/runtime/bin/tls_socket.h
File runtime/bin/tls_socket.h (right):

http://codereview.chromium.org/10916081/diff/51003/runtime/bin/tls_socket.h#n...
runtime/bin/tls_socket.h:29: class TlsFilter {
Some overall comment on the filter would be nice as well as documentation of the
states that it goes through. We need to land this soon, so feel free to do that
as a follow-up change.

http://codereview.chromium.org/10916081/diff/51003/runtime/bin/tls_socket.h#n...
runtime/bin/tls_socket.h:31: enum BufferIndex { kReadPlaintext,
We normally format enums like this:

enum BufferIndex {
  kReadPlaintext,
  ...
};

http://codereview.chromium.org/10916081/diff/51003/runtime/bin/tls_socket.h#n...
runtime/bin/tls_socket.h:55: Dart_Handle stringStart_;
Please use C++ naming convention for these.

http://codereview.chromium.org/10916081/diff/51003/runtime/bin/tls_socket.h#n...
runtime/bin/tls_socket.h:65: // TODO(whesse): Implement thread-safety of NSS
library initialization.
This seems like an important TODO and it should be very easy to fix. Look at the
mutex implementations used in the rest of the dart:io code.

http://codereview.chromium.org/10916081/diff/51003/sdk/lib/io/tls_socket.dart
File sdk/lib/io/tls_socket.dart (right):

http://codereview.chromium.org/10916081/diff/51003/sdk/lib/io/tls_socket.dart...
sdk/lib/io/tls_socket.dart:5: abstract class TlsSocket implements Socket {
Could you add a class comment for DartDoc please?

http://codereview.chromium.org/10916081/diff/51003/sdk/lib/io/tls_socket.dart...
sdk/lib/io/tls_socket.dart:31: static final int kReadPlaintext = 0;
Style update. These needs to be in sync with some C++ constants, right? Maybe
add that to the comment so the connection is clearer?

http://codereview.chromium.org/10916081/diff/51003/sdk/lib/io/tls_socket.dart...
sdk/lib/io/tls_socket.dart:43: _socket.onWrite = _tlsWriteHandler;
Can't we simplify this. It seems to me that the _tlsWriteHandler is always used.
Can we just set it here and always set it again in _tlsWriteHandler itself. Then
there is no need to set it in so many places?

http://codereview.chromium.org/10916081/diff/51003/sdk/lib/io/tls_socket.dart...
sdk/lib/io/tls_socket.dart:95: _readEncryptedData();
Does this mean that you are reading out the data before calling the data
callback? Currently, the model is that we don't read anything out from the
system before a data callback. The read method does the reading of the data from
the system.

http://codereview.chromium.org/10916081/diff/51003/sdk/lib/io/tls_socket.dart...
sdk/lib/io/tls_socket.dart:151: int readList(List<int> data, int offset, int
bytes) {
We are missing the read method on sockets that Soren added.

http://codereview.chromium.org/10916081/diff/51003/sdk/lib/io/tls_socket.dart...
sdk/lib/io/tls_socket.dart:200: // without blocking.  If not all the data is
written, enable the
This comment mentions more than the method is doing.

http://codereview.chromium.org/10916081/diff/51003/sdk/lib/io/tls_socket.dart...
sdk/lib/io/tls_socket.dart:274: // TODO(whesse): Specify an abstract class for
TlsFilter
This TODO is now done?

http://codereview.chromium.org/10916081/diff/51003/sdk/lib/io/tls_socket.dart...
sdk/lib/io/tls_socket.dart:280: static final int kSize = 8 * 1024;
Change to Dart constant style.

http://codereview.chromium.org/10916081/diff/51003/sdk/lib/io/tls_socket.dart...
sdk/lib/io/tls_socket.dart:299: abstract class TlsFilter {
This is public and needs doc comments. Alternatively, keep it private until you
are ready to document and expose it to users.

http://codereview.chromium.org/10916081/diff/51003/tests/standalone/io/tls_so...
File tests/standalone/io/tls_socket_test.dart (right):

http://codereview.chromium.org/10916081/diff/51003/tests/standalone/io/tls_so...
tests/standalone/io/tls_socket_test.dart:13: new Path.fromNative(new
Options().script).directoryPath.append('pkcert/');
I still don't see a pkcert directory in this changelist.

http://codereview.chromium.org/10916081/diff/51003/tests/standalone/io/tls_so...
tests/standalone/io/tls_socket_test.dart:35: print(fullPage);
Remove the print. Not sure it is usefully to get google.dk html printed when you
run the test.

[Søren Gjesse, 2012-11-12 12:04:56]

http://codereview.chromium.org/10916081/diff/51003/runtime/bin/tls_socket.cc
File runtime/bin/tls_socket.cc (right):

http://codereview.chromium.org/10916081/diff/51003/runtime/bin/tls_socket.cc#...
runtime/bin/tls_socket.cc:162: Dart_Handle tls_external_buffer_class =
ThrowIfError(
Wouldn't it be simpler to just look up the class "_TlsExternalBuffer" directly?

http://codereview.chromium.org/10916081/diff/51003/runtime/bin/tls_socket.cc#...
runtime/bin/tls_socket.cc:180: DartUtils::NewString("data"),
Move DartUtils::NewString("data") out out the loop.

http://codereview.chromium.org/10916081/diff/51003/runtime/bin/tls_socket.cc#...
runtime/bin/tls_socket.cc:197: // TODO: Allow read-write opening of
pkcert_database using
Please add whesse to TODO.

http://codereview.chromium.org/10916081/diff/51003/runtime/bin/tls_socket.cc#...
runtime/bin/tls_socket.cc:199: SECStatus status =
Fits on one line.

http://codereview.chromium.org/10916081/diff/51003/runtime/bin/tls_socket.cc#...
runtime/bin/tls_socket.cc:246: int index = PR_EnumerateHostEnt(0, &host_entry,
443, &host_address);
Should 443 be hardcoded here? How about TLS on other ports?

http://codereview.chromium.org/10916081/diff/51003/runtime/bin/tls_socket.cc#...
runtime/bin/tls_socket.cc:287: intptr_t TlsFilter::ProcessBuffer(int
buffer_index) {
Maybe add a short comment on how the four buffers work together with the filter.

Thinking about it would it work if this was just called ProcessBuffers and
always processed all four buffers?

http://codereview.chromium.org/10916081/diff/51003/runtime/bin/tls_socket.cc#...
runtime/bin/tls_socket.cc:307: bytes_processed = PR_Read(memio_,
Indentation.

http://codereview.chromium.org/10916081/diff/51003/runtime/bin/tls_socket.cc#...
runtime/bin/tls_socket.cc:364: bytes_processed = PR_Write(memio_,
Indentation.

http://codereview.chromium.org/10916081/diff/51003/sdk/lib/io/tls_socket.dart
File sdk/lib/io/tls_socket.dart (right):

http://codereview.chromium.org/10916081/diff/51003/sdk/lib/io/tls_socket.dart...
sdk/lib/io/tls_socket.dart:71: _tlsFilter.connect(_host);
How about the port - is that not needed here?

http://codereview.chromium.org/10916081/diff/51003/sdk/lib/io/tls_socket.dart...
sdk/lib/io/tls_socket.dart:95: _readEncryptedData();
Does encrypted data on the socket always mean that there is unencrypted data
available? We probably can call onData even if read will not read anything, but
I think that's not a good idea.

http://codereview.chromium.org/10916081/diff/51003/sdk/lib/io/tls_socket.dart...
sdk/lib/io/tls_socket.dart:104: _socket.close();
I don't think we should close the socket here. If the socket is closed for
reading it should still be possible to write more data. I think that it should
only be called in close().

http://codereview.chromium.org/10916081/diff/51003/sdk/lib/io/tls_socket.dart...
sdk/lib/io/tls_socket.dart:215: _readEncryptedData();
Please provide a comment on why you are also calling _readEncryptedData here.

http://codereview.chromium.org/10916081/diff/51003/sdk/lib/io/tls_socket.dart...
sdk/lib/io/tls_socket.dart:280: static final int kSize = 8 * 1024;
We should consider using a larger buffer size.

http://codereview.chromium.org/10916081/diff/51003/tests/standalone/io/tls_so...
File tests/standalone/io/tls_socket_test.dart (right):

http://codereview.chromium.org/10916081/diff/51003/tests/standalone/io/tls_so...
tests/standalone/io/tls_socket_test.dart:6: // VMOptions=--short_socket_read
How about --short_socket_write and both --short_socket_read and
--short_socket_write.

[Bill Hesse, 2012-11-13 20:11:07]

I think this is looking much nicer, due to comments and cleanup.  Please take a
look.

tls_socket.cc:162: I'm reconsidering, and may go with the suggestion of looking
up "_TlsExternalBuffer" in library "dart:io" directly - that would only be one
call.

Regarding being lazy about reading encrypted data from the underlying socket, we
definitely can't be lazy if we want to support the "available" (bytes available)
getter on Socket, which we need to do.

http://codereview.chromium.org/10916081/diff/51003/runtime/bin/tls_socket.cc
File runtime/bin/tls_socket.cc (right):

http://codereview.chromium.org/10916081/diff/51003/runtime/bin/tls_socket.cc#...
runtime/bin/tls_socket.cc:16: #include <nss.h>
On 2012/11/12 11:39:08, Mads Ager wrote:
> Can we list this in alphabetic order or are there dependencies on the order
> here?

Done.

http://codereview.chromium.org/10916081/diff/51003/runtime/bin/tls_socket.cc#...
runtime/bin/tls_socket.cc:23: #include "bin/net/nss_memio.h"
On 2012/11/12 11:39:08, Mads Ager wrote:
> Ditto?

Done.

Is it allowable to put dart_api separately, since it is a big deal that a file
uses the Dart API?

http://codereview.chromium.org/10916081/diff/51003/runtime/bin/tls_socket.cc#...
runtime/bin/tls_socket.cc:36:
ThrowIfError(Dart_GetNativeInstanceField(dart_this,
On 2012/11/12 11:39:08, Mads Ager wrote:
> Please move the first argument to the next line as well when using the one
> argument per line style.

Done.

http://codereview.chromium.org/10916081/diff/51003/runtime/bin/tls_socket.cc#...
runtime/bin/tls_socket.cc:46:
ThrowIfError(Dart_SetNativeInstanceField(dart_this,
On 2012/11/12 11:39:08, Mads Ager wrote:
> Ditto

Done.

http://codereview.chromium.org/10916081/diff/51003/runtime/bin/tls_socket.cc#...
runtime/bin/tls_socket.cc:162: Dart_Handle tls_external_buffer_class =
ThrowIfError(
On 2012/11/12 12:04:57, Søren Gjesse wrote:
> Wouldn't it be simpler to just look up the class "_TlsExternalBuffer"
directly?

I beleive we need a library for that, and so the number of lookups are the same.
 And this does get the correct dynamic type of the object.

http://codereview.chromium.org/10916081/diff/51003/runtime/bin/tls_socket.cc#...
runtime/bin/tls_socket.cc:188: handshake_start_ =
ThrowIfError(Dart_NewPersistentHandle(start));
This is only called once, in the constructor of the TlsFilter.  The handles are
now destroyed in TlsFilter::Destroy.

On 2012/11/12 11:39:08, Mads Ager wrote:
> Where are these persistent handles destroyed? Is this only called once? Can we
> either assert that the handles are null or make sure to destroy them before we
> allocated new ones?

http://codereview.chromium.org/10916081/diff/51003/runtime/bin/tls_socket.cc#...
runtime/bin/tls_socket.cc:197: // TODO: Allow read-write opening of
pkcert_database using
On 2012/11/12 11:39:08, Mads Ager wrote:
> Let's remove this TODO for now? We are not currently planning to allow writing
> to the database.

Done.

http://codereview.chromium.org/10916081/diff/51003/runtime/bin/tls_socket.cc#...
runtime/bin/tls_socket.cc:199: SECStatus status =
On 2012/11/12 12:04:57, Søren Gjesse wrote:
> Fits on one line.

Done.

http://codereview.chromium.org/10916081/diff/51003/runtime/bin/tls_socket.cc#...
runtime/bin/tls_socket.cc:202: ReportError("Unsuccessful NSS_Init call.\n",
PR_GetError());
On 2012/11/12 11:39:08, Mads Ager wrote:
> Remove the \n from the strings here and let ReportError deal with that?

Done.

http://codereview.chromium.org/10916081/diff/51003/runtime/bin/tls_socket.cc#...
runtime/bin/tls_socket.cc:210: ReportError("Called TlsFilter::InitializeLibrary
more than once", 0);
On 2012/11/12 11:39:08, Mads Ager wrote:
> Indentation is off.

Done.

http://codereview.chromium.org/10916081/diff/51003/runtime/bin/tls_socket.cc#...
runtime/bin/tls_socket.cc:283: // TODO(whesse): Destroy OpenSSL objects here.
On 2012/11/12 11:39:08, Mads Ager wrote:
> OpenSSL?!?
> 
> I made this comment in my last round of comments as well.

Done.

http://codereview.chromium.org/10916081/diff/51003/runtime/bin/tls_socket.cc#...
runtime/bin/tls_socket.cc:287: intptr_t TlsFilter::ProcessBuffer(int
buffer_index) {
On 2012/11/12 12:04:57, Søren Gjesse wrote:
> Maybe add a short comment on how the four buffers work together with the
filter.
> 
> Thinking about it would it work if this was just called ProcessBuffers and
> always processed all four buffers?

Because it is easiest to get an int return value back into dart, and then adjust
all the fields using that value, it is easiest to make it 4 separate calls.  We
also want to know individually if they succeed or not.

http://codereview.chromium.org/10916081/diff/51003/runtime/bin/tls_socket.cc#...
runtime/bin/tls_socket.cc:295: if (unsafe_start < 0 || unsafe_start >=
buffer_size_ ||
Yes, this is a "can't happen" situation.  We control these values in our Dart
implementation code.

On 2012/11/12 11:39:08, Mads Ager wrote:
> What happens here if we get a bigint? Can that happen? Is the length
> user-supplied here? I think GetIntegerValue will return 0 in that case in
> release mode (it just asserts that it could get the value). You could use
> GetInt64Value here which returns a boolean and checks that the integer fits in
> 64 bits.
> 
> However, we are in control of the sizes here aren't we? The Dart code should
not
> allow something that is larger than buffer_size_. So maybe an ASSERT here is
> actually enough. This is a "can't happen" situation, right?

http://codereview.chromium.org/10916081/diff/51003/runtime/bin/tls_socket.cc#...
runtime/bin/tls_socket.cc:307: bytes_processed = PR_Read(memio_,
On 2012/11/12 12:04:57, Søren Gjesse wrote:
> Indentation.

Done.

http://codereview.chromium.org/10916081/diff/51003/runtime/bin/tls_socket.cc#...
runtime/bin/tls_socket.cc:364: bytes_processed = PR_Write(memio_,
On 2012/11/12 12:04:57, Søren Gjesse wrote:
> Indentation.

Done.

http://codereview.chromium.org/10916081/diff/51003/runtime/bin/tls_socket.h
File runtime/bin/tls_socket.h (right):

http://codereview.chromium.org/10916081/diff/51003/runtime/bin/tls_socket.h#n...
runtime/bin/tls_socket.h:29: class TlsFilter {
On 2012/11/12 11:39:08, Mads Ager wrote:
> Some overall comment on the filter would be nice as well as documentation of
the
> states that it goes through. We need to land this soon, so feel free to do
that
> as a follow-up change.

Done.

http://codereview.chromium.org/10916081/diff/51003/runtime/bin/tls_socket.h#n...
runtime/bin/tls_socket.h:31: enum BufferIndex { kReadPlaintext,
On 2012/11/12 11:39:08, Mads Ager wrote:
> We normally format enums like this:
> 
> enum BufferIndex {
>   kReadPlaintext,
>   ...
> };

Done.

http://codereview.chromium.org/10916081/diff/51003/runtime/bin/tls_socket.h#n...
runtime/bin/tls_socket.h:55: Dart_Handle stringStart_;
On 2012/11/12 11:39:08, Mads Ager wrote:
> Please use C++ naming convention for these.

Done.

http://codereview.chromium.org/10916081/diff/51003/runtime/bin/tls_socket.h#n...
runtime/bin/tls_socket.h:65: // TODO(whesse): Implement thread-safety of NSS
library initialization.
On 2012/11/12 11:39:08, Mads Ager wrote:
> This seems like an important TODO and it should be very easy to fix. Look at
the
> mutex implementations used in the rest of the dart:io code.

Done.

http://codereview.chromium.org/10916081/diff/51003/sdk/lib/io/tls_socket.dart
File sdk/lib/io/tls_socket.dart (right):

http://codereview.chromium.org/10916081/diff/51003/sdk/lib/io/tls_socket.dart...
sdk/lib/io/tls_socket.dart:5: abstract class TlsSocket implements Socket {
On 2012/11/12 11:39:08, Mads Ager wrote:
> Could you add a class comment for DartDoc please?

Done.

http://codereview.chromium.org/10916081/diff/51003/sdk/lib/io/tls_socket.dart...
sdk/lib/io/tls_socket.dart:31: static final int kReadPlaintext = 0;
On 2012/11/12 11:39:08, Mads Ager wrote:
> Style update. These needs to be in sync with some C++ constants, right? Maybe
> add that to the comment so the connection is clearer?

Done.

http://codereview.chromium.org/10916081/diff/51003/sdk/lib/io/tls_socket.dart...
sdk/lib/io/tls_socket.dart:43: _socket.onWrite = _tlsWriteHandler;
We actually don't want to set it in _tlsWriteHandler itself - it is a one-shot
handler, and we don't usually want it to be enabled.  Only when we are
handshaking or when we have pending data in the write pipeline, or when the
write handler is set on the TlsSocket.

But a few of the sets are redundant, and have been removed.
There also seems to be a missing call to it in writeEncrypted, if we are blocked
on the socket write - added there.

On 2012/11/12 11:39:08, Mads Ager wrote:
> Can't we simplify this. It seems to me that the _tlsWriteHandler is always
used.
> Can we just set it here and always set it again in _tlsWriteHandler itself.
Then
> there is no need to set it in so many places?

http://codereview.chromium.org/10916081/diff/51003/sdk/lib/io/tls_socket.dart...
sdk/lib/io/tls_socket.dart:71: _tlsFilter.connect(_host);
Of course it is.  Added.

On 2012/11/12 12:04:57, Søren Gjesse wrote:
> How about the port - is that not needed here?

http://codereview.chromium.org/10916081/diff/51003/sdk/lib/io/tls_socket.dart...
sdk/lib/io/tls_socket.dart:95: _readEncryptedData();
We need to read data before the user-visible data callback, for the SSL
handshake.  We can create two data handlers, so that we only do this in the
handshake phase.

But we also need to read encrypted data to find out whether we will have any
plaintext data available.  I think it is more important not to send a data event
to the user with no actual data available, than to try and be totally lazy.



On 2012/11/12 11:39:08, Mads Ager wrote:
> Does this mean that you are reading out the data before calling the data
> callback? Currently, the model is that we don't read anything out from the
> system before a data callback. The read method does the reading of the data
from
> the system.

http://codereview.chromium.org/10916081/diff/51003/sdk/lib/io/tls_socket.dart...
sdk/lib/io/tls_socket.dart:104: _socket.close();
Sounds right.  We will need to test that this works, once we control both ends
of the socket.  Since this also affects fireCloseEvent, and the destruction of
the filter, I'd like to make it a separate CL.

On 2012/11/12 12:04:57, Søren Gjesse wrote:
> I don't think we should close the socket here. If the socket is closed for
> reading it should still be possible to write more data. I think that it should
> only be called in close().

http://codereview.chromium.org/10916081/diff/51003/sdk/lib/io/tls_socket.dart...
sdk/lib/io/tls_socket.dart:151: int readList(List<int> data, int offset, int
bytes) {
Added, along with all other methods on the socket interface.
Bug filed for the unimplemented SocketStream methods.

On 2012/11/12 11:39:08, Mads Ager wrote:
> We are missing the read method on sockets that Soren added.

http://codereview.chromium.org/10916081/diff/51003/sdk/lib/io/tls_socket.dart...
sdk/lib/io/tls_socket.dart:200: // without blocking.  If not all the data is
written, enable the
Yes, and it is an error that this does not do all of this.
We want to write more than the buffer size, with a loop: Adding a TODO.

Also added to _writeEncrypted the code to flush the WRITE_PLAINTEXT buffer.

On 2012/11/12 11:39:08, Mads Ager wrote:
> This comment mentions more than the method is doing.

http://codereview.chromium.org/10916081/diff/51003/sdk/lib/io/tls_socket.dart...
sdk/lib/io/tls_socket.dart:215: _readEncryptedData();
Removed.

On 2012/11/12 12:04:57, Søren Gjesse wrote:
> Please provide a comment on why you are also calling _readEncryptedData here.

http://codereview.chromium.org/10916081/diff/51003/sdk/lib/io/tls_socket.dart...
sdk/lib/io/tls_socket.dart:274: // TODO(whesse): Specify an abstract class for
TlsFilter
On 2012/11/12 11:39:08, Mads Ager wrote:
> This TODO is now done?

Done.

http://codereview.chromium.org/10916081/diff/51003/sdk/lib/io/tls_socket.dart...
sdk/lib/io/tls_socket.dart:280: static final int kSize = 8 * 1024;
On 2012/11/12 11:39:08, Mads Ager wrote:
> Change to Dart constant style.

Done.

http://codereview.chromium.org/10916081/diff/51003/sdk/lib/io/tls_socket.dart...
sdk/lib/io/tls_socket.dart:280: static final int kSize = 8 * 1024;
On 2012/11/12 12:04:57, Søren Gjesse wrote:
> We should consider using a larger buffer size.

The TLS specification does specify that packet size can be set (connected to MTU
size) to powers of 2 between 2^9 and 2^14.  So 2^14 is twice as big.  But we do
have 4 of these for each socket.

http://codereview.chromium.org/10916081/diff/51003/sdk/lib/io/tls_socket.dart...
sdk/lib/io/tls_socket.dart:299: abstract class TlsFilter {
On 2012/11/12 11:39:08, Mads Ager wrote:
> This is public and needs doc comments. Alternatively, keep it private until
you
> are ready to document and expose it to users.

I'm not convinced that we ever want to expose it.  Making it private.

http://codereview.chromium.org/10916081/diff/51003/tests/standalone/io/tls_so...
File tests/standalone/io/tls_socket_test.dart (right):

http://codereview.chromium.org/10916081/diff/51003/tests/standalone/io/tls_so...
tests/standalone/io/tls_socket_test.dart:6: // VMOptions=--short_socket_read
Comment added - TODO below explains why these don't work yet.
On 2012/11/12 12:04:57, Søren Gjesse wrote:
> How about --short_socket_write and both --short_socket_read and
> --short_socket_write.

http://codereview.chromium.org/10916081/diff/51003/tests/standalone/io/tls_so...
tests/standalone/io/tls_socket_test.dart:13: new Path.fromNative(new
Options().script).directoryPath.append('pkcert/');
Added.
On 2012/11/12 11:39:08, Mads Ager wrote:
> I still don't see a pkcert directory in this changelist.

http://codereview.chromium.org/10916081/diff/51003/tests/standalone/io/tls_so...
tests/standalone/io/tls_socket_test.dart:35: print(fullPage);
On 2012/11/12 11:39:08, Mads Ager wrote:
> Remove the print. Not sure it is usefully to get google.dk html printed when
you
> run the test.

Done.

http://codereview.chromium.org/10916081/diff/47023/runtime/bin/dartutils.h
File runtime/bin/dartutils.h (right):

http://codereview.chromium.org/10916081/diff/47023/runtime/bin/dartutils.h#ne...
runtime/bin/dartutils.h:136: ASSERT(str != NULL);
There is no reason not to make an empty string here, if we need one.  I see no
other way to make an empty string in Dart, and I need to make them.

[Søren Gjesse, 2012-11-14 08:18:59]

LGTM!

http://codereview.chromium.org/10916081/diff/41025/runtime/bin/tls_socket.cc
File runtime/bin/tls_socket.cc (right):

http://codereview.chromium.org/10916081/diff/41025/runtime/bin/tls_socket.cc#...
runtime/bin/tls_socket.cc:56: TlsFilter* filter = new TlsFilter;
Move the creation of TlsFilter down to just before SetTlsFilter. You can then
avoid the delete filter in the if below.

http://codereview.chromium.org/10916081/diff/41025/runtime/bin/tls_socket.cc#...
runtime/bin/tls_socket.cc:188: Dart_NewExternalByteArray(buffers_[i],
buffer_size_, NULL, NULL));
You need to specify a peer pointer and callback here to free the buffer from the
C heap.

http://codereview.chromium.org/10916081/diff/41025/runtime/bin/tls_socket.cc#...
runtime/bin/tls_socket.cc:209: SECStatus status = NSS_Init(pkcert_database);
There might or might not be some UFT-8 issues here. Most likely only on Windows.
Depends on how NSS_Init handles non ASCII paths. See
http://code.google.com/p/dart/source/detail?r=14851. You can postpone that to a
follow-up CL.

http://codereview.chromium.org/10916081/diff/41025/runtime/bin/tls_socket.cc#...
runtime/bin/tls_socket.cc:310: intptr_t length =
static_cast<intptr_t>(unsafe_length);
Maybe add local variable

uint8_t* buffer = buffers_[buffer_index]

and use that instead of buffers_[buffer_index] below.

http://codereview.chromium.org/10916081/diff/41025/runtime/bin/tls_socket.cc#...
runtime/bin/tls_socket.cc:318: bytes_free);
As the buffers are circular shouldn't we try to fill the interval [0, start[
with data as well? That of cause would mean that start + length could become >
buffer_size_.

http://codereview.chromium.org/10916081/diff/41025/runtime/bin/tls_socket.h
File runtime/bin/tls_socket.h (right):

http://codereview.chromium.org/10916081/diff/41025/runtime/bin/tls_socket.h#n...
runtime/bin/tls_socket.h:22: static void ReportError(const char* message) {
Rename to ThrowException to indicate that this will not return?

http://codereview.chromium.org/10916081/diff/41025/runtime/bin/tls_socket.h#n...
runtime/bin/tls_socket.h:30: static void ReportPRError(const char* message) {
Ditto

http://codereview.chromium.org/10916081/diff/41025/runtime/bin/tls_socket_pat...
File runtime/bin/tls_socket_patch.dart (right):

http://codereview.chromium.org/10916081/diff/41025/runtime/bin/tls_socket_pat...
runtime/bin/tls_socket_patch.dart:47: List<_TlsExternalBuffer> buffers;
Add final.

http://codereview.chromium.org/10916081/diff/41025/sdk/lib/io/tls_socket.dart
File sdk/lib/io/tls_socket.dart (right):

http://codereview.chromium.org/10916081/diff/41025/sdk/lib/io/tls_socket.dart...
sdk/lib/io/tls_socket.dart:112: }
Indentation

http://codereview.chromium.org/10916081/diff/41025/sdk/lib/io/tls_socket.dart...
sdk/lib/io/tls_socket.dart:145: // until it blocks.  If the write blocks,
_writeEncryptedData sets
Please change block to "would block" (or something like that) - we never block.

http://codereview.chromium.org/10916081/diff/41025/sdk/lib/io/tls_socket.dart...
sdk/lib/io/tls_socket.dart:252: if ((_status == CONNECTED || _status == CLOSED)
&& plaintext.free > 0) {
Why are we trying to process data when closed?

http://codereview.chromium.org/10916081/diff/41025/sdk/lib/io/tls_socket.dart...
sdk/lib/io/tls_socket.dart:346: 
Could this maybe be made into a real circular buffer?

[Mads Ager (google), 2012-11-14 10:18:53]

LGTM with the comments addressed.

https://codereview.chromium.org/10916081/diff/51003/runtime/bin/tls_socket.cc
File runtime/bin/tls_socket.cc (right):

https://codereview.chromium.org/10916081/diff/51003/runtime/bin/tls_socket.cc...
runtime/bin/tls_socket.cc:256: void TlsFilter::Handshake() {
On 2012/11/12 11:39:08, Mads Ager wrote:
> This looks a little convoluted to me.
> 
> Don't we always want the handshake_start_ and handshake_finish_ callbacks? How
> about not relying on SSL_ForceHandshake returning PR_WOULD_BLOCK for that?
> 
> void TlsFilter::Handshake() {
>   if (!in_handshake_) {
>     // Notify dart code that handshake is starting.
>     ThrowIfError(Dart_InvokeClosure(handleshake_start_, 0, NULL);
>     in_handshake_ = true;
>   }
>   SECStatus status = SSL_Forcehandshake(memio_);
>   if (status == SECSuccess) {
>     // Notify dart code that handshake is complete.
>     ThrowIfError(Dart_InvokeClosure(handshake_finish, ...));
>   } else if (error != PR_WOULD_BLOCK_ERROR) {
>     ReportError("...", ...)
>   }
>   // Forcehandshake failed with PR_WOULD_BLOCK_ERROR.
>   // TlsFilter::Handshake will be called again when more
>   // data has arrived on the socket.
> }
> 
> Writing this, do we need the start callback at all? Can't the dart code setup
> the handler, then call this function? Then we only need to call the
> handshake_done callback?

How about this comment, Bill? Can this be simplified?

[Bill Hesse, 2012-11-14 13:33:29]

All comments addressed.

https://codereview.chromium.org/10916081/diff/51003/runtime/bin/tls_socket.cc
File runtime/bin/tls_socket.cc (right):

https://codereview.chromium.org/10916081/diff/51003/runtime/bin/tls_socket.cc...
runtime/bin/tls_socket.cc:256: void TlsFilter::Handshake() {
On 2012/11/14 10:18:54, Mads Ager wrote:
> On 2012/11/12 11:39:08, Mads Ager wrote:
> > This looks a little convoluted to me.
> > 
> > Don't we always want the handshake_start_ and handshake_finish_ callbacks?
How
> > about not relying on SSL_ForceHandshake returning PR_WOULD_BLOCK for that?
> > 
> > void TlsFilter::Handshake() {
> >   if (!in_handshake_) {
> >     // Notify dart code that handshake is starting.
> >     ThrowIfError(Dart_InvokeClosure(handleshake_start_, 0, NULL);
> >     in_handshake_ = true;
> >   }
> >   SECStatus status = SSL_Forcehandshake(memio_);
> >   if (status == SECSuccess) {
> >     // Notify dart code that handshake is complete.
> >     ThrowIfError(Dart_InvokeClosure(handshake_finish, ...));
> >   } else if (error != PR_WOULD_BLOCK_ERROR) {
> >     ReportError("...", ...)
> >   }
> >   // Forcehandshake failed with PR_WOULD_BLOCK_ERROR.
> >   // TlsFilter::Handshake will be called again when more
> >   // data has arrived on the socket.
> > }
> > 
> > Writing this, do we need the start callback at all? Can't the dart code
setup
> > the handler, then call this function? Then we only need to call the
> > handshake_done callback?
> 
> How about this comment, Bill? Can this be simplified?

Done.

https://codereview.chromium.org/10916081/diff/41025/runtime/bin/tls_socket.cc
File runtime/bin/tls_socket.cc (right):

https://codereview.chromium.org/10916081/diff/41025/runtime/bin/tls_socket.cc...
runtime/bin/tls_socket.cc:56: TlsFilter* filter = new TlsFilter;
On 2012/11/14 08:18:59, Søren Gjesse wrote:
> Move the creation of TlsFilter down to just before SetTlsFilter. You can then
> avoid the delete filter in the if below.

Done.

https://codereview.chromium.org/10916081/diff/41025/runtime/bin/tls_socket.cc...
runtime/bin/tls_socket.cc:188: Dart_NewExternalByteArray(buffers_[i],
buffer_size_, NULL, NULL));
On 2012/11/14 08:18:59, Søren Gjesse wrote:
> You need to specify a peer pointer and callback here to free the buffer from
the
> C heap.

We are freeing the buffers manually in TlsFilter::Destroy, at the same time that
we delete the persistent handles to the _TlsExternalBuffer buffer objects
holding the ExternalByteArrays.

https://codereview.chromium.org/10916081/diff/41025/runtime/bin/tls_socket.cc...
runtime/bin/tls_socket.cc:209: SECStatus status = NSS_Init(pkcert_database);
On 2012/11/14 08:18:59, Søren Gjesse wrote:
> There might or might not be some UFT-8 issues here. Most likely only on
Windows.
> Depends on how NSS_Init handles non ASCII paths. See
> http://code.google.com/p/dart/source/detail?r=14851. You can postpone that to
a
> follow-up CL.

TODO added.

https://codereview.chromium.org/10916081/diff/41025/runtime/bin/tls_socket.cc...
runtime/bin/tls_socket.cc:310: intptr_t length =
static_cast<intptr_t>(unsafe_length);
On 2012/11/14 08:18:59, Søren Gjesse wrote:
> Maybe add local variable
> 
> uint8_t* buffer = buffers_[buffer_index]
> 
> and use that instead of buffers_[buffer_index] below.

Done.

https://codereview.chromium.org/10916081/diff/41025/runtime/bin/tls_socket.h
File runtime/bin/tls_socket.h (right):

https://codereview.chromium.org/10916081/diff/41025/runtime/bin/tls_socket.h#...
runtime/bin/tls_socket.h:22: static void ReportError(const char* message) {
On 2012/11/14 08:18:59, Søren Gjesse wrote:
> Rename to ThrowException to indicate that this will not return?

Done.

https://codereview.chromium.org/10916081/diff/41025/runtime/bin/tls_socket.h#...
runtime/bin/tls_socket.h:30: static void ReportPRError(const char* message) {
On 2012/11/14 08:18:59, Søren Gjesse wrote:
> Ditto

Done.

https://codereview.chromium.org/10916081/diff/41025/runtime/bin/tls_socket_pa...
File runtime/bin/tls_socket_patch.dart (right):

https://codereview.chromium.org/10916081/diff/41025/runtime/bin/tls_socket_pa...
runtime/bin/tls_socket_patch.dart:47: List<_TlsExternalBuffer> buffers;
On 2012/11/14 08:18:59, Søren Gjesse wrote:
> Add final.

I just made this non-final, for the sake of explicitly deleting it (and the
references to the _TlsExternalBuffers) before I free the external data backing
them.

This is not necessary, since no method in TlsFilterImpl should be called after
destroy, but I think it is good to do.

https://codereview.chromium.org/10916081/diff/41025/sdk/lib/io/tls_socket.dart
File sdk/lib/io/tls_socket.dart (right):

https://codereview.chromium.org/10916081/diff/41025/sdk/lib/io/tls_socket.dar...
sdk/lib/io/tls_socket.dart:112: }
On 2012/11/14 08:18:59, Søren Gjesse wrote:
> Indentation

Done.

https://codereview.chromium.org/10916081/diff/41025/sdk/lib/io/tls_socket.dar...
sdk/lib/io/tls_socket.dart:145: // until it blocks.  If the write blocks,
_writeEncryptedData sets
On 2012/11/14 08:18:59, Søren Gjesse wrote:
> Please change block to "would block" (or something like that) - we never
block.

Done.

https://codereview.chromium.org/10916081/diff/41025/sdk/lib/io/tls_socket.dar...
sdk/lib/io/tls_socket.dart:252: if ((_status == CONNECTED || _status == CLOSED)
&& plaintext.free > 0) {
On 2012/11/14 08:18:59, Søren Gjesse wrote:
> Why are we trying to process data when closed?

The status == CLOSED is the status when the underlying socket is closed.  We may
still have data in our read buffers and filter, and will not send a close event
until those are empty.  Maybe we should split this into two states, CLOSED and
CLOSE_PENDING.

https://codereview.chromium.org/10916081/diff/41025/sdk/lib/io/tls_socket.dar...
sdk/lib/io/tls_socket.dart:346: 
On 2012/11/14 08:18:59, Søren Gjesse wrote:
> Could this maybe be made into a real circular buffer?

Added as a TODO.  I would not do this until we have performance measurement in
place.  It adds complexity, and may not be hit in the normal use case.

[Søren Gjesse, 2012-11-14 13:43:11]

https://codereview.chromium.org/10916081/diff/41025/runtime/bin/tls_socket.cc
File runtime/bin/tls_socket.cc (right):

https://codereview.chromium.org/10916081/diff/41025/runtime/bin/tls_socket.cc...
runtime/bin/tls_socket.cc:188: Dart_NewExternalByteArray(buffers_[i],
buffer_size_, NULL, NULL));
On 2012/11/14 13:33:30, Bill Hesse wrote:
> On 2012/11/14 08:18:59, Søren Gjesse wrote:
> > You need to specify a peer pointer and callback here to free the buffer from
> the
> > C heap.
> 
> We are freeing the buffers manually in TlsFilter::Destroy, at the same time
that
> we delete the persistent handles to the _TlsExternalBuffer buffer objects
> holding the ExternalByteArrays.

Even though we are freeing the persistent handles there is no guarantee that the
Dart object will not stay alive for some time. It will then reference an already
freed memory area. I think we should just let the GC do the work.

[Bill Hesse, 2012-11-14 16:06:46]

https://codereview.chromium.org/10916081/diff/41025/runtime/bin/tls_socket.cc
File runtime/bin/tls_socket.cc (right):

https://codereview.chromium.org/10916081/diff/41025/runtime/bin/tls_socket.cc...
runtime/bin/tls_socket.cc:188: Dart_NewExternalByteArray(buffers_[i],
buffer_size_, NULL, NULL));
On 2012/11/14 13:43:12, Søren Gjesse wrote:
> On 2012/11/14 13:33:30, Bill Hesse wrote:
> > On 2012/11/14 08:18:59, Søren Gjesse wrote:
> > > You need to specify a peer pointer and callback here to free the buffer
from
> > the
> > > C heap.
> > 
> > We are freeing the buffers manually in TlsFilter::Destroy, at the same time
> that
> > we delete the persistent handles to the _TlsExternalBuffer buffer objects
> > holding the ExternalByteArrays.
> 
> Even though we are freeing the persistent handles there is no guarantee that
the
> Dart object will not stay alive for some time. It will then reference an
already
> freed memory area. I think we should just let the GC do the work.

OK.  Will change.

[Bob Nystrom, 2012-11-14 18:06:36]

http://codereview.chromium.org/10916081/diff/43003/lib/io/tls_socket.dart
File lib/io/tls_socket.dart (right):

http://codereview.chromium.org/10916081/diff/43003/lib/io/tls_socket.dart#new...
lib/io/tls_socket.dart:32: static final int kReadPlaintext = 0;
On 2012/11/12 11:39:08, Mads Ager wrote:
> > On 2012/11/01 10:09:01, Mads Ager wrote:
> > > This is using a different naming style than the other constants.
> > >
> On 2012/11/11 22:34:34, Bill Hesse wrote:
> > Should we be consistent?  Which do you prefer?
> > It does seem like the states are like enum constants, while the k-constants
> are
> > actual values.
> 
> Of course we should be consistent!
> 
> The are all just 'static final int' and we have a naming convention for those.
> In the rest of dart:io we have been using UPPER_CASE for all of these. Please
do
> so here as well. These are all constants and therefore use the same naming
> convention.
> 

Style nit. These are technically not constants, they're finals. Either of the
following would be OK according to the style guide:

static const READ_PLAINTEXT = 0;
static final readPlaintext = 0;

If it's "conceptually constant" but not an actual const declaration, it should
be camelCase without any prefix.

The idea here is that this way users of a library can tell which things can be
used in their own constant expressions and which cannot. If you were to name
this READ_PLAINTEXT but leave it static final, it would confuse users who then
get an error in something like const [READ_PLAINTEXT].