Bug 12595 – REGRESSION: Can't add item to cart at lnt.com (JS type error)

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Maciej Stachowiak]

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>

[Alexey Proskuryakov]

(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!

[Maciej Stachowiak]

Created attachment 13270 [details]
the reduction

Attached the mentioned reduction.

[Alexey Proskuryakov]

Created attachment 13361 [details]
another test

Firefox even includes removed form elements in the collection!

[Darin Adler]

Created attachment 13572 [details]
patch with change log and test case

And now, so do we!

[Maciej Stachowiak]

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>

[Maciej Stachowiak]

Comment on attachment 13572 [details]
patch with change log and test case

r-

[Maciej Stachowiak]

I put my comments and r- in the wrong bug.

[Darin Adler]

Comment on attachment 13572 [details]
patch with change log and test case

I believe Adele was reviewing this when Maciej accidentally marked it review-.

[Geoffrey Garen]

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?

[Maciej Stachowiak]

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.

[Darin Adler]

(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.

[Darin Adler]

Committed revision 20148.

[Beth Dakin]

Re-opening this bug. I had to back it out with r20214 because it causes a memory trasher. Steps to reproduce trasher forthcoming.

[Beth Dakin]

Comment on attachment 13572 [details]
patch with change log and test case

r-minusing the patch so as not to cause confusion.

[Beth Dakin]

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.

[Antti Koivisto]

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.

[Adele Peterson]

Comment on attachment 13675 [details]
new patch

m_autocomplete(true)

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

I should've checked in a layout test for that...

[Antti Koivisto]

Eh. I just copied those lines from Darin patch. Is m_autocomplete supposed to be true or false here?

[Adele Peterson]

its supposed to be true.  Darin's patch accidently changed it to false...then I changed it back.

[Adele Peterson]

Comment on attachment 13675 [details]
new patch

As long as you switch the m_autocomplete line, r=me!

[Antti Koivisto]

r20260, with m_autocomplete(true)!

[Ian 'Hixie' Hickson]

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).