BidiContext caching is not thread-safe

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Chris Lord]

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.

[Chris Lord]

Created attachment 425144 [details]
Patch

[Darin Adler]

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]

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

Created attachment 425255 [details]
Patch

[EWS]

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]

<rdar://problem/76259907>