Bug 53380

Created attachment 80572 [details] Patch

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)

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

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

(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

(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

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

Created attachment 81303 [details] Fix for Linux (It will be good if someone can test this too)

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.

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

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

Comment on attachment 80572 [details] Patch @Kimmo: Sure, let's land this for now.

Comment on attachment 80572 [details] Patch Patch on the way

Created attachment 81363 [details] Patch

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.

Created attachment 81368 [details] Patch

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

Created attachment 81371 [details] Patch oops

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

Comment on attachment 81371 [details] Patch Clearing flags on attachment: 81371 Committed r77765: <http://trac.webkit.org/changeset/77765>

All reviewed patches have been landed. Closing bug.

Thank you Benjamin and Kling!