Bug 48511 – [GTK] Implement Process/Thread Launcher classes and WebProcessGtkMain for WebKit2

• Component: WebKit2
• Status: RESOLVED FIXED

[Amruth Raj]

Implement Process/Thread Launcher classes and WebProcessGtkMain for WebKit2

[Amruth Raj]

Created attachment 72325 [details]
Implements Process/Thread Launcher classes and WebProcessGtkMain

[Ravi Phaneendra Kasibhatla]

Adding myself to the CC list for this bug.

[Ravi Phaneendra Kasibhatla]

Created attachment 72686 [details]
ProcessLauncher and ThreadLauncher implementation for WebKit2 GTK port

Contains ProcessLauncher and ThreadLauncher implementation for WebKit2 GTK port.
Also contains the WebKitProcessMain class implementation.

[Martin Robinson]

Comment on attachment 72686 [details]
ProcessLauncher and ThreadLauncher implementation for WebKit2 GTK port

View in context: https://bugs.webkit.org/attachment.cgi?id=72686&action=review

Looks good! Some comments below.

> WebKit2/UIProcess/Launcher/gtk/ProcessLauncherGtk.cpp:55
> +    String commandLine(WEBKIT_WEB_PROCESS_PATH);

I don't understand why you are using a String here. Why not just use WEBKIT_WEB_PROCESS_PATH below?

> WebKit2/UIProcess/Launcher/gtk/ProcessLauncherGtk.cpp:68
> +        ASSERT_NOT_REACHED();
> +    }

I think it's better to try to handle the error here somehow, even if it's just printing a message.

> WebKit2/UIProcess/Launcher/gtk/ProcessLauncherGtk.cpp:81
> +    notImplemented();

Is this not implemented or simply an empty implementation?

> WebKit2/WebProcess/gtk/WebProcessMainGtk.cpp:43
> +    // FIXME: Implement.

This looks like a good place for notImplemented() :) I think this file is also missing stubs for platformShutdown and platformClearResourceCaches.

> WebKit2/WebProcess/gtk/WebProcessMainGtk.cpp:54
> +    g_type_init();

Do you also need to call g_thread_init() here? Only after 2.24 does g_type_init() call g_thread_init().

[Ravi Phaneendra Kasibhatla]

Created attachment 77753 [details]
ProcessLauncher and ThreadLauncher implementation for WebKit2 GTK port

ProcessLauncher class implementation for WebKit2 GTK port. Also has stubbed implements port specific virtual functions of WebProcess.h, ThreadLauncher class.
The main() function implementation and WebProcessMain() implementation for WebKitWebProcess binary.

[Ravi Phaneendra Kasibhatla]

(In reply to comment #4)
> (From update of attachment 72686 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=72686&action=review
> 
> Looks good! Some comments below.
> 
> > WebKit2/UIProcess/Launcher/gtk/ProcessLauncherGtk.cpp:55
> > +    String commandLine(WEBKIT_WEB_PROCESS_PATH);
> 
> I don't understand why you are using a String here. Why not just use WEBKIT_WEB_PROCESS_PATH below?
Used String object just to be in WebKit usage pattern (for datatypes). Anyways I have modified code to actually pick the WebKitWebProcess binary from same folder in which current binary resides.

> 
> > WebKit2/UIProcess/Launcher/gtk/ProcessLauncherGtk.cpp:68
> > +        ASSERT_NOT_REACHED();
> > +    }
> 
> I think it's better to try to handle the error here somehow, even if it's just printing a message.
I am not sure what error handling can be done here, so just added a print statement (as you suggested).
> 
> > WebKit2/UIProcess/Launcher/gtk/ProcessLauncherGtk.cpp:81
> > +    notImplemented();
> 
> Is this not implemented or simply an empty implementation?
Its an empty implementation right now. Corrected it.
> 
> > WebKit2/WebProcess/gtk/WebProcessMainGtk.cpp:43
> > +    // FIXME: Implement.
> 
> This looks like a good place for notImplemented() :) I think this file is also missing stubs for platformShutdown and platformClearResourceCaches.
> 
Done.
> > WebKit2/WebProcess/gtk/WebProcessMainGtk.cpp:54
> > +    g_type_init();
> 
> Do you also need to call g_thread_init() here? Only after 2.24 does g_type_init() call g_thread_init().
Done.

[Martin Robinson]

Comment on attachment 77753 [details]
ProcessLauncher and ThreadLauncher implementation for WebKit2 GTK port

View in context: https://bugs.webkit.org/attachment.cgi?id=77753&action=review

> WebKit2/UIProcess/Launcher/gtk/ProcessLauncherGtk.cpp:60
> +    char currentExePath[128] = {0};
> +    ssize_t numOfCharsRead = readlink("/proc/self/exe", currentExePath, 127);
> +    if (numOfCharsRead == -1)
> +        applicationPath = "/usr/local/bin/"
> +    else
> +        applicationPath = currentExePath;

One major issue here is that this will only work on systems with procfs. I'm not sure that's even true of OS X.  If possible we need to have some platform independent way to get the binary path. Maybe it should just be some #ifdefs. I'm not sure what the best solution is here. It's unfortunate that this never made it into GLib.

Another issue is that I believe this code makes the assumption that the path is in the latin1 character set. That obviously won't work when the filesystem uses UTF-8. I think should should use GLib functions which can convert paths into UTF-8 strings.

> WebKit2/UIProcess/Launcher/gtk/ProcessLauncherGtk.cpp:61
> +    applicationPath = applicationPath.substring(0, applicationPath.reverseFind("/")+1) + WEBKIT_WEB_PROCESS_NAME;

I think instead of searching through the string, it's better to use GLib functions for munging paths, such as g_path_get_dirname. Of course, the path will need to be a raw string, since it expects it to be in the native format.

> WebKit2/UIProcess/Launcher/gtk/ProcessLauncherGtk.cpp:74
> +        // FIXME:: What else can we do in this case?

One extra : here. :)

[Ravi Phaneendra Kasibhatla]

Created attachment 78237 [details]
ProcessLauncher and ThreadLauncher implementation for WebKit2 GTK port

[Ravi Phaneendra Kasibhatla]

(In reply to comment #7)
> (From update of attachment 77753 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=77753&action=review
> 
> > WebKit2/UIProcess/Launcher/gtk/ProcessLauncherGtk.cpp:60
> > +    char currentExePath[128] = {0};
> > +    ssize_t numOfCharsRead = readlink("/proc/self/exe", currentExePath, 127);
> > +    if (numOfCharsRead == -1)
> > +        applicationPath = "/usr/local/bin/"
> > +    else
> > +        applicationPath = currentExePath;
> 
> One major issue here is that this will only work on systems with procfs. I'm not sure that's even true of OS X.  If possible we need to have some platform independent way to get the binary path. Maybe it should just be some #ifdefs. I'm not sure what the best solution is here. It's unfortunate that this never made it into GLib.
Made changes as we discussed. Added a new API in FileSystemGtk.cpp and implemented for UNIX/LINUX ports. Other ports yet to be implemented.
> 
> Another issue is that I believe this code makes the assumption that the path is in the latin1 character set. That obviously won't work when the filesystem uses UTF-8. I think should should use GLib functions which can convert paths into UTF-8 strings.
Done.
> 
> > WebKit2/UIProcess/Launcher/gtk/ProcessLauncherGtk.cpp:61
> > +    applicationPath = applicationPath.substring(0, applicationPath.reverseFind("/")+1) + WEBKIT_WEB_PROCESS_NAME;
> 
> I think instead of searching through the string, it's better to use GLib functions for munging paths, such as g_path_get_dirname. Of course, the path will need to be a raw string, since it expects it to be in the native format.
> 
Done.
> > WebKit2/UIProcess/Launcher/gtk/ProcessLauncherGtk.cpp:74
> > +        // FIXME:: What else can we do in this case?
> 
> One extra : here. :)
Done. :)

[Martin Robinson]

Comment on attachment 78237 [details]
ProcessLauncher and ThreadLauncher implementation for WebKit2 GTK port

View in context: https://bugs.webkit.org/attachment.cgi?id=78237&action=review

Look good, but we need to be a bit careful about path encoding.

> WebCore/platform/gtk/FileSystemGtk.cpp:190
> +#if OS(UNIX)
> +#if OS(LINUX)

I think it makes more sense to use #if OS(LINUX) for the first block and then do #elif OS(UNIX) for the second block.

> WebCore/platform/gtk/FileSystemGtk.cpp:192
> +    char pathFromProc[128] = {0};

This should probably be sized PATH_MAX, but 128 is definitely too small. :)

> WebCore/platform/gtk/FileSystemGtk.cpp:196
> +    return directoryName(String::fromUTF8(pathFromProc));

You really want to use filenameToString here, since Linux paths aren't necessarily UTF-8, but since you're just going just use it with GLib later, I recommend just returning a CString from this function.

> WebCore/platform/gtk/FileSystemGtk.cpp:207
> +    return directoryName(String::fromUTF8(currentExePath.get()));
> +#else
> +    return String();
> +#endif

Same comment here. Just return a CString. See below at the callsite for why.

> WebKit2/WebProcess/gtk/WebProcessMainGtk.cpp:45
> +    if (getenv("WEBKIT2_DEBUG_MODE"))
> +        sleep(30);

I would prefer to use the same environment variable as Qt, WEBKIT2_PAUSE_WEB_PROCESS_ON_LAUNCH and to also surround this with an #ifndef NDEBUG.

> WebKit2/UIProcess/Launcher/gtk/ProcessLauncherGtk.cpp:43
> +#define WEBKIT_WEB_PROCESS_NAME "WebKitWebProcess" 

This should either be:
1. a const char* gWebKitWebProcessName = "WebKitWebProcess";
2. Passed in by the build system.
3. Used as a constant below.

> WebKit2/UIProcess/Launcher/gtk/ProcessLauncherGtk.cpp:51
> +    if (socketpair(AF_UNIX, SOCK_STREAM, 0, sockets) < 0) 
> +        return;

Better print an error and ASSERT_NOT_REACHED here too.

> WebKit2/UIProcess/Launcher/gtk/ProcessLauncherGtk.cpp:57
> +        String binaryPath = applicationDirectoryPath() + String("/") + String(WEBKIT_WEB_PROCESS_NAME);

You should use g_build_filename here, as and just work with raw char arrays.

> WebKit2/UIProcess/Launcher/gtk/ProcessLauncherGtk.cpp:58
> +        execl(binaryPath.utf8().data(), WEBKIT_WEB_PROCESS_NAME, socket.utf8().data(), (char*)0);

If you end up sticking with execl, please just use NULL here instead of casting 0.

[Amruth Raj]

Created attachment 78312 [details]
Addressed review comments

[Amruth Raj]

Incorporated all the comments from https://bugs.webkit.org/show_bug.cgi?id=48511#c10

[Martin Robinson]

Comment on attachment 78312 [details]
Addressed review comments

View in context: https://bugs.webkit.org/attachment.cgi?id=78312&action=review

Thanks for the quick update! There's one more significant issue here that I can see before landing.

> WebCore/platform/gtk/FileSystemGtk.cpp:196
> +    String filename = filenameToString(pathFromProc);
> +    return directoryName(filename).utf8();

Here you should just return pathFromProc. Sorry that I wasn't clear about that. Converting a CString to a String directly assumes that it's in the latin1 encoding. And then using the UTF-8 encoding might break the subsequent call to execl if the target filesystem is not UTF-8 encoded.

> WebCore/platform/gtk/FileSystemGtk.cpp:204
> +    String filename = filenameToString(currentExePath.get());
> +    return directoryName(fileName).utf8();

Please just return currentExePath here.

[Amruth Raj]

Created attachment 78323 [details]
Removed the unnecessary encoding

[Amruth Raj]

(In reply to comment #13)
> (From update of attachment 78312 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=78312&action=review
> 
> Thanks for the quick update! There's one more significant issue here that I can see before landing.
> 
> > WebCore/platform/gtk/FileSystemGtk.cpp:196
> > +    String filename = filenameToString(pathFromProc);
> > +    return directoryName(filename).utf8();
> 
> Here you should just return pathFromProc. Sorry that I wasn't clear about that. Converting a CString to a String directly assumes that it's in the latin1 encoding. And then using the UTF-8 encoding might break the subsequent call to execl if the target filesystem is not UTF-8 encoded.
> 
> > WebCore/platform/gtk/FileSystemGtk.cpp:204
> > +    String filename = filenameToString(currentExePath.get());
> > +    return directoryName(fileName).utf8();
> 
> Please just return currentExePath here.

Addressed the comments above. Not doing any encoding now and just returning pathFromProc/currentExePath

[Martin Robinson]

Comment on attachment 78323 [details]
Removed the unnecessary encoding

View in context: https://bugs.webkit.org/attachment.cgi?id=78323&action=review

Thanks!

> WebCore/platform/gtk/FileSystemGtk.cpp:196
> +    return CString(dirname.get());

The CString call isn't strictly necessary here. I'll fix this before landing.

> WebCore/platform/gtk/FileSystemGtk.cpp:204
> +    return CString(dirname.get());

Ditto.

[Amruth Raj]

Created attachment 78325 [details]
Removed the explicit constructor call

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/platform/FileSystem.h', u'Source/WebCore/platform/gtk/FileSystemGtk.cpp', u'WebKit2/ChangeLog', u'WebKit2/UIProcess/Launcher/gtk/ProcessLauncherGtk.cpp', u'WebKit2/UIProcess/Launcher/gtk/ThreadLauncherGtk.cpp', u'WebKit2/WebProcess/gtk/WebProcessGtk.cpp', u'WebKit2/WebProcess/gtk/WebProcessMainGtk.cpp', u'WebKit2/WebProcess/gtk/WebProcessMainGtk.h', u'WebKit2/gtk/MainGtk.cpp']" exit_code: 1
WebKit2/UIProcess/Launcher/gtk/ProcessLauncherGtk.cpp:62:  Use 0 instead of NULL.  [readability/null] [5]
Total errors found: 1 in 10 files


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

[Martin Robinson]

Comment on attachment 78325 [details]
Removed the explicit constructor call

Thanks!

[WebKit Commit Bot]

Comment on attachment 78325 [details]
Removed the explicit constructor call

Rejecting attachment 78325 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-4', 'land-attachment', '--force-clean', '--ignore-builders', '--non-interactive', '--parent-command=commit-queue', 78325]" exit_code: 1
Last 500 characters of output:
ommit-queue/Tools/Scripts/webkitpy/tool/commands/stepsequence.py", line 64, in _run
    step(tool, options).run(state)
  File "/mnt/git/webkit-commit-queue/Tools/Scripts/webkitpy/tool/steps/validatereviewer.py", line 57, in run
    if self._has_valid_reviewer(changelog_entry):
  File "/mnt/git/webkit-commit-queue/Tools/Scripts/webkitpy/tool/steps/validatereviewer.py", line 42, in _has_valid_reviewer
    if changelog_entry.reviewer():
AttributeError: 'NoneType' object has no attribute 'reviewer'

Full output: http://queues.webkit.org/results/7475011

[Ravi Phaneendra Kasibhatla]

Created attachment 78342 [details]
Patch on latest trunk for commit

Reviewed patch on latest svn trunk

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/platform/FileSystem.h', u'Source/WebCore/platform/gtk/FileSystemGtk.cpp', u'WebKit2/ChangeLog', u'WebKit2/UIProcess/Launcher/gtk/ProcessLauncherGtk.cpp', u'WebKit2/UIProcess/Launcher/gtk/ThreadLauncherGtk.cpp', u'WebKit2/WebProcess/gtk/WebProcessGtk.cpp', u'WebKit2/WebProcess/gtk/WebProcessMainGtk.cpp', u'WebKit2/WebProcess/gtk/WebProcessMainGtk.h', u'WebKit2/gtk/MainGtk.cpp']" exit_code: 1
WebKit2/UIProcess/Launcher/gtk/ProcessLauncherGtk.cpp:62:  Use 0 instead of NULL.  [readability/null] [5]
Total errors found: 1 in 10 files


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

[WebKit Commit Bot]

Comment on attachment 78342 [details]
Patch on latest trunk for commit

Clearing flags on attachment: 78342

Committed r75347: <http://trac.webkit.org/changeset/75347>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 78342 [details]:

http/tests/xmlhttprequest/failed-auth.html bug 51835 (author: [e-mail])
The commit-queue is continuing to process your patch.