[Webkit-unassigned] [Bug 51984] New: [WK2][Qt] Multiple problems with MemoryMappedPool

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=51984

           Summary: [WK2][Qt] Multiple problems with MemoryMappedPool
           Product: WebKit
           Version: 528+ (Nightly build)
          Platform: PC
        OS/Version: Linux
            Status: NEW
          Severity: Normal
          Priority: P2
         Component: WebKit2
        AssignedTo: webkit-unassigned at lists.webkit.org
        ReportedBy: kimmo.t.kinnunen at nokia.com
                CC: kbalazs at webkit.org, kenneth at webkit.org


There's multiple problems with MemoryMappedPool currently

1) Cleanup depends on UI process crash handlers
P1)  Unreliable, makes quitting ui process slow

2) Race condition in sharing the buffers with variable isFree
P2) The implementation is not sound for concurrency

3) Pool grows forever
P3) At somepoint address space runs out

4) Implementation does not check syscall failures
P4) Causes segfaults due to failed mmap etc

--

Here's how I'd see these fixed

1) Pass file descriptors to open but deleted files via IPC.

Use this protocol to create the mmapped files:
-- process 0
1) fd = open temp file
2) resize(fd)
3) mmap(fd)
4) delete(fd)
5) send fd via local domain socket or fifo
-- process 1
1) handle ipc messaeg
2) fd = decode fd
3) mmap(fd)
4) use fd.

This needs the ConnectionQt.cpp to be modified to use unix socket api or FIFO api to pass all the data. The Qt abstraction of QLocalServer and QLocalSocket cannot express sending FDs. However, I don't see this as a bad thing -- it should just remove unnecessary stuff.


2) Race condition in sharing the buffers with variable isFree

To have concurrently valid implementation, the access to the page variables should be controlled with cross-process semaphore. The isFree implements just a corner-case optimization for avoiding to call mmap and most likely this is not optimization at all. Thus usage should be removed.

After sending a memory area to other process, contract should be that the memory region cannot be used at all.

One scheme to achieve what isFree is doing, see below:

have two variables:

QList<MappedMemory*> m_usableRegions

QList<MappedMemory*> m_sentRegions

usableRegions gets appended when a process receives a memory region, uses it and then marks it disposed. These are the pages that can be sent back to the other process.

sentRegions is the list that optimizes mmap: it stores list of regions that have been mapped to this process memory at some point.


3) Pool grows forever

Process should just prune m_sentRegions to NUM_SLOTS

If mmap is a performance problem, the semaphore needs to be used


4) Implementation does not check syscall failures

The implementation should check status of file opens, mmap operations etc.


I would see much of the code move to use posix apis due to requirement of passing fds.

To pre-empt some comments about using posix apis:

For linux, symbian and macos x, this should just work. This is what qt is doing behind the scenes anyway.

For windows, the memory and ipc should most likely use Windows implementation and we should contribute bug fixes there.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.