Bug 22001 – AtomicStringImpl* keys of event listener maps can outlive their strings

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Alexey Proskuryakov]

Turns out that we actually have bugs caused by not ref'ing HashMap and HashSet keys, see attachment.

We should audit other uses of StringImpl* and AtomicStringImpl*, too.

[Alexey Proskuryakov]

Created attachment 24799 [details]
test case

[Simon Fraser (smfr)]

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?

[Darin Adler]

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.

[Alexey Proskuryakov]

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.

[Alexey Proskuryakov]

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.

[Alexey Proskuryakov]

Created attachment 24807 [details]
proposed fix

[Darin Adler]

Comment on attachment 24807 [details]
proposed fix

r=me

[Alexey Proskuryakov]

Committed r38064.