Bug 179804 – The WebProcess should use the NSRunLoop runloop type.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Per Arne Vollan]

It currently uses the NSApplicationMain runloop type.

[Per Arne Vollan]

Created attachment 327123 [details]
Patch

[Brent Fulgham]

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.

[EWS Watchlist]

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.

[EWS Watchlist]

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

[Per Arne Vollan]

Created attachment 327170 [details]
Patch

[Per Arne Vollan]

(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.

[Brent Fulgham]

(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! :-)

[EWS Watchlist]

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.

[EWS Watchlist]

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

[Per Arne Vollan]

Created attachment 327238 [details]
Patch

[Per Arne Vollan]

Created attachment 327658 [details]
Patch

[EWS Watchlist]

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.

[EWS Watchlist]

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

[Per Arne Vollan]

Created attachment 327707 [details]
Patch

[EWS Watchlist]

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.

[EWS Watchlist]

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

[Per Arne Vollan]

Created attachment 327759 [details]
Patch

[EWS Watchlist]

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.

[EWS Watchlist]

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

[Brent Fulgham]

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.

[Per Arne Vollan]

Created attachment 327867 [details]
Patch

[EWS Watchlist]

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.

[EWS Watchlist]

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.

[EWS Watchlist]

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

[Per Arne Vollan]

Created attachment 328137 [details]
Patch

[Per Arne Vollan]

Created attachment 328351 [details]
Patch

[Brent Fulgham]

<rdar://problem/14012823>

[Brent Fulgham]

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.

[Brent Fulgham]

@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.

[chris fleizach]

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

[Per Arne Vollan]

Created attachment 328580 [details]
Patch

[Per Arne Vollan]

Created attachment 328581 [details]
Patch

[chris fleizach]

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

[Per Arne Vollan]

(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!

[Per Arne Vollan]

Created attachment 328599 [details]
Patch

[Brent Fulgham]

Comment on attachment 328599 [details]
Patch

Looks good. Let's give it a whirl!

[Per Arne Vollan]

Created attachment 328617 [details]
Patch

[Per Arne Vollan]

(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?

[Brent Fulgham]

Comment on attachment 328617 [details]
Patch

This still looks fine. Feel free to land if EWS is still green with this revised version.

[WebKit Commit Bot]

Comment on attachment 328617 [details]
Patch

Clearing flags on attachment: 328617

Committed r225597: <https://trac.webkit.org/changeset/225597>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Simon Fraser (smfr)]

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.

[Brent Fulgham]

(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.

[Jonathan Bedard]

*** Bug 170688 has been marked as a duplicate of this bug. ***