Implements remaining SecurityContext calls for iOS

Implements remaining SecurityContext calls for iOS

BUG=
R=asiva@google.com, iposva@google.com

Committed: https://github.com/dart-lang/sdk/commit/522ce88fe644cb98317f745f118443a8bc924703

[zra, 2016-04-01 20:31:20]

zra@google.com changed reviewers:
+ iposva@google.com

[zra, 2016-04-04 16:44:15]

Updated API docs. PTAL.

[zra, 2016-04-06 20:51:58]

zra@google.com changed reviewers:
+ asiva@google.com

[zra, 2016-04-06 20:52:04]

Offline, we determined it was necessary to reference count the native filter and
certificate context objects to avoid dangling pointers in the case that
finalizers for their Dart wrappers race with the IO Service thread. I've added
the reference counting, PTAL.

[zra, 2016-04-07 20:21:26]

ping

[Ivan Posva, 2016-04-08 13:18:21]

LGTM, but I would prefer if Siva also takes a look at the reference counting.

-Ivan

https://codereview.chromium.org/1852783003/diff/100001/runtime/bin/reference_...
File runtime/bin/reference_counting.h (right):

https://codereview.chromium.org/1852783003/diff/100001/runtime/bin/reference_...
runtime/bin/reference_counting.h:72: ASSERT(target_ != NULL);
ASSERT that ref_count_ is greater than 1?

https://codereview.chromium.org/1852783003/diff/100001/runtime/bin/reference_...
runtime/bin/reference_counting.h:85: template <class Target>
There is good documentation for the above classes, which seems to be missing
from here. Can you please add it, as it is hard to understand the purpose of
this class otherwise.

[zra, 2016-04-08 16:07:48]

https://codereview.chromium.org/1852783003/diff/100001/runtime/bin/reference_...
File runtime/bin/reference_counting.h (right):

https://codereview.chromium.org/1852783003/diff/100001/runtime/bin/reference_...
runtime/bin/reference_counting.h:72: ASSERT(target_ != NULL);
On 2016/04/08 13:18:21, Ivan Posva wrote:
> ASSERT that ref_count_ is greater than 1?

Done.

https://codereview.chromium.org/1852783003/diff/100001/runtime/bin/reference_...
runtime/bin/reference_counting.h:85: template <class Target>
On 2016/04/08 13:18:21, Ivan Posva wrote:
> There is good documentation for the above classes, which seems to be missing
> from here. Can you please add it, as it is hard to understand the purpose of
> this class otherwise.

Done.

[siva, 2016-04-12 19:03:01]

LGTM.

I had a question regarding the public stuff being exposed from one of the Scope
classes.

https://codereview.chromium.org/1852783003/diff/120001/runtime/bin/reference_...
File runtime/bin/reference_counting.h (right):

https://codereview.chromium.org/1852783003/diff/120001/runtime/bin/reference_...
runtime/bin/reference_counting.h:52: delete static_cast<Derived*>(this);
You are using atomic operations so I assume the intent is to be thread safe.

How can we be sure that another thread is not running Retain() just after we
have finished the atomic decrement and before we delete this?

https://codereview.chromium.org/1852783003/diff/120001/runtime/bin/reference_...
runtime/bin/reference_counting.h:57: uintptr_t ref_count() const { return
ref_count_; }
ref_count() is private and never used in this class why do we need it?

https://codereview.chromium.org/1852783003/diff/120001/runtime/bin/reference_...
runtime/bin/reference_counting.h:147: }
Should this 'get' method be provided because it makes the class unsafe, somebody
could do a get and then do various number of Retain/Release operations and mess
up the scoping (e.g: more Releases than Retains.

https://codereview.chromium.org/1852783003/diff/120001/runtime/bin/secure_soc...
File runtime/bin/secure_socket_ios.h (right):

https://codereview.chromium.org/1852783003/diff/120001/runtime/bin/secure_soc...
runtime/bin/secure_socket_ios.h:47: delete mutex_;
Should we delete this mutex at the end of the constructor? CFRelease is called
holding the lock in most places below and hence I was wondering if we should do
the same here and then delete the mutex.

[zra, 2016-04-13 16:46:21]

https://codereview.chromium.org/1852783003/diff/120001/runtime/bin/reference_...
File runtime/bin/reference_counting.h (right):

https://codereview.chromium.org/1852783003/diff/120001/runtime/bin/reference_...
runtime/bin/reference_counting.h:52: delete static_cast<Derived*>(this);
On 2016/04/12 19:03:01, siva wrote:
> You are using atomic operations so I assume the intent is to be thread safe.
> 
> How can we be sure that another thread is not running Retain() just after we
> have finished the atomic decrement and before we delete this?

Changed so that ref_count_ is initialized to 1 in the constructor. Then, an
explicit call to Retain() is changed to assert that old > 0 after the increment.

https://codereview.chromium.org/1852783003/diff/120001/runtime/bin/reference_...
runtime/bin/reference_counting.h:57: uintptr_t ref_count() const { return
ref_count_; }
On 2016/04/12 19:03:00, siva wrote:
> ref_count() is private and never used in this class why do we need it?

It's used only for the ASSERT below in RefCntReleaseScope. I've added a comment.

https://codereview.chromium.org/1852783003/diff/120001/runtime/bin/reference_...
runtime/bin/reference_counting.h:147: }
On 2016/04/12 19:03:01, siva wrote:
> Should this 'get' method be provided because it makes the class unsafe,
somebody
> could do a get and then do various number of Retain/Release operations and
mess
> up the scoping (e.g: more Releases than Retains.

When the RetainedPointer is a field in a class, this is the only way to access
the underlying pointer. Being able to pass the pointer to someone else who
Retains it is the intended way to use these.

https://codereview.chromium.org/1852783003/diff/120001/runtime/bin/secure_soc...
File runtime/bin/secure_socket_ios.h (right):

https://codereview.chromium.org/1852783003/diff/120001/runtime/bin/secure_soc...
runtime/bin/secure_socket_ios.h:47: delete mutex_;
On 2016/04/12 19:03:01, siva wrote:
> Should we delete this mutex at the end of the constructor? CFRelease is called
> holding the lock in most places below and hence I was wondering if we should
do
> the same here and then delete the mutex.

Done.

[zra, 2016-04-13 17:27:42]

Description was changed from

==========
Implements remaining SecurityContext calls for iOS

BUG=
==========

to

==========
Implements remaining SecurityContext calls for iOS

BUG=
R=asiva@google.com, iposva@google.com

Committed:
https://github.com/dart-lang/sdk/commit/522ce88fe644cb98317f745f118443a8bc924703
==========

[zra, 2016-04-13 17:27:45]

Committed patchset #8 (id:140001) manually as
522ce88fe644cb98317f745f118443a8bc924703 (presubmit successful).