Bug 182499 - Implement stopping of run loop in the WebContent process when using NSRunLoop.
Summary: Implement stopping of run loop in the WebContent process when using NSRunLoop.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Per Arne Vollan
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-02-05 11:46 PST by Per Arne Vollan
Modified: 2018-02-22 08:32 PST (History)
8 users (show)

See Also:


Attachments
Patch (9.82 KB, patch)
2018-02-05 11:59 PST, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (9.26 KB, patch)
2018-02-05 14:51 PST, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (9.31 KB, patch)
2018-02-05 17:59 PST, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (9.26 KB, patch)
2018-02-06 11:53 PST, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (9.24 KB, patch)
2018-02-06 12:23 PST, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews101 for mac-sierra (2.36 MB, application/zip)
2018-02-06 13:06 PST, EWS Watchlist
no flags Details
Patch (9.28 KB, patch)
2018-02-06 13:56 PST, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (9.23 KB, patch)
2018-02-06 14:09 PST, Per Arne Vollan
bfulgham: review+
commit-queue: commit-queue-
Details | Formatted Diff | Diff
Patch (8.21 KB, patch)
2018-02-16 10:53 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 2018-02-05 11:46:34 PST
This is currently only implemented when using the NSApplication event loop.
Comment 1 Per Arne Vollan 2018-02-05 11:59:48 PST
Created attachment 333106 [details]
Patch
Comment 2 Simon Fraser (smfr) 2018-02-05 12:49:06 PST
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.
Comment 3 Radar WebKit Bug Importer 2018-02-05 13:02:23 PST
<rdar://problem/37247424>
Comment 4 Anders Carlsson 2018-02-05 13:42:08 PST
(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.
Comment 5 Per Arne Vollan 2018-02-05 14:38:37 PST
(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!
Comment 6 Per Arne Vollan 2018-02-05 14:51:37 PST
Created attachment 333124 [details]
Patch
Comment 7 Per Arne Vollan 2018-02-05 17:59:08 PST
Created attachment 333137 [details]
Patch
Comment 8 Simon Fraser (smfr) 2018-02-05 18:05:57 PST
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.
Comment 9 Per Arne Vollan 2018-02-06 11:33:47 PST
(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!
Comment 10 Simon Fraser (smfr) 2018-02-06 11:45:52 PST
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.
Comment 11 Per Arne Vollan 2018-02-06 11:50:20 PST
(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.
Comment 12 Per Arne Vollan 2018-02-06 11:53:02 PST
Created attachment 333200 [details]
Patch
Comment 13 Per Arne Vollan 2018-02-06 12:23:31 PST
Created attachment 333203 [details]
Patch
Comment 14 EWS Watchlist 2018-02-06 13:06:54 PST
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
Comment 15 EWS Watchlist 2018-02-06 13:06:55 PST
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
Comment 16 Per Arne Vollan 2018-02-06 13:56:43 PST
Created attachment 333215 [details]
Patch
Comment 17 Per Arne Vollan 2018-02-06 14:09:33 PST
Created attachment 333217 [details]
Patch
Comment 18 Brent Fulgham 2018-02-16 09:39:51 PST
Comment on attachment 333217 [details]
Patch

I think this final version of the patch looks right. r=me.
Comment 19 Per Arne Vollan 2018-02-16 09:56:52 PST
Comment on attachment 333217 [details]
Patch

Thanks for reviewing!
Comment 20 WebKit Commit Bot 2018-02-16 09:57:37 PST
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
Comment 21 Per Arne Vollan 2018-02-16 10:53:22 PST
Created attachment 334052 [details]
Patch
Comment 22 WebKit Commit Bot 2018-02-16 11:38:18 PST
Comment on attachment 334052 [details]
Patch

Clearing flags on attachment: 334052

Committed r228569: <https://trac.webkit.org/changeset/228569>
Comment 23 Anders Carlsson 2018-02-16 11:45:28 PST
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?
Comment 24 Per Arne Vollan 2018-02-19 10:45:16 PST
(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!