Bug 179804 - The WebProcess should use the NSRunLoop runloop type.
Summary: The WebProcess should use the NSRunLoop runloop type.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Per Arne Vollan
URL:
Keywords: InRadar
: 170688 (view as bug list)
Depends on:
Blocks:
 
Reported: 2017-11-16 16:15 PST by Per Arne Vollan
Modified: 2018-01-23 16:52 PST (History)
8 users (show)

See Also:


Attachments
Patch (3.02 KB, patch)
2017-11-16 16:18 PST, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews105 for mac-elcapitan-wk2 (3.16 MB, application/zip)
2017-11-16 17:40 PST, EWS Watchlist
no flags Details
Patch (3.06 KB, patch)
2017-11-17 07:43 PST, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews106 for mac-elcapitan-wk2 (2.78 MB, application/zip)
2017-11-17 08:58 PST, EWS Watchlist
no flags Details
Patch (2.49 KB, patch)
2017-11-17 14:31 PST, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (3.14 KB, patch)
2017-11-27 12:24 PST, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews106 for mac-elcapitan-wk2 (2.75 MB, application/zip)
2017-11-27 13:17 PST, EWS Watchlist
no flags Details
Patch (4.93 KB, patch)
2017-11-27 15:53 PST, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews104 for mac-elcapitan-wk2 (2.95 MB, application/zip)
2017-11-27 19:40 PST, EWS Watchlist
no flags Details
Patch (3.12 KB, patch)
2017-11-28 09:42 PST, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews104 for mac-elcapitan-wk2 (2.97 MB, application/zip)
2017-11-28 10:41 PST, EWS Watchlist
no flags Details
Patch (4.38 KB, patch)
2017-11-29 09:45 PST, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews107 for mac-elcapitan-wk2 (2.98 MB, application/zip)
2017-11-29 10:49 PST, EWS Watchlist
no flags Details
Patch (5.73 KB, patch)
2017-12-01 12:47 PST, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (5.91 KB, patch)
2017-12-04 09:27 PST, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (6.04 KB, patch)
2017-12-06 10:02 PST, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (6.11 KB, patch)
2017-12-06 10:07 PST, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (6.11 KB, patch)
2017-12-06 11:20 PST, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (6.19 KB, patch)
2017-12-06 13:40 PST, Per Arne Vollan
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Per Arne Vollan 2017-11-16 16:15:43 PST
It currently uses the NSApplicationMain runloop type.
Comment 1 Per Arne Vollan 2017-11-16 16:18:29 PST
Created attachment 327123 [details]
Patch
Comment 2 Brent Fulgham 2017-11-16 16:38:55 PST
Comment on attachment 327123 [details]
Patch

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

> Source/WebKit/Shared/mac/ChildProcessMac.mm:77
> +void ChildProcess::launchServicesCheckIn()

It looks like you need a stub here for iOS as well as other processes.
Comment 3 EWS Watchlist 2017-11-16 17:40:15 PST
Comment on attachment 327123 [details]
Patch

Attachment 327123 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/5266430

Number of test failures exceeded the failure limit.
Comment 4 EWS Watchlist 2017-11-16 17:40:16 PST
Created attachment 327136 [details]
Archive of layout-test-results from ews105 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6
Comment 5 Per Arne Vollan 2017-11-17 07:43:19 PST
Created attachment 327170 [details]
Patch
Comment 6 Per Arne Vollan 2017-11-17 07:51:27 PST
(In reply to Brent Fulgham from comment #2)
> Comment on attachment 327123 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=327123&action=review
> 
> > Source/WebKit/Shared/mac/ChildProcessMac.mm:77
> > +void ChildProcess::launchServicesCheckIn()
> 
> It looks like you need a stub here for iOS as well as other processes.

Thanks! Updated patch. It seems some media tests are failing on mac-WK2.
Comment 7 Brent Fulgham 2017-11-17 08:37:01 PST
(In reply to Per Arne Vollan from comment #6)
> > It looks like you need a stub here for iOS as well as other processes.
> 
> Thanks! Updated patch. It seems some media tests are failing on mac-WK2.

Interesting. Far less breakage than I feared. Let's keep pushing! :-)
Comment 8 EWS Watchlist 2017-11-17 08:58:36 PST
Comment on attachment 327170 [details]
Patch

Attachment 327170 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/5275158

Number of test failures exceeded the failure limit.
Comment 9 EWS Watchlist 2017-11-17 08:58:37 PST
Created attachment 327177 [details]
Archive of layout-test-results from ews106 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6
Comment 10 Per Arne Vollan 2017-11-17 14:31:12 PST
Created attachment 327238 [details]
Patch
Comment 11 Per Arne Vollan 2017-11-27 12:24:28 PST
Created attachment 327658 [details]
Patch
Comment 12 EWS Watchlist 2017-11-27 13:17:36 PST
Comment on attachment 327658 [details]
Patch

Attachment 327658 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/5377936

Number of test failures exceeded the failure limit.
Comment 13 EWS Watchlist 2017-11-27 13:17:38 PST
Created attachment 327665 [details]
Archive of layout-test-results from ews106 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6
Comment 14 Per Arne Vollan 2017-11-27 15:53:24 PST
Created attachment 327707 [details]
Patch
Comment 15 EWS Watchlist 2017-11-27 19:40:37 PST
Comment on attachment 327707 [details]
Patch

Attachment 327707 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/5383493

Number of test failures exceeded the failure limit.
Comment 16 EWS Watchlist 2017-11-27 19:40:38 PST
Created attachment 327721 [details]
Archive of layout-test-results from ews104 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6
Comment 17 Per Arne Vollan 2017-11-28 09:42:45 PST
Created attachment 327759 [details]
Patch
Comment 18 EWS Watchlist 2017-11-28 10:41:25 PST
Comment on attachment 327759 [details]
Patch

Attachment 327759 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/5391115

Number of test failures exceeded the failure limit.
Comment 19 EWS Watchlist 2017-11-28 10:41:26 PST
Created attachment 327764 [details]
Archive of layout-test-results from ews104 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6
Comment 20 Brent Fulgham 2017-11-28 10:44:00 PST
Comment on attachment 327759 [details]
Patch

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

> Source/WebKit/Shared/mac/ChildProcessMac.mm:83
> +    [NSApp finishLaunching];

I'm not sure if we want to be making these calls in the final version of this patch. The goal would be to get away from NSApplication/NSApp entirely.
Comment 21 Per Arne Vollan 2017-11-29 09:45:48 PST
Created attachment 327867 [details]
Patch
Comment 22 EWS Watchlist 2017-11-29 09:48:00 PST
Attachment 327867 [details] did not pass style-queue:


ERROR: Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 23 EWS Watchlist 2017-11-29 10:49:58 PST
Comment on attachment 327867 [details]
Patch

Attachment 327867 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/5405055

Number of test failures exceeded the failure limit.
Comment 24 EWS Watchlist 2017-11-29 10:49:59 PST
Created attachment 327872 [details]
Archive of layout-test-results from ews107 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6
Comment 25 Per Arne Vollan 2017-12-01 12:47:31 PST
Created attachment 328137 [details]
Patch
Comment 26 Per Arne Vollan 2017-12-04 09:27:24 PST
Created attachment 328351 [details]
Patch
Comment 27 Brent Fulgham 2017-12-04 11:13:03 PST
<rdar://problem/14012823>
Comment 28 Brent Fulgham 2017-12-04 11:15:29 PST
Comment on attachment 328351 [details]
Patch

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

r=me, but please run that one change by AX people.

> Source/WebCore/ChangeLog:4
> +        https://bugs.webkit.org/show_bug.cgi?id=179804

<rdar://problem/14012823>

> Source/WebKit/ChangeLog:4
> +        https://bugs.webkit.org/show_bug.cgi?id=179804

<rdar://problem/14012823>

> Source/WebKit/Platform/IPC/mac/ConnectionMac.mm:590
> +        return;

Does this early return cause accessibility stuff to fail? We should confirm with James Craig or someone else on the accessibility team.
Comment 29 Brent Fulgham 2017-12-04 11:16:30 PST
@James and Chris: Can you check the change in IPC/mac/ConnectionMac.mm to see if it's likely to cause a problem in the WebContent process.

I suspect not, but wanted to double-check.
Comment 30 chris fleizach 2017-12-04 12:31:44 PST
Comment on attachment 328351 [details]
Patch

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

>> Source/WebKit/Platform/IPC/mac/ConnectionMac.mm:590
>> +        return;
> 
> Does this early return cause accessibility stuff to fail? We should confirm with James Craig or someone else on the accessibility team.

pretty sure this is wrong
Comment 31 Per Arne Vollan 2017-12-06 10:02:30 PST
Created attachment 328580 [details]
Patch
Comment 32 Per Arne Vollan 2017-12-06 10:07:47 PST
Created attachment 328581 [details]
Patch
Comment 33 chris fleizach 2017-12-06 11:10:16 PST
Comment on attachment 328581 [details]
Patch

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

> Source/WebKit/Platform/IPC/mac/ConnectionMac.mm:590
> +    if (NSApp && [NSApp isRunning])

if (NSApp isRunning)

is a sufficient check
Comment 34 Per Arne Vollan 2017-12-06 11:19:22 PST
(In reply to chris fleizach from comment #33)
> Comment on attachment 328581 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=328581&action=review
> 
> > Source/WebKit/Platform/IPC/mac/ConnectionMac.mm:590
> > +    if (NSApp && [NSApp isRunning])
> 
> if (NSApp isRunning)
> 
> is a sufficient check

I will update the patch. Thanks for reviewing, all!
Comment 35 Per Arne Vollan 2017-12-06 11:20:24 PST
Created attachment 328599 [details]
Patch
Comment 36 Brent Fulgham 2017-12-06 13:38:55 PST
Comment on attachment 328599 [details]
Patch

Looks good. Let's give it a whirl!
Comment 37 Per Arne Vollan 2017-12-06 13:40:21 PST
Created attachment 328617 [details]
Patch
Comment 38 Per Arne Vollan 2017-12-06 13:46:00 PST
(In reply to Brent Fulgham from comment #36)
> Comment on attachment 328599 [details]
> Patch
> 
> Looks good. Let's give it a whirl!

Thanks Brent! Didn't see this before I uploaded another patch with just minor modifications. Could you look at it once more?
Comment 39 Brent Fulgham 2017-12-06 13:46:37 PST
Comment on attachment 328617 [details]
Patch

This still looks fine. Feel free to land if EWS is still green with this revised version.
Comment 40 WebKit Commit Bot 2017-12-06 14:24:34 PST
Comment on attachment 328617 [details]
Patch

Clearing flags on attachment: 328617

Committed r225597: <https://trac.webkit.org/changeset/225597>
Comment 41 WebKit Commit Bot 2017-12-06 14:24:36 PST
All reviewed patches have been landed.  Closing bug.
Comment 42 Simon Fraser (smfr) 2017-12-13 12:35:48 PST
Comment on attachment 328617 [details]
Patch

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

> Source/WebCore/ChangeLog:9
> +        The WebProcess should use the NSRunLoop runloop type.
> +        https://bugs.webkit.org/show_bug.cgi?id=179804
> +        <rdar://problem/14012823>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        No new tests.

Why? There's no explanatory text here at all.
Comment 43 Brent Fulgham 2017-12-13 12:56:35 PST
(In reply to Simon Fraser (smfr) from comment #42)
> > Source/WebCore/ChangeLog:9
> > +        The WebProcess should use the NSRunLoop runloop type.
> > +        https://bugs.webkit.org/show_bug.cgi?id=179804
> > +        <rdar://problem/14012823>
> 
> Why? There's no explanatory text here at all.

This is part of an effort to make it harder to use the WebContent process as an attack target by reducing the number of system services it has access to.

NSApplication was designed years and years ago to be the basis for full-featured user applications. It's really inappropriate for something like the WebContent process, and it's an important hardening step for us to move away from it.

We should have put these comments in the ChangeLog so you would have better context.
Comment 44 Jonathan Bedard 2018-01-23 16:52:31 PST
*** Bug 170688 has been marked as a duplicate of this bug. ***