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>
Created attachment 382873 [details] proposed patch.
Comment on attachment 382873 [details] proposed patch. Need to fix non abstraction to also work for non USE(CF) ports.
Created attachment 382877 [details] proposed patch.
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?
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.
(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
Looks good to me. Sorry for the regression.
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); ... } ```
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.
Thanks for the reviews. Landed in r252124: <http://trac.webkit.org/r252124>.