Closed Bug 882653 Opened 7 years ago Closed 7 years ago

Unhelpful error message, "TypeError: Value does not implement interface Node" when passing null to replaceChild or appendChild

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla24

People

(Reporter: jaws, Assigned: bzbarsky)

References

(Blocks 1 open bug)

Details

Attachments

(5 files, 1 obsolete file)

If your JS code accidentally passes null to replaceChild or appendChild, you get an unhelpful error messages "Value does not implement interface Node".

I don't know how much it will hurt perf, but we should be able to say something along the lines of "Passing null as argument 1 is not valid for replaceChild".
The main issue in terms of perf is that will involve lots and lots more string relocations and somewhat bigger code.  And a bit more complexity in the codegen.
For this testcase:

  <script>
    document.documentElement.appendChild.call({}, new Image());
  </script>
  <script>
    Object.getOwnPropertyDescriptor(Element.prototype, "innerHTML").get.call({});
  </script>
  <script>
    Object.getOwnPropertyDescriptor(Element.prototype, "innerHTML").set.call({});
  </script>

this patch gives this output:

  [16:31:11.257] TypeError: appendChild called on an object that does not implement interface Node. @ file:///private/tmp/test.html:10
  [16:31:11.258] TypeError: Element getter called on an object that does not implement interface Element. @ file:///private/tmp/test.html:13
  [16:31:11.258] TypeError: Element setter called on an object that does not implement interface Element. @ file:///private/tmp/test.html:16
Attachment #762277 - Flags: review?(bugs)
Assignee: nobody → bzbarsky
For this testcase:

  <script>
    document.documentElement.appendChild(5);
  </script>
  <script>
    document.documentElement.appendChild(null);
  </script>

this patch gives this output:

[18:05:45.334] TypeError: Argument 1 of Node.appendChild is not an object. @ file:///private/tmp/test.html:4
[18:05:45.335] TypeError: Argument 1 of Node.appendChild is not an object. @ file:///private/tmp/test.html:7
Attachment #762335 - Flags: review?(bugs)
(In reply to Boris Zbarsky from comment #1)
> The main issue in terms of perf is that will involve lots and lots more
> string relocations and somewhat bigger code.

Well, only if you construct all the error messages at build time. (Are exceptions exceptional enough that you don't have to worry about the perf of constructing their messages at run time?)
> Well, only if you construct all the error messages at build time.

It's pretty hard to construct them at runtime without having just as many relocations for the pieces you construct them from....

> Are exceptions exceptional enough that you don't have to worry about the perf of
> constructing their messages at run time?

I suspect so, yes.
For this testcase:

  <script>
    document.documentElement.appendChild({});
  </script>

this patch gives this output:

  [21:19:58.236] TypeError: Argument 1 of Node.appendChild does not implement interface Node. @ file:///private/tmp/test.html:10
Attachment #762437 - Flags: review?(bugs)
For this testcase:

  <script>
    document.createElement("canvas").getContext("2d").fill("bogus")
  </script>
  <script>
    document.createTreeWalker(document, 0xFFFFFFFF, { acceptNode: 5 }).nextNode()
  </script>
  <script>
    (new TextEncoder).encode("", new RegExp())
  </script>

this patch gives this output:

  [22:10:53.803] TypeError: Argument 1 of CanvasRenderingContext2D.fill 'bogus' is not a valid value for enumeration CanvasWindingRule. @ file:///private/tmp/test.html:4
  [22:10:53.804] TypeError: Property 'acceptNode' is not callable. @ file:///private/tmp/test.html:7
  [22:10:53.805] TypeError: Argument 2 of TextEncoder.encode can't be converted to a dictionary. @ file:///private/tmp/test.html:10
Attachment #762451 - Flags: review?(bugs)
For this testcase:

  <script>
    document.createElement("canvas").getContext("webgl").uniform1fv(null, new RegExp())
  </script>
  <script>
    document.createElement("select").add({});
  </script>
  <script>
    document.createElement("canvas").getContext("2d").createLinearGradient(0, 1, 0, 1).addColorStop(NaN, "");
  </script>

this patch gives this output:

  [22:32:07.274] TypeError: Argument 2 is not valid for any of the 2-argument overloads of WebGLRenderingContext.uniform1fv. @ file:///private/tmp/test.html:4
  [22:32:07.275] TypeError: Argument 1 of HTMLSelectElement.add could not be converted to any of: HTMLOptionElement, HTMLOptGroupElement. @ file:///private/tmp/test.html:7
  [22:32:07.278] TypeError: Argument 1 of CanvasGradient.addColorStop is not a finite floating-point value. @ file:///private/tmp/test.html:10
Attachment #762459 - Flags: review?(bugs)
Attachment #762437 - Attachment is obsolete: true
Attachment #762437 - Flags: review?(bugs)
Comment on attachment 762277 [details] [diff] [review]
part 1.  Improve error reporting for bogus this objects.

I think getter and setter should be without the starting {0}, since that
leads to duplicating interface name in error.

Also, jorendorff>	smaug: use nullptr everywhere
so, s/NULL/nullptr/
Attachment #762277 - Flags: review?(bugs) → review+
Attachment #762459 - Flags: review?(bugs) → review+
Comment on attachment 762335 [details] [diff] [review]
part 2.  Make the "not an object" error reporting better in WebIDL bindings.


>+    # Unfortunately, .capitalize() on a string will lowercase things inside the
>+    # string, which we do not want.
>+    def capitalize(string):
>+        return string[0].capitalize() + string[1:]
Could this be firstCap(). Such is used in many other codegens, for example in
http://mxr.mozilla.org/mozilla-central/source/xpcom/idl-parser/header.py?rev=d764382ed4cf&mark=22-23#22



>@@ -5430,20 +5468,20 @@ class CGSpecializedForwardingSetter(CGSp
>         assert all(ord(c) < 128 for c in attrName)
>         assert all(ord(c) < 128 for c in forwardToAttrName)
>         return CGIndenter(CGGeneric("""JS::RootedValue v(cx);
> if (!JS_GetProperty(cx, obj, "%s", v.address())) {
>   return false;
> }
> 
> if (!v.isObject()) {
>-  return ThrowErrorMessage(cx, MSG_NOT_OBJECT);
>+  return ThrowErrorMessage(cx, MSG_NOT_OBJECT, "%s.%s");
> }
> 
>-return JS_SetProperty(cx, &v.toObject(), "%s", args.handleAt(0).address());""" % (attrName, forwardToAttrName))).define()
>+return JS_SetProperty(cx, &v.toObject(), "%s", args.handleAt(0).address());""" % (attrName, self.descriptor.interface.identifier.name, attrName, forwardToAttrName))).define()
(Multiline strings don't exactly make this easier to read, but no need to change)
Attachment #762335 - Flags: review?(bugs) → review+
> Could this be firstCap().

Yes.
Attachment #762451 - Flags: review?(bugs) → review+
Attachment #762512 - Flags: review?(bugs) → review+
Flags: in-testsuite? → in-testsuite+
You need to log in before you can comment on or make changes to this bug.