Bug 199764 - Make WKURLSchemeTask thread safe
Summary: Make WKURLSchemeTask thread safe
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Brady Eidson
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-07-12 15:53 PDT by Brady Eidson
Modified: 2019-07-15 21:37 PDT (History)
10 users (show)

See Also:


Attachments
Patch (17.27 KB, patch)
2019-07-12 15:58 PDT, Brady Eidson
no flags Details | Formatted Diff | Diff
Patch (17.37 KB, patch)
2019-07-15 13:54 PDT, Brady Eidson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brady Eidson 2019-07-12 15:53:27 PDT
Make WKURLSchemeTask thread safe

<rdar://problem/50471863>
Comment 1 Brady Eidson 2019-07-12 15:58:33 PDT
Created attachment 374047 [details]
Patch
Comment 2 Build Bot 2019-07-12 16:00:31 PDT
Attachment 374047 [details] did not pass style-queue:


ERROR: Source/WebKit/UIProcess/WebURLSchemeTask.cpp:202:  Declaration has space between type name and * in NSURLRequest *WebURLSchemeTask  [whitespace/declaration] [3]
ERROR: Source/WebKit/UIProcess/API/Cocoa/WKURLSchemeTask.mm:37:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 2 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Alex Christensen 2019-07-15 10:23:52 PDT
Comment on attachment 374047 [details]
Patch

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

> Source/WTF/wtf/MainThread.cpp:177
> +enum class MainStyle {

There's no reason not to have : bool here.  I think it's good practice, even if we don't store this anywhere yet.

> Source/WTF/wtf/MainThread.cpp:182
> +static void callOnMainAndWait(WTF::Function<void()>&& function, MainStyle mainStyle)

WTF:: unnecessary many places in this patch.

> Source/WebKit/UIProcess/API/Cocoa/WKURLSchemeTask.mm:37
> +static WebKit::WebURLSchemeTask::ExceptionType getExceptionTypeFromMainRunLoop(WTF::Function<WebKit::WebURLSchemeTask::ExceptionType ()>&& function)

Extra space, WTF:: unnecessary
Comment 4 Geoffrey Garen 2019-07-15 10:39:10 PDT
Comment on attachment 374047 [details]
Patch

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

> Source/WTF/wtf/MainThread.cpp:179
> +    Thread,
> +    RunLoop

I don't understand the distinction you're making between Thread and RunLoop here. There's one RunLoop per thread, so what is the actual difference here?

> Source/WTF/wtf/MainThread.cpp:185
>      if (isMainThread()) {
>          function();

isMainThread() is true on the WebThread, even when isMainRunLoop() is false. This seems like a bug.
Comment 5 Alex Christensen 2019-07-15 10:40:58 PDT
Comment on attachment 374047 [details]
Patch

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

>> Source/WTF/wtf/MainThread.cpp:179
>> +    RunLoop
> 
> I don't understand the distinction you're making between Thread and RunLoop here. There's one RunLoop per thread, so what is the actual difference here?

This is to not cause problems with UIWebView web thread stuff.

>> Source/WTF/wtf/MainThread.cpp:185
>>          function();
> 
> isMainThread() is true on the WebThread, even when isMainRunLoop() is false. This seems like a bug.

You're right.  This should be isMainRunLoop when mainStyle is RunLoop.
Comment 6 Geoffrey Garen 2019-07-15 10:50:35 PDT
Comment on attachment 374047 [details]
Patch

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

>>> Source/WTF/wtf/MainThread.cpp:179
>>> +    RunLoop
>> 
>> I don't understand the distinction you're making between Thread and RunLoop here. There's one RunLoop per thread, so what is the actual difference here?
> 
> This is to not cause problems with UIWebView web thread stuff.

For legacy compatibility, callOnMainThread and isMainThread apply to either the main thread or the web thread.

In new code, we don't need to maintain that extremely confusing naming style.

I'd suggest 

    Thread => MainThreadOrWebThread
    RunLoop => MainThread

Also, when practical, we should probably globally rename the older functions too.
Comment 7 Brady Eidson 2019-07-15 13:15:49 PDT
(In reply to Alex Christensen from comment #3)
> Comment on attachment 374047 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=374047&action=review
> 
> > Source/WTF/wtf/MainThread.cpp:177
> > +enum class MainStyle {
> 
> There's no reason not to have : bool here.  I think it's good practice, even
> if we don't store this anywhere yet.
> 
> > Source/WTF/wtf/MainThread.cpp:182
> > +static void callOnMainAndWait(WTF::Function<void()>&& function, MainStyle mainStyle)
> 
> WTF:: unnecessary many places in this patch.

To not cause a style split inside MainThread.cpp, I intend to leave those.

Fixing others.

> 
> > Source/WebKit/UIProcess/API/Cocoa/WKURLSchemeTask.mm:37
> > +static WebKit::WebURLSchemeTask::ExceptionType getExceptionTypeFromMainRunLoop(WTF::Function<WebKit::WebURLSchemeTask::ExceptionType ()>&& function)
> 
> Extra space, WTF:: unnecessary

Style-bot says its an extra space because nobody has ever successfully taught style body our established lambda style.

This is our established lambda style.

---

I'm definitely fixing the actual mainrunloop utility function
Comment 8 Brady Eidson 2019-07-15 13:19:41 PDT
(In reply to Geoffrey Garen from comment #6)
> Comment on attachment 374047 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=374047&action=review
> 
> >>> Source/WTF/wtf/MainThread.cpp:179
> >>> +    RunLoop
> >> 
> >> I don't understand the distinction you're making between Thread and RunLoop here. There's one RunLoop per thread, so what is the actual difference here?
> > 
> > This is to not cause problems with UIWebView web thread stuff.
> 
> For legacy compatibility, callOnMainThread and isMainThread apply to either
> the main thread or the web thread.
> 
> In new code, we don't need to maintain that extremely confusing naming style.
> 
> I'd suggest 
> 
>     Thread => MainThreadOrWebThread
>     RunLoop => MainThread
> 
> Also, when practical, we should probably globally rename the older functions
> too.

I totally agree with this.

But I'm not sure how I can partially rename things in this patch - before that global renaming - without significantly ADDING to the confusion.
Comment 9 Brady Eidson 2019-07-15 13:54:11 PDT
Created attachment 374145 [details]
Patch
Comment 10 Build Bot 2019-07-15 13:56:38 PDT
Attachment 374145 [details] did not pass style-queue:


ERROR: Source/WebKit/UIProcess/WebURLSchemeTask.cpp:202:  Declaration has space between type name and * in NSURLRequest *WebURLSchemeTask  [whitespace/declaration] [3]
ERROR: Source/WebKit/UIProcess/API/Cocoa/WKURLSchemeTask.mm:37:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 2 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 11 WebKit Commit Bot 2019-07-15 16:45:14 PDT
Comment on attachment 374145 [details]
Patch

Clearing flags on attachment: 374145

Committed r247461: <https://trac.webkit.org/changeset/247461>
Comment 12 WebKit Commit Bot 2019-07-15 16:45:15 PDT
All reviewed patches have been landed.  Closing bug.
Comment 13 Fujii Hironori 2019-07-15 21:37:24 PDT
Committed r247473: <https://trac.webkit.org/changeset/247473>