Bug 203873 – WTF::RunLoop should not depend on isMainThread() idiom.

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Mark Lam]

The isMainThread() idiom is only meaningful for WebCore.  It is less meaningful for JSC since a VM instance can be entered from multiple threads, as long as only one thread enters it at any time.  Hence, the concept of a main thread doesn't make sense at the JSC level.

Since r251036, we started using a WTF::String to represent the RunLoop mode.  This caused problems for JSC clients when USE(CF) since it necessitated the use of StringWrapperCFAllocator to track the life cycle of the CFStringRef generated from the WTF::String.

To fix this problem, we should restore the original behavior of using CFStringRefs as the RunLoop mode token.

<rdar://problem/56524251>

[Mark Lam]

Created attachment 382873 [details]
proposed patch.

[Mark Lam]

Comment on attachment 382873 [details]
proposed patch.

Need to fix non abstraction to also work for non USE(CF) ports.

[Mark Lam]

Created attachment 382877 [details]
proposed patch.

[Saam Barati]

Comment on attachment 382877 [details]
proposed patch.

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

> Source/WTF/ChangeLog:7
> +        Reviewed by NOBODY (OOPS!).

can you also open a bug about the Date function which uses createCFString?

[Mark Lam]

Comment on attachment 382877 [details]
proposed patch.

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

>> Source/WTF/ChangeLog:7
>> +        Reviewed by NOBODY (OOPS!).
> 
> can you also open a bug about the Date function which uses createCFString?

Will do.

[Mark Lam]

(In reply to Mark Lam from comment #5)
> Comment on attachment 382877 [details]
> proposed patch.
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=382877&action=review
> 
> >> Source/WTF/ChangeLog:7
> >> +        Reviewed by NOBODY (OOPS!).
> > 
> > can you also open a bug about the Date function which uses createCFString?
> 
> Will do.

See https://bugs.webkit.org/show_bug.cgi?id=203875

[Ryosuke Niwa]

Looks good to me. Sorry for the regression.

[Devin Rousso]

Comment on attachment 382877 [details]
proposed patch.

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

r=me as well, nice fix!

> Source/WTF/wtf/RunLoop.h:56
> +#if USE(CF)
> +using RunLoopMode = CFStringRef;
> +#define DefaultRunLoopMode kCFRunLoopDefaultMode
> +#else
> +using RunLoopMode = unsigned;
> +#define DefaultRunLoopMode 0
> +#endif

Should we move these to be inside `RunLoop`?

```
    class RunLoop : public FunctionDispatcher {
        ...

#if USE(CF)
        using Mode = CFStringRef;
        using DefaultMode = kCFRunLoopDefaultMode;
#else
        using Mode = unsigned;
        using DefaultMode = 0;
#endif

        ...
        
        WTF_EXPORT_PRIVATE CycleResult static cycle(Mode = DefaultMode);
        
        ...
    }
```

[Mark Lam]

Comment on attachment 382877 [details]
proposed patch.

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

>> Source/WTF/wtf/RunLoop.h:56
>> +#endif
> 
> Should we move these to be inside `RunLoop`?
> 
> ```
>     class RunLoop : public FunctionDispatcher {
>         ...
> 
> #if USE(CF)
>         using Mode = CFStringRef;
>         using DefaultMode = kCFRunLoopDefaultMode;
> #else
>         using Mode = unsigned;
>         using DefaultMode = 0;
> #endif
> 
>         ...
>         
>         WTF_EXPORT_PRIVATE CycleResult static cycle(Mode = DefaultMode);
>         
>         ...
>     }
> ```

This unfortunately is not possible because you can't do "using DefaultMode = kCFRunLoopDefaultMode;".  We can only use "using" for types, and kCFRunLoopDefaultMode is not a type.  We also can't use "static constexpr DefaultMode = kCFRunLoopDefaultMode;" because kCFRunLoopDefaultMode is not a constexpr expression.  That's why I settled on the current form that uses the #define for DefaultRunLoopMode.  I'll stick with this unless we run into a namespace collision issue.

[Mark Lam]

Thanks for the reviews.  Landed in r252124: <http://trac.webkit.org/r252124>.