[Webkit-unassigned] [Bug 51474] [Qt][WK2] Incomplete clean up on termination

Mon Jan 3 11:40:43 PST 2011

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

--- Comment #15 from Kimmo Kinnunen <kimmo.t.kinnunen at nokia.com>  2011-01-03 11:40:43 PST ---
(In reply to comment #14)
> Created an attachment (id=77816)
 --> (https://bugs.webkit.org/attachment.cgi?id=77816&action=review) [details]
> patch v4
> 
> Returned to QCoreApplication::aboutToQuit. It had became reliable after I stopped ignoring thread affinity.
> No crash handling, just termination. The patch contains the renames and the functional changes in one, but git diff -M is hopefully
> making it easier to review (it just shows the differing lines).

If I understand correctly, you moved the file deletion to UI process? I don't think that solves any existing problem correctly?

If I do killall -9 MiniBrowser, won't that fail?

How about 'killall -9 MiniBrowser QtWebProcess' ?

I don't think you can rely on crash handlers. You cannot rely on any explicit cleanup.  And you should not have to.

(In reply to comment #11)
> (In reply to comment #8)
> But we should assure that those destructors will be executed. By terminating the web process with SIGKILL that's not the case.
> Furthermore, SHM segments must be released on both side unless the system won't free it up (that is the case on Unix).

Sounds like that SHM API is not very robust nor up to modern standards. Maybe there's something better that actually functions correctly?

> Of course crash handling is nasty. The main purpose of doing that was to save the bots from being full of stale files and shm segments in
> the case when trunk is in a crashing state for a while. This is not that important however since a production release should be stable enough
> to not suffering from this. However, at least on normal termination those staling stuff should be cleaned.

This rationale is actually completely inverse of what is important.

On your buildbots, you can surely fix this by scripts:
'sudo find /tmp -iname '*QtWebKit*' -delete'

On "production" browser there can exist no possibility that stale files would prevent startup.

> > 4) One of the main features of WebKit is that it is fast to kill the process. Now you are again introducing code that will swap in random pages of memory during exit.
> That's true but how could we do any cleaning without that? I know this is a policy but I do not think that it is a main feature :)

I already explained:
1. open file
2. communicate file to the other process
3. open file in other process
3. delete file in other process

There's variations of this scheme, most of them a bit more robust than above simple strategy. I think all of them are more robust than deleting the files explicitly in some exit condition.

While the patch4 might solve current bugs, i don't think it's any solution to the problem of never leaving files behind. Whether or not that should block review, that's not for me to decide.

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