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.
Created attachment 209642 [details] Patch
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"
The ChangeLog should mention that this matches the behavior of FF and IE.
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.
Created attachment 209722 [details] Patch
(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.
(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'.
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.
Created attachment 209968 [details] Patch
(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.
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 &&.
Created attachment 210072 [details] Patch
(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.
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
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
I don't think the failing test: fast/workers/termination-with-port-messages.html is related to this patch.
Comment on attachment 210072 [details] Patch Clearing flags on attachment: 210072 Committed r154881: <http://trac.webkit.org/changeset/154881>
All reviewed patches have been landed. Closing bug.
This caused bug 120293.
(In reply to comment #19) > This caused bug 120293. bug 120293 is THIS bug.
(In reply to comment #20) > (In reply to comment #19) > > This caused bug 120293. > > bug 120293 is THIS bug. Oops! I meant bug 120577.
Re-opened since this is blocked by bug 120643
Created attachment 210435 [details] Patch
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.
Reopening the issue since the associated patch was reverted due to crash https://bugs.webkit.org/show_bug.cgi?id=120577
Comment on attachment 210435 [details] Patch r=me assuming the test failure is not re-introduced
Shouldn't the new patch have a regression test for that crash?
(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.
(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.
Comment on attachment 210435 [details] Patch Clearing flags on attachment: 210435 Committed r155093: <http://trac.webkit.org/changeset/155093>