Bug 22365 – Add a test to verify the results of DOM constructors

• Component: DOM
• Status: RESOLVED FIXED

[Pam Greene (IRC:pamg)]

Most DOM nodes' constructors throw an exception when called, some have a working constructor, and some node types have no constructor at all.  Add a test to track that.

[Pam Greene (IRC:pamg)]

Created attachment 25288 [details]
New test + result

[Sam Weinig]

Comment on attachment 25288 [details]
New test + result

> +
> +// These nodes have a working constructor.
> +var nodes_constructor = [
> +  'DOMParser',
> +  'XMLHttpRequest', 'XMLSerializer',
> +  'XPathEvaluator',
> +  'XSLTProcessor'
> +];

You are missing Audio, Option, Image, Worker, and MessageChannel here.

> +
> +// These nodes have no constructor.
> +var nodes_no_constructor = [
> +  'EventTargetNode',
> +  'BarInfo', 'CanvasGradient',
> +  'CanvasPattern', 'Console',
> +  'DOMSelection', 'DOMWindow', 'History',
> +  'UndetectableHTMLCollection',
I don't think this is a real class in WebCore.  We do have class called HTMLAllCollection which is undetectable though.  Perhaps that is what you were thinking of.


> +  'HTMLOptionsCollection', 'InspectorController',
> +  'Location',
> +  'Navigator',
> +  'NodeIterator',
> +  'RGBColor', 'Screen',
> +  'TreeWalker',
> +  'XPathExpression', 'XPathNSResolver',
> +  'EventTarget', 'EventListener',
> +  'NPObject'
> +];

Some of these should probably have constructors (of the non-constructible variety), but I guess it is good to test our current behavior.

> +
> +function TryAllocate(node) {
> +  var Cons = this[node];
> +  if (!Cons) return 'no constructor';
> +  try { return new Cons(); }
> +  catch (e) { return 'exception'; }
> +}
> +
> +function check(name, expected) {
> +  actual = TryAllocate(node);
> +  if (actual == expected)
> +    document.write("PASS: " + name + " '" + expected + "'<br>");
> +  else
> +    document.write("FAIL: " + name + " wanted '" + expected + "', got '" + actual + "'<br>");
> +}
> +
> +
> +for (var i = 0; i < nodes_exception.length; i++) {
> +  var node = nodes_exception[i];
> +  check(node, 'exception');
> +}
> +
> +for (var i = 0; i < nodes_no_constructor.length; i++) {
> +  var node = nodes_no_constructor[i];
> +  check(node, 'no constructor');
> +}
> +
> +for (var i = 0; i < nodes_constructor.length; i++) {
> +  var node = nodes_constructor[i];
> +  check(node, '[object ' + node + ']');
> +}
In general, we like to keep all code in the WebKit style guidelines, even tests, which mean 4 space indentation.

r=me with those changes.

[Darin Adler]

(In reply to comment #2)
> Some of these should probably have constructors (of the non-constructible
> variety), but I guess it is good to test our current behavior.

While it's good to have test results that reflect our current behavior, it's not necessary to have those results say "test passed". Instead the current behavior can be defined as "expected failure". That approach is worth considering in a test like this one.

[Pam Greene (IRC:pamg)]

Created attachment 25317 [details]
Patch addressing Sam's comments

(In reply to comment #2)

> > +// These nodes have a working constructor.
> > +var nodes_constructor = [
> > +  'DOMParser',
> > +  'XMLHttpRequest', 'XMLSerializer',
> > +  'XPathEvaluator',
> > +  'XSLTProcessor'
> > +];
> 
> You are missing Audio, Option, Image, Worker, and MessageChannel here.
 
Audio, Option, and Image all have constructors, although with slightly nonstandard object names; Worker throws an exception; and MessageChannel has no constructor.  I've added them to the appropriate lists.

> > +  'UndetectableHTMLCollection',
> I don't think this is a real class in WebCore.  We do have class called
> HTMLAllCollection which is undetectable though.  Perhaps that is what you were
> thinking of.

Looks like UndetectableHTMLCollection is a V8ism. It's not causing any problems to test it in JSC too, but if you object I'll take it out. (I wouldn't propose testing lots of V8isms in the layout tests, but as one of ~150 items in a list it seems both reasonable and harmless.)

HTMLAllCollection is in the nodes_no_constructor list.

> Some of these should probably have constructors (of the non-constructible
> variety), but I guess it is good to test our current behavior.

Let me know which nodes are in the wrong lists, and I'll gladly move them.

> In general, we like to keep all code in the WebKit style guidelines, even
> tests, which mean 4 space indentation.

Done.

[Darin Adler]

(In reply to comment #4)
> Looks like UndetectableHTMLCollection is a V8ism. It's not causing any problems
> to test it in JSC too, but if you object I'll take it out. (I wouldn't propose
> testing lots of V8isms in the layout tests, but as one of ~150 items in a list
> it seems both reasonable and harmless.)

It's a bug to have that constructor visible from JavaScript. It's fine for V8 to have a different way of handling this internally, but that implementation detail should be invisible to scripts.

[Pam Greene (IRC:pamg)]

(In reply to comment #5)
> (In reply to comment #4)
> > Looks like UndetectableHTMLCollection is a V8ism.
 
> It's a bug to have that constructor visible from JavaScript. It's fine for V8
> to have a different way of handling this internally, but that implementation
> detail should be invisible to scripts.

Unless I'm misunderstanding the test, it's verifying that the constructor is not visible. The 'no constructor' result for UndetectableHTMLCollection is the same one given if you check a node named 'ThisDoesNotExist'.

[Sam Weinig]

Comment on attachment 25317 [details]
Patch addressing Sam's comments

> +
> +// These nodes throw an exception.
> +var nodes_exception = [

Many of the objects tested here are not Nodes, they are just objects exposed by the DOM. 

> +    'Attr', 'CharacterData', 'CDATASection', 'Comment', 'Document',
> +    'DocumentFragment', 'DocumentType', 'Element', 'Entity',
> +    'EntityReference', 'HTMLDocument', 'Node', 'Notation',
> +    'ProcessingInstruction', 'Text', 'HTMLAnchorElement',
> +    'HTMLAppletElement', 'HTMLAreaElement', 'HTMLBaseElement',
> +    'HTMLBaseFontElement', 'HTMLBlockquoteElement', 'HTMLBodyElement',
> +    'HTMLBRElement', 'HTMLButtonElement', 'HTMLCanvasElement',
> +    'HTMLDirectoryElement', 'HTMLDivElement', 'HTMLDListElement',
> +    'HTMLEmbedElement', 'HTMLFieldSetElement', 'HTMLFontElement',
> +    'HTMLFormElement', 'HTMLFrameElement', 'HTMLFrameSetElement',
> +    'HTMLHeadingElement', 'HTMLHeadElement', 'HTMLHRElement',
> +    'HTMLHtmlElement', 'HTMLIFrameElement', 'HTMLImageElement',
> +    'HTMLInputElement', 'HTMLIsIndexElement', 'HTMLLabelElement',
> +    'HTMLLegendElement', 'HTMLLIElement', 'HTMLLinkElement',
> +    'HTMLMapElement', 'HTMLMarqueeElement', 'HTMLMenuElement',
> +    'HTMLMetaElement', 'HTMLModElement', 'HTMLObjectElement',
> +    'HTMLOListElement', 'HTMLOptGroupElement', 'HTMLOptionElement',
> +    'HTMLParagraphElement', 'HTMLParamElement', 'HTMLPreElement',
> +    'HTMLQuoteElement', 'HTMLScriptElement', 'HTMLSelectElement',
> +    'HTMLStyleElement', 'HTMLTableCaptionElement',
> +    'HTMLTableColElement', 'HTMLTableElement',
> +    'HTMLTableSectionElement', 'HTMLTableCellElement',
> +    'HTMLTableRowElement', 'HTMLTextAreaElement', 'HTMLTitleElement',
> +    'HTMLUListElement', 'HTMLElement', 'CanvasRenderingContext2D',
> +    'Clipboard', 'Counter', 'CSSCharsetRule', 'CSSFontFaceRule',
> +    'CSSImportRule', 'CSSMediaRule', 'CSSPageRule',
> +    'CSSPrimitiveValue', 'CSSRule', 'CSSRuleList',
> +    'CSSStyleDeclaration', 'CSSStyleRule', 'CSSStyleSheet',
> +    'CSSValue', 'CSSValueList', 'DOMImplementation', 'Event',
> +    'HTMLCollection', 'KeyboardEvent', 'MediaList', 'MimeType',
> +    'MimeTypeArray', 'MouseEvent', 'MutationEvent', 'NamedNodeMap',
> +    'NodeFilter', 'NodeList', 'OverflowEvent', 'Plugin',
> +    'PluginArray', 'Range', 'Rect', 'StyleSheet', 'StyleSheetList',
> +    'TextEvent', 'UIEvent', 'WheelEvent', 'XPathResult', 'Worker'

Worker should not throw an exception, it should be in the list of types with working constructors, even if it is not currently.  How did you make this list?  Is it derived from the list in DOMWindow.idl?

> +];
> +
> +// These nodes have a working constructor.
> +var nodes_constructor = [
> +    'DOMParser', 'XMLHttpRequest', 'XMLSerializer', 'XPathEvaluator',
> +    'XSLTProcessor'
> +];
> +
> +// These nodes have no constructor.
> +var nodes_no_constructor = [
> +    'EventTargetNode', 'BarInfo', 'CanvasGradient', 'CanvasPattern',
> +    'Console', 'DOMSelection', 'DOMWindow', 'History',
> +    'UndetectableHTMLCollection', 'HTMLOptionsCollection',
> +    'InspectorController', 'Location', 'Navigator', 'NodeIterator',
> +    'RGBColor', 'Screen', 'TreeWalker', 'XPathExpression',
> +    'XPathNSResolver', 'EventTarget', 'EventListener', 'NPObject',
> +    'HTMLAllCollection', 'MessageChannel'
> +];

BarInfo, CanvasGradient, CanvasPattern, Console, DOMSelection, DOMWindowm, History, HTMLOptionsCollection, Location, Navigator, NodeIterator, RGBColor, Screen, TreeWalker and XPathExpression should all have non-constructible constructors, and therefore should go in the first list.

MessageChannel should have a working constructor, so should go in the second list.

> +
> +// These nodes have a working constructor, but their object names differ.
> +// This is therefore a map from node to constructor.
> +var nodes_different_constructor = {
> +    'Audio': 'HTMLAudioElement',
> +    'Option': 'HTMLOptionElement',
> +    'Image': 'HTMLImageElement'
> +}
> +
> +function TryAllocate(node) {
> +    var Cons = this[node];
> +    if (!Cons) return 'no constructor';
> +    try { return new Cons(); }
> +    catch (e) { return 'exception'; }
> +}
> +
> +function check(name, expected) {
> +    actual = TryAllocate(node);
> +    if (actual == expected) {
> +      document.write("PASS: " + name + " '" + expected + "'<br>");
> +    } else {
> +      document.write("FAIL: " + name + " wanted '" + expected + "', got '" +
> +          actual + "'<br>");
> +    }
> +}
> +
> +
> +for (var i = 0; i < nodes_exception.length; i++) {
> +    var node = nodes_exception[i];
> +    check(node, 'exception');
> +}
> +
> +for (var i = 0; i < nodes_no_constructor.length; i++) {
> +    var node = nodes_no_constructor[i];
> +    check(node, 'no constructor');
> +}
> +
> +for (var i = 0; i < nodes_constructor.length; i++) {
> +    var node = nodes_constructor[i];
> +    check(node, '[object ' + node + ']');
> +}
> +
> +for (var node in nodes_different_constructor) {
> +    check(node, '[object ' + nodes_different_constructor[node] + ']');
> +}

It would be nice if this test used the shouldBe() style tests. 

r-.

[Pam Greene (IRC:pamg)]

Created attachment 25457 [details]
Shuffled objects and switched to standard JS wrapper

(In reply to comment #7)

> Many of the objects tested here are not Nodes, they are just objects exposed by
> the DOM. 

Renamed the variables for clarity.

Shuffled objects as indicated.

> It would be nice if this test used the shouldBe() style tests.

Done.

[Alexey Proskuryakov]

(In reply to comment #7)
> Worker should not throw an exception, it should be in the list of types with
> working constructors, even if it is not currently. 

This is not true - the Worker constructor requires an URL argument.

[Pam Greene (IRC:pamg)]

Moved 'Worker' into the exceptions section, expanded the comment there to explain that some of the objects may have otherwise working constructors that aren't valid without arguments, updated results, and landed as r38865.