Bug 12794 - REGRESSION: TripTik planner at aaa.com never finishes loading due to unclosed canvas tag
Summary: REGRESSION: TripTik planner at aaa.com never finishes loading due to unclosed...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 420+
Hardware: Macintosh OS X 10.4
: P1 Normal
Assignee: Darin Adler
URL: http://www.aaa.com
Keywords: HasReduction, InRadar, Regression
Depends on:
Blocks:
 
Reported: 2007-02-16 13:57 PST by Simon Pride
Modified: 2007-03-13 16:33 PDT (History)
2 users (show)

See Also:


Attachments
test case (87 bytes, text/html)
2007-02-24 00:33 PST, Alexey Proskuryakov
no flags Details
patch with change log and test case (36.87 KB, patch)
2007-03-10 10:10 PST, Darin Adler
mjs: review-
Details | Formatted Diff | Diff
revised patch, with change log and regression tests (50.56 KB, patch)
2007-03-13 08:58 PDT, Darin Adler
mjs: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Simon Pride 2007-02-16 13:57:07 PST
Using the interactive TripTik planner, the applet in the left frame never finishes loading. Works fine in release Safari 2.0.4.

Sample URL (contains session ID cruft):
http://ww1.aaa.com/scripts/WebObjects.dll/AAAOnline.woa/4014/wo/VULCxz1Spe682RojJvOSHw/0.9.13.8.3.0.1.2.2.1.1.0.1.1.0.0
Comment 1 Alexey Proskuryakov 2007-02-16 22:54:08 PST
Confirmed as a regression. There is a "maximum call stack exceeded" error in the console.

Steps to reproduce:
1. Open http://www.aaa.com
2. Enter some U.S. zip code (such as 95014)
3. Click "TripTik(R) Travel Planner" link under Maps & Directions.
Comment 2 Alexey Proskuryakov 2007-02-23 14:08:16 PST
The "maximum call stack exceeded" error is red herring - it is caused by unreliable error logging code. The actual problem appears to be a result of an unclosed <canvas> tag:

...
<canvas ID="dRouteCanvas" width=2000 height=2000 />
</DIV>
Comment 3 Darin Adler 2007-02-23 14:09:40 PST
Does this fail with Firefox too? I ask because I believe Firefox would also consider this an unclosed <canvas> tag.
Comment 4 Simon Pride 2007-02-23 14:23:39 PST
> Does this fail with Firefox too? I ask because I believe Firefox would also
consider this an unclosed <canvas> tag.

No, it works fine in the current version of Firefox (2.0.0.1 on PowerPC). 
Comment 5 Darin Adler 2007-02-23 14:32:43 PST
We should figure out why WebKit's rule doesn't match Firefox's. Why does it close the canvas element? Is it the /> at the end of the element? Is it the subsequent </div> or some other markup? Or what?
Comment 6 Simon Pride 2007-02-23 14:40:57 PST
Is it naive to ask why Safari 2.0.4 with its Tiger 10.4.8 version of WebKit (419.3?) handles it OK? 
Comment 7 Darin Adler 2007-02-23 14:50:21 PST
(In reply to comment #6)
> Is it naive to ask why Safari 2.0.4 with its Tiger 10.4.8 version of WebKit
> (419.3?) handles it OK? 

In Tiger WebKit, <canvas> is a standalone element like <img>, and so there's no such things as an "open" canvas element.

TOT WebKit has changed to match other browsers and the WhatWG draft specification, and thus you have to close the <canvas> element with a separate </canvas> in HTML documents. Firefox matches TOT WebKit in this regard, but somehow the site is still working. That's what we have to figure out.
Comment 8 Alexey Proskuryakov 2007-02-24 00:33:05 PST
Created attachment 13358 [details]
test case

> We should figure out why WebKit's rule doesn't match Firefox's. Why does it
> close the canvas element? Is it the /> at the end of the element? Is it the
> subsequent </div> or some other markup? Or what?

Looks like it's a <div> </div> pair around the canvas - the closing div alone isn't enough.
Comment 9 Maciej Stachowiak 2007-02-27 19:34:15 PST
<rdar://problem/5028154>
Comment 10 Darin Adler 2007-03-10 08:06:11 PST
I tested Firefox with other tags;

    title, style, iframe, noembed, noframes, noscript

Only canvas had the special behavior, where the </div> closed the element.
Comment 11 Darin Adler 2007-03-10 08:07:31 PST
Also, I tested with multiple elements, and it only worked if the element closed was the one opened just before the canvas. I'm guessing this is a canvas-specific quirk in Firefox.
Comment 12 Darin Adler 2007-03-10 08:12:41 PST
A </p> works, as does </div>, but not </a>, </b>, or </form>.

And the element doesn't have to be the closest enclosing one.
Comment 13 Darin Adler 2007-03-10 08:14:34 PST
My mistake. Closing any open element seems to work. My experiments with <a> and <b> were presumably in cases where those elements were closed already.
Comment 14 Darin Adler 2007-03-10 08:29:29 PST
Seems that the Gecko HTML parser classifies canvas as a "special".

    http://lxr.mozilla.org/seamonkey/source/parser/htmlparser/src/nsElementTable.cpp

And that file contains all sorts of rules for closing. I'm still not sure how to modify our parser to have similar behavior in this case.
Comment 15 Darin Adler 2007-03-10 10:10:03 PST
Created attachment 13577 [details]
patch with change log and test case
Comment 16 mitz 2007-03-10 11:27:58 PST
Comment on attachment 13577 [details]
patch with change log and test case

This sentence is not quite right: "Changed the Document parameter to the constructor to instead of HTMLDocument".
Comment 17 Maciej Stachowiak 2007-03-12 17:47:43 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 18 Maciej Stachowiak 2007-03-12 17:50:47 PDT
Comment on attachment 13577 [details]
patch with change log and test case

r-
Comment 19 Darin Adler 2007-03-13 08:58:15 PDT
Created attachment 13611 [details]
revised patch, with change log and regression tests
Comment 20 Maciej Stachowiak 2007-03-13 16:08:24 PDT
Comment on attachment 13611 [details]
revised patch, with change log and regression tests

r=me
Comment 21 Darin Adler 2007-03-13 16:33:27 PDT
Committed revision 20170. <http://trac.webkit.org/projects/webkit/changeset/20170>