Bug 184799 – Make WAKScrollView delegate a weak property

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[David Kilzer (:ddkilzer)]

Make WAKScrollView delegate a weak property to prevent crashes where the delegate has been released.

<rdar://problem/39469669>

[David Kilzer (:ddkilzer)]

Created attachment 338374 [details]
Patch v1

[David Kilzer (:ddkilzer)]

ERROR: Source/WebCore/platform/ios/wak/WAKScrollView.mm:65:  Should not have spaces around = in property synthesis.  [whitespace/property] [4]

Should I just delete the _delegate instance variable?

[EWS Watchlist]

Attachment 338374 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/ios/wak/WAKScrollView.mm:65:  Should not have spaces around = in property synthesis.  [whitespace/property] [4]
Total errors found: 1 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[David Kilzer (:ddkilzer)]

(In reply to Build Bot from comment #3)
> Attachment 338374 [details] did not pass style-queue:
> 
> 
> ERROR: Source/WebCore/platform/ios/wak/WAKScrollView.mm:65:  Should not have
> spaces around = in property synthesis.  [whitespace/property] [4]
> Total errors found: 1 in 3 files
> 
> 
> If any of these errors are false positives, please file a bug against
> check-webkit-style.

Ha!  Even better reason to remove it (build failure on iOS Simulator):

In file included from ./platform/ios/wak/WAKScrollView.mm:27:
./platform/ios/wak/WAKScrollView.h:41:8: error: existing instance variable '_delegate' for __weak property 'delegate' must be __weak
    id _delegate;
       ^
./platform/ios/wak/WAKScrollView.h:60:32: note: property declared here
@property (nonatomic, weak) id delegate;
                               ^
In file included from /Volumes/Data/EWS/WebKit/WebKitBuild/Release-iphonesimulator/DerivedSources/WebCore/unified-sources/UnifiedSource45-mm.mm:1:
./platform/ios/wak/WAKScrollView.mm:65:13: note: property synthesized here
@synthesize delegate = _delegate;
            ^
1 error generated.

[David Kilzer (:ddkilzer)]

Created attachment 338378 [details]
Patch v2

[Simon Fraser (smfr)]

Comment on attachment 338378 [details]
Patch v2

This patch is effectively a no-op. 'weak' just means non-retained, which is what the old code was doing.

[Simon Fraser (smfr)]

Maybe you're right:
https://developer.apple.com/library/content/releasenotes/ObjectiveC/RN-TransitioningToARC/Introduction/Introduction.html
"A weak reference does not extend the lifetime of the object it points to, and automatically becomes nil when there are no strong references to the object."

Have you tested this?

[David Kilzer (:ddkilzer)]

Created attachment 338887 [details]
Source for testing objc-arc and objc-weak

[David Kilzer (:ddkilzer)]

(In reply to Simon Fraser (smfr) from comment #7)
> Maybe you're right:
> https://developer.apple.com/library/content/releasenotes/ObjectiveC/RN-
> TransitioningToARC/Introduction/Introduction.html
> "A weak reference does not extend the lifetime of the object it points to,
> and automatically becomes nil when there are no strong references to the
> object."
> 
> Have you tested this?

Using Attachment #338887 [details]:

With ARC:

$ clang -fobjc-arc -x objective-c -o weak-smfr-arc -framework Foundation weak-smfr.m 
$ ./weak-smfr-arc 
2018-04-26 10:17:42.295 weak-smfr-arc[97821:14281815] Before release: 0x7f916d40cb20
2018-04-26 10:17:42.295 weak-smfr-arc[97821:14281815] After release: 0x0

With MRR:

$ clang -x objective-c -o weak-smfr-mrr -framework Foundation weak-smfr.m 
$ ./weak-smfr-mrr 
2018-04-26 10:20:25.478 weak-smfr-mrr[97834:14292493] Before release: 0x7f88f2e06b00
2018-04-26 10:20:25.479 weak-smfr-mrr[97834:14292493] After release: 0x7f88f2e06b00

With MRR + WEAK:

$ clang -fobjc-weak -x objective-c -o weak-smfr-weak -framework Foundation weak-smfr.m 
$ ./weak-smfr-weak 
2018-04-26 10:20:38.310 weak-smfr-weak[97838:14293436] Before release: 0x7ff2eef04bd0
2018-04-26 10:20:38.310 weak-smfr-weak[97838:14293436] After release: 0x0

So, yes, this feature works as it was designed.  :)

[David Kilzer (:ddkilzer)]

Created attachment 338889 [details]
Patch v3 (fixed ChangeLog typo from v2)

[WebKit Commit Bot]

Comment on attachment 338889 [details]
Patch v3 (fixed ChangeLog typo from v2)

Clearing flags on attachment: 338889

Committed r231058: <https://trac.webkit.org/changeset/231058>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.