Bug 53380 – [Qt] WebKit does not build on Mac with WebKit 2

• Component: WebKit2
• Status: RESOLVED FIXED

[Benjamin Poulain]

WebKit does not build on Mac if WebKit 2 is enabled.

[Benjamin Poulain]

Created attachment 80572 [details]
Patch

[Benjamin Poulain]

I basically fixed the build issues one by one until it compiles, link and run :)

There is two things I am not sure, maybe Kimmo can tell:
-is "mkostemp(tempNameC, O_CREAT | O_CLOEXEC | O_RDWR))" subject to race condition on close-on-exec? Because the fallback use fcntl() after the creation, so there is some time without close-on-exec.
-is it ok to pass no flag to recvmsg()? The doc is unclear (it says one or more flags)

[Eric Seidel (no email)]

Sam may like to see wk2 changes go by, even though he doesn't work with qt.

[Kimmo Kinnunen]

(In reply to comment #2)
> I basically fixed the build issues one by one until it compiles, link and run :)

Nice!
 
> There is two things I am not sure, maybe Kimmo can tell:
> -is "mkostemp(tempNameC, O_CREAT | O_CLOEXEC | O_RDWR))" subject to race condition on close-on-exec? Because the fallback use fcntl() after the creation, so there is some time without close-on-exec.

The race is about other thread forking the process after mkostemp but before the fctnl. In this case it is not a problem, and mkstemp + fcntl suffices. If you want to refine the patch, IMHO it could be used as the only codepath in order to reduce the #ifdef'ing.

> -is it ok to pass no flag to recvmsg()? The doc is unclear (it says one or more flags)

I interpret the man page ambiguous on flag. As far as I tested, passing 0 is good.

[Siddharth Mathur]

(In reply to comment #4)
> (In reply to comment #2)
> > I basically fixed the build issues one by one until it compiles, link and run :)
> 
> Nice!
> 
> > There is two things I am not sure, maybe Kimmo can tell:
> > -is "mkostemp(tempNameC, O_CREAT | O_CLOEXEC | O_RDWR))" subject to race condition on close-on-exec? Because the fallback use fcntl() after the creation, so there is some time without close-on-exec.
> 
> The race is about other thread forking the process after mkostemp but before the fctnl. In this case it is not a problem, and mkstemp + fcntl suffices. If you want to refine the patch, IMHO it could be used as the only codepath in order to reduce the #ifdef'ing.
> 
> > -is it ok to pass no flag to recvmsg()? The doc is unclear (it says one or more flags)
> 
> I interpret the man page ambiguous on flag. As far as I tested, passing 0 is good.

Since I see that kling has r- the patch, Kimmo, would you be kind enough to upload a patch that fixes this issue for Mac and/or Linux distros that don't support 'mkostemp' and the new flags? 

../../../Source/WebKit2/Platform/qt/SharedMemoryQt.cpp: In static member function 'static WTF::PassRefPtr<WebKit::SharedMemory> WebKit::SharedMemory::create(size_t)':
../../../Source/WebKit2/Platform/qt/SharedMemoryQt.cpp:109: error: 'O_CLOEXEC' was not declared in this scope
../../../Source/WebKit2/Platform/qt/SharedMemoryQt.cpp:109: error: 'mkostemp' was not declared in this scope
../../../Source/WebKit2/Platform/qt/SharedMemoryQt.cpp: In member function 'bool WebKit::SharedMemory::createHandle(WebKit::SharedMemory::Handle&, WebKit::SharedMemory::Protection)':
../../../Source/WebKit2/Platform/qt/SharedMemoryQt.cpp:199: error: 'O_CLOEXEC' was not declared in this scope
make[1]: *** [obj/release/SharedMemoryQt.o] Error 1
make[1]: *** Waiting for unfinished jobs....
../../../Source/WebKit2/Platform/CoreIPC/qt/ConnectionQt.cpp: In member function 'void CoreIPC::Connection::readyReadHandler()':
../../../Source/WebKit2/Platform/CoreIPC/qt/ConnectionQt.cpp:168: error: 'MSG_CMSG_CLOEXEC' was not declared in this scope
make[1]: *** [obj/release/ConnectionQt.o] Error 1

[Kimmo Kinnunen]

(In reply to comment #5)
> Since I see that kling has r- the patch, Kimmo, would you be kind enough to upload a patch that fixes this issue for Mac and/or Linux distros that don't support 'mkostemp' and the new flags? 

(12:32:13 PM) kling: kimmok: benjaminp said he wanted to make a new version based on your comments, and told me to r- the old one

[Siddharth Mathur]

Created attachment 81289 [details]
Fix for Linux distros that don't support mkostemp (only mkstemp)

[Siddharth Mathur]

Created attachment 81303 [details]
Fix for Linux 

(It will be good if someone can test this too)

[Benjamin Poulain]

Comment on attachment 81303 [details]
Fix for Linux 

And what? If the system is not POSIX.1-2008 you just ignore close on exec?

Seem bogus to me. I said I would update for something more generic, not just ignore the stuff I don't like.

[Early Warning System Bot]

Attachment 81303 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7699336

[Kimmo Kinnunen]

(In reply to comment #7)
> Created an attachment (id=81289) [details]
> Fix for Linux distros that don't support mkostemp (only mkstemp)

Siddharth, was there something that didn't work for you in Benjamin's original patch?

Benjamin, Andreas:
Since this is fairly important, could we get the Benjamin's original patch in to get things compiling again? With cq+ perhaps, so mac compiles wouldn't block? And then decreasing the #ifdef'ing would come as second priority..

[Andreas Kling]

Comment on attachment 80572 [details]
Patch

@Kimmo: Sure, let's land this for now.

[Benjamin Poulain]

Comment on attachment 80572 [details]
Patch

Patch on the way

[Benjamin Poulain]

Created attachment 81363 [details]
Patch

[Andreas Kling]

Comment on attachment 81363 [details]
Patch

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

r=me

> Source/WebKit2/Platform/qt/SharedMemoryQt.cpp:206
> +    while ((fcntl(duplicatedHandle, F_SETFD,  FD_CLOEXEC | accessModeFile(protection)) == -1)) {

Extra space before FD_CLOEXEC.

> Source/WebKit2/UIProcess/WebPageProxy.cpp:1093
> +#if defined(__APPLE__) && !PLATFORM(QT)

This is quite ugly. I wish we'd use something other than __APPLE__ for mac-port-specific stuff.

[Benjamin Poulain]

Created attachment 81368 [details]
Patch

[Early Warning System Bot]

Attachment 81368 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7701411

[Benjamin Poulain]

Created attachment 81371 [details]
Patch

oops

[WebKit Review Bot]

Attachment 81368 [details] did not build on mac:
Build output: http://queues.webkit.org/results/7697938

[WebKit Commit Bot]

Comment on attachment 81371 [details]
Patch

Clearing flags on attachment: 81371

Committed r77765: <http://trac.webkit.org/changeset/77765>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Siddharth Mathur]

Thank you Benjamin and Kling!