Bug 110019 - REGRESSION(r143076): Crash when calling removeNamedItem or removeNamedItemNS with a non-existent attribute of newly created element
Summary: REGRESSION(r143076): Crash when calling removeNamedItem or removeNamedItemNS ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P1 Normal
Assignee: Nobody
URL:
Keywords: Regression
Depends on:
Blocks:
 
Reported: 2013-02-16 08:52 PST by Peter Nelson
Modified: 2013-02-16 15:49 PST (History)
6 users (show)

See Also:


Attachments
HTML document (based on the Acid3 test) that demonstrates the issue (771 bytes, text/plain)
2013-02-16 08:52 PST, Peter Nelson
no flags Details
Patch (1.98 KB, patch)
2013-02-16 10:45 PST, Peter Nelson
kling: review+
kling: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Peter Nelson 2013-02-16 08:52:07 PST
Created attachment 188713 [details]
HTML document (based on the Acid3 test) that demonstrates the issue

Currently WebCore crashes when either removeNamedItem or removeNamedItemNS are called with a non-existent attribute of a newly created element.

This bug causes a crash in test 67 of Acid3.

Attached is a small HTML document (based on the Acid3 test) that demonstrates the issue.

Tested with latest nightly on both Windows and OSX.
Comment 1 Peter Nelson 2013-02-16 10:45:57 PST
Created attachment 188720 [details]
Patch
Comment 2 Brent Fulgham 2013-02-16 11:44:33 PST
Comment on attachment 188720 [details]
Patch

This seems like a reasonable check. I'm surprised it wasn't needed previously. Is it possible that some external logic is supposed to be preventing a call into this routine if the mode has no attributed?
Comment 3 Peter Nelson 2013-02-16 11:55:12 PST
(In reply to comment #2)
It may have been changeset 143076 (https://trac.webkit.org/changeset/143076) that broke it -- ElementData is no longer implicitly created when .attributes is referenced.
Comment 4 Andreas Kling 2013-02-16 12:46:37 PST
Comment on attachment 188720 [details]
Patch

Good catch! Looks like our NamedNodeMap test coverage isn't what it should be. :/
Let's improve that by adding a layout test with this patch.
Comment 5 Andreas Kling 2013-02-16 13:36:25 PST
Comment on attachment 188720 [details]
Patch

I didn't realize we already have a copy of ACID3 in-tree that catches these bugs.
r=me as we don't need a new test and this looks good. Thanks!
Comment 6 Brent Fulgham 2013-02-16 15:42:48 PST
(In reply to comment #5)
> (From update of attachment 188720 [details])
> I didn't realize we already have a copy of ACID3 in-tree that catches these bugs.
> r=me as we don't need a new test and this looks good. Thanks!

If we have a test in the tree that covers this logic, why didn't we see it before Peter did? Could we indicate which test covers this behavior in the ChangeLog?
Comment 7 Andreas Kling 2013-02-16 15:45:37 PST
(In reply to comment #6)
> (In reply to comment #5)
> > (From update of attachment 188720 [details] [details])
> > I didn't realize we already have a copy of ACID3 in-tree that catches these bugs.
> > r=me as we don't need a new test and this looks good. Thanks!
> 
> If we have a test in the tree that covers this logic, why didn't we see it before Peter did? Could we indicate which test covers this behavior in the ChangeLog?

http/tests/misc/acid3.html is crashing on the bots ATM. I'll land this patch and add a comment to the ChangeLog about it.
Comment 8 Andreas Kling 2013-02-16 15:49:06 PST
Committed r143115: <http://trac.webkit.org/changeset/143115>