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
View All Add an attachment (proposed patch, testcase, etc.)

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>