Summary: | The WebProcess should use the NSRunLoop runloop type. | ||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Per Arne Vollan <pvollan> | ||||||||||||||||||||||||||||||||||||||||
Component: | WebKit2 | Assignee: | Per Arne Vollan <pvollan> | ||||||||||||||||||||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||||||||||||||||||||
Severity: | Normal | CC: | bfulgham, cfleizach, commit-queue, ews-watchlist, jcraig, mitz, rniwa, simon.fraser | ||||||||||||||||||||||||||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||||||||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||||||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||||||||||||||||||||||
See Also: | https://bugs.webkit.org/show_bug.cgi?id=182025 | ||||||||||||||||||||||||||||||||||||||||||
Attachments: |
|
Description
Per Arne Vollan
2017-11-16 16:15:43 PST
Created attachment 327123 [details]
Patch
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 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. 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
Created attachment 327170 [details]
Patch
(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. (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 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. 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
Created attachment 327238 [details]
Patch
Created attachment 327658 [details]
Patch
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. 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
Created attachment 327707 [details]
Patch
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. 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
Created attachment 327759 [details]
Patch
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. 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 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. Created attachment 327867 [details]
Patch
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 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. 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
Created attachment 328137 [details]
Patch
Created attachment 328351 [details]
Patch
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. @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 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 Created attachment 328580 [details]
Patch
Created attachment 328581 [details]
Patch
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 (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! Created attachment 328599 [details]
Patch
Comment on attachment 328599 [details]
Patch
Looks good. Let's give it a whirl!
Created attachment 328617 [details]
Patch
(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 on attachment 328617 [details]
Patch
This still looks fine. Feel free to land if EWS is still green with this revised version.
Comment on attachment 328617 [details] Patch Clearing flags on attachment: 328617 Committed r225597: <https://trac.webkit.org/changeset/225597> All reviewed patches have been landed. Closing bug. 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. (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. *** Bug 170688 has been marked as a duplicate of this bug. *** |