Bug 233983

Summary: Move computeUserPrefersSimplified to Language.cpp and add locking around it
Product: WebKit Reporter: Cameron McCormack (:heycam) <heycam>
Component: Layout and RenderingAssignee: Cameron McCormack (:heycam) <heycam>
Status: RESOLVED FIXED    
Severity: Normal CC: benjamin, bfulgham, cdumez, cmarcelo, ews-watchlist, mattwoodrow, mmaxfield, simon.fraser, webkit-bug-importer, zalan
Priority: P2 Keywords: InRadar
Version: WebKit Local Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 233488    
Attachments:
Description Flags
Patch
none
Patch none

Cameron McCormack (:heycam)
Reported 2021-12-07 22:55:10 PST
Move computeUserPrefersSimplified to Language.cpp and add locking around it
Attachments
Patch (11.25 KB, patch)
2021-12-07 22:59 PST, Cameron McCormack (:heycam) 		no flags
DetailsFormatted DiffDiff
Patch (9.85 KB, patch)
2022-07-19 22:10 PDT, Matt Woodrow 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Cameron McCormack (:heycam)
Comment 1 2021-12-07 22:59:55 PST
Created attachment 446309 [details] Patch
Myles C. Maxfield
Comment 2 2021-12-08 21:30:59 PST
Comment on attachment 446309 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=446309&action=review > Source/WTF/wtf/Language.cpp:137 > + Vector<String>& override = preferredLanguagesOverride(); |override| is per-thread? It's surprising to see it move from inside the critical section to outside. > Source/WTF/wtf/Language.cpp:149 > - return isolatedCopy(languages); > + return languages; How confident are you that this is safe? > Source/WTF/wtf/Language.cpp:164 > + if (equalIgnoringASCIICase(language, "zh-tw")) > + return false; > + if (equalIgnoringASCIICase(language, "zh-cn")) > + return true; Wow I can't believe I wrote this. https://www.rfc-editor.org/rfc/rfc4647
Cameron McCormack (:heycam)
Comment 3 2021-12-09 22:56:24 PST
(In reply to Myles C. Maxfield from comment #2) > |override| is per-thread? It's surprising to see it move from inside the > critical section to outside. It's not. This function (computeUserPreferredLanguages, which is where the local `override` variable is) is annotated with WTF_REQUIRES_LOCK(languagesLock). > > Source/WTF/wtf/Language.cpp:149 > > - return isolatedCopy(languages); > > + return languages; > > How confident are you that this is safe? What I've written is safe now, because userPreferredLanguages now does the isolatedCopy, and computeUserPrefersSimplifiedChinese doesn't let the reference escape. I really just wanted to avoid having computeUserPrefersSimplifiedChinese clone the array for no good reason. The thread safety annotations won't help if someone writes another function that calls computeUserPrefersSimplifiedChinese and does let it escape. If you have a suggestion for how to rearrange the code to protect against that I'd be happy to entertain it.
Cameron McCormack (:heycam)
Comment 4 2021-12-09 22:59:24 PST
(One way would be to duplicate the contents of computeUserPreferredLanguages into the two functions using it.)
Radar WebKit Bug Importer
Comment 5 2021-12-14 22:56:15 PST
<rdar://problem/86507413>
Matt Woodrow
Comment 6 2022-07-19 22:10:44 PDT
Created attachment 461034 [details] Patch
EWS
Comment 7 2022-07-20 13:17:16 PDT
mattwoodrow@apple.com does not have reviewer permissions according to https://raw.githubusercontent.com/WebKit/WebKit/main/metadata/contributors.json. Rejecting attachment 461034 [details] from commit queue.
EWS
Comment 8 2022-07-20 14:10:24 PDT
Committed 252655@main (352ffc3fc81e): <https://commits.webkit.org/252655@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 461034 [details].
Note You need to log in before you can comment on or make changes to this bug.