Bug 3677 – Safari fail to access a second time an element whose content was dynamically modified.

• Component: DOM
• Status: RESOLVED FIXED

[Oliver Christen]

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

[Joost de Valk (AlthA)]

Bug confirmed, it does work in firefox.

[Anders Carlsson]

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 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]

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 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 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]

(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]

Created attachment 2923 [details]
Improve test case

Here's a new patch with an improved test case.

[Maciej Stachowiak]

Comment on attachment 2923 [details]
Improve test case

r=me since Darin already reviewed and now there is a test case.

[Oliver Christen]

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)]

Anders is a commiter, so I'll leave this one for him to commit.

[Anders Carlsson]

This was committed to ToT a long time ago; closing.

[Oliver Christen]

but the bug still exist on safari Build 312.3.1, is that really normal ?

[Anders Carlsson]

Yes; AFAIK it's up to Apple to merge the changes back to previous versions of WebKit

[Oliver Christen]

who should i bug for that ?