Bug 48520 - [Qt][WK2] SharedMemory is broken
Summary: [Qt][WK2] SharedMemory is broken
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
: 48619 (view as bug list)
Depends on:
Blocks:
 
Reported: 2010-10-28 06:49 PDT by Balazs Kelemen
Modified: 2010-11-02 09:45 PDT (History)
3 users (show)

See Also:


Attachments
proposed patch (2.05 KB, patch)
2010-10-28 06:58 PDT, Balazs Kelemen
no flags Details | Formatted Diff | Diff
qsharedmemory based implementation (6.80 KB, patch)
2010-11-02 02:38 PDT, Balazs Kelemen
no flags Details | Formatted Diff | Diff
Patch (6.90 KB, patch)
2010-11-02 06:55 PDT, Balazs Kelemen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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