Summary: | REGRESSION: Can't add item to cart at lnt.com (JS type error) | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Maciej Stachowiak <mjs> | ||||||||||
Component: | DOM | Assignee: | Antti Koivisto <koivisto> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | ap, bdakin, ian, KwhiteRight | ||||||||||
Priority: | P1 | Keywords: | HasReduction, InRadar, Regression | ||||||||||
Version: | 420+ | ||||||||||||
Hardware: | Mac | ||||||||||||
OS: | OS X 10.4 | ||||||||||||
Attachments: |
|
Description
Maciej Stachowiak
2007-02-04 11:03:26 PST
(In reply to comment #0) > i was working on this, but moved onto other bugs. I will get back to this > later but i'm attaching a reduction of the problem. I would be nice to have the reduction in Bugzilla, if possible! Created attachment 13270 [details]
the reduction
Attached the mentioned reduction.
Created attachment 13361 [details]
another test
Firefox even includes removed form elements in the collection!
Created attachment 13572 [details]
patch with change log and test case
And now, so do we!
Some comments I noticed before deciding to question the approach: 1) I'd like to see more tests for cases that should or should not close the canvas. 2) The correct spelling is "well-formed", not "wellformed" or "well formed". And, finally, <canvas> should not skip parsing its contents - it should just prevent them from rendering only. That appears to be what Firefox does. One special case to consider is a <script> inside <canvas>, Firefox appears to execute it: <canvas> <b>fallback</b> <script> alert('x'); </script> </canvas> Comment on attachment 13572 [details]
patch with change log and test case
r-
I put my comments and r- in the wrong bug. Comment on attachment 13572 [details]
patch with change log and test case
I believe Adele was reviewing this when Maciej accidentally marked it review-.
I started reviewing, but I don't understand this result: + shouldBe('form.original', 'a'); + shouldBe('form.elements.original', 'a'); + + debug(''); + debug("now change the form item a's name to second"); + debug(''); + + a.name="second"; + + shouldBe('form.original', 'a'); + shouldBe('form.elements.original', 'undefined'); Why does changing a.name make it inaccessible under its old name in the elements collection? Does the test explanation apply only to named access in the form, with a different behavior for named access in the elements collection? Comment on attachment 13572 [details]
patch with change log and test case
Adele actually r+'d this - reinstating that now. I think Darin's test case is indeed meant to illustrate the difference between named access and the elements collection.
(In reply to comment #9) > Why does changing a.name make it inaccessible under its old name in the > elements collection? Because that's what Gecko does. > Does the test explanation apply only to named access in > the form, with a different behavior for named access in the elements > collection? Yes, that's right. You can see that in the code change too. Committed revision 20148. Re-opening this bug. I had to back it out with r20214 because it causes a memory trasher. Steps to reproduce trasher forthcoming. Comment on attachment 13572 [details]
patch with change log and test case
r-minusing the patch so as not to cause confusion.
Here are some ways to reproduce the memory trasher if anyone wants to: Way One: Reload http://www.signonsandiego.com/news/riverside/ several times. Sometime you only have to reload once, but other times you have to reload many times. Way Two: 1. Go to dominos.com 2. Click "Order now" 3. Enter an address, and click "Continue" (This part is tricky, some addresses don't provide the pizza store that repros the bug. One that does is 218 Broadway Revere, MA 02151) 4. On the Delivery or Pick up page, click "Next" 5. Under "Build your own pizza" select "Choose size and crust" 6. Design a pizza and add it. It will come up with an error saying that you have no selected a size or crust type because there is another bug with radio buttons on this page, which I am about to file. 7. Keep trying to add the pizza until it crashes. Way Three: 1. Go to http://hotjobs.yahoo.com/ 2. Type "San jose, CA" in City & State/ Zip field. Click the Search button. 3. After search results appears, click the Search button. 4. With the Search results still displayed, click the search button again. Crash. This last one actually hits and assertion on Debug builds. Created attachment 13675 [details]
new patch
I ended up rewriting the patch since I observed the behavior of Darin's patch did not quite match Firefox. Firefox seems to remember all names that have been used to access an element. Names that have not been used are forgotten. I expanded the test case to cover this and some other behaviors.
I did not include the large and somewhat risky looking refactoring from Darin's patch to get rid of DOMNamedNodesCollection. The issues it fixes (at least that you can't use item() on DOMNamedNodesCollections) are not directly related to this bug, are not regressions and should be handled separately.
No memory thrashing observed with this patch on those pages.
Comment on attachment 13675 [details]
new patch
m_autocomplete(true)
!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!
I should've checked in a layout test for that...
Eh. I just copied those lines from Darin patch. Is m_autocomplete supposed to be true or false here? its supposed to be true. Darin's patch accidently changed it to false...then I changed it back. Comment on attachment 13675 [details]
new patch
As long as you switch the m_autocomplete line, r=me!
r20260, with m_autocomplete(true)! The HTML5 spec now defines this. A couple of cases where Safari differs from the spec (and from other some of theo ther UAs): when an element is removed from the DOM, it is removed from the persistence map (Safari and Firefox keep persisting it); when two elements have the same name, and the name is accessed, nothing is persisted (IE persists the collection, Safari persists the first element). |