Bug 120293 – setAttributeNode() does not set the new value to an existing attribute if specified attribute is in a different case.

• Component: DOM
• Status: RESOLVED FIXED

[Arpita Bahuguna]

Created attachment 209629 [details]
setAttributeNode.html

For the attached testcase (setAttributeNode.html), a green colored div block should be visible instead of a red one.

Note that obtaining the attribute's value using getAttributeNode() shall still return the correct value i.e., the new one.
However, the new value is not being correctly set internally to the existing node.

[Arpita Bahuguna]

Created attachment 209642 [details]
Patch

[Andreas Kling]

Comment on attachment 209642 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=209642&action=review

When you set a new Attr node with the same name in different case, is the name of the existing attribute supposed to be updated as well?
Or should the original name always stay, unless the attribute is removed and re-added?

> Source/WebCore/dom/Element.cpp:954
> +    QualifiedName attrNodeName = attributeAt(index).name();

This variable should be a const QualifiedName&, to avoid unnecessary ref()/deref().
"attrNodeName" is not a good name, as it there isn't necessarily an Attr node present (yet.)
I would call this something like "existingAttributeName" or "nameOfExistingAttribute"

[Andreas Kling]

The ChangeLog should mention that this matches the behavior of FF and IE.

[Darin Adler]

Comment on attachment 209642 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=209642&action=review

Fix looks fine, but need to remove the excess argument for setAttributeInternal.

>> Source/WebCore/dom/Element.cpp:954
>> +    QualifiedName attrNodeName = attributeAt(index).name();
> 
> This variable should be a const QualifiedName&, to avoid unnecessary ref()/deref().
> "attrNodeName" is not a good name, as it there isn't necessarily an Attr node present (yet.)
> I would call this something like "existingAttributeName" or "nameOfExistingAttribute"

The right name for this is “name”. And the “name” argument to setAttributeInternal must be removed, because it is not used at all. That’s the right way to do it. No point in passing an unused argument that would just introduce a bug if we used it for anything.

[Arpita Bahuguna]

Created attachment 209722 [details]
Patch

[Arpita Bahuguna]

(In reply to comment #2)
> (From update of attachment 209642 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=209642&action=review
> 
Many thanks for the review Kling!

> When you set a new Attr node with the same name in different case, is the name of the existing attribute supposed to be updated as well?
> Or should the original name always stay, unless the attribute is removed and re-added?
> 
As per our discussion on IRC today, I'll work on making our Attribute's and Attr's behavior similar.

> > Source/WebCore/dom/Element.cpp:954
> > +    QualifiedName attrNodeName = attributeAt(index).name();
> 
> This variable should be a const QualifiedName&, to avoid unnecessary ref()/deref().
> "attrNodeName" is not a good name, as it there isn't necessarily an Attr node present (yet.)
> I would call this something like "existingAttributeName" or "nameOfExistingAttribute"
Have made the required changes.

[Arpita Bahuguna]

(In reply to comment #4)
> (From update of attachment 209642 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=209642&action=review
> 
> Fix looks fine, but need to remove the excess argument for setAttributeInternal.
> 
> >> Source/WebCore/dom/Element.cpp:954
> >> +    QualifiedName attrNodeName = attributeAt(index).name();
> > 
> > This variable should be a const QualifiedName&, to avoid unnecessary ref()/deref().
> > "attrNodeName" is not a good name, as it there isn't necessarily an Attr node present (yet.)
> > I would call this something like "existingAttributeName" or "nameOfExistingAttribute"
> 
> The right name for this is “name”. And the “name” argument to setAttributeInternal must be removed, because it is not used at all. That’s the right way to do it. No point in passing an unused argument that would just introduce a bug if we used it for anything.

Thanks for the review Darin but "name" is required for passing on to addAttributeInternal() called from setAttributeInternal() in case the index equals 'attributeNotFound'.

[Darin Adler]

Comment on attachment 209722 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=209722&action=review

> Source/WebCore/dom/Element.cpp:972
> +    const QualifiedName& existingAttributeName = attributeAt(index).name();

Seems a bit wasteful to fetch this if inSynchronizationOfLazyAttribute is true and newValue is no different from the old value.

[Arpita Bahuguna]

Created attachment 209968 [details]
Patch

[Arpita Bahuguna]

(In reply to comment #8)
> (From update of attachment 209722 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=209722&action=review
> 
> > Source/WebCore/dom/Element.cpp:972
> > +    const QualifiedName& existingAttributeName = attributeAt(index).name();
> 
> Seems a bit wasteful to fetch this if inSynchronizationOfLazyAttribute is true and newValue is no different from the old value.

Thanks for the r+ Darin. Have submitted another patch which takes care of the above case but perhaps makes the code look a bit messy. (??)
Would appreciate your comments on the same.

[Darin Adler]

Comment on attachment 209968 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=209968&action=review

> Source/WebCore/dom/Element.cpp:973
> +    const QualifiedName& attributeName = (!inSynchronizationOfLazyAttribute && valueChanged) ? attributeAt(index).name() : name;

Needs to be ||, not &&.

[Arpita Bahuguna]

Created attachment 210072 [details]
Patch

[Arpita Bahuguna]

(In reply to comment #11)
> (From update of attachment 209968 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=209968&action=review
> 
> > Source/WebCore/dom/Element.cpp:973
> > +    const QualifiedName& attributeName = (!inSynchronizationOfLazyAttribute && valueChanged) ? attributeAt(index).name() : name;
> 
> Needs to be ||, not &&.

Sorry about that Darin. :( uploaded another patch.

[Build Bot]

Comment on attachment 210072 [details]
Patch

Attachment 210072 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/1613008

New failing tests:
fast/workers/termination-with-port-messages.html

[Build Bot]

Created attachment 210073 [details]
Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-15  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.4

[Arpita Bahuguna]

I don't think the failing test: fast/workers/termination-with-port-messages.html is related to this patch.

[WebKit Commit Bot]

Comment on attachment 210072 [details]
Patch

Clearing flags on attachment: 210072

Committed r154881: <http://trac.webkit.org/changeset/154881>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Andreas Kling]

This caused bug 120293.

[Ryosuke Niwa]

(In reply to comment #19)
> This caused bug 120293.

bug 120293 is THIS bug.

[Andreas Kling]

(In reply to comment #20)
> (In reply to comment #19)
> > This caused bug 120293.
> 
> bug 120293 is THIS bug.

Oops! I meant bug 120577.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 120643

[Arpita Bahuguna]

Created attachment 210435 [details]
Patch

[Arpita Bahuguna]

Have uploaded another patch for handling the crash. This uses a local QualifiedName instead of const QualifiedName& in the hope that even if the corresponding attribute object dies, its name shall persist through the lifetime of the setAttributeInternal() function.

I assume that the object dies coz of GC getting triggered at some point, however I have not been able to simulate this crash even by making testcases that forcefully trigger a gc.

The callstacks show that the crash occurs at the first instance of trying to use the attributeName. Another crash callstack:
Crash Type: Heap-use-after-free READ 8
Crash Address: 0x6040000ae630
Crash State:
  - crash stack -
  WebCore::HTMLMediaElement::parseAttribute
  WebCore::Element::attributeChanged
  - free stack -
  WebCore::Element::setAttributeInternal
  WebCore::DOMTokenList::addInternal

another one:
0   com.apple.WebCore                 0x000000010d19995d WebCore::HTMLElement::eventNameForAttributeName(WebCore::QualifiedName const&) const + 29
1   com.apple.WebCore                 0x000000010d19ce60 WebCore::HTMLElement::parseAttribute(WebCore::QualifiedName const&, WTF::AtomicString const&) + 288
2   com.apple.WebCore                 0x000000010d08dada WebCore::Element::attributeChanged(WebCore::QualifiedName const&, WTF::AtomicString const&, WebCore::Element::AttributeModificationReason) + 42
3   com.apple.WebCore                 0x000000010d0946b4 WebCore::Element::setAttributeInternal(unsigned int, WebCore::QualifiedName const&, WTF::AtomicString const&, WebCore::Element::SynchronizationOfLazyAttribute) + 500
4   com.apple.WebCore                 0x000000010d08d9e4 WebCore::Element::setAttribute(WTF::AtomicString const&, WTF::AtomicString const&, int&) + 260

So the crash occurs at the first instance of trying to use the attributeName since that is no longer valid. (?) So even if we use a local copy instead of a reference, attributeName would still be invalid. (??)

My previous approach for fixing this was to call ensureUniqueElementData().attributeAt(index).name() instead of attributeAt(index).name() so that even if the attributes had been freed, ensureUniqueElementData() shall re-create them.

[Arpita Bahuguna]

Reopening the issue since the associated patch was reverted due to crash https://bugs.webkit.org/show_bug.cgi?id=120577

[Darin Adler]

Comment on attachment 210435 [details]
Patch

r=me assuming the test failure is not re-introduced

[Alexey Proskuryakov]

Shouldn't the new patch have a regression test for that crash?

[Arpita Bahuguna]

(In reply to comment #26)
> (From update of attachment 210435 [details])
> r=me assuming the test failure is not re-introduced

Thanks for the review Darin. I'll land the patch and hopefully there shouldn't be any more regressions.

[Arpita Bahuguna]

(In reply to comment #27)
> Shouldn't the new patch have a regression test for that crash?

Hi Alexy, yes, ideally it should. I did try by making cases that would force a gc but could not reproduce the crash for any scenario.

[WebKit Commit Bot]

Comment on attachment 210435 [details]
Patch

Clearing flags on attachment: 210435

Committed r155093: <http://trac.webkit.org/changeset/155093>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.