Bug 110019

Summary: REGRESSION(r143076): Crash when calling removeNamedItem or removeNamedItemNS with a non-existent attribute of newly created element
Product: WebKit Reporter: Peter Nelson <peter>
Component: DOMAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: bfulgham, cmarcelo, kling, ojan.autocc, peter, webkit.review.bot
Priority: P1 Keywords: Regression
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
HTML document (based on the Acid3 test) that demonstrates the issue
none
Patch kling: review+, kling: commit-queue-

Peter Nelson
Reported 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.
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-
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Peter Nelson
Comment 1 2013-02-16 10:45:57 PST
Created attachment 188720 [details] Patch
Brent Fulgham
Comment 2 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?
Peter Nelson
Comment 3 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.
Andreas Kling
Comment 4 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.
Andreas Kling
Comment 5 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!
Brent Fulgham
Comment 6 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?
Andreas Kling
Comment 7 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.
Andreas Kling
Comment 8 2013-02-16 15:49:06 PST
Committed r143115: <http://trac.webkit.org/changeset/143115>
Note You need to log in before you can comment on or make changes to this bug.