New issue
Advanced search Search tips

Issue 637065 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Aug 2016
Cc:
EstimatedDays: ----
NextAction: ----
OS: iOS , Mac
Pri: 3
Type: Bug



Sign in to add a comment

ARC migration: scoped_nsobject_unittest{,_arc}.mm tests success failure depends on the linking order

Project Member Reported by sdefresne@chromium.org, Aug 11 2016

Issue description

With https://codereview.chromium.org/2219323002 I introduced an intermediate source_set when linking executable on iOS. This reordered the file on the command-line when running the linker which cause failure in scoped_nsobject_unittest{,_arc}.mm on 32-bit architecture for fat binaries (but could have been any combination, pure luck).

[ RUN      ] ScopedNSObjectTest.ScopedNSObject
../../base/mac/scoped_nsobject_unittest.mm:37: Failure
Value of: [p1 retainCount]
  Actual: 3
Expected: 2u
Which is: 2
[  FAILED  ] ScopedNSObjectTest.ScopedNSObject (0 ms)

After investigation, it is due to the ARC migration. The call "p3 = p1;" invokes the base::scoped_nsobject<NSObject>::operator =(const base::scoped_nsobject<NSObject>&) operator that ends up calling base::ScopedTypeRef<NSObject>::get() method. Since this is a templated method, the linker only keep one of the multiple instantiations.

As the same binary links both scoped_nsobject_unittest{,_arc}.o objects files, there are at least two implementation of that method, one compiled without ARC and one compiled with ARC. Without the intermediate source_set, the non-ARC version is selected, with the source_set, the ARC version is selected.

The non-ARC version does not contains any call to retain/release while the ARC version does call objc_retainAutoreleaseAndReturn. Those are functionally equivalent the retain/release calls are balanced, but the ARC version cause the refcount to be increased until the current autorelease pool is released.

Putting the "p3 = p1;" in a separate block with an explicit auto-release pool drain fix the test in all configurations.
 
Project Member

Comment 1 by bugdroid1@chromium.org, Aug 12 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/269f2a7139aa0df611b294d0d4e23b6ca4791915

commit 269f2a7139aa0df611b294d0d4e23b6ca4791915
Author: sdefresne <sdefresne@chromium.org>
Date: Fri Aug 12 19:25:08 2016

Fix ScopedNSObject tests to deal with selection of ScopedTypeRef::get().

As the same binary links both scoped_nsobject_unittest{,_arc}.o objects
files, there are at least two implementation of that method, one compiled
without ARC and one compiled with ARC. Depending on the order of object
files on the linker command-line the implementation selected may change.

The non-ARC version does not contains any call to retain/release while the
ARC version does call objc_retainAutoreleaseAndReturn. Those are functionally
equivalent the retain/release calls are balanced, but the ARC version cause
the refcount to be increased until the current autorelease pool is released.

Putting the "p3 = p1;" in a separate block with an explicit auto-release pool
drain fix the test in all configurations.

BUG= 637065 

Review-Url: https://codereview.chromium.org/2235353003
Cr-Commit-Position: refs/heads/master@{#411730}

[modify] https://crrev.com/269f2a7139aa0df611b294d0d4e23b6ca4791915/base/mac/scoped_nsobject_unittest.mm
[modify] https://crrev.com/269f2a7139aa0df611b294d0d4e23b6ca4791915/base/mac/scoped_nsobject_unittest_arc.mm

Status: Fixed (was: Started)
Project Member

Comment 3 by bugdroid1@chromium.org, Aug 12 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/40865bdc7372f779689422a0c36cf8073d050366

commit 40865bdc7372f779689422a0c36cf8073d050366
Author: sdefresne <sdefresne@chromium.org>
Date: Fri Aug 12 21:30:37 2016

Always enable intermediate source_set.

The _use_intermediate_source_set variable was there to workaround a
bug in scoped_nsobject_unittest{,_arc}.mm that was exhibited by the
introduction of an intermediate source_set. Remove the variable and
consider that it is always set to "true".

BUG= 637065 

Review-Url: https://codereview.chromium.org/2245513002
Cr-Commit-Position: refs/heads/master@{#411779}

[modify] https://crrev.com/40865bdc7372f779689422a0c36cf8073d050366/build/config/ios/rules.gni

Project Member

Comment 4 by bugdroid1@chromium.org, Aug 13 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/8bcd27585d5c063b9c19aba05ffe310781181402

commit 8bcd27585d5c063b9c19aba05ffe310781181402
Author: justincohen <justincohen@chromium.org>
Date: Sat Aug 13 03:59:36 2016

Revert of Always enable intermediate source_set. (patchset #2 id:20001 of https://codereview.chromium.org/2245513002/ )

Reason for revert:
This is breaking earl grey tests downstream.  I wonder why upstream eg tests aren't on the cq?

Before:
          cmd LC_ID_DYLIB
      cmdsize 64
         name @rpath/EarlGrey.framework/EarlGrey (offset 24)
After:
          cmd LC_ID_DYLIB
      cmdsize 72
         name obj/ios/third_party/earl_grey/x64/EarlGrey (offset 24)

Original issue's description:
> Always enable intermediate source_set.
>
> The _use_intermediate_source_set variable was there to workaround a
> bug in scoped_nsobject_unittest{,_arc}.mm that was exhibited by the
> introduction of an intermediate source_set. Remove the variable and
> consider that it is always set to "true".
>
> BUG= 637065 
>
> Committed: https://crrev.com/40865bdc7372f779689422a0c36cf8073d050366
> Cr-Commit-Position: refs/heads/master@{#411779}

TBR=dpranke@chromium.org,sdefresne@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG= 637065 

Review-Url: https://codereview.chromium.org/2245903002
Cr-Commit-Position: refs/heads/master@{#411873}

[modify] https://crrev.com/8bcd27585d5c063b9c19aba05ffe310781181402/build/config/ios/rules.gni

Project Member

Comment 5 by bugdroid1@chromium.org, Sep 6 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/c6c0b40fcf2fd87ca23e90f6bd352267b9fd080f

commit c6c0b40fcf2fd87ca23e90f6bd352267b9fd080f
Author: sdefresne <sdefresne@chromium.org>
Date: Tue Sep 06 16:34:44 2016

Fix ios_framework_bundle target when using intermediate source sets.

Split the config that add the header map to the include dirs and pass
the -install_name flag to the linker as the former is only used while
compiling the source file and the latter is only used by the link step.

BUG= 637065 

Review-Url: https://codereview.chromium.org/2314053002
Cr-Commit-Position: refs/heads/master@{#416639}

[modify] https://crrev.com/c6c0b40fcf2fd87ca23e90f6bd352267b9fd080f/build/config/ios/rules.gni

Project Member

Comment 6 by bugdroid1@chromium.org, Sep 7 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/0cbc7aa6773e0085993968788af9cc4efafc4b81

commit 0cbc7aa6773e0085993968788af9cc4efafc4b81
Author: sdefresne <sdefresne@chromium.org>
Date: Wed Sep 07 11:08:43 2016

Enable use of intermediate source set by iOS templates.

Enable use of intermediate source set by ios_app_bundle and
ios_framework_bundle templates as this should reduce false
dependencies.

BUG= 637065 

Review-Url: https://codereview.chromium.org/2317703002
Cr-Commit-Position: refs/heads/master@{#416906}

[modify] https://crrev.com/0cbc7aa6773e0085993968788af9cc4efafc4b81/build/config/ios/rules.gni

Project Member

Comment 7 by bugdroid1@chromium.org, Sep 8 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/5475013e3ff37f8882f7bf58911b655ce9ab2f04

commit 5475013e3ff37f8882f7bf58911b655ce9ab2f04
Author: sdefresne <sdefresne@chromium.org>
Date: Thu Sep 08 16:51:45 2016

Remove _use_intermediate_source_set variable.

As the variable is now true (it's end state), it can be removed and
the corresponding legacy code.

BUG= 637065 

Review-Url: https://codereview.chromium.org/2316683002
Cr-Commit-Position: refs/heads/master@{#417316}

[modify] https://crrev.com/5475013e3ff37f8882f7bf58911b655ce9ab2f04/build/config/ios/rules.gni

Sign in to add a comment