Closed Bug 742156 Opened 8 years ago Closed 8 years ago

Stringifying EventTarget throws

Categories

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

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla15
Tracking Status
firefox14 + fixed

People

(Reporter: bzbarsky, Assigned: peterv)

References

Details

(Keywords: regression)

Attachments

(1 file, 1 obsolete file)

Testcase:

  javascript:alert(EventTarget)

the issue is we're calling Function.prototype.toString.  We never added an own toString on our interface objects....
Attached patch v1 (obsolete) — Splinter Review
This is causing orange on the Jetpack tests.
Assignee: nobody → peterv
Status: NEW → ASSIGNED
(In reply to Peter Van der Beken [:peterv] from comment #1)
> Created attachment 612126 [details] [diff] [review]
> v1
> 
> This is causing orange on the Jetpack tests.

Tracking for release to prevent issues hiding behind these orange tests. Let's go with the simplest fix (test or code change) to get this working again.

If that's not the only reason to track this bug for release, please clarify. Thanks!
Attached patch v1.1Splinter Review
Attachment #612126 - Attachment is obsolete: true
Attachment #620287 - Flags: review?(bzbarsky)
Comment on attachment 620287 [details] [diff] [review]
v1.1

>+++ b/dom/bindings/Utils.cpp
>+// passed a non-Function object we als need to provide our own toString method

s/als/also/

>+InterfaceObjectToString(JSContext* cx, unsigned argc, JS::Value *vp)

>+  JS::Value thisv = JS_THIS(cx, vp);

You could use JS_THIS_OBJECT and then just null-check it; then no need for thisv.

Note that JS_THIS always returns either JSVAL_NULL or an ObjectValue(), so the "undefined" case can't even be hit here.

r=me with those fixed.
Attachment #620287 - Flags: review?(bzbarsky) → review+
https://hg.mozilla.org/mozilla-central/rev/efa51cd286ee
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Comment on attachment 620287 [details] [diff] [review]
v1.1

[Approval Request Comment]
Regression caused by (bug #): bug 740069
User impact if declined: seems to break Jetpack tests, not sure if it affects addons. Could cause website breakage due to unexpected exceptions.
Testing completed (on m-c, etc.): landed on m-c (and has a testcase)
Risk to taking this patch (and alternatives if risky): low-risk
String changes made by this patch: None
Attachment #620287 - Flags: approval-mozilla-aurora?
Comment on attachment 620287 [details] [diff] [review]
v1.1

[Triage Comment]
Approving for Aurora 14.
Attachment #620287 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.