Bug 12595 - REGRESSION: Can't add item to cart at lnt.com (JS type error)
Summary: REGRESSION: Can't add item to cart at lnt.com (JS type error)
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 420+
Hardware: Mac OS X 10.4
: P1 Normal
Assignee: Antti Koivisto
URL:
Keywords: HasReduction, InRadar, Regression
Depends on:
Blocks:
 
Reported: 2007-02-04 11:03 PST by Maciej Stachowiak
Modified: 2009-01-14 16:57 PST (History)
4 users (show)

See Also:


Attachments
the reduction (606 bytes, text/html)
2007-02-20 00:14 PST, Maciej Stachowiak
no flags Details
another test (450 bytes, text/html)
2007-02-24 05:02 PST, Alexey Proskuryakov
no flags Details
patch with change log and test case (41.54 KB, patch)
2007-03-09 20:22 PST, Darin Adler
bdakin: review-
Details | Formatted Diff | Diff
new patch (15.54 KB, patch)
2007-03-16 16:10 PDT, Antti Koivisto
adele: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Maciej Stachowiak 2007-02-04 11:03:26 PST
2006-09-07 18:24:48 Alice Liu:
* SUMMARY
I tried to add this item at linens and things http://www.lnt.com/product/index.jsp?productId=2127427&cp=1331607.1331947.2160003&page=2&doVSearch=no&pageBucket=0&parentPage=family to my cart, but a JS error prevented it from working.  TypeError: Undefined value

* STEPS TO REPRODUCE
1) go to an item page at lnt.com, like http://www.lnt.com/product/index.jsp?productId=2127427&cp=1331607.1331947.2160003&page=2&doVSearch=no&pageBucket=0&parentPage=family
2) press the blue "add tiems to cart" button

* RESULTS
nothing, JS stopped processing because something is undefined.  

* REGRESSION
no idea yet, I'm on Leopard 9A241 rt now, but i haven't tried this on Tiger safari yet. 

2006-09-08 15:24:25 Alice Liu:
Yes, this is a regression from Tiger.  

2006-09-12 16:44:50 Alice Liu:
Safari BRB Reviewed

2006-12-20 15:57:38 Alice Liu:
i am working on this

2007-01-26 17:13:28 Alice Liu:
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.  Running this test in Safari 2.0, Safari TOT, firefox, and IE will show you the differences.

<rdar://problem/4722863>
Comment 1 Alexey Proskuryakov 2007-02-13 21:52:47 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!
Comment 2 Maciej Stachowiak 2007-02-20 00:14:26 PST
Created attachment 13270 [details]
the reduction

Attached the mentioned reduction.
Comment 3 Alexey Proskuryakov 2007-02-24 05:02:59 PST
Created attachment 13361 [details]
another test

Firefox even includes removed form elements in the collection!
Comment 4 Darin Adler 2007-03-09 20:22:13 PST
Created attachment 13572 [details]
patch with change log and test case

And now, so do we!
Comment 5 Maciej Stachowiak 2007-03-12 16:59:54 PDT
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 6 Maciej Stachowiak 2007-03-12 17:00:36 PDT
Comment on attachment 13572 [details]
patch with change log and test case

r-
Comment 7 Maciej Stachowiak 2007-03-12 17:47:37 PDT
I put my comments and r- in the wrong bug.
Comment 8 Darin Adler 2007-03-12 20:58:53 PDT
Comment on attachment 13572 [details]
patch with change log and test case

I believe Adele was reviewing this when Maciej accidentally marked it review-.
Comment 9 Geoffrey Garen 2007-03-12 23:20:45 PDT
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 10 Maciej Stachowiak 2007-03-12 23:46:22 PDT
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.
Comment 11 Darin Adler 2007-03-13 06:28:45 PDT
(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.
Comment 12 Darin Adler 2007-03-13 07:33:57 PDT
Committed revision 20148.
Comment 13 Beth Dakin 2007-03-15 12:27:03 PDT
Re-opening this bug. I had to back it out with r20214 because it causes a memory trasher. Steps to reproduce trasher forthcoming.
Comment 14 Beth Dakin 2007-03-15 12:31:07 PDT
Comment on attachment 13572 [details]
patch with change log and test case

r-minusing the patch so as not to cause confusion.
Comment 15 Beth Dakin 2007-03-15 13:09:28 PDT
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.
Comment 16 Antti Koivisto 2007-03-16 16:10:47 PDT
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 17 Adele Peterson 2007-03-16 16:38:57 PDT
Comment on attachment 13675 [details]
new patch

m_autocomplete(true)

!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!

I should've checked in a layout test for that...
Comment 18 Antti Koivisto 2007-03-16 17:00:28 PDT
Eh. I just copied those lines from Darin patch. Is m_autocomplete supposed to be true or false here?
Comment 19 Adele Peterson 2007-03-16 17:02:13 PDT
its supposed to be true.  Darin's patch accidently changed it to false...then I changed it back.
Comment 20 Adele Peterson 2007-03-17 01:10:23 PDT
Comment on attachment 13675 [details]
new patch

As long as you switch the m_autocomplete line, r=me!
Comment 21 Antti Koivisto 2007-03-17 05:09:45 PDT
r20260, with m_autocomplete(true)!
Comment 22 Ian 'Hixie' Hickson 2009-01-14 16:57:30 PST
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).