Bug 48520

Summary: [Qt][WK2] SharedMemory is broken
Product: WebKit Reporter: Balazs Kelemen <kbalazs>
Component: PlatformAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, eric, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: All   
Attachments:
Description Flags
proposed patch
none
qsharedmemory based implementation
none
Patch none

Description Balazs Kelemen 2010-10-28 06:49:58 PDT
We have implemented SharedMemory with the MappedMemoryPool that was designed to UpdateChunk.
We should not allow the pool to recycle the mapped chunk until it is used at the receiving side.
Comment 1 Balazs Kelemen 2010-10-28 06:58:12 PDT
Created attachment 72185 [details]
proposed patch
Comment 2 Andreas Kling 2010-10-28 07:46:35 PDT
Comment on attachment 72185 [details]
proposed patch

r=me
Comment 3 Balazs Kelemen 2010-10-28 07:52:17 PDT
Comment on attachment 72185 [details]
proposed patch

Clearing flags on attachment: 72185

Committed r70776: <http://trac.webkit.org/changeset/70776>
Comment 4 Balazs Kelemen 2010-10-28 07:52:25 PDT
All reviewed patches have been landed.  Closing bug.
Comment 5 Balazs Kelemen 2010-11-01 14:28:50 PDT
My assumtion was wrong. m_data is used after we have passed the handle at least there:
VisitedLinkProvider.cpp:122
-------
RefPtr<SharedMemory> currentTableMemory = m_table.sharedMemory();
...
const LinkHash* currentLinkHashes = static_cast<const LinkHash*>(currentTableMemory->data());
for (unsigned i = 0; i < currentTableSize; ++i) {
    LinkHash linkHash = currentLinkHashes[i];
-------

This caused crashes on our wk2 bot: http://webkit.sed.hu/buildbot/waterfall?show=x86-32%20Linux%20Qt%20Release%20WebKit2
To correctly fix it we need am additional member in SharedMemory (a bool for example).
Comment 6 Balazs Kelemen 2010-11-01 15:07:32 PDT
After running some tests I have realized that a bool is not enough
(tests had been timed out instead of crashing with the fix).
The ownership of the shared memory is shared across processes so
we need something that acts as a handle. Ideally we should use
QSharedMemory but as antti noted it has some problems on MAC:
http://bugreports.qt.nokia.com/browse/QTBUG-9446.
Comment 7 Balazs Kelemen 2010-11-02 02:06:55 PDT
*** Bug 48619 has been marked as a duplicate of this bug. ***
Comment 8 Balazs Kelemen 2010-11-02 02:38:19 PDT
Created attachment 72641 [details]
qsharedmemory based implementation

Fix almost all crashes in run-webkit-tests -2
Comment 9 Balazs Kelemen 2010-11-02 06:55:11 PDT
Created attachment 72659 [details]
Patch

Doing key generation more safely by the use of UUuid.
Comment 10 Andreas Kling 2010-11-02 07:06:29 PDT
Comment on attachment 72659 [details]
Patch

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

This is beautiful, r=me!

As discussed on IRC, if the QUuid generation turns out to be too expensive, we can always replace it by something more light-weight later on.

> WebKit2/Platform/qt/SharedMemoryQt.cpp:134
> +    // m_impl must be not null and it must points to a valid QSharedMemory object.

s/not null/non-null/
s/points/point/
Comment 11 Balazs Kelemen 2010-11-02 08:49:11 PDT
Committed in http://trac.webkit.org/changeset/71118.
Comment 12 WebKit Review Bot 2010-11-02 09:45:52 PDT
http://trac.webkit.org/changeset/71118 might have broken GTK Linux 64-bit Debug
The following tests are not passing:
fast/text/international/plane2.html