Bug 12595 - REGRESSION: Can't add item to cart at lnt.com (JS type error)
: REGRESSION: Can't add item to cart at lnt.com (JS type error)
Status: RESOLVED FIXED
: WebKit
HTML DOM
: 420+
: Macintosh Mac OS X 10.4
: P1 Normal
Assigned To:
:
: HasReduction, InRadar, Regression
:
:
  Show dependency treegraph
 
Reported: 2007-02-04 11:03 PST by
Modified: 2009-01-14 16:57 PST (History)


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-
Review Patch | Details | Formatted Diff | Diff
new patch (15.54 KB, patch)
2007-03-16 16:10 PST, Antti Koivisto
adele: review+
Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 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 From 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 From 2007-02-20 00:14:26 PST -------
Created an attachment (id=13270) [details]
the reduction

Attached the mentioned reduction.
------- Comment #3 From 2007-02-24 05:02:59 PST -------
Created an attachment (id=13361) [details]
another test

Firefox even includes removed form elements in the collection!
------- Comment #4 From 2007-03-09 20:22:13 PST -------
Created an attachment (id=13572) [details]
patch

And now, so do we!
------- Comment #5 From 2007-03-12 16:59:54 PST -------
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 From 2007-03-12 17:00:36 PST -------
(From update of attachment 13572 [details])
r-
------- Comment #7 From 2007-03-12 17:47:37 PST -------
I put my comments and r- in the wrong bug.
------- Comment #8 From 2007-03-12 20:58:53 PST -------
(From update of attachment 13572 [details])
I believe Adele was reviewing this when Maciej accidentally marked it review-.
------- Comment #9 From 2007-03-12 23:20:45 PST -------
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 From 2007-03-12 23:46:22 PST -------
(From update of attachment 13572 [details])
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 From 2007-03-13 06:28:45 PST -------
(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 From 2007-03-13 07:33:57 PST -------
Committed revision 20148.
------- Comment #13 From 2007-03-15 12:27:03 PST -------
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 From 2007-03-15 12:31:07 PST -------
(From update of attachment 13572 [details])
r-minusing the patch so as not to cause confusion.
------- Comment #15 From 2007-03-15 13:09:28 PST -------
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 From 2007-03-16 16:10:47 PST -------
Created an attachment (id=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 From 2007-03-16 16:38:57 PST -------
(From update of attachment 13675 [details])
m_autocomplete(true)

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

I should've checked in a layout test for that...
------- Comment #18 From 2007-03-16 17:00:28 PST -------
Eh. I just copied those lines from Darin patch. Is m_autocomplete supposed to be true or false here?
------- Comment #19 From 2007-03-16 17:02:13 PST -------
its supposed to be true.  Darin's patch accidently changed it to false...then I changed it back.
------- Comment #20 From 2007-03-17 01:10:23 PST -------
(From update of attachment 13675 [details])
As long as you switch the m_autocomplete line, r=me!
------- Comment #21 From 2007-03-17 05:09:45 PST -------
r20260, with m_autocomplete(true)!
------- Comment #22 From 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).