This feature needs deep investigation, but looks like we can achieve similar design as the Mac sandbox using seccomp with filters on Linux. We might be able to block the WebProcess from opening files (delegating the task to the UIProcess and perform some policy check) and/or constrain the file access to certain files/folders.
An alternative that might require less work and could be a first step to have some sandboxing would be to use setuid-sandbox[1], which is the standalone version of the sandbox used in Chrome on Linux (though they are working on using seccomp too AFAIK). [1] http://code.google.com/p/setuid-sandbox/
(In reply to comment #1) > An alternative that might require less work and could be a first step to have some sandboxing would be to use setuid-sandbox[1], which is the standalone version of the sandbox used in Chrome on Linux (though they are working on using seccomp too AFAIK). > > [1] http://code.google.com/p/setuid-sandbox/ Renata is working on this track at bug 90005.
Created attachment 156368 [details] Work in progress of SeccompFilters v1
Created attachment 156370 [details] Work in progress of SeccompFilters v2 Fixed a nit.
Attachment 156370 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/PlatformEfl.cmake', u'Sourc..." exit_code: 1 Source/WebKit2/Shared/seccomp/SeccompFilters.cpp:37: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] Source/WebKit2/Shared/seccomp/SeccompFilters.cpp:170: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] Total errors found: 2 in 12 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 156370 [details] Work in progress of SeccompFilters v2 View in context: https://bugs.webkit.org/attachment.cgi?id=156370&action=review Looks pretty ok for a start > Source/WebKit2/Shared/seccomp/SeccompFilters.cpp:50 > + // No being able to initialize seccomp is considered a security > + // hole and the application should not go beyond this point. In fact, security breach? > Source/WebKit2/Shared/seccomp/SeccompFilters.cpp:63 > + seccomp_release(m_seccompContext); why not just m_context > Source/WebKit2/Shared/seccomp/SeccompFilters.cpp:66 > +void SeccompFilters::setupFilters() initializeFilters? or just initialize() > Source/WebKit2/Shared/seccomp/SeccompFilters.cpp:68 > + // Prioritize the are the most frequent calls. I didn't get that English > Source/WebKit2/Shared/seccomp/SeccompFilters.cpp:75 > + // in the way it is implemented now. It is only blocking syscalls we are not right now > Source/WebKit2/Shared/seccomp/SeccompFilters.cpp:78 > + // The list bellow summarizes the syscall whitelist needed by a EFL below why EFL here, in a non EFL file? > Source/WebKit2/Shared/seccomp/SeccompFilters.cpp:114 > + // Used by g_system_thread_set_name(). > + seccomp_rule_add_or_crash(m_seccompContext, SCMP_ACT_ALLOW, SCMP_SYS(prctl), 0); > + if USE(GLIB) ? > Source/WebKit2/Shared/seccomp/SeccompFilters.h:38 > +class SeccompFilters { > +public: > + SeccompFilters(); > + virtual ~SeccompFilters(); who will inherit from this?
Comment on attachment 156370 [details] Work in progress of SeccompFilters v2 View in context: https://bugs.webkit.org/attachment.cgi?id=156370&action=review Thanks for having a look. >> Source/WebKit2/Shared/seccomp/SeccompFilters.cpp:66 >> +void SeccompFilters::setupFilters() > > initializeFilters? or just initialize() +1 for inititalize and platformInitialize() >> Source/WebKit2/Shared/seccomp/SeccompFilters.cpp:68 >> + // Prioritize the are the most frequent calls. > > I didn't get that English "Prioritize the most frequent calls." >> Source/WebKit2/Shared/seccomp/SeccompFilters.cpp:78 >> + // The list bellow summarizes the syscall whitelist needed by a EFL > > below > > why EFL here, in a non EFL file? This WARNING section is a temporary documentation of the work in progress. I'm just describing the test environment in case someone wants to give it a try. Probably if you try the same rule set on Qt (and I'm working on testing this right now), it will crash. >> Source/WebKit2/Shared/seccomp/SeccompFilters.cpp:114 >> + > > if USE(GLIB) ? It should go to platformSetupFilters() after the triage. >> Source/WebKit2/Shared/seccomp/SeccompFilters.h:38 >> + virtual ~SeccompFilters(); > > who will inherit from this? No one. I first wrote this as an interface and later changed to the platformInitialize() style.
Created attachment 156397 [details] Work in progress of SeccompFilters v3
Comment on attachment 156397 [details] Work in progress of SeccompFilters v3 View in context: https://bugs.webkit.org/attachment.cgi?id=156397&action=review Reviewing without looking at the seccomp-specific bits yet. I wonder if the EFL-independent part shouldn't be sent in a separate patch, together with build-webkit changes. > Source/WebKit2/PlatformEfl.cmake:168 > + LIST (APPEND WebKit2_LIBRARIES > + ${LIBSECCOMP_LIBRARIES} > + ) You also need to include its include path to the list of included directories. > Source/WebKit2/Shared/seccomp/SeccompFilters.cpp:36 > +#define seccomp_rule_add_or_crash(context, action, syscall, count, ...) \ Isn't it better to uppercase this macro name to make it clear it is a macro? > Source/cmake/FindLibSeccomp.cmake:31 > +INCLUDE(FindPkgConfig) > + > +PKG_CHECK_MODULES(LIBSECCOMP REQUIRED libseccomp>=1.0.0) Please see https://bugs.webkit.org/show_bug.cgi?id=91864#c18 for my comments on why this should be expanded into proper CMake FIND_LIBRARY/FIND_PATH calls.
Comment on attachment 156397 [details] Work in progress of SeccompFilters v3 Attachment 156397 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/13425732
Comment on attachment 156397 [details] Work in progress of SeccompFilters v3 View in context: https://bugs.webkit.org/attachment.cgi?id=156397&action=review >> Source/WebKit2/Shared/seccomp/SeccompFilters.cpp:36 >> +#define seccomp_rule_add_or_crash(context, action, syscall, count, ...) \ > > Isn't it better to uppercase this macro name to make it clear it is a macro? This macro is temporary. I'm trying to land a patch on seccomp that would allow me to wrap this in a method. >> Source/cmake/FindLibSeccomp.cmake:31 >> +PKG_CHECK_MODULES(LIBSECCOMP REQUIRED libseccomp>=1.0.0) > > Please see https://bugs.webkit.org/show_bug.cgi?id=91864#c18 for my comments on why this should be expanded into proper CMake FIND_LIBRARY/FIND_PATH calls. Let's discuss this one when the patch is final. I have a feeling that this one is going to be long... :)
Created attachment 156604 [details] Work in progress of SeccompFilters v4 Added to Qt buildsystem. It could load pages just fine.
Created attachment 156605 [details] Work in progress of SeccompFilters v4 - correct
Created attachment 161845 [details] Work in progress of SeccompFilters v4 - rebased
Comment on attachment 161845 [details] Work in progress of SeccompFilters v4 - rebased Attachment 161845 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/13717845
Created attachment 168170 [details] SeccompFilters on Qt (x86/64) It works with these modifications on Qt on x86/64.
I've measured the pageload time with Methanol using the syscall set that is enough to load pages correctly on Qt on an x86/64 machine. I ran the tests on sites that can be found on alexa top50. The goal of the measurement was that whether applying seccomp filters causes relevant slow-down on real world examples. I've observed ~1% slow-down using these sites. This table contains the times with and without seccomp filters. Each page was loaded 30 times, the maximal variance was 3%. test site w/ libseccomp w/o libseccomp difference blogger.com 31,3 31 0,9% facebook.com 53,3 52,9 0,8% google.com 16,2 16 1,3% live.com 18,2 17,9 1,7% microsoft.com 120,9 119,8 0,9% qq.com 144,2 141,7 1,8% taobao.com 768,2 767,8 0,0% vkontakte.ru 28,3 27,8 1,7% wordpress.com 112,2 111,6 0,5% yandex.ru 129,8 129,2 0,4% conduit.com 136,1 135,6 0,3% ebay.com 84,4 83,3 1,4% fc2.com 145,3 142,9 1,7% linkedin.com 80,7 80,2 0,6% mail.ru 105,5 104,3 1,2% msn.com 225,5 224,9 0,3% wikipedia.org 53,6 53,6 0,0% yahoo.com 90,2 89,4 0,9% youtube.com 151,4 150,5 0,6% summary 2495,3 2480,5 0,6%
(In reply to comment #17) > I've measured the pageload time with Methanol using the syscall set that is enough to load pages correctly on Qt on an x86/64 machine. I ran the tests on sites that can be found on alexa top50. The goal of the measurement was that whether applying seccomp filters causes relevant slow-down on real world examples. I've observed ~1% slow-down using these sites. > This table contains the times with and without seccomp filters. Each page was loaded 30 times, the maximal variance was 3%. > > test site w/ libseccomp w/o libseccomp difference > > blogger.com 31,3 31 0,9% > facebook.com 53,3 52,9 0,8% > google.com 16,2 16 1,3% > live.com 18,2 17,9 1,7% > microsoft.com 120,9 119,8 0,9% > qq.com 144,2 141,7 1,8% > taobao.com 768,2 767,8 0,0% > vkontakte.ru 28,3 27,8 1,7% > wordpress.com 112,2 111,6 0,5% > yandex.ru 129,8 129,2 0,4% > conduit.com 136,1 135,6 0,3% > ebay.com 84,4 83,3 1,4% > fc2.com 145,3 142,9 1,7% > linkedin.com 80,7 80,2 0,6% > mail.ru 105,5 104,3 1,2% > msn.com 225,5 224,9 0,3% > wikipedia.org 53,6 53,6 0,0% > yahoo.com 90,2 89,4 0,9% > youtube.com 151,4 150,5 0,6% > > summary 2495,3 2480,5 0,6% Thanks for doing this! It is really good to have these measurements.
Created attachment 178024 [details] Work in progress of SeccompFilters v5 I have updated the patch, now we need WebKit at hash 4a0b22f1bf4c3ea7553ab4655c4d16d0312cc683 and Qt5 r40 to build WK with it. The system calls that are also needed for a non-minimal Qt build to run Methanol tests properly can be found in SeccompFiltersQt.cpp. After some attempts to limit open's parameter I see why we should do syscall hijacking in run-time and why we can't use libseccomp for it. One of the causes is threading. I will attach a file that contains the open calls by WebProcess after loading the rules. When I took limitation for the first parameter this way // addRule(SCMP_ACT_ALLOW, SCMP_SYS(open)); const char* filePath = "/proc/self/maps"; // it is the first file that should be opened after loading the filters uint64t convertedFilePath = (uint64_t) filePath; // because SCMP_CMP should be used with argument type scmp_datum_t (uint64_t) seccomp_rule_add(m_context, SCMP_SYS_ALLOW, SCMP_SYS(open), 1, SCMP_CMP(0, SCMP_CMP_EQ, convertedFilePath, convertedFilePath))); the WebProcess crashes when it tries to reach that file despite of it is on the whitelist. I experimented also with file descriptor argument, the same error, "Bad system call". The debugging's result is that WebProcess crashes in Source/WTF/wtf/StackBounds.cpp in method WTF::StackBounds::initialize at line pthread_getattr_np(thread, &sattr); If I interpreted it correctly, the problem is an other thread doesn't see the rule we made in the main thread. Do you have any idea for it? The other problem: we will only know the needed caches' paths after seccomp filters are activated, but we should take limitations for them statically, so we cannot add them to the whitelist simply. What do you think, will libseccomp support some kind of wildcard/regexp in respect of parameters? So what would be the right track: waiting for libseccomp's solution, or use an own for it and make a hybrid one by using libseccomp and raw kernel headers too? (I successfully gained the limited open's parameter in run-time with the help of http://lxr.free-electrons.com/source/samples/seccomp/bpf-direct.c but it is strongly platform-dependent because of the registers. In any case with this we can examine the file's path in run-time and make some more rule.)
Created attachment 178025 [details] Opened files by QtWebProcess It were made with strace, shows the open calls after loading seccomp filters. These files are needed to display Google.
I have almost forgotten, I used ifdefs to allow syscalls for 32 and 64 bit systems, e.g. #ifdef __x86_64__ addRule(SCMP_ACT_ALLOW, SCMP_SYS(lseek)); #else #ifdef __i386__ addRule(SCMP_ACT_ALLOW, SCMP_SYS(_llseek)); #endif #endif What do you think about it?
(In reply to comment #21) > I have almost forgotten, I used ifdefs to allow syscalls for 32 and 64 bit systems, e.g. > > #ifdef __x86_64__ > addRule(SCMP_ACT_ALLOW, SCMP_SYS(lseek)); > #else > #ifdef __i386__ > addRule(SCMP_ACT_ALLOW, SCMP_SYS(_llseek)); > #endif > #endif > > What do you think about it? Hi Nandor, I'm glad to tell you that I'm back on working on this issue. I'm right now implementing the syscall broker and some improvements to libseccomp itself. I'm redesigning the Seccomp class on WK to make it more flexible and with less ifdef's (if any). I'll try to uploading ASAP as soon as I have something working. Cheers,
Created attachment 181544 [details] Patch
Attachment 181544 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WTF/ChangeLog', u'Source/WTF/wtf/As..." exit_code: 1 Source/WebKit2/Shared/linux/OpenSyscall.cpp:42: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] Total errors found: 1 in 20 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 181545 [details] Patch
Comment on attachment 181545 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=181545&action=review > Source/cmake/OptionsEfl.cmake:82 > +WEBKIT_OPTION_DEFAULT_PORT_VALUE(ENABLE_SECCOMP_FILTERS ON) In case we land this patch, this will be set to OFF. It is ON here to make it sure that the patch builds correctly on the bots.
Attachment 181545 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'Source/WTF/ChangeLog', u'Sou..." exit_code: 1 Source/WebKit2/Shared/linux/OpenSyscall.cpp:42: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] Total errors found: 1 in 26 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 181545 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=181545&action=review >> Source/WebKit2/Shared/linux/OpenSyscall.cpp:42 >> +COMPILE_ASSERT(O_RDONLY == 0, O_RDONLY); > > Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] COMPILE_ASSERT(O_RDONLY + 1 == 1, O_RDONLY); passes, but IMO is ugly. I need these asserts to make sure the value of these flags matches my assumptions. Looks like the spec doesn't mandate a value to this constants, different libc's might give my different values (although I think this is very unlikely).
Comment on attachment 181545 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=181545&action=review >>> Source/WebKit2/Shared/linux/OpenSyscall.cpp:42 >>> +COMPILE_ASSERT(O_RDONLY == 0, O_RDONLY); >> >> Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] > > COMPILE_ASSERT(O_RDONLY + 1 == 1, O_RDONLY); passes, but IMO is ugly. > > I need these asserts to make sure the value of these flags matches my assumptions. Looks like the spec doesn't mandate a value to this constants, different libc's might give my different values (although I think this is very unlikely). I think the style script expects: COMPILE_ASSERT(!O_RDONLY, O_RDONLY); But really, I don't know if it is better in this context.
How does this relate to bug 90005? Until there is consensus on the proper way to move forward with sandboxing, I don't think this should be landed.
(In reply to comment #30) > How does this relate to bug 90005? Until there is consensus on the proper way to move forward with sandboxing, I don't think this should be landed. This is a different approach for sandboxing the render/plugin/network process on Linux but not an competitor to the setuid sandbox. Chromium is moving towards Seccomp Filters on Linux, but not exclusively [1]. It is actually stacking seccomp-bpf (an approach similar to this patch) on top of the setuid sandbox (something like bug 90005) [2]. I would still appreciate comments to this patch from you and other reviewers even though you marked it as r-. A good start would be having a look at the unit tests [3]. The SetUp() method is a nice example of how a port can set some filesystem policies. [1] http://blog.cr0.org/2012/09/introducing-chromes-next-generation.html [2] http://www.insanitybit.com/2012/08/14/chrome-seccomp-bpf-sandbox/ [3] Tools/TestWebKitAPI/Tests/WebKit2/SeccompFilters.cpp
Created attachment 181667 [details] Patch Call for comments, not be landed or officially reviewed.
Comment on attachment 181667 [details] Patch Attachment 181667 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/15755131
Comment on attachment 181667 [details] Patch Attachment 181667 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/15741999
Created attachment 182790 [details] Patch Updated patch: add more syscalls and tests.
Created attachment 184566 [details] Patch
Comment on attachment 184566 [details] Patch This patch is now ready for review. The main change from the latest version is the addition of file policies and support for Qt. Whitepaper with a high level overview of this implementation: https://docs.google.com/document/d/1Fe2ZSEazdqzxBWHDjGF8WuYwsI6-C95Ljmn-QiMyl94/edit Incremental patch set on my development branch: http://www.tmpsantos.com.br/cgi-bin/gitweb.cgi?p=webkit.git;a=shortlog;h=refs/heads/seccomp_filters
Comment on attachment 184566 [details] Patch Attachment 184566 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/16110264
Comment on attachment 184566 [details] Patch Attachment 184566 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/16072970
Comment on attachment 184566 [details] Patch Attachment 184566 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/16112158
Comment on attachment 184566 [details] Patch Attachment 184566 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/16119210 New failing tests: inspector-protocol/debugger-terminate-dedicated-worker-while-paused.html
Comment on attachment 184566 [details] Patch Attachment 184566 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/16112413
Comment on attachment 184566 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=184566&action=review In general IMHO seccomp filters are the way to go. I have a few smaller comments below. > Source/WTF/wtf/Assertions.cpp:352 > +#if !ENABLE(SECCOMP_FILTERS) Then ChangeLog explains the existance of this #ifdef, but I think it would be also very useful to have as a comment in the code here. > Source/WebKit2/WebProcess/efl/SeccompFiltersEfl.cpp:52 > +void SeccompFiltersEfl::platformInitialize() A lot of the things in SeccompFiltersEfl is duplicated in SeccompFiltersQt. Why not share more settings? It might also be worth documenting where certain policies originate from, i.e. /run/shm (it is /dev/shm on my machine?) from shared memory for example. Alternatively would it make sense if the policies didn't come from .cpp files but from external files that are perhaps parsed at build time, to allow for easier sharing of settings between ports _and_ allow for better granularity with regards to the enabling/disabling of features, such as GStreamer? > Source/WebKit2/WebProcess/efl/SeccompFiltersEfl.cpp:71 > + ADD_POLICY(directoryPolicy, String::fromUTF8(efreet_data_home_get()) + "/WebKitEfl", ReadAndWrite); > + ADD_POLICY(directoryPolicy, String::fromUTF8(efreet_cache_home_get()) + "/WebKitEfl", ReadAndWrite); Is this something that should come from the application layer perhaps? A setting on the API level? > Source/WebKit2/WebProcess/efl/SeccompFiltersEfl.cpp:106 > + // is not right. We need to check with these directories on gstreamer > + // can be configured. Should this be #ifdef'ed via USE(GSTREAMER) then?
Created attachment 186868 [details] Patch
Comment on attachment 186868 [details] Patch Major changes from the previous patch: - Addressed issues pointed by Simon (thanks for reviewing). - Directories paths for databases, cache, storage, etc are now coming from the UIProcess. - Most of the directories and policies are now shared between EFL and Qt. - Added more tests: threaded stress test that executes 15k syscalls (hooked into policies) spread in 5 threads. - Ready to be enabled on the EFL and Qt bots if necessary. No apparent regressions on my tests.
Comment on attachment 186868 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=186868&action=review > Source/WTF/ChangeLog:8 > + Do no setup a handler for SIGSYS when Seccomp Filters are enabled. do no? > Source/WebCore/ChangeLog:6 > + Make the DATA_DIR global since it is now need on WebCore and WebKit2. needed* on -> for > Source/WebKit2/ChangeLog:25 > + The broker process should be initialized as early as possible on the > + sandboxed process main, because it only does a fork(), which is cheap main process ? > Source/WebKit2/ChangeLog:31 > + Currently the broker supports only directory policies, but file policies > + could be implemented in the future to make narrower the filesystem > + exposed to the WebProcess. only supports* > Source/WebKit2/Shared/linux/SeccompFilters/OpenSyscall.cpp:157 > + // things like the entry does not exist. like if the ... > Source/WebKit2/Shared/linux/SeccompFilters/OpenSyscall.cpp:180 > +bool OpenSyscall::decode(CoreIPC::ArgumentDecoder* decoder) > +{ > + if (!decoder->decode(m_path)) > + return false; > + if (!decoder->decode(m_flags)) > + return false; > + > + return decoder->decode(m_mode); > +} you dont need to decode type()? > Source/WebKit2/Shared/linux/SeccompFilters/OpenSyscall.h:53 > + // Syscall implementation. > + virtual void setResult(const SyscallResult*); Any reason to not call it SystemCall ? > Source/WebKit2/Shared/linux/SeccompFilters/SeccompBroker.cpp:67 > + int m_ncpus; actual cpus or cpu threads? > Source/WebKit2/Shared/linux/SeccompFilters/SeccompBroker.cpp:72 > +static ssize_t sendMessage(int socket, void* data, size_t size, int fd=-1) space around = > Source/WebKit2/Shared/linux/SeccompFilters/SeccompBroker.cpp:155 > +static void registerSysSignalHandler() I would call it SystemSignalHandler... > Source/WebKit2/Shared/linux/SeccompFilters/SeccompBroker.cpp:238 > + // Malloc will sometimes check the number of CPUs present on the system will sometime* > Source/WebKit2/Shared/linux/SeccompFilters/SeccompBroker.cpp:247 > +bool SeccompBrokerClient::handleIfTryingToBlockSysSignal(ucontext_t* ucontext) const handleAttempt...? > Source/WebKit2/Shared/linux/SeccompFilters/SyscallPolicy.cpp:205 > + // Mime type resolution. MIME
Comment on attachment 186868 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=186868&action=review > Source/WebKit2/WebProcess/qt/SeccompFiltersWebProcessQt.cpp:56 > + const char* path = getenv("QTDIR"); > + if (path) { > + QFileInfo qtDir(QString::fromUtf8(path)); > + ADD_POLICY(&m_policy, Directory, qtDir.canonicalFilePath(), ReadAndWrite); > + } QTDIR is dead :) You could use QLibraryInfo::location(QLibraryInfo::PrefixPath) instead, but for distros that's going to be /usr, which sort of defeats the purpose :) So I think parts of the paths of QLibraryInfo would make more sense to add, i.e. ArchData, LibrariesPath, LibrariesExecutablesPath, etc.
Comment on attachment 186868 [details] Patch From a high-level perspective this looks good to me. I like how there's very little duplication between Efl and Qt. I think the default paths that remain can be fine-tuned in the future.
Created attachment 189320 [details] Patch
(In reply to comment #49) > Created an attachment (id=189320) [details] > Patch Andersca, Kling: The patch is ready for review. It is fully functional for EFL and Qt, has unit tests for all the features and documentation [1]. The implementation for GTK is ongoing on bug 110014 and being coordinated with the GTK developers. I will propose enabling it by default on the EFL bots on bug 110365 as soon as it lands. Thanks a lot Kenneth and Simon for pre-reviewing. [1] https://docs.google.com/document/d/1Fe2ZSEazdqzxBWHDjGF8WuYwsI6-C95Ljmn-QiMyl94/edit?usp=sharing
(In reply to comment #50) > (In reply to comment #49) > > Created an attachment (id=189320) [details] [details] > > Patch > > Andersca, Kling: The patch is ready for review. It is fully functional for EFL and Qt, has unit tests for all the features and documentation [1]. > > The implementation for GTK is ongoing on bug 110014 and being coordinated with the GTK developers. I will propose enabling it by default on the EFL bots on bug 110365 as soon as it lands. > > Thanks a lot Kenneth and Simon for pre-reviewing. > > [1] https://docs.google.com/document/d/1Fe2ZSEazdqzxBWHDjGF8WuYwsI6-C95Ljmn-QiMyl94/edit?usp=sharing Any chances of getting this patch reviewed this week?
andersca, could you have a look at this one? It has backing from EFL, Qt and GTK+.
Created attachment 194031 [details] Patch Refactoring. Made it easier to read by removing things like macros.
Some WK2 owner could please review this patch?
May a WebKit2 Owner please review this patch?
(In reply to comment #55) > May a WebKit2 Owner please review this patch? Thiago - I haven't followed this, is there a pre-review from someone who understands the Linux details? I'm not an expert on the low-level Linux APIs being used here.
(In reply to comment #56) > (In reply to comment #55) > > May a WebKit2 Owner please review this patch? > > Thiago - I haven't followed this, is there a pre-review from someone who understands the Linux details? I'm not an expert on the low-level Linux APIs being used here. Qt is represented here by Simon, which did a pre-review: https://bugs.webkit.org/show_bug.cgi?id=89875#c48 EFL by Kenneth that also pre-reviewed: https://bugs.webkit.org/show_bug.cgi?id=89875#c52 And GTK folks are following it in a different bug: https://bugs.webkit.org/show_bug.cgi?id=110014#c5
(In reply to comment #57) > (In reply to comment #56) > > (In reply to comment #55) > > > May a WebKit2 Owner please review this patch? > > > > Thiago - I haven't followed this, is there a pre-review from someone who understands the Linux details? I'm not an expert on the low-level Linux APIs being used here. I thought the idea of the owners thing was not that the owner reviewing the patch understands everything, but just checks the patch looks sane in general, relying on port reviewers for the real review. Otherwise it's impossible to land patches like this one. On the other hand I understand it's not easy for owners to know if the patch has been approved by a port reviewers, since we can't set r+ flag. Maybe we can try to find ways to make this easier. For example, allow port reviewers to set r+, but settings also cq-, then the owner can set cq+, or remove the cq flag or simply comment whether he approves the patch or not. We can also add a new bugzilla keyword, that is only added to bugs that has already been reviewed but are just waiting for an owner to approve it to make it easier to find patches blocked by webkit2 owners. > Qt is represented here by Simon, which did a pre-review: > https://bugs.webkit.org/show_bug.cgi?id=89875#c48 > > EFL by Kenneth that also pre-reviewed: > https://bugs.webkit.org/show_bug.cgi?id=89875#c52 > > And GTK folks are following it in a different bug: > https://bugs.webkit.org/show_bug.cgi?id=110014#c5
(In reply to comment #58) > (In reply to comment #57) > > (In reply to comment #56) > > > (In reply to comment #55) > > > > May a WebKit2 Owner please review this patch? > > > > > > Thiago - I haven't followed this, is there a pre-review from someone who understands the Linux details? I'm not an expert on the low-level Linux APIs being used here. > > I thought the idea of the owners thing was not that the owner reviewing the patch understands everything, but just checks the patch looks sane in general, relying on port reviewers for the real review. Yes, that's why I asked for pointers to the pre-reviews, which were graciously provided. I'll do my best to do a quick sanity check today. I'm actually really excited to see WebProcess sandboxing come to Linux. > Otherwise it's impossible to land patches like this one. On the other hand I understand it's not easy for owners to know if the patch has been approved by a port reviewers, since we can't set r+ flag. Maybe we can try to find ways to make this easier. For example, allow port reviewers to set r+, but settings also cq-, then the owner can set cq+, or remove the cq flag or simply comment whether he approves the patch or not. We can also add a new bugzilla keyword, that is only added to bugs that has already been reviewed but are just waiting for an owner to approve it to make it easier to find patches blocked by webkit2 owners. I think something like that (or a new bugzilla flag) for module owner sign-off makes a lot of sense.
Comment on attachment 194031 [details] Patch This looks fine to me, given the pre-reviews from various ports and the lack of effect on cross-platform code.
Comment on attachment 194031 [details] Patch Rejecting attachment 194031 [details] from commit-queue. Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-commit-queue.appspot.com', '--bot-id=webkit-cq-02', 'validate-changelog', '--non-interactive', 194031, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit /Volumes/Data/EWS/WebKit/Source/WebCore/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive). Full output: http://webkit-commit-queue.appspot.com/results/17599093
Committed r147998: <http://trac.webkit.org/changeset/147998>