Opened 10 years ago
Closed 9 years ago
#12005 closed bug (fixed)
DataTranslations: crashes when the selected translator is uninstalled.
Reported by: | Janus | Owned by: | axeld |
---|---|---|---|
Priority: | normal | Milestone: | Unscheduled |
Component: | Preferences/DataTranslations | Version: | R1/Development |
Keywords: | Cc: | ||
Blocked By: | Blocking: | ||
Platform: | All |
Description
This problem is systematic. Segmentation violation on RemoveChild() view detach().
Attachments (3)
Change History (9)
by , 9 years ago
Attachment: | 0001-Remove-unnecessary-whitespace-in-AppDefs.h.patch added |
---|
comment:1 by , 9 years ago
patch: | 0 → 1 |
---|
by , 9 years ago
Attachment: | 0002-Make-sure-images-containing-BTranslators-are-not-unl.patch added |
---|
Patch (part 2)
comment:2 by , 9 years ago
(From the commit message for the second patch)
When a translator is uninstalled, BTranslatorPrivate::_RemoveTranslators is called. This method used to unload the image containing the translator after calling Release() on it resulting in several problems:
- If the translator was still busy, e.g. translating something while being installed, it crashed since the image was unloaded even though its refcount was larger than 0.
- Applications using code from one of the translators (e.g. its config view) would crash when the translator is uninstalled (this is bug #12005).
This problem is now fixed. The roster keeps track of all translators whose image it manages (even if the translator was already removed from the roster). It also keeps a refcount to all images. When a translator's refcount drops to zero and it belonged to a roster at some point, it does not delete itself, but notifies the roster that it is ready to destruct, which then removes it from the roster if the translator is still in it, destroys the translator, decrements the refcount of the image and if the new refcount is zero, unloads the image. All of this is done in a message handler, since if the translator called TranslatorDeleted like before, the unloaded image would be referenced when the stack is walked up.
Finally, the DataTranslations preflet is required to Acquire() the translator whose config view it is showing, because otherwise its refcount could be reduced to 0 and the image unloaded. BTranslatorRoster now enables users to acquire a translator by ID. By the time the translator has to be released, it might not be part of the roster anymore though. Since BTranslatorRoster tries not to give out raw pointers to the translators it manages, users who acquire a translator through a roster now are given a BTranslatorReleaseDelegate, which allows for releasing the BTranslator exactly once and then self-destructs.
comment:3 by , 9 years ago
The patch looks mostly good apart from some coding style issues, although I would consider the BTranslatorReleaseDelegate
a temporary kludge: the only reason you would want to acquire a reference to a translator is because you are using its view. If that's the case, the view should be burdened with the reference counting, not the application making use of it. As we discussed, this could be achieved by using a wrapping reference counting view that the BTranslatorRoster
returns.
Finally, the coding style issues I spotted (just the usual nitpicking):
- We have two blank lines between different sections or functions. For example, in TranslatorRoster.h you introduced three places where there is now only one blank line instead (there are other places where you accidentally removed a blank line, or have not inserted two in the source files, too).
- Names like
del
orit
(if it's not an iterator) should be avoided. In these cases, names likedeleteRequest
,deleteMessage
(fordel
), andtranslator
(forit
) would be better. - Operators almost always go to the next line (see TranslatorRoster.cpp line 324).
- Return type has an extra line (line 1175).
by , 9 years ago
Attachment: | 0002-Make-sure-images-containing-BTranslators-are-not-unl.2.patch added |
---|
Replacement for the other patch 0002
comment:4 by , 9 years ago
I have attached a replacement for the second patch which hopefully addresses all of the style issues. If there is anything left, feel free to point it out.
comment:5 by , 9 years ago
Status: | new → in-progress |
---|
comment:6 by , 9 years ago
Resolution: | → fixed |
---|---|
Status: | in-progress → closed |
Took a bit longer than anticipated, but I've applied it in hrev49837, thanks!
Patch (part 1)