Bug 48511

Summary: [GTK] Implement Process/Thread Launcher classes and WebProcessGtkMain for WebKit2
Product: WebKit Reporter: Amruth Raj <amruthraj>
Component: WebKit2Assignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: andersca, commit-queue, gustavo, kbalazs, mrobinson, ravi.kasibhatla, webkit.review.bot, xan.lopez
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Linux   
Bug Depends on:    
Bug Blocks: 52805    
Attachments:
Description Flags
Implements Process/Thread Launcher classes and WebProcessGtkMain
none
ProcessLauncher and ThreadLauncher implementation for WebKit2 GTK port
mrobinson: review-
ProcessLauncher and ThreadLauncher implementation for WebKit2 GTK port
mrobinson: review-
ProcessLauncher and ThreadLauncher implementation for WebKit2 GTK port
mrobinson: review-
Addressed review comments
mrobinson: review-
Removed the unnecessary encoding
none
Removed the explicit constructor call
none
Patch on latest trunk for commit none

Description Amruth Raj 2010-10-28 04:50:52 PDT
Implement Process/Thread Launcher classes and WebProcessGtkMain for WebKit2
Comment 1 Amruth Raj 2010-10-29 06:24:35 PDT
Created attachment 72325 [details]
Implements Process/Thread Launcher classes and WebProcessGtkMain
Comment 2 Ravi Phaneendra Kasibhatla 2010-10-29 07:15:43 PDT
Adding myself to the CC list for this bug.
Comment 3 Ravi Phaneendra Kasibhatla 2010-11-02 09:46:13 PDT
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.
Comment 4 Martin Robinson 2010-12-30 09:35:03 PST
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().
Comment 5 Ravi Phaneendra Kasibhatla 2011-01-01 12:42:01 PST
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.
Comment 6 Ravi Phaneendra Kasibhatla 2011-01-01 12:45:22 PST
(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.
Comment 7 Martin Robinson 2011-01-04 09:37:00 PST
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. :)
Comment 8 Ravi Phaneendra Kasibhatla 2011-01-07 07:23:40 PST
Created attachment 78237 [details]
ProcessLauncher and ThreadLauncher implementation for WebKit2 GTK port
Comment 9 Ravi Phaneendra Kasibhatla 2011-01-07 08:51:49 PST
(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. :)
Comment 10 Martin Robinson 2011-01-07 10:26:45 PST
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.
Comment 11 Amruth Raj 2011-01-08 09:17:25 PST
Created attachment 78312 [details]
Addressed review comments
Comment 12 Amruth Raj 2011-01-08 09:19:54 PST
Incorporated all the comments from https://bugs.webkit.org/show_bug.cgi?id=48511#c10
Comment 13 Martin Robinson 2011-01-08 09:40:13 PST
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.
Comment 14 Amruth Raj 2011-01-08 11:34:39 PST
Created attachment 78323 [details]
Removed the unnecessary encoding
Comment 15 Amruth Raj 2011-01-08 11:36:04 PST
(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
Comment 16 Martin Robinson 2011-01-08 11:39:48 PST
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.
Comment 17 Amruth Raj 2011-01-08 12:08:46 PST
Created attachment 78325 [details]
Removed the explicit constructor call
Comment 18 WebKit Review Bot 2011-01-08 12:10:42 PST
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.
Comment 19 Martin Robinson 2011-01-08 12:20:20 PST
Comment on attachment 78325 [details]
Removed the explicit constructor call

Thanks!
Comment 20 WebKit Commit Bot 2011-01-08 19:13:32 PST
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
Comment 21 Ravi Phaneendra Kasibhatla 2011-01-08 23:08:48 PST
Created attachment 78342 [details]
Patch on latest trunk for commit

Reviewed patch on latest svn trunk
Comment 22 WebKit Review Bot 2011-01-08 23:11:29 PST
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.
Comment 23 WebKit Commit Bot 2011-01-09 00:57:53 PST
Comment on attachment 78342 [details]
Patch on latest trunk for commit

Clearing flags on attachment: 78342

Committed r75347: <http://trac.webkit.org/changeset/75347>
Comment 24 WebKit Commit Bot 2011-01-09 00:58:01 PST
All reviewed patches have been landed.  Closing bug.
Comment 25 WebKit Commit Bot 2011-01-09 01:38:50 PST
The commit-queue encountered the following flaky tests while processing attachment 78342 [details]:

http/tests/xmlhttprequest/failed-auth.html bug 51835 (author: ap@webkit.org)
The commit-queue is continuing to process your patch.