Summary: | Safari fail to access a second time an element whose content was dynamically modified. | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Oliver Christen <oliver.christen> | ||||||||
Component: | DOM | Assignee: | 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
Oliver Christen
2005-06-23 05:47:32 PDT
Bug confirmed, it does work in firefox. 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.
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.
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.
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.
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.
(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. Created attachment 2923 [details]
Improve test case
Here's a new patch with an improved test case.
Comment on attachment 2923 [details]
Improve test case
r=me since Darin already reviewed and now there is a test case.
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? Anders is a commiter, so I'll leave this one for him to commit. This was committed to ToT a long time ago; closing. but the bug still exist on safari Build 312.3.1, is that really normal ? Yes; AFAIK it's up to Apple to merge the changes back to previous versions of WebKit who should i bug for that ? |