Bug 45984 – [Qt] QtWebProcess should clean up shared memory map files on close

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Andras Becsi]

UpdateChunk uses mapped shared memory files, these files should be placed into the temporary directory and removed on QtWebProcess close. Implement a singleton pool class to track shared memory and clean up files from disk if RunLoop stops.

[Andras Becsi]

Created attachment 67929 [details]
proposed patch

[WebKit Review Bot]

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

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebKit2/Shared/qt/MappedMemory.h:32:  Alphabetical sorting problem.  [build/include_order] [4]
WebKit2/Shared/qt/MappedMemory.cpp:27:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 2 in 7 files


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

[Kenneth Rohde Christiansen]

Comment on attachment 67929 [details]
proposed patch

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

This seems to include code written by a.o. Antti. Why isn't he credited?

> WebKit2/Shared/qt/UpdateChunk.cpp:4
> + * Copyright (C) 2010 Andras Becsi ([e-mail]), University of Szeged

Are we now going to get copyrights statements for everyone working at WebKit at University of Szeged?

[Andras Becsi]

Created attachment 67931 [details]
proposed patch

[WebKit Review Bot]

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

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebKit2/Shared/qt/MappedMemory.cpp:27:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 1 in 7 files


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

[Andras Becsi]

(In reply to comment #3)
> (From update of attachment 67929 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=67929&action=prettypatch
> 
> This seems to include code written by a.o. Antti. Why isn't he credited?

MappedMemory.h does include code written by Antti, I suppose, so I left the copyright text unchanged, but MappedMemory.cpp shouldn't.
If I'm wrong, please point me to the snippet which includes code from Antti.

> 
> > WebKit2/Shared/qt/UpdateChunk.cpp:4
> > + * Copyright (C) 2010 Andras Becsi ([e-mail]), University of Szeged
> 
> Are we now going to get copyrights statements for everyone working at WebKit at University of Szeged?

I think this should be a decision of each individual developer. I usually use this template, if it is not appropriate I'll change it.

[Antonio Gomes]

> > > WebKit2/Shared/qt/UpdateChunk.cpp:4
> > > + * Copyright (C) 2010 Andras Becsi ([e-mail]), University of Szeged
> > 
> > Are we now going to get copyrights statements for everyone working at WebKit at University of Szeged?
> 
> I think this should be a decision of each individual developer. I usually use this template, if it is not appropriate I'll change it.

Personally, I would keep Szeged Copyright and add a "Contributor(s)"  field with people names.

But in the end, it is up to you, I think. You get to see that is reasonable.

[Kenneth Rohde Christiansen]

> Personally, I would keep Szeged Copyright and add a "Contributor(s)"  field with people names.
> 
> But in the end, it is up to you, I think. You get to see that is reasonable.

We kind of have ChangeLogs and git blame for these kinds of things.

[Andras Becsi]

Created attachment 68004 [details]
proposed patch

Changed, to get the focus back on the code.

[WebKit Review Bot]

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

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebKit2/Shared/qt/MappedMemory.cpp:27:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 1 in 7 files


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

[Kenneth Rohde Christiansen]

Comment on attachment 68004 [details]
proposed patch

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

> WebKit2/ChangeLog:4
> +

Would be nice with a comment as "Based on code by Antti Koivisto" or what ever.

[Andras Becsi]

(In reply to comment #11)
> (From update of attachment 68004 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=68004&action=prettypatch
> 
> > WebKit2/ChangeLog:4
> > +
> 
> Would be nice with a comment as "Based on code by Antti Koivisto" or what ever.

I do not fully understand why you think this is needed.
I only relocated the MappedMemory struct code (which was previously in UpdateChunk.cpp, and checked in by Antti) to MappedMemory.h. That is why I left the copyright as it was in UpdateChunk.cpp. This is only needed because I store MappedMemory in my MappedMemoryPool class which is the implementation I mention in the ChangeLog. I do not see the "based on" relation here, but maybe I am wrong.

[Andras Becsi]

Created attachment 68005 [details]
proposed patch

Update ChangeLog text to avoid confusion.

[WebKit Review Bot]

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

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebKit2/Shared/qt/MappedMemory.cpp:27:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 1 in 7 files


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

[Kenneth Rohde Christiansen]

(In reply to comment #12)
> (In reply to comment #11)
> > (From update of attachment 68004 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=68004&action=prettypatch
> > 
> > > WebKit2/ChangeLog:4
> > > +
> > 
> > Would be nice with a comment as "Based on code by Antti Koivisto" or what ever.
> 
> I do not fully understand why you think this is needed.
> I only relocated the MappedMemory struct code (which was previously in UpdateChunk.cpp, and checked in by Antti) to MappedMemory.h. That is why I left the copyright as it was in UpdateChunk.cpp. This is only needed because I store MappedMemory in my MappedMemoryPool class which is the implementation I mention in the ChangeLog. I do not see the "based on" relation here, but maybe I am wrong.

Ah OK, that wasn't obvious from the diff - I should probably have looked better.

[Kenneth Rohde Christiansen]

Comment on attachment 68005 [details]
proposed patch

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

> WebKit2/ChangeLog:14
> +        * Shared/qt/MappedMemory.cpp: Added.

It seems that you didn't do it correctly, as this was a moved/renamed file. Not a newly added one.

You need to use git mv or the svn equivalent. r- because of this.

[Andras Becsi]

(In reply to comment #16)
> (From update of attachment 68005 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=68005&action=prettypatch
> 
> > WebKit2/ChangeLog:14
> > +        * Shared/qt/MappedMemory.cpp: Added.
> 
> It seems that you didn't do it correctly, as this was a moved/renamed file. Not a newly added one.
> 
> You need to use git mv or the svn equivalent. r- because of this.

No, it was not. MappedMemory.cpp is a new file, which implements the MappedMemoryPool class. The moved code snippet is the MappedMemory struct which is in MappedMemory.h and was previously in UpdateChunk.cpp (and forward declared in UpdateChunk.h), there is no moved file in my patch.
But I should probably rename MappedMemory.cpp to MappedMemoryPool.cpp, since this is what it implements, and this might be the root of confusion.

[Andras Becsi]

Created attachment 68006 [details]
proposed patch

Renamed MappedMemory.cpp to MappedMemoryPool.cpp and made the ChangeLog more informative.

[WebKit Review Bot]

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

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebKit2/Shared/qt/MappedMemoryPool.cpp:27:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 1 in 7 files


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

[Balazs Kelemen]

(In reply to comment #18)
> Created an attachment (id=68006) [details]
> proposed patch
> 
> Renamed MappedMemory.cpp to MappedMemoryPool.cpp and made the ChangeLog more informative.

Generally looks good to me. The main problem I see is calling cleanUp in RunLoop::stop.

> WebKit2/Platform/qt/RunLoopQt.cpp:69
> +    WebKit::MappedMemoryPool::instance()->cleanUp();

This does not belongs to here. For assuring that cleanUp will be called on exit I would use the QCoreApplication::aboutToQuit signal. MappedMemoryPool could be a QObject with the cleanUp slot and you could connect that to the signal in the ctor. Could you take a chance on that?

> WebKit2/Shared/qt/MappedMemory.h:47
> +    Vector<MappedMemory>* pool();

I think wrapper methods for accessing the pool instead of public access would be somewhat more encapsulated.

> WebKit2/Shared/qt/MappedMemoryPool.cpp:47
> +        MappedMemory* chunk = &m_pool.at(i);

I would use a reference here.

(Sorry for the irregular way of review but I have still no bug edit permission currently.)

[Andras Becsi]

Created attachment 68099 [details]
proposed patch v2

Uploading new patch which takes Balazs's comment into consideration.

Thanks Balazs for the reasonable suggestions.

[WebKit Review Bot]

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

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebKit2/Shared/qt/MappedMemoryPool.cpp:27:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
WebKit2/Shared/qt/MappedMemoryPool.cpp:33:  Missing space after ,  [whitespace/comma] [3]
Total errors found: 2 in 6 files


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

[Andras Becsi]

Created attachment 68109 [details]
proposed patch v2

Corrected the weird style issue.

[WebKit Review Bot]

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

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebKit2/Shared/qt/MappedMemoryPool.cpp:27:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 1 in 6 files


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

[Csaba Osztrogonác]

Comment on attachment 68109 [details]
proposed patch v2

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

> WebKit2/Shared/qt/MappedMemoryPool.cpp:29
> +#include "MappedMemory.h"
> +
> +#include "StdLibExtras.h"

#include "config.h" should be the first include,
please add it before landing. Otherwise LGTM, r=me.

[Csaba Osztrogonác]

Comment on attachment 68109 [details]
proposed patch v2

We shouldn't include config.h in WebKit2 code,
I realized it is a bug in check-webkit-style script:
https://bugs.webkit.org/show_bug.cgi?id=46032

Style is correct, so cq+

[WebKit Commit Bot]

Comment on attachment 68109 [details]
proposed patch v2

Clearing flags on attachment: 68109

Committed r67945: <http://trac.webkit.org/changeset/67945>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Commit Bot]

Comment on attachment 68109 [details]
proposed patch v2

Rejecting patch 68109 from commit-queue.

Failed to run "[u'/Users/abarth/git/webkit-queue/WebKitTools/Scripts/svn-apply', u'--force']" exit_code: 1
Last 500 characters of output:
#1 FAILED at 1.
Hunk #2 FAILED at 32.
Hunk #3 FAILED at 55.
Hunk #4 FAILED at 81.
4 out of 4 hunks FAILED -- saving rejects to file WebKit2/Shared/qt/UpdateChunk.cpp.rej
patching file WebKit2/Shared/qt/UpdateChunk.h
Hunk #1 FAILED at 27.
Hunk #2 FAILED at 38.
2 out of 2 hunks FAILED -- saving rejects to file WebKit2/Shared/qt/UpdateChunk.h.rej
patching file WebKit2/WebKit2.pro
Hunk #1 FAILED at 162.
Hunk #2 FAILED at 294.
2 out of 2 hunks FAILED -- saving rejects to file WebKit2/WebKit2.pro.rej

Full output: http://queues.webkit.org/results/4020089

[Csaba Osztrogonác]

Eric, have you got any idea what happened?
CQ landed the patch and after that it can't apply again.

[Andras Becsi]

Comment on attachment 68109 [details]
proposed patch v2

Clearing flags.
Something strange happened with the commit-bot. The change was already landed by the commit-queue before.

[Antti Koivisto]

If the app crashes the map files will still be left hanging around forever, so this doesn't really solve the problem. Maybe we could have a directory namespace for our map files and just wipe it out completely?

[Andras Becsi]

(In reply to comment #32)
> If the app crashes the map files will still be left hanging around forever, so this doesn't really solve the problem. Maybe we could have a directory namespace for our map files and just wipe it out completely?

How would you wipe out the dir if the app crashes. As I see it you still would have to implement a crash handler, or something similar.
On crash the files are left in /tmp/ now which is a tmpfs or is whiped out on reboot on Linux (at least by distributions I now).

[Antti Koivisto]

It would probably be sufficient to wipe out the leftover files next time the app is used. That stops unlimited accumulation of files. 

Kimmo suggested it might be a good idea to actually unlink the files right away after they have been mmapd in the client. Then there would be no need for explicit cleanup on exit.

[Andras Becsi]

(In reply to comment #34)
> It would probably be sufficient to wipe out the leftover files next time the app is used. That stops unlimited accumulation of files. 
> 
> Kimmo suggested it might be a good idea to actually unlink the files right away after they have been mmapd in the client. Then there would be no need for explicit cleanup on exit.

If we clean up leftover files at startup we need a way to find files, which are currently unused, there could be files of another QtWebProcess which are currently used.
I would rather prefer a concept Kimmo suggested then we do not need complex magic like this.

[Eric Seidel (no email)]

The commit-queue recently moved from one machine to a cluster, and we had locking problems for a bit (wehre multiple machines would attempt to land the same patch).  Hopefully those are all solved.

[Balazs Kelemen]

Hey guys, stop flooding a closed bug, go on with this stuff in https://bugs.webkit.org/show_bug.cgi?id=46252 ;) . This had been created
for continuing the debate.