Bug 203873 - WTF::RunLoop should not depend on isMainThread() idiom.
Summary: WTF::RunLoop should not depend on isMainThread() idiom.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mark Lam
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-11-05 17:49 PST by Mark Lam
Modified: 2019-11-05 22:37 PST (History)
17 users (show)

See Also:


Attachments
proposed patch. (11.22 KB, patch)
2019-11-05 17:58 PST, Mark Lam
no flags Details | Formatted Diff | Diff
proposed patch. (11.20 KB, patch)
2019-11-05 18:08 PST, Mark Lam
saam: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Lam 2019-11-05 17:49:02 PST
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>
Comment 1 Mark Lam 2019-11-05 17:58:23 PST
Created attachment 382873 [details]
proposed patch.
Comment 2 Mark Lam 2019-11-05 18:04:55 PST
Comment on attachment 382873 [details]
proposed patch.

Need to fix non abstraction to also work for non USE(CF) ports.
Comment 3 Mark Lam 2019-11-05 18:08:46 PST
Created attachment 382877 [details]
proposed patch.
Comment 4 Saam Barati 2019-11-05 18:14:58 PST
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 5 Mark Lam 2019-11-05 18:21:15 PST
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.
Comment 6 Mark Lam 2019-11-05 18:27:19 PST
(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
Comment 7 Ryosuke Niwa 2019-11-05 18:59:02 PST
Looks good to me. Sorry for the regression.
Comment 8 Devin Rousso 2019-11-05 19:24:02 PST
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 9 Mark Lam 2019-11-05 22:30:48 PST
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.
Comment 10 Mark Lam 2019-11-05 22:37:37 PST
Thanks for the reviews.  Landed in r252124: <http://trac.webkit.org/r252124>.