Bug 68439 – [Qt][WebKit2][Mac] WebProcess should exit automatically when UIProcess dies.

• Component: New Bugs
• Status: RESOLVED FIXED

[Zeno Albisser]

[Qt][WebKit2][Mac] WebProcess should exit automatically when UIProcess dies.

[Zeno Albisser]

Created attachment 108003 [details]
patch for feedback.

[Zeno Albisser]

Created attachment 108336 [details]
patch for feedback.

[Adam Roben (:aroben)]

This should already be happening via WebProcess::didClose. Is that not being called for you?

[Adam Roben (:aroben)]

WebProcess::didClose will only be called if the main thread isn't hung. Even if the main thread *is* hung, ChildProcess::didCloseOnConnectionWorkQueue should be called. That function sets things up so that the web process will exit in 10 seconds if the main thread still hasn't responded.

[Zeno Albisser]

It seems there are several issues with the current implementation.
The didClose / didCloseOnConnectionWorkQueue is not working with the Qt port because the unix sockets IPC mechanism does not provide us any meaningful notification when the ui process died.
Even when the UIProcess is exited "properly" by the user, this approach does not work due to Leaking a WebContext and therefor not closing the IPC socket properly either.


The scheduleWorkOnTermination function does not work either, because we use a SIGNAL/SLOT connection to bind to the QProcess own finished() signal. Which of course never triggers because it would only trigger when the process itself exited already.

I will try to come up with a sensible solution at least for the latter problem.

[Zeno Albisser]

actually i misunderstood the purpose of the scheduleWorkOnTermination function... so at least that one is working properly. :-)  ... but it does not serve the purpose i was assuming.

[Zeno Albisser]

Created attachment 108672 [details]
patch for review.

[WebKit Review Bot]

Attachment 108672 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1

Source/WebKit2/WebProcess/qt/WebProcessQt.cpp:70:  Missing space after ,  [whitespace/comma] [3]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Zeno Albisser]

Created attachment 108673 [details]
patch for review - fixed coding style issues

i think this is the best thing we can do for now.
But unless we want to fix circular dependencies of WebContext, WebProcessProxy and WebPageProxy and change the IPC mechanism of the Qt port, this is probably the way to go.

[Zeno Albisser]

i will change it from using exit() to qApp->quit() and try that again. ...

[Alexis Menard (darktears)]

Comment on attachment 108673 [details]
patch for review - fixed coding style issues

Not sure if we will support Leopard but Grand Central Dispatch was added in Snow Leopard.

[Andreas Kling]

(In reply to comment #11)
> (From update of attachment 108673 [details])
> Not sure if we will support Leopard but Grand Central Dispatch was added in Snow Leopard.

We should support whatever Qt 5 will support. I don't think there's a problem adding SL-only things to trunk right now, we can figure this out later if someone decides that Qt 5.0 needs to support Leopard.

[Zeno Albisser]

(In reply to comment #11)
> (From update of attachment 108673 [details])
> Not sure if we will support Leopard but Grand Central Dispatch was added in Snow Leopard.

I am aware of that, but i consider it a minor issue in this ecosystem.
Do we have any guidelines on that?
Or do we know of potential WK2 users using Pre Snow Leopard?

[Zeno Albisser]

Created attachment 109468 [details]
patch for review. - use qApp->quit() instead of exit()

[Alexis Menard (darktears)]

Comment on attachment 109468 [details]
patch for review. - use qApp->quit() instead of exit()

I think its good as we use the same approach than the Mac port. Jut a nitpick QApplication::quit() it is way nicer to read and understand than this magic hacked qApp pointer :D.

[Zeno Albisser]

Created attachment 109476 [details]
patch for review. - use QCoreApplication::quit() instead of qApp->quit()

[Andreas Kling]

Comment on attachment 109476 [details]
patch for review. - use QCoreApplication::quit() instead of qApp->quit()

LGTM.

[WebKit Review Bot]

Comment on attachment 109476 [details]
patch for review. - use QCoreApplication::quit() instead of qApp->quit()

Clearing flags on attachment: 109476

Committed r96494: <http://trac.webkit.org/changeset/96494>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.