Summary: | Leak: TextCodecICU::registerCodecs is leaking | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Brian Holt <brian.holt> | ||||||
Component: | Text | Assignee: | Alexey Proskuryakov <ap> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | ap, cdumez, commit-queue, darin, ews-watchlist, glenn, joepeck, menard, mrobinson, psolanki, simon.fraser | ||||||
Priority: | P2 | Keywords: | InRadar | ||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
See Also: |
https://bugs.webkit.org/show_bug.cgi?id=154737 https://bugs.webkit.org/show_bug.cgi?id=160931 |
||||||||
Bug Depends on: | |||||||||
Bug Blocks: | 116317, 106716 | ||||||||
Attachments: |
|
Description
Brian Holt
2013-07-09 07:03:52 PDT
Created attachment 206319 [details]
Patch
Comment on attachment 206319 [details]
Patch
I think that the leak may be real, but the fix is not right.
Generally, these strings are permanently stored in textCodecMap, and not leaked. One case where a leak may occur is when the same webStandardName is registered multiple times - when this happens, map value is thrown away.
However, I don't think that it's correct to remove fastStrDup. The result of ucnv_getCanonicalName is not documented to remain useful indefinitely - for example, they may be reusing a static buffer. So, we need our own copy of the string.
(In reply to comment #2) > (From update of attachment 206319 [details]) > I think that the leak may be real, but the fix is not right. > > Generally, these strings are permanently stored in textCodecMap, and not leaked. One case where a leak may occur is when the same webStandardName is registered multiple times - when this happens, map value is thrown away. > > However, I don't think that it's correct to remove fastStrDup. The result of ucnv_getCanonicalName is not documented to remain useful indefinitely - for example, they may be reusing a static buffer. So, we need our own copy of the string. Thanks for your comments. It seems that there might be some inconsistency in the way that the memory is handled. If we need our own copy of the string, should we also make a copy of canonicalConverterName on line 193? And if we're making copies then should the responsibility fall to TextCodecMap to free the additional data? But the additionalData is const implying (at least to me) that the TextCodecMap doesn't own that data. What would you suggest would be the best way to address the leak? > If we need our own copy of the string, should we also make a copy of canonicalConverterName on line 193? Indeed! > And if we're making copies then should the responsibility fall to TextCodecMap to free the additional data? But the additionalData is const implying (at least to me) that the TextCodecMap doesn't own that data. Generally speaking, constness is separate from ownership, it's perfectly fine to free a const pointer. The tricky part here is that additional data is not necessarily a string pointer - TextCodecMap passes a pointer to a number. So the function to delete additional data needs to be provided by caller. > What would you suggest would be the best way to address the leak? I think that we shouldn't enumerate ICU aliases at all, and have our own encoding name table that's suitable for the web instead (similarly to what TextCodecMac does for legacy Mac encodings). That would be a valuable architectural improvement, and it would sidestep this issue entirely. Given the complexity of fixing only this leak, we should consider just implementing the above idea. *** Bug 136973 has been marked as a duplicate of this bug. *** *** Bug 143975 has been marked as a duplicate of this bug. *** It would be nice to fix this leak. But keep in mind that we leaked intentionally when originally creating this code, and it’s a little tricky to design this out. I like Alexey’s suggestion above. This was fixed in r204605, just need to remove the special case from leaks parser. Created attachment 327782 [details]
stop silencing the leak
Comment on attachment 327782 [details]
stop silencing the leak
r=me
Comment on attachment 327782 [details] stop silencing the leak Clearing flags on attachment: 327782 Committed r225240: <https://trac.webkit.org/changeset/225240> All reviewed patches have been landed. Closing bug. |