987222 - Errors from <marquee onstart> are shown as "System JS"

Status: RESOLVED FIXED

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

Description

[Jesse Ruderman]

Attachment: testcase 1

System JS : ERROR file:///Users/jruderman/Desktop/m.html:8 - ReferenceError: k is not defined

Comment 1

[Jesse Ruderman]

Attachment: testcase 2

System JS : ERROR (null):0 - uncaught exception: [Opaque]

Comment 2

[Boris Zbarsky [:bzbarsky]]

Those are using the error reporter for the XBL compilation global, not for the window.

And the reason for that is that the event handler is being created in the XBL scope in marquee.xml like so:

267                   var contentFn = new (XPCNativeWrapper.unwrap(window).Function)("event", aValue);
268                   this["_on" + aName] = function(e) { return contentFn.call(this, e); };
...
273                 this.addEventListener(aName, this["_on" + aName], false);

I guess the sane thing to do here would be to bind contentFn instead of creating a wrapper function, right?  That should give us the right JSContext for error reporting.

Note that I expect this also fails to properly report parse errors in aValue...  And of course .onstart is totally messed up (it returns a string, not the function).

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: Minimal fix. The bind() needs to be in the content compartment, sadly, not on an Xray? :(

Comment 4

[Boris Zbarsky [:bzbarsky]]

bobby, you have most recent blame on this code... I _think_ what I did there is reasonable and should not break the other stuff, but please check carefully?

Comment 5

[Bobby Holley (:bholley)]

Comment on attachment 8395782 [details] [diff] [review]
Minimal fix.  The bind() needs to be in the content compartment, sadly, not on an Xray?  :(

Review of attachment 8395782 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with the below.

::: layout/style/xbl-marquee/xbl-marquee.xml
@@ +259,5 @@
>              case "string":
>                if (!aIgnoreNextCall) {
>                  try {
>                    // Work around bug 872772 by wrapping the cross-compartment-
>                    // wrapped function in a function from this scope.

So, the reason that we were wrapping in a function before wasn't to implement the bind, actually (the bind was in fact an adendum in bug 871887). It was to work around bug 872772, which is now fixed. So this comment can go away.

@@ +263,5 @@
>                    // wrapped function in a function from this scope.
>                    //
>                    // NB: Make sure to wrap the constructor in parentheses to
>                    // deal with the weird precedence of |new| in JS.
> +                  var contentFn = new (XPCNativeWrapper.unwrap(window).Function)("event", aValue).bind(this);

We should now change this to use the Xrayed Function constructor here. Note that we'll still need to waive before the call to .bind. So make this:

var contentFn = XPCNativeWrapper.unwrap(new window.Function("event", aValue)).bind(this);

And please file a followup bug blocked on bug 976148 to remove the waiving here.

Comment 6

[Bobby Holley (:bholley)]

Actually, I think this would be even better:

var contentFun = new window.Function("event", aValue);
var boundFunc = Function.prototype.bind.call(XPCNativeWrapper.unwrap(contentFun), this);

And use boundFunc, so that we don't rely on the |bind| method in the content scope. But then we're back where we started in terms of the error in this bug. :-(

(In reply to Boris Zbarsky [:bz] from comment #2)
> Those are using the error reporter for the XBL compilation global, not for
> the window.

I think you mean the XBL scope, which is very different from the XBL compilation global.

Comment 7

[Bobby Holley (:bholley)]

Comment on attachment 8395782 [details] [diff] [review]
Minimal fix.  The bind() needs to be in the content compartment, sadly, not on an Xray?  :(

I think this isn't the right solution. Hang on while I set up the bugs.

Comment 8

[Bobby Holley (:bholley)]

The two dependent bugs here are the things we need to make this error less sucky. I've filed bug 987672 for the work in comment 6.

Comment 9

[Boris Zbarsky [:bzbarsky]]

> But then we're back where we started in terms of the error in this bug. :-(

Right, exactly.

> I think you mean the XBL scope, which is very different from the XBL compilation global.

I really wish we could just somehow attach the DOM error reporter to the XBL scope...

So are you basically opposed to us doing anything about this bug until those deps are fixed, or do you just want the changes from comment 6?

Comment 10

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky [:bz] from comment #9)
> So are you basically opposed to us doing anything about this bug until those
> deps are fixed

I think so, yeah. Error Xrays are coming soon (Q2 goal), and I'm not wild about trying to fix any of the JSErrorReporter wackiness until the underlying architecture is better.

> or do you just want the changes from comment 6?

The changes from comment 6 that can land now are rolled into bug 987672.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Is this still an issue?  It's not clear to me what the steps to reproduce are here....

Comment 12

[Sylvestre Ledru [:Sylvestre]]

No assignee, updating the status.

Comment 13

[Boris Zbarsky [:bzbarsky]]

This got fixed by switching marquee away from XBL.