Bug 22001 - AtomicStringImpl* keys of event listener maps can outlive their strings
Summary: AtomicStringImpl* keys of event listener maps can outlive their strings
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac OS X 10.5
: P2 Normal
Assignee: Alexey Proskuryakov
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2008-10-31 08:22 PDT by Alexey Proskuryakov
Modified: 2008-11-01 00:26 PDT (History)
1 user (show)

See Also:


Attachments
test case (481 bytes, text/html)
2008-10-31 08:23 PDT, Alexey Proskuryakov
no flags Details
proposed fix (17.18 KB, patch)
2008-10-31 12:25 PDT, Alexey Proskuryakov
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alexey Proskuryakov 2008-10-31 08:22:06 PDT
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.
Comment 1 Alexey Proskuryakov 2008-10-31 08:23:39 PDT
Created attachment 24799 [details]
test case
Comment 2 Simon Fraser (smfr) 2008-10-31 09:04:13 PDT
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?
Comment 3 Darin Adler 2008-10-31 10:07:22 PDT
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.
Comment 4 Alexey Proskuryakov 2008-10-31 11:25:03 PDT
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.

Comment 5 Alexey Proskuryakov 2008-10-31 11:39:19 PDT
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.
Comment 6 Alexey Proskuryakov 2008-10-31 12:25:49 PDT
Created attachment 24807 [details]
proposed fix
Comment 7 Darin Adler 2008-10-31 12:37:54 PDT
Comment on attachment 24807 [details]
proposed fix

r=me
Comment 8 Alexey Proskuryakov 2008-11-01 00:26:08 PDT
Committed r38064.