Bug 3677

Summary: Safari fail to access a second time an element whose content was dynamically modified.
Product: WebKit Reporter: Oliver Christen <oliver.christen>
Component: DOMAssignee: Anders Carlsson <andersca>
Status: RESOLVED FIXED    
Severity: Normal    
Priority: P3    
Version: 412   
Hardware: Mac   
OS: OS X 10.3   
URL: http://www.fafnirden.com/tmp/safari_bug/test.html
Attachments:
Description Flags
Improve handling of duplicate ids
darin: review-
Address comments
darin: review-
Improve test case mjs: review+

Oliver Christen
Reported 2005-06-23 05:47:32 PDT
When one modify the content of an element dynamically using either DOM functions or innerHTML property (via javascript), the element content is acceded correctly the first time the script run. But on the second run, Safari fail to access the (previously) modified content and report it as being Null. Meaning its not possible to modify again the same element's content. see url provided for details of the script and live exemple. The problem occure on Safari 1.2 and 1.3 (not tested on 2.0, as i dont have it). Script work fine with: [win] Mozilla/Firefox/Opera/IE6 [linux] Firefox/Konkeror/Epiphany [mac] Firefox/NS7
Attachments
Improve handling of duplicate ids (6.33 KB, patch)
2005-06-23 17:25 PDT, Anders Carlsson
darin: review-
Address comments (5.78 KB, patch)
2005-06-24 01:29 PDT, Anders Carlsson
darin: review-
Improve test case (6.39 KB, patch)
2005-07-12 05:24 PDT, Anders Carlsson
mjs: review+
Joost de Valk (AlthA)
Comment 1 2005-06-23 09:52:34 PDT
Bug confirmed, it does work in firefox.
Anders Carlsson
Comment 2 2005-06-23 17:25:42 PDT
Created attachment 2610 [details] Improve handling of duplicate ids Turns out that the problem is because the document contains two elements with the same id for a short while. When one element is removed, the id table is not being updated. Here's a patch that fixes it. I'm a little concerned with always inserting a count value of 1 in the id count table, but that shouldn't be too much overhead.
Darin Adler
Comment 3 2005-06-23 22:48:24 PDT
Comment on attachment 2610 [details] Improve handling of duplicate ids The usual way to handle something where there's a cache is to make the member mutable rather than making the method non-const. I suggest putting mutable in from of the m_idCount declaration, and leaving the methods const. In DocumentImpl::getElementById, you should use qId when calling find on m_elementsById. The code that checks the ID of an element should call hasID before calling getAttribute(ATTR_ID); it's more efficient that way since most elements don't have IDs. I think it's a little strange to use a QDict<int>, but actually store the integers as pointers. Maybe QDict<void> would be a better way to declare it for now. And later we should change it to an actual integer dictionary. This: + int idCount; + if ((idCount = (int)m_idCount.find(qId))) { would be better written like this: if (int idCount = (int)m_idCount.find(qId)) { But instead, I think the code should assert that an idCount is found. If it's not, we have an internal inconsistency error and I'd like to drop into the debugger. It's probably best to put the remove code in an "if (idCount <= 1)" and then assert that the count is 1 in that branch. Or something similar and equivalent. Glad you used the strategy I suggested to fix the bug; I think this is simple and will be nice and efficient. One further idea about how to make m_idCount smaller that would make the code only slightly more complex: The m_idCount dictionary could hold counts that do not include the one in m_elementsById. Thus we'd only bump the count by 1 if there was already an element in m_elementsById, and we'd only decrement if it's an element other than the one in m_elementsById being removed. Also, when an element was found and put into m_elementsById, we would decrement the count. The benefit here is that the m_idCount table would then be empty almost all the time; we could even have the member variable be a pointer and allocate it only when needed. Documents without duplicate IDs would have less overhead. Just an idea -- the existing code is fine too.
Anders Carlsson
Comment 4 2005-06-24 01:29:17 PDT
Created attachment 2616 [details] Address comments I implemented your idea of making the count table smaller. It's not possible to use a void dict since the QDict destructor (optionally) deletes the pointer, and you can't delete a void pointer. Maciej told me he used a char QDict for a similar integer-in-pointer hack so I did the same.
Darin Adler
Comment 5 2005-06-24 08:29:55 PDT
Comment on attachment 2616 [details] Address comments The code in DocumentImpl::getElementById says "(char *)idCount - 1" in a place where it should say "(char *)(idCount - 1)". That will work, but only because pointer arithmetic and integer arithmetic happen to be the same in this case. I'm going to mark this r=me, although I think we should make that change. I wondered at first whether m_accessKeyDictValid needed to be set in the getElementById function. Then I realized that I don't understand the code setting m_accessKeyDictValid at all. Why on earth does the presence or absence of an ID have anything to do with the validity of the access key dictionary? Clearly this patch makes things no worse in that respect, but I'm pretty sure this points to a bug in the access key support.
Darin Adler
Comment 6 2005-06-29 22:14:32 PDT
Comment on attachment 2616 [details] Address comments I just realized that this bug does not have a suitable layout test. The bug fix is great, but I'm going to have to mark this "review-" until we have a layout test. I though the original test.html could be used as a layout test, but I was mistaken -- it's too complicated, and requires user input.
Anders Carlsson
Comment 7 2005-06-30 00:35:55 PDT
(In reply to comment #6) > (From update of attachment 2616 [details] [edit]) > I just realized that this bug does not have a suitable layout test. The bug fix > is great, but I'm going to have to mark this "review-" until we have a layout > test. I though the original test.html could be used as a layout test, but I was > mistaken -- it's too complicated, and requires user input. > + layout-tests/fast/dom/ids/duplicate-ids-expected.txt: Added. + layout-tests/fast/dom/ids/duplicate-ids.html: Added. + layout-tests/fast/dom/ids/switch-children-expected.txt: Added. + layout-tests/fast/dom/ids/switch-children.html: Added. These are in the patch. I guess they can be improved to specify what's being tested and the expected outcome.
Anders Carlsson
Comment 8 2005-07-12 05:24:26 PDT
Created attachment 2923 [details] Improve test case Here's a new patch with an improved test case.
Maciej Stachowiak
Comment 9 2005-07-13 20:06:01 PDT
Comment on attachment 2923 [details] Improve test case r=me since Darin already reviewed and now there is a test case.
Oliver Christen
Comment 10 2005-11-14 02:40:30 PST
it seems the bug still exist on safari Build 312.3.1, is that normal ? Will the bug be fixed in a futur released version?
Eric Seidel (no email)
Comment 11 2005-11-29 02:11:53 PST
Anders is a commiter, so I'll leave this one for him to commit.
Anders Carlsson
Comment 12 2005-11-30 10:27:23 PST
This was committed to ToT a long time ago; closing.
Oliver Christen
Comment 13 2005-11-30 23:11:26 PST
but the bug still exist on safari Build 312.3.1, is that really normal ?
Anders Carlsson
Comment 14 2005-12-01 00:36:12 PST
Yes; AFAIK it's up to Apple to merge the changes back to previous versions of WebKit
Oliver Christen
Comment 15 2005-12-01 00:48:31 PST
who should i bug for that ?
Note You need to log in before you can comment on or make changes to this bug.