Bug 210888 - [Cocoa] REGRESSION (r260485): Crash in Legacy WebKit createMenu item function (reproducible under Asan)
Summary: [Cocoa] REGRESSION (r260485): Crash in Legacy WebKit createMenu item function...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Mac All
: P1 Critical
Assignee: Darin Adler
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-04-22 16:38 PDT by Darin Adler
Modified: 2020-04-22 17:51 PDT (History)
4 users (show)

See Also:


Attachments
Patch (1.87 KB, patch)
2020-04-22 16:39 PDT, Darin Adler
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Darin Adler 2020-04-22 16:38:08 PDT
[Cocoa] REGRESSION (r260485): Crash in Legacy WebKit createMenu item function (reproducible under Asan)
Comment 1 Darin Adler 2020-04-22 16:39:07 PDT
Created attachment 397287 [details]
Patch
Comment 2 Darin Adler 2020-04-22 16:39:37 PDT
<rdar://problem/62207161>
Comment 3 Alex Christensen 2020-04-22 17:25:53 PDT
Comment on attachment 397287 [details]
Patch

I verified this fixes the issue.
Comment 4 EWS 2020-04-22 17:28:02 PDT
Committed r260545: <https://trac.webkit.org/changeset/260545>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 397287 [details].
Comment 5 Alex Christensen 2020-04-22 17:31:41 PDT
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.
Comment 6 Darin Adler 2020-04-22 17:51:49 PDT
(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!