Bug 182499 – Implement stopping of run loop in the WebContent process when using NSRunLoop.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Per Arne Vollan]

This is currently only implemented when using the NSApplication event loop.

[Per Arne Vollan]

Created attachment 333106 [details]
Patch

[Simon Fraser (smfr)]

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.

[Radar WebKit Bug Importer]

<rdar://problem/37247424>

[Anders Carlsson]

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

[Per Arne Vollan]

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

[Per Arne Vollan]

Created attachment 333124 [details]
Patch

[Per Arne Vollan]

Created attachment 333137 [details]
Patch

[Simon Fraser (smfr)]

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.

[Per Arne Vollan]

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

[Simon Fraser (smfr)]

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.

[Per Arne Vollan]

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

[Per Arne Vollan]

Created attachment 333200 [details]
Patch

[Per Arne Vollan]

Created attachment 333203 [details]
Patch

[EWS Watchlist]

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

[EWS Watchlist]

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

[Per Arne Vollan]

Created attachment 333215 [details]
Patch

[Per Arne Vollan]

Created attachment 333217 [details]
Patch

[Brent Fulgham]

Comment on attachment 333217 [details]
Patch

I think this final version of the patch looks right. r=me.

[Per Arne Vollan]

Comment on attachment 333217 [details]
Patch

Thanks for reviewing!

[WebKit Commit Bot]

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

[Per Arne Vollan]

Created attachment 334052 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 334052 [details]
Patch

Clearing flags on attachment: 334052

Committed r228569: <https://trac.webkit.org/changeset/228569>

[Anders Carlsson]

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?

[Per Arne Vollan]

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