Bug 210888 – [Cocoa] REGRESSION (r260485): Crash in Legacy WebKit createMenu item function (reproducible under Asan)

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Darin Adler]

[Cocoa] REGRESSION (r260485): Crash in Legacy WebKit createMenu item function (reproducible under Asan)

[Darin Adler]

Created attachment 397287 [details]
Patch

[Darin Adler]

<rdar://problem/62207161>

[Alex Christensen]

Comment on attachment 397287 [details]
Patch

I verified this fixes the issue.

[EWS]

Committed r260545: <https://trac.webkit.org/changeset/260545>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 397287 [details].

[Alex Christensen]

View in context: https://bugs.webkit.org/attachment.cgi?id=397287&action=review

> Source/WebKitLegacy/mac/ChangeLog:11
> +        (createMenuItem): Speculative fix: Go back to using a local variable. Apparently
> +        the Objective-C for loop doesn't extend the lifetime of its argument the way the
> +        C++ range-based for loop does, so the local variable is needed.

You can actually replace this ObjC for-in loop with a C++ range-based for loop and that doesn't fix the bug.  The bug is that the RetainPtr is a temporary that goes out of scope immediately.  The for loop (C++ or ObjC) iterates over the result of .get() so it keeps a raw NSMutableArray * in scope for the entirety of the iteration, but it's pointing to an array that has been freed.  Just another bug that ARC would fix.

[Darin Adler]

(In reply to Alex Christensen from comment #5)
> You can actually replace this ObjC for-in loop with a C++ range-based for
> loop and that doesn't fix the bug.

Oh, right, because of the ".get()" because C++ extends the life of the expression, but not the subexpression before ".get()".

Darn, frustrated I got that wrong!