Bug 224179

Summary:

BidiContext caching is not thread-safe

Product:

WebKit

Reporter:

Chris Lord <clord>

Component:

WebCore Misc.

Assignee:

Chris Lord <clord>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

darin, webkit-bug-importer

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

Bug Blocks:

183720, 186759

Attachments:

Description	Flags
Patch	none
Patch	none

Chris Lord

Reported 2021-04-05 02:42:04 PDT

I'm not sure how this escaped, I thought I'd dealt with it in my branch but here we are. Debug mode crashes when using text with OffscreenCanvas because of the ref/deref thread check in BidiContext. Workers need to use uncached contexts or it needs to be thread-safe.

Attachments
Patch (4.42 KB, patch) 2021-04-05 03:50 PDT, Chris Lord	no flags	Details Formatted Diff Diff
Patch (4.51 KB, patch) 2021-04-06 02:12 PDT, Chris Lord	no flags	Details Formatted Diff Diff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.

Chris Lord

Comment 1 2021-04-05 03:50:41 PDT

Created attachment 425144 [details] Patch

Darin Adler

Comment 2 2021-04-05 16:56:30 PDT

Comment on attachment 425144 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=425144&action=review > Source/WebCore/platform/text/BidiContext.cpp:65 > + static BidiContext* ltrContext; > + static std::once_flag ltrContextOnceFlag; > + std::call_once(ltrContextOnceFlag, [&]() { > + ltrContext = &createUncached(0, U_LEFT_TO_RIGHT, false, FromStyleOrDOM, 0).leakRef(); > + }); > + return *ltrContext; I think the superior idiom is something more like this that does not use leakRef: static NeverDestroyed<RefPtr<BidiContext>> ltrContext; static std::once_flag ltrContextOnceFlag; std::call_once(ltrContextOnceFlag, [&]() { ltrContext = createUncached(0, U_LEFT_TO_RIGHT, false, FromStyleOrDOM, 0); }); return *ltrContext.get();

Chris Lord

Comment 3 2021-04-06 02:12:10 PDT

(In reply to Darin Adler from comment #2) > Comment on attachment 425144 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=425144&action=review > > > Source/WebCore/platform/text/BidiContext.cpp:65 > > + static BidiContext* ltrContext; > > + static std::once_flag ltrContextOnceFlag; > > + std::call_once(ltrContextOnceFlag, [&]() { > > + ltrContext = &createUncached(0, U_LEFT_TO_RIGHT, false, FromStyleOrDOM, 0).leakRef(); > > + }); > > + return *ltrContext; > > I think the superior idiom is something more like this that does not use > leakRef: > > static NeverDestroyed<RefPtr<BidiContext>> ltrContext; > static std::once_flag ltrContextOnceFlag; > std::call_once(ltrContextOnceFlag, [&]() { > ltrContext = createUncached(0, U_LEFT_TO_RIGHT, false, > FromStyleOrDOM, 0); > }); > return *ltrContext.get(); Funny, I played with this exact structure and not sure why I didn't settle with it... Anyway, I agree, will change.

Chris Lord

Comment 4 2021-04-06 02:12:42 PDT

Created attachment 425255 [details] Patch

EWS

Comment 5 2021-04-06 02:59:41 PDT

Committed r275500: <https://commits.webkit.org/r275500> All reviewed patches have been landed. Closing bug and clearing flags on attachment 425255 [details].

Radar WebKit Bug Importer

Comment 6 2021-04-06 03:00:19 PDT

<rdar://problem/76259907>

Note You need to log in before you can comment on or make changes to this bug.