Summary: | Implement stopping of run loop in the WebContent process when using NSRunLoop. | ||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Per Arne Vollan <pvollan> | ||||||||||||||||||||
Component: | WebKit Misc. | Assignee: | Per Arne Vollan <pvollan> | ||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||
Severity: | Normal | CC: | andersca, bfulgham, commit-queue, ews-watchlist, rniwa, simon.fraser, thorton, webkit-bug-importer | ||||||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||
Attachments: |
|
Description
Per Arne Vollan
2018-02-05 11:46:34 PST
Created attachment 333106 [details]
Patch
Comment on attachment 333106 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=333106&action=review > Source/WebKit/ChangeLog:4 > + https://bugs.webkit.org/show_bug.cgi?id=182499 Radar? > Source/WebKit/Shared/mac/ChildProcessMac.mm:226 > + [[NSRunLoop mainRunLoop] performBlock: ^{ No space after : > Source/WebKit/Shared/mac/ChildProcessMac.mm:227 > + exit(0); Is this really the correct place to exit? How do we exit now? Do we have other async tasks that have to complete before we can exit()? So many questions. (In reply to Simon Fraser (smfr) from comment #2) > Is this really the correct place to exit? How do we exit now? Do we have > other async tasks that have to complete before we can exit()? So many > questions. I definitely don't think it is, especially not for plug-in processes. Plug-in processes make extensive use of AppKit, so I don't know why they are being changed to use NSRunLoop. (In reply to Anders Carlsson from comment #4) > (In reply to Simon Fraser (smfr) from comment #2) > > Is this really the correct place to exit? How do we exit now? Do we have > > other async tasks that have to complete before we can exit()? So many > > questions. > > I definitely don't think it is, especially not for plug-in processes. > > Plug-in processes make extensive use of AppKit, so I don't know why they are > being changed to use NSRunLoop. Ah, yes, thanks, the plug-in process is not being changed to use NSRunLoop, so the plugin process part of this patch is incorrect. I will update the patch. Thanks for reviewing, all! Created attachment 333124 [details]
Patch
Created attachment 333137 [details]
Patch
Comment on attachment 333137 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=333137&action=review > Source/WebKit/Shared/mac/ChildProcessMac.mm:225 > + ASSERT(![NSApp isRunning] && [NSRunLoop mainRunLoop]); NSApp is a global which is initialized when an NSApplication is created, so (hopefully) it will be nil always, so ![NSApp isRunning] is always true. (In reply to Simon Fraser (smfr) from comment #8) > Comment on attachment 333137 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=333137&action=review > > > Source/WebKit/Shared/mac/ChildProcessMac.mm:225 > > + ASSERT(![NSApp isRunning] && [NSRunLoop mainRunLoop]); > > NSApp is a global which is initialized when an NSApplication is created, so > (hopefully) it will be nil always, so ![NSApp isRunning] is always true. The initial commit (r225597) which moved from using the NSApplication run loop to using NSRunLoop, broke VoiceOver, because NSApp was nil. To fix this, the call [NSApplication sharedApplication] was added (r226807) to the method WebProcess::platformInitializeWebProcess. Calling [NSApplication sharedApplication] will set NSApp, so NSApp will currently not be nil after this call. However, we should indeed look further into making VoiceOver work without relying on NSApp. Thanks for reviewing! I suggest we remove all uses of NSApp from WebCore code, since, in future, it's no longer guaranteed to be non-nil. I found several places that used it, and it's really unclear what their behavior should be if we don't have a sharedApplication. (In reply to Simon Fraser (smfr) from comment #10) > I suggest we remove all uses of NSApp from WebCore code, since, in future, > it's no longer guaranteed to be non-nil. I found several places that used > it, and it's really unclear what their behavior should be if we don't have a > sharedApplication. Thanks, that's a good point! I'll create a bug for this. Created attachment 333200 [details]
Patch
Created attachment 333203 [details]
Patch
Comment on attachment 333203 [details] Patch Attachment 333203 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/6386367 New failing tests: transitions/transition-display-property.html Created attachment 333206 [details]
Archive of layout-test-results from ews101 for mac-sierra
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101 Port: mac-sierra Platform: Mac OS X 10.12.6
Created attachment 333215 [details]
Patch
Created attachment 333217 [details]
Patch
Comment on attachment 333217 [details]
Patch
I think this final version of the patch looks right. r=me.
Comment on attachment 333217 [details]
Patch
Thanks for reviewing!
Comment on attachment 333217 [details] Patch Rejecting attachment 333217 [details] from commit-queue. Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-01', 'apply-attachment', '--no-update', '--non-interactive', 333217, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit Last 500 characters of output: project.pbxproj Hunk #1 succeeded at 1753 (offset 18 lines). Hunk #2 succeeded at 2180 (offset 22 lines). Hunk #3 FAILED at 3540. Hunk #4 FAILED at 3585. 2 out of 4 hunks FAILED -- saving rejects to file Tools/TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj.rej patching file Tools/TestWebKitAPI/Tests/WebKitCocoa/WebProcessTerminate.mm Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force', '--reviewer', u'Brent Fulgham']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit Full output: http://webkit-queues.webkit.org/results/6538153 Created attachment 334052 [details]
Patch
Comment on attachment 334052 [details] Patch Clearing flags on attachment: 334052 Committed r228569: <https://trac.webkit.org/changeset/228569> Comment on attachment 333217 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=333217&action=review > Source/WebKit/Shared/mac/ChildProcessMac.mm:223 > +void ChildProcess::stopNSRunLoop() This function doesn't do what it says it does. > Source/WebKit/Shared/mac/ChildProcessMac.mm:228 > + [[NSRunLoop mainRunLoop] performBlock:^{ > + exit(0); > + }]; Why do a performBlock if all you're going to do is exit? (In reply to Anders Carlsson from comment #23) > Comment on attachment 333217 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=333217&action=review > > > Source/WebKit/Shared/mac/ChildProcessMac.mm:223 > > +void ChildProcess::stopNSRunLoop() > > This function doesn't do what it says it does. > > > Source/WebKit/Shared/mac/ChildProcessMac.mm:228 > > + [[NSRunLoop mainRunLoop] performBlock:^{ > > + exit(0); > > + }]; > > Why do a performBlock if all you're going to do is exit? When using the NSApplication run loop, control is returned back to the event loop before 'exit(0)' is called. My thinking was to also do this when using NSRunLoop. I believe the NSRunLoop implementation will also dispatch events already in the queue before exiting. This behavior is a little different from when using the NSApplication run loop, which will exit when returning to the event loop. Thanks for reviewing! |