Bug 48985 – [Qt][WK2] Left over files and shared memory segments

• Component: WebKit2
• Status: RESOLVED FIXED

[Balazs Kelemen]

On a crash we left over socket files from the beginning of our wk2 port.
Since the usage of QSharedMemory (http://trac.webkit.org/changeset/71118) the situation is even worst.
We do not delete those even on normal termination because their owner (SharedMemory) is not deleted.
This is because of the policy in WebKit to avoid destructors on exit. There is at least one shared memory
segment that we left over, namely the one that is used by VisitedLinkTable. Unix system does not clean up
those segments after the process dies or terminates. (Yesterday we were needed to shutdown our wk2 bot because
the amount of left over shared memory semaphores has reached the kernel limit). I would like to find a solution to
clean up those things on terminate and on crash as well.

[Balazs Kelemen]

Test case (on Linux):
1. launch MiniBrowser and close it
2. ipcs - in the output you can see the left over segments and semaphores:
------ Shared Memory Segments --------
key        shmid      owner      perms      bytes      nattch     status
<other process's staff>
0x510038a6 822312969  balazs    666        4096       0 <- from VisitedLinkTable
------ Semaphore Arrays --------
key        semid      owner      perms      nsems
0x510038a5 268795904  balazs    666        1

To clear them, use these commands:
ipcrm -s <semid>
ipcrm -m <shmid>

[Balazs Kelemen]

Created attachment 72928 [details]
proposed patch

I am using signal.h on every platform. I do not really know what are the differences on Windows and Symbian
but I tried a tiny example on Windows with MinGW and it worked. After a quick googling I found that it should
work on Symbian as well. I think the definition of HAVE_SIGNAL_H in Platform.h is wrong.

[WebKit Review Bot]

Attachment 72928 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--diff-files', u'WebKit2/ChangeLog', u'WebKit2/Platform/qt/SharedMemoryQt.cpp', u'WebKit2/Shared/qt/CrashHandler.cpp', u'WebKit2/Shared/qt/CrashHandler.h', u'WebKit2/UIProcess/Launcher/qt/ProcessLauncherQt.cpp', u'WebKit2/WebKit2.pro']" exit_code: 1
WebKit2/Shared/qt/CrashHandler.cpp:53:  Missing space before ( in foreach(  [whitespace/parens] [5]
Total errors found: 1 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Andreas Kling]

Comment on attachment 72928 [details]
proposed patch

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

Looks good in general, some problems though:

> WebKit2/Shared/qt/CrashHandler.cpp:38
> +CrashHandler::CrashHandler()
> +{

Missing initialization of m_inDeleteObjects!

> WebKit2/Shared/qt/CrashHandler.cpp:41
> +    signal(SIGABRT, &CrashHandler::signalHandler);
> +    signal(SIGSEGV, &CrashHandler::signalHandler);
> +    signal(SIGINT, &CrashHandler::signalHandler);

You probably want to catch SIGFPE, SIGILL, SIGQUIT, SIGTRAP and SIGBUS as well here.

> WebKit2/Shared/qt/CrashHandler.cpp:54
> +    foreach(QObject* object, m_objects)
> +        delete object;

qDeleteAll(m_objects);

> WebKit2/Shared/qt/CrashHandler.h:48
> +    void addToDeletion(QObject* object)

addToDeletion() sounds weird to me. deleteOnCrash() perhaps? markForDeletionOnCrash()?

[Balazs Kelemen]

Created attachment 72940 [details]
Patch

Changed regarding to Kling's comments

[Andreas Kling]

Comment on attachment 72940 [details]
Patch

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

r=me, three small things:

> WebKit2/Shared/qt/CrashHandler.cpp:30
> +#include <QCoreApplication>

Is this include needed?

> WebKit2/Shared/qt/CrashHandler.h:34
> +#include <QCoreApplication>

Is this include needed?

> WebKit2/Shared/qt/CrashHandler.h:74
> +#endif // CleanupHandler_h

#endif // CrashHandler_h

> WebKit2/UIProcess/Launcher/qt/ProcessLauncherQt.cpp:119
> +    // Do not leave socket files on the disk even on crash!
> +    CrashHandler::instance()->markForDeletionOnCrash(this);

This call is missing a corresponding didDelete() in ~ProcessLauncherHelper()

[Balazs Kelemen]

(In reply to comment #6)
> (From update of attachment 72940 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=72940&action=review
> 
> r=me, three small things:
> 
> > WebKit2/Shared/qt/CrashHandler.cpp:30
> > +#include <QCoreApplication>
> 
> Is this include needed?
No
> 
> > WebKit2/Shared/qt/CrashHandler.h:34
> > +#include <QCoreApplication>
> 
> Is this include needed?
No :)
> 
> > WebKit2/Shared/qt/CrashHandler.h:74
> > +#endif // CleanupHandler_h
> 
> #endif // CrashHandler_h
> 
> > WebKit2/UIProcess/Launcher/qt/ProcessLauncherQt.cpp:119
> > +    // Do not leave socket files on the disk even on crash!
> > +    CrashHandler::instance()->markForDeletionOnCrash(this);
> 
> This call is missing a corresponding didDelete() in ~ProcessLauncherHelper()
I omit that because ProcessLauncherHelper lives as long as the process, however
we can CRASH even after ~ProcessLauncherHelper() is called so you are right.

I will fix the issues (and do final a smoke test) before landing.

[Balazs Kelemen]

Committed in http://trac.webkit.org/changeset/71409

[Balazs Kelemen]

Comment on attachment 72940 [details]
Patch

Clearing flags