[iOS/OS X] Allow base::scoped_nsobject<> to be used when ARC is enabled.

[iOS/OS X] Allow base::scoped_nsobject<> to be used when ARC is enabled.

Add annotations (http://clang-analyzer.llvm.org/annotations.html) to
prevent clang from mucking with the objects reference count while
generating base::scoped_nsobject<> code when Automatic Reference
Counting (a.k.a. ARC) is enabled.

Use static function to perform explicit reference counting without
causing compilation errors when the file is included from a source
file with ARC enabled.

When using ARC, base::scoped_nsobject<> constructor and reset() method
have a different signature that does not allow passing the policy and
default to base::scoped_policy::RETAIN (this cause no visible change as
ARC insert a call to release). The ns_consumed annotation is supposed
to inform ARC not to insert that call, but does not work (see upstream
issue https://llvm.org/bugs/show_bug.cgi?id=27887).

Design document:
https://docs.google.com/document/d/1dEZ-pdHP6Q32yffHtrQjeUDGAD1MuPGaGOrMfdi1ihk/edit#

BUG=599992
Committed: https://crrev.com/fd625125b8a6a3aceaf09993a5f74cfe5368b17f
Cr-Commit-Position: refs/heads/master@{#400132}

[sdefresne, 2016-04-01 21:23:08]

sdefresne@chromium.org changed reviewers:
+ mark@chromium.org, thakis@chromium.org

[sdefresne, 2016-04-01 21:23:09]

I've only tested this on iOS, so it may break on the bots, but could you take a
look to see if the general idea is viable or not.

Regarding __attribute((ns_consumed)), if the two bugs mentioned are fixed and we
want to use it, we may have to do some refactoring on ScopedTypedRef (not sure
how this can be done cleanly though) or duplicate the code (I hope not).

[sdefresne, 2016-04-01 21:28:16]

Description was changed from

==========
[iOS/OS X] Allow base::scoped_nsobject<> to be used when ARC is enabled.

Add annotations (http://clang-analyzer.llvm.org/annotations.html) to
prevent clang from mucking with the objects reference count while
generating base::scoped_nsobject<> code when Automatic Reference
Counting (a.k.a. ARC) is enabled.

Use static function to perform explicit reference counting without
causing compilation errors when the file is included from a source
file with ARC enabled.

When using ARC, base::scoped_nsobject<> constructor and reset() method
must be passed base::scoped_policy::RETAIN as second argument otherwise
the object will be deallocated by ARC.

The annotation __attribute((ns_consumed)) is supposed to fix this, but
it does not work with ToT clang due to bugs 599978 and 599980. Another
problem is that ScopedTypeRef<> is used with both NSObject and CFTypeRef
and the __attribute((ns_consumed)) does not work with CFTypeRef, and
cannot be set on a template parameter.

BUG=None
==========

to

==========
[iOS/OS X] Allow base::scoped_nsobject<> to be used when ARC is enabled.

Add annotations (http://clang-analyzer.llvm.org/annotations.html) to
prevent clang from mucking with the objects reference count while
generating base::scoped_nsobject<> code when Automatic Reference
Counting (a.k.a. ARC) is enabled.

Use static function to perform explicit reference counting without
causing compilation errors when the file is included from a source
file with ARC enabled.

When using ARC, base::scoped_nsobject<> constructor and reset() method
must be passed base::scoped_policy::RETAIN as second argument otherwise
the object will be deallocated by ARC.

The annotation __attribute((ns_consumed)) is supposed to fix this, but
it does not work with ToT clang due to bugs 599978 and 599980. Another
problem is that ScopedTypeRef<> is used with both NSObject and CFTypeRef
and the __attribute((ns_consumed)) does not work with CFTypeRef, and
cannot be set on a template parameter.

BUG=599992
==========

[sdefresne, 2016-04-04 01:57:04]

+stkhapugin: FYI

[sdefresne, 2016-04-04 01:57:28]

+rohitrao: FYI

[Nico, 2016-04-04 17:53:45]

The code looks like it might work.

I think this could benefit from a design document though, that explains what the
desired end state is, what the chosen path to get there is, risks along the way,
etc.


Outline something like the following, I suppose:
"""
Background -- what's ObjC's memory mgt model & ref counting; what's arc
(requires some runtime supp for zeroing weak refs; enabled by compiler switch)

Overview -- want to be able to use arc in chrome and eventually everywhere,
don't want to big-bang switch, main motivation is ios; will switch
target-by-target. advantages of switching. describe desired end state.

Risks & Mitigations

- due to target-by-target switching, lower-level libraries (e.g. base) must work
in both arc and non-arc mode, here's what doesn't work and how we'll fix

- ios most motivated -- what guarantees that everything gets converted (is this
even desired? probably yes, since else you need to always remember if arc is
enabled for a target or not, since the same code line might be correct when arc
is on but buggy when it isn't). what about third-party code

Alternatives considered
"""

Once that's in decent shape, probably want to send one of those "intent to
implement" threads to chromium-dev (i.e. make document public)

https://codereview.chromium.org/1855483004/diff/20001/base/mac/scoped_block.mm
File base/mac/scoped_block.mm (right):

https://codereview.chromium.org/1855483004/diff/20001/base/mac/scoped_block.m...
base/mac/scoped_block.mm:6: 
Is the idea that we'll have this file around in non-arc mode in perpetuity? If
not, when would we remove it?

https://codereview.chromium.org/1855483004/diff/20001/base/mac/scoped_nsobject.h
File base/mac/scoped_nsobject.h (right):

https://codereview.chromium.org/1855483004/diff/20001/base/mac/scoped_nsobjec...
base/mac/scoped_nsobject.h:54: static NST InvalidValue()
__attribute((ns_returns_not_retained)) {
is this one needed? [nil retain] is a noop, right?

https://codereview.chromium.org/1855483004/diff/20001/base/mac/scoped_nsobjec...
base/mac/scoped_nsobject.h:57: static NST Retain(__unsafe_unretained NST nst)
`Retain() returns_not_retained` needs a comment on why returns_not_retained is
correct on a method called Retain :-)

https://codereview.chromium.org/1855483004/diff/20001/base/mac/scoped_nsobjec...
base/mac/scoped_nsobject.h:98: class scoped_nsobject : public
scoped_nsprotocol<NST*> {
in arc mode, you probably want to add a static_assert(policy == RETAIN)
somewhere, if that's a requirement.

It seems like a somewhat unfortunate requirement.

https://codereview.chromium.org/1855483004/diff/20001/build/config/compiler/B...
File build/config/compiler/BUILD.gn (right):

https://codereview.chromium.org/1855483004/diff/20001/build/config/compiler/B...
build/config/compiler/BUILD.gn:1297: cflags = [ "-gdwarf-2" ]
?

https://codereview.chromium.org/1855483004/diff/20001/ios/web/net/cert_verifi...
File ios/web/net/cert_verifier_block_adapter.mm (right):

https://codereview.chromium.org/1855483004/diff/20001/ios/web/net/cert_verifi...
ios/web/net/cert_verifier_block_adapter.mm:3: // found in the LICENSE file.
why is this a .mm file now?

[Mark Mentovai, 2016-04-05 00:09:15]

I echo Nico’s comments.

https://codereview.chromium.org/1855483004/diff/20001/base/mac/scoped_block.h
File base/mac/scoped_block.h (right):

https://codereview.chromium.org/1855483004/diff/20001/base/mac/scoped_block.h...
base/mac/scoped_block.h:17: id ScopedBlockTraitsRetain(id block);
Blocks are normally typed as const void*. If you use that, does it break ARC?
That would be worth a comment, because the type mismatch compared to <Block.h>
is obvious.

Does this file need to be #included by any non-Objective C++ files?

https://codereview.chromium.org/1855483004/diff/20001/base/mac/scoped_block.mm
File base/mac/scoped_block.mm (right):

https://codereview.chromium.org/1855483004/diff/20001/base/mac/scoped_block.m...
base/mac/scoped_block.mm:14: void ScopedBlockTraitsRelease(id block) {
nit: Blank line between functions.

[sdefresne, 2016-06-08 09:38:03]

Patchset #9 (id:160001) has been deleted

[sdefresne, 2016-06-09 08:45:08]

sdefresne@chromium.org changed reviewers:
+ rohitrao@chromium.org

[sdefresne, 2016-06-09 08:45:09]

Mark/Nico:

Could you take a look at the latest patchset that uses the workaround  for
ns_consumed bug (https://llvm.org/bugs/show_bug.cgi?id=27887) and tell me what
you think of it?

[sdefresne, 2016-06-10 17:07:47]

Ping?

[Mark Mentovai, 2016-06-10 20:14:22]

I think that this looks fine now.

Nico had asked for a design doc, do we have something like that? Six months from
now, it’ll be tough to glean the design decisions from a chain of code review
comments and e-mails, some of which were private.

[Nico, 2016-06-10 20:18:39]

I'm not surprisingly in favor of fixing the compiler instead. but if Mark likes
this then go ahead with this :-)

[sdefresne, 2016-06-14 17:46:50]

Description was changed from

==========
[iOS/OS X] Allow base::scoped_nsobject<> to be used when ARC is enabled.

Add annotations (http://clang-analyzer.llvm.org/annotations.html) to
prevent clang from mucking with the objects reference count while
generating base::scoped_nsobject<> code when Automatic Reference
Counting (a.k.a. ARC) is enabled.

Use static function to perform explicit reference counting without
causing compilation errors when the file is included from a source
file with ARC enabled.

When using ARC, base::scoped_nsobject<> constructor and reset() method
must be passed base::scoped_policy::RETAIN as second argument otherwise
the object will be deallocated by ARC.

The annotation __attribute((ns_consumed)) is supposed to fix this, but
it does not work with ToT clang due to bugs 599978 and 599980. Another
problem is that ScopedTypeRef<> is used with both NSObject and CFTypeRef
and the __attribute((ns_consumed)) does not work with CFTypeRef, and
cannot be set on a template parameter.

BUG=599992
==========

to

==========
[iOS/OS X] Allow base::scoped_nsobject<> to be used when ARC is enabled.

Add annotations (http://clang-analyzer.llvm.org/annotations.html) to
prevent clang from mucking with the objects reference count while
generating base::scoped_nsobject<> code when Automatic Reference
Counting (a.k.a. ARC) is enabled.

Use static function to perform explicit reference counting without
causing compilation errors when the file is included from a source
file with ARC enabled.

When using ARC, base::scoped_nsobject<> constructor and reset() method
must be passed base::scoped_policy::RETAIN as second argument otherwise
the object will be deallocated by ARC.

The annotation __attribute((ns_consumed)) is supposed to fix this, but
it does not work with ToT clang due to bugs 599978 and 599980. Another
problem is that ScopedTypeRef<> is used with both NSObject and CFTypeRef
and the __attribute((ns_consumed)) does not work with CFTypeRef, and
cannot be set on a template parameter.

Design document:
https://docs.google.com/document/d/1dEZ-pdHP6Q32yffHtrQjeUDGAD1MuPGaGOrMfdi1i...

BUG=599992
==========

[sdefresne, 2016-06-14 17:47:13]

On 2016/06/10 20:14:22, Mark Mentovai wrote:
> I think that this looks fine now.
> 
> Nico had asked for a design doc, do we have something like that? Six months
from
> now, it’ll be tough to glean the design decisions from a chain of code review
> comments and e-mails, some of which were private.

Design document is here (updated description to include the link too):
https://docs.google.com/document/d/1dEZ-pdHP6Q32yffHtrQjeUDGAD1MuPGaGOrMfdi1i...

[Mark Mentovai, 2016-06-14 18:17:31]

I am in favor of fixing the compiler too. In fact, it’s my preference.

Nico, you’re in that world. Can we lean on anyone for help with
https://llvm.org/bugs/show_bug.cgi?id=27887?

I have no problem landing this as-is now while we wait for clang to be fixed.

LGTM

[Nico, 2016-06-14 18:29:35]

On 2016/06/14 18:17:31, Mark Mentovai wrote:
> I am in favor of fixing the compiler too. In fact, it’s my preference.
> 
> Nico, you’re in that world. Can we lean on anyone for help with
> https://llvm.org/bugs/show_bug.cgi?id=27887?

sdefresne :-P

I don't think apple cares about this use case, else this would already work.
rjmccall is probably willing to give advice, but I'd expect that we'd have to
fix this ourselves.

[sdefresne, 2016-06-16 10:40:05]

Description was changed from

==========
[iOS/OS X] Allow base::scoped_nsobject<> to be used when ARC is enabled.

Add annotations (http://clang-analyzer.llvm.org/annotations.html) to
prevent clang from mucking with the objects reference count while
generating base::scoped_nsobject<> code when Automatic Reference
Counting (a.k.a. ARC) is enabled.

Use static function to perform explicit reference counting without
causing compilation errors when the file is included from a source
file with ARC enabled.

When using ARC, base::scoped_nsobject<> constructor and reset() method
must be passed base::scoped_policy::RETAIN as second argument otherwise
the object will be deallocated by ARC.

The annotation __attribute((ns_consumed)) is supposed to fix this, but
it does not work with ToT clang due to bugs 599978 and 599980. Another
problem is that ScopedTypeRef<> is used with both NSObject and CFTypeRef
and the __attribute((ns_consumed)) does not work with CFTypeRef, and
cannot be set on a template parameter.

Design document:
https://docs.google.com/document/d/1dEZ-pdHP6Q32yffHtrQjeUDGAD1MuPGaGOrMfdi1i...

BUG=599992
==========

to

==========
[iOS/OS X] Allow base::scoped_nsobject<> to be used when ARC is enabled.

Add annotations (http://clang-analyzer.llvm.org/annotations.html) to
prevent clang from mucking with the objects reference count while
generating base::scoped_nsobject<> code when Automatic Reference
Counting (a.k.a. ARC) is enabled.

Use static function to perform explicit reference counting without
causing compilation errors when the file is included from a source
file with ARC enabled.

When using ARC, base::scoped_nsobject<> constructor and reset() method
have a different signature that does not allow passing the policy and default
to base::scoped_policy::RETAIN (this cause no visible change as ARC insert
a call to release). The ns_consumed annotation is supposed to inform ARC
not to insert that call, but does not work, see [1].

[1]: https://llvm.org/bugs/show_bug.cgi?id=27887.

Design document:
https://docs.google.com/document/d/1dEZ-pdHP6Q32yffHtrQjeUDGAD1MuPGaGOrMfdi1i...

BUG=599992
==========

[sdefresne, 2016-06-16 10:41:08]

Description was changed from

==========
[iOS/OS X] Allow base::scoped_nsobject<> to be used when ARC is enabled.

Add annotations (http://clang-analyzer.llvm.org/annotations.html) to
prevent clang from mucking with the objects reference count while
generating base::scoped_nsobject<> code when Automatic Reference
Counting (a.k.a. ARC) is enabled.

Use static function to perform explicit reference counting without
causing compilation errors when the file is included from a source
file with ARC enabled.

When using ARC, base::scoped_nsobject<> constructor and reset() method
have a different signature that does not allow passing the policy and default
to base::scoped_policy::RETAIN (this cause no visible change as ARC insert
a call to release). The ns_consumed annotation is supposed to inform ARC
not to insert that call, but does not work, see [1].

[1]: https://llvm.org/bugs/show_bug.cgi?id=27887.

Design document:
https://docs.google.com/document/d/1dEZ-pdHP6Q32yffHtrQjeUDGAD1MuPGaGOrMfdi1i...

BUG=599992
==========

to

==========
[iOS/OS X] Allow base::scoped_nsobject<> to be used when ARC is enabled.

Add annotations (http://clang-analyzer.llvm.org/annotations.html) to
prevent clang from mucking with the objects reference count while
generating base::scoped_nsobject<> code when Automatic Reference
Counting (a.k.a. ARC) is enabled.

Use static function to perform explicit reference counting without
causing compilation errors when the file is included from a source
file with ARC enabled.

When using ARC, base::scoped_nsobject<> constructor and reset() method
have a different signature that does not allow passing the policy and
default to base::scoped_policy::RETAIN (this cause no visible change as
ARC insert a call to release). The ns_consumed annotation is supposed
to inform ARC not to insert that call, but does not work (see upstream
issue https://llvm.org/bugs/show_bug.cgi?id=27887).

Design document:
https://docs.google.com/document/d/1dEZ-pdHP6Q32yffHtrQjeUDGAD1MuPGaGOrMfdi1i...

BUG=599992
==========

[sdefresne, 2016-06-16 10:41:21]

Patchset #12 (id:240001) has been deleted

[sdefresne, 2016-06-16 10:46:26]

On 2016/06/14 18:29:35, Nico (traveling...slow) wrote:
> On 2016/06/14 18:17:31, Mark Mentovai wrote:
> > I am in favor of fixing the compiler too. In fact, it’s my preference.
> > 
> > Nico, you’re in that world. Can we lean on anyone for help with
> > https://llvm.org/bugs/show_bug.cgi?id=27887?
> 
> sdefresne :-P
> 
> I don't think apple cares about this use case, else this would already work.
> rjmccall is probably willing to give advice, but I'd expect that we'd have to
> fix this ourselves.

Thank you both for the reviews.

I also think that we should fix the bug in clang itself. I'll probably have time
to dedicate to this once the migration to GN is complete (I'm currently the main
contributor to the migration on iOS).

[sdefresne, 2016-06-16 11:47:18]

The CQ bit was checked by sdefresne@chromium.org

[sdefresne, 2016-06-16 11:47:19]

The patchset sent to the CQ was uploaded after l-g-t-m from mark@chromium.org
Link to the patchset: https://codereview.chromium.org/1855483004/#ps260001
(title: "Add missing copy constructor from scoped_nsobject<subclass> to
scoped_nsobject<class>")

[commit-bot: I haz the power, 2016-06-16 11:47:35]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1855483004/260001

[commit-bot: I haz the power, 2016-06-16 11:51:21]

Description was changed from

==========
[iOS/OS X] Allow base::scoped_nsobject<> to be used when ARC is enabled.

Add annotations (http://clang-analyzer.llvm.org/annotations.html) to
prevent clang from mucking with the objects reference count while
generating base::scoped_nsobject<> code when Automatic Reference
Counting (a.k.a. ARC) is enabled.

Use static function to perform explicit reference counting without
causing compilation errors when the file is included from a source
file with ARC enabled.

When using ARC, base::scoped_nsobject<> constructor and reset() method
have a different signature that does not allow passing the policy and
default to base::scoped_policy::RETAIN (this cause no visible change as
ARC insert a call to release). The ns_consumed annotation is supposed
to inform ARC not to insert that call, but does not work (see upstream
issue https://llvm.org/bugs/show_bug.cgi?id=27887).

Design document:
https://docs.google.com/document/d/1dEZ-pdHP6Q32yffHtrQjeUDGAD1MuPGaGOrMfdi1i...

BUG=599992
==========

to

==========
[iOS/OS X] Allow base::scoped_nsobject<> to be used when ARC is enabled.

Add annotations (http://clang-analyzer.llvm.org/annotations.html) to
prevent clang from mucking with the objects reference count while
generating base::scoped_nsobject<> code when Automatic Reference
Counting (a.k.a. ARC) is enabled.

Use static function to perform explicit reference counting without
causing compilation errors when the file is included from a source
file with ARC enabled.

When using ARC, base::scoped_nsobject<> constructor and reset() method
have a different signature that does not allow passing the policy and
default to base::scoped_policy::RETAIN (this cause no visible change as
ARC insert a call to release). The ns_consumed annotation is supposed
to inform ARC not to insert that call, but does not work (see upstream
issue https://llvm.org/bugs/show_bug.cgi?id=27887).

Design document:
https://docs.google.com/document/d/1dEZ-pdHP6Q32yffHtrQjeUDGAD1MuPGaGOrMfdi1i...

BUG=599992
==========

[commit-bot: I haz the power, 2016-06-16 11:51:25]

Committed patchset #12 (id:260001)

[commit-bot: I haz the power, 2016-06-16 11:51:30]

CQ bit was unchecked

[commit-bot: I haz the power, 2016-06-16 11:53:00]

Description was changed from

==========
[iOS/OS X] Allow base::scoped_nsobject<> to be used when ARC is enabled.

Add annotations (http://clang-analyzer.llvm.org/annotations.html) to
prevent clang from mucking with the objects reference count while
generating base::scoped_nsobject<> code when Automatic Reference
Counting (a.k.a. ARC) is enabled.

Use static function to perform explicit reference counting without
causing compilation errors when the file is included from a source
file with ARC enabled.

When using ARC, base::scoped_nsobject<> constructor and reset() method
have a different signature that does not allow passing the policy and
default to base::scoped_policy::RETAIN (this cause no visible change as
ARC insert a call to release). The ns_consumed annotation is supposed
to inform ARC not to insert that call, but does not work (see upstream
issue https://llvm.org/bugs/show_bug.cgi?id=27887).

Design document:
https://docs.google.com/document/d/1dEZ-pdHP6Q32yffHtrQjeUDGAD1MuPGaGOrMfdi1i...

BUG=599992
==========

to

==========
[iOS/OS X] Allow base::scoped_nsobject<> to be used when ARC is enabled.

Add annotations (http://clang-analyzer.llvm.org/annotations.html) to
prevent clang from mucking with the objects reference count while
generating base::scoped_nsobject<> code when Automatic Reference
Counting (a.k.a. ARC) is enabled.

Use static function to perform explicit reference counting without
causing compilation errors when the file is included from a source
file with ARC enabled.

When using ARC, base::scoped_nsobject<> constructor and reset() method
have a different signature that does not allow passing the policy and
default to base::scoped_policy::RETAIN (this cause no visible change as
ARC insert a call to release). The ns_consumed annotation is supposed
to inform ARC not to insert that call, but does not work (see upstream
issue https://llvm.org/bugs/show_bug.cgi?id=27887).

Design document:
https://docs.google.com/document/d/1dEZ-pdHP6Q32yffHtrQjeUDGAD1MuPGaGOrMfdi1i...

BUG=599992

Committed: https://crrev.com/fd625125b8a6a3aceaf09993a5f74cfe5368b17f
Cr-Commit-Position: refs/heads/master@{#400132}
==========

[commit-bot: I haz the power, 2016-06-16 11:53:01]

Patchset 12 (id:??) landed as
https://crrev.com/fd625125b8a6a3aceaf09993a5f74cfe5368b17f
Cr-Commit-Position: refs/heads/master@{#400132}