Bug 223695 - Metal ANGLE crashes LayoutTests/inspector/canvas/updateShader-webgl.html
Summary: Metal ANGLE crashes LayoutTests/inspector/canvas/updateShader-webgl.html
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: ANGLE (show other bugs)
Version: WebKit Nightly Build
Hardware: All Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-03-24 09:32 PDT by Kyle Piddington
Modified: 2021-03-24 18:07 PDT (History)
9 users (show)

See Also:


Attachments
Patch (1.55 KB, patch)
2021-03-24 09:33 PDT, Kyle Piddington
no flags Details | Formatted Diff | Diff
Patch (4.53 KB, patch)
2021-03-24 16:46 PDT, Kyle Piddington
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (5.01 KB, patch)
2021-03-24 17:03 PDT, Kyle Piddington
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kyle Piddington 2021-03-24 09:32:24 PDT
Metal ANGLE is crashing in this test (LayoutTests/inspector/canvas/updateShader-webgl.html), due to a bad NSDictionary. 
Switch to using allocated dictionaries, rather than stack constants.
Comment 1 Kyle Piddington 2021-03-24 09:33:58 PDT
Created attachment 424141 [details]
Patch
Comment 2 EWS Watchlist 2021-03-24 09:35:20 PDT
Note that there are important steps to take when updating ANGLE. See https://trac.webkit.org/wiki/UpdatingANGLE
Comment 3 Alexey Proskuryakov 2021-03-24 13:12:45 PDT
Comment on attachment 424141 [details]
Patch

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

> Source/ThirdParty/ANGLE/ChangeLog:4
> +Switch to using allocated dictionaries, rather than stack constants.

What is the exact mechanism for this failure? Literals are not stack constants, and should generally work everywhere.

I wonder if this is covering for an over-release somewhere with a leak.

> Source/ThirdParty/ANGLE/src/libANGLE/renderer/metal/ProgramMtl.mm:251
> -        mDefaultSubstitutionDictionary = @{@"TRANSFORM_FEEDBACK_ENABLED": @"0"};
> +        mDefaultSubstitutionDictionary = [[NSDictionary alloc] initWithDictionary:@{@"TRANSFORM_FEEDBACK_ENABLED": @"0"}];

There is a code path above that also initializes mDefaultSubstitutionDictionary with a literal. Worth updating that for consistency?
Comment 4 Kenneth Russell 2021-03-24 13:16:51 PDT
I'd like to defer review to ap@ who clearly understands this code better than me, but happy to study and officially review if needed.
Comment 5 Kenneth Russell 2021-03-24 13:17:30 PDT
Also: possible to add the crashing stack trace as a comment here for posterity?
Comment 6 Alexey Proskuryakov 2021-03-24 13:36:35 PDT
An ASan report would likely be best for understanding what leads to the crash.
Comment 7 Kyle Piddington 2021-03-24 16:46:43 PDT
Created attachment 424200 [details]
Patch
Comment 8 Kyle Piddington 2021-03-24 17:03:58 PDT
Created attachment 424202 [details]
Patch
Comment 9 EWS 2021-03-24 18:06:23 PDT
Committed r274990: <https://commits.webkit.org/r274990>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 424202 [details].
Comment 10 Radar WebKit Bug Importer 2021-03-24 18:07:16 PDT
<rdar://problem/75815100>