Bug 233983 – Move computeUserPrefersSimplified to Language.cpp and add locking around it

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Cameron McCormack (:heycam)]

Move computeUserPrefersSimplified to Language.cpp and add locking around it

[Cameron McCormack (:heycam)]

Created attachment 446309 [details]
Patch

[Myles C. Maxfield]

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)]

(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)]

(One way would be to duplicate the contents of computeUserPreferredLanguages into the two functions using it.)

[Radar WebKit Bug Importer]

<rdar://problem/86507413>

[Matt Woodrow]

Created attachment 461034 [details]
Patch

[EWS]

[e-mail] 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]

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].