Summary: | AtomicStringImpl* keys of event listener maps can outlive their strings | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Alexey Proskuryakov <ap> | ||||||
Component: | WebCore Misc. | Assignee: | Alexey Proskuryakov <ap> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | simon.fraser | ||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | Mac | ||||||||
OS: | OS X 10.5 | ||||||||
Attachments: |
|
Description
Alexey Proskuryakov
2008-10-31 08:22:06 PDT
Created attachment 24799 [details]
test case
It would be good if the testcase printed a PASS or FAIL; it's hard to see what it's doing. Have you identified the HashMap/HashSet in question? I'm not sure I agree with the title of this bug. Sure, it'd be easier to program with a map and set that did a ref/deref on its keys. But there are at least two other solutions to problems at any individual call site: 1) Add code to remove the string when it no longer should be in the collection. That code can be run by the owner of the string. 2) Add explicit ref/deref. These are both possibly tricky to get right, and may be inferior solutions to the problem. But I generally would prefer not to name bugs after a particular solution to a problem. I was (and still am) going to tackle this myself, so the test is indeed somewhat sub-standard, and the bug title indeed implies the solution I plan to employ. This particular case is XMLHttpRequest::m_eventListeners, but similar code exists elsewhere. On the other hand, the promise to audit all the uses of AtomicStringImpl* as a key was a bit presumptuous, as many of these are in code I'm not closely familiar with. So I'm going to fix just the EventListenersMap bug here. Created attachment 24807 [details]
proposed fix
Comment on attachment 24807 [details]
proposed fix
r=me
|