307984 - Line numbers in errors from evalInSandbox are incorrect

Status: RESOLVED INACTIVE

(Core :: XPConnect, defect)

Description

[Aaron Boodman]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b4) Gecko/20050908 Firefox/1.4
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b4) Gecko/20050908 Firefox/1.4

When an error is thrown from within evalInSandbox, the line number which is
reported is based on the line number of the evalInSandbox call itself. I think
that it should be the line number inside the evalInSandbox call.

for instance, this code should generate an error on line number 1:

Components.utils.evalInSandbox("throw new Error()", somesandbox);

I wouldn't actually care if it consistently did what it does here, because I
could compensate for that. However in Greasemonkey, when evalInSandbox is
getting called in a much more complex situation, I noticed that the line number
is not so closely related to the line number of the evalInSandbox call.

I think it would be easier to deal with this consistently if evalInSandbox just
ignored any external lines and reported the line number of the error within the
eval'd text.

Reproducible: Always

Steps to Reproduce:
1. Run attachment, say yes to security prompt
2. Open JS console and view error

Actual Results:  
Error: foobar
Source File: file:///C:/Documents%20and%20Settings/aa/Desktop/evalInSandbox.html
Line: 42

Expected Results:  
Error: foobar
Source File: file:///C:/Documents%20and%20Settings/aa/Desktop/evalInSandbox.html
Line: 1

Comment 1

[Aaron Boodman]

Attachment: testcase

Comment 2

[Boris Zbarsky [:bzbarsky]]

The script filename used for the sandboxed script is the filename of the script calling evalInSandbox(), and while we're doing that we should be using the line number of the evalInSandbox() call, I would think.  Using the filename of the calling script but the line number in the sandboxed script seems like a recipe for major confusion.

So the problem is that we treat the sccript to be run in the sandbox as starting at the line where the evalInSandbox() call is made.  And if the script contains newlines the line numbers end up pretty meaningless, as Aaron pointed out...

I'm not sure how to fix this usefully.  The problem is that the exception would ideally contain information both about where in the sandboxed script the problem happened and where the evalInSandbox call was made (e.g. so that the JS console can be used to actually find the relevant callsite).

Perhaps we should be throwing two exceptions here?  Or at least reporting two things to the JS console?

Comment 3

[Aaron Boodman]

If it's possible to do so, simply treating the evalInSandbox call as another frame in the stack ought to be fine. So:

bar.js 15 doSomething
foo.js:143 Components.utils.evalInSandbox
<evaluated code>:15 myFunction

Comment 4

[Boris Zbarsky [:bzbarsky]]

Doing that will make it pretty impossible to use the JS console usefully with errors inside evalInSandbox....

Comment 5

[Aaron Boodman]

How so? Isn't what usually shows up in the error console the inner-most frame of the stack + line number + error message?

That means, in this case it would show:

Error Message       : line no
<evaluated code>

That is far better than the current behavior, which shows the wrong file plus an inrrelevant line number.

In combination with the feature request https://bugzilla.mozilla.org/show_bug.cgi?id=307985, one could rethrow the error adding a sensible filename.

----

If you don't like that, another option would be to allow users of evalInSandbox to register the filename to be reported by the console, or to change the signature of evalInSandbox to take a path as input:

1) sandbox.setFilename("foo.user.js");
Components.utils.evalInSandbox(contentsOfFooUserJs, sandbox);

2) Components.utils.evalInSandbox(pathToFooUserJs, sandbox);

Comment 6

[Boris Zbarsky [:bzbarsky]]

The problem is that the JS console would show the evaluated code, but that would give you no idea _who_ evaluated it and hence where it should be fixed.

Comment 7

[Aaron Boodman]

In the two places I know of evalInSandbox being used, I think that is precisely the behavior you want.

One is Greasemonkey. If an error occurs in a Greasemonkey script, it is not Greasemonkey's fault. Showing the line number of the call inside Greasemonkey is useless. You - as the Greasemonkey script developer - need to know the user script and the line number where the problem occured. 

The other is pac scripts. Again, if there is an error in a pac script it is not Mozilla's fault. Giving the line number of the location where pac scripts are evaluated is not helpful. You - as the pac script developer - need to know that it was the pac script which errored and what line number.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Ah, fair enough.  So it sounds like we want an API to give a script URI or filename to the sandbox and then just pass that and 0 as the starting line number in evalInSandbox.  I can probably hack up something like that if brendan and mrbkap think that's reasonable.

Comment 9

[Blake Kaplan (:mrbkap) (inactive)]

Could we just make the 3rd and 4th parameters to evalInSandbox an optional location (filename and linenumber)?

Comment 10

[Boris Zbarsky [:bzbarsky]]

Sounds like a decent plan to me.  Aaron?

Comment 11

[Brendan Eich [:brendan]]

(In reply to comment #8)
> Ah, fair enough.  So it sounds like we want an API to give a script URI or
> filename to the sandbox and then just pass that and 0 as the starting line
> number in evalInSandbox.  I can probably hack up something like that if brendan
> and mrbkap think that's reasonable.

0 is not a valid lineno in SpiderMonkey.

(In reply to comment #9)
> Could we just make the 3rd and 4th parameters to evalInSandbox an optional
> location (filename and linenumber)?

Brilliant!

We should do something for eval, too (and Script, or kill Script); separate bug.

/be

Comment 12

[Boris Zbarsky [:bzbarsky]]

Actually, I doubt we need to pass in a line number to evalInSandbox.  We just want to pass a filename and then let the lines in the text be numbered normally, I think.

Comment 13

[Brendan Eich [:brendan]]

(In reply to comment #12)
> Actually, I doubt we need to pass in a line number to evalInSandbox.  We just
> want to pass a filename and then let the lines in the text be numbered
> normally, I think.

The problem here, and with eval, is shown by this JS shell session, staring with line number 1:

js> var x = 3, y = 4;
js> eval("x * \n" +
         "yy")
typein:3: ReferenceError: yy is not defined
js> eval("xx * \n" +
         "y")
typein:4: ReferenceError: xx is not defined
js> var s = "x * \n" +
            "yy";
js> eval(s)
typein:9: ReferenceError: yy is not defined
js> zz
typein:9: ReferenceError: zz is not defined

Notice how eval(s) used 1 + the line number of the eval, or 9 (the next line's number) as the error number, because yy followed a newline in s.

Would it not be better to let eval users cite a line number such as line 6 (the line on which 'var s = ...' begins)?

We could make up some new filename notation, a la fake URI schemes, and let the eval source numbering start with 1 in the pseudo-file.  It may be a bit late to retrofit such a scheme into SpiderMonkey, jsd, Venkman, and people's brains.  Thoughts?

/be

Comment 14

[Brendan Eich [:brendan]]

(In reply to comment #13)
> We could make up some new filename notation, a la fake URI schemes, and let the
> eval source numbering start with 1 in the pseudo-file.

Something like x-js-eval:<eval's filename>:<eval's ending lineno>, but we don't use file: URLs in all embeddings, and : is not reserved in filenames on all (or even many still-living) OSes.  Cc'ing Silver to get his thoughts.

/be

Comment 15

[Aaron Boodman]

(In reply to comment #9)
> Could we just make the 3rd and 4th parameters to evalInSandbox an optional
> location (filename and linenumber)?

Sounds great to me. The optional line number parameter is also useful to me, because I actually wrap some boiler plate code around user script source that I'd like to exclude from the line numbers if possible.


(In reply to comment #13)
> Notice how eval(s) used 1 + the line number of the eval, or 9 (the next line's
> number) as the error number, because yy followed a newline in s.

I'm not sure I understand what you're proposing here. Wouldn't starting counting from line 6 have the same effect as we were proposing with simply starting counting from 1 on the input to eval?

Comment 16

[Brendan Eich [:brendan]]

> (In reply to comment #13)
> > Notice how eval(s) used 1 + the line number of the eval, or 9 (the next line's
> > number) as the error number, because yy followed a newline in s.
> 
> I'm not sure I understand what you're proposing here. Wouldn't starting
> counting from line 6 have the same effect as we were proposing with simply
> starting counting from 1 on the input to eval?

No, I'm saying *I*, the driver of that interactive JS shell session, or *you*, wrapping boilerplate in GM code at a known lineno, could pass the right 4th arg for the situation.  That's all.  No AI magic.

But I like the fake-filename idea more and more, on reflection.

/be

Comment 17

[Aaron Boodman]

(In reply to comment #16)
> No, I'm saying *I*, the driver of that interactive JS shell session, or *you*,
> wrapping boilerplate in GM code at a known lineno, could pass the right 4th 
> arg for the situation.  That's all.  

Oh, cool. We are in agreement then.


> But I like the fake-filename idea more and more, on reflection.

Can you explain it more? Is this just generating a filename if none is provided, or what?

Comment 18

[James Ross]

So, let's get this straight:
 - The line number has to be from the eval source itself and not affected by
   the location of the call to eval/InSandbox.
 - The filename is the problem, though, because there isn't one that can be
   sensibly worked out from the environment (or useful for viewing source).

Would it be safe to use a descriptive string instead of a URI for the sourceName? Perhaps it could even default to "evalInSandbox at scheme://path/file:line"?

I'm not so keen on the lineno adjustments, though (not to mention the inability for the user to actually view the source, unless we give a data: URI for it...). Doesn't Spidermonkey pick out special comments indicating source information? If so, that might be more useful for Greasemonkey's wrapping of user-scripts.

Comment 19

[Blake Kaplan (:mrbkap) (inactive)]

I'm missing the motivation for a new uri scheme. It seems like it's overly complicating a relatively simple problem. If we want to pack the filename/linenumber info into one parameter, we could create a new object that's basically just a pair of {'filename', lineno}.

Comment 20

[Brendan Eich [:brendan]]

(In reply to comment #18)
> So, let's get this straight:
>  - The line number has to be from the eval source itself and not affected by
>    the location of the call to eval/InSandbox.

Right.

>  - The filename is the problem, though, because there isn't one that can be
>    sensibly worked out from the environment (or useful for viewing source).

The filename of eval's caller is available, but it may not name the file from which the program source passed to eval came.  That source might even be computed from arbitrary bits, so there's no point in trying to track source data flow in general.

The real issue is the line number.  The filename of eval's caller should be enough to identify the call site.  But we need a separate method -- separate from anything to-do with the caller's filename and lineno -- to show the line within N > 1 lines of (computed, literal, or a mix) program source that eval sees.

> Would it be safe to use a descriptive string instead of a URI for the
> sourceName? Perhaps it could even default to "evalInSandbox at
> scheme://path/file:line"?

Sure, that's more readable than a fake URI scheme.  The fake URI scheme would have the advantage (see below) that it'd be view-source-able.

> I'm not so keen on the lineno adjustments, though (not to mention the inability
> for the user to actually view the source, unless we give a data: URI for
> it...).

> Doesn't Spidermonkey pick out special comments indicating source
> information? If so, that might be more useful for Greasemonkey's wrapping of
> user-scripts.

Yes, "//@line n" sets the next line's number to n.

(In reply to comment #19)
> I'm missing the motivation for a new uri scheme. It seems like it's overly
> complicating a relatively simple problem. If we want to pack the
> filename/linenumber info into one parameter, we could create a new object
> that's basically just a pair of {'filename', lineno}.

First, I'd like us to fix the same problem for ECMA-standard eval along with any other such native functions that run the JS compiler, and passing an object or other type encoding extra information doesn't address that.

Second, the fake URI idea should be view-source'able, as data:, wyciwyg: and view-source: are.  This would be the way to make the JS console show the error in context, even if the program source were computed from random number generators.

Bear with me here, it'll get simpler after it gets complicated: suppose we made a Necko protocol handler for the fake scheme that could ask the JS engine for the source that it compiled.  That source would have to be saved along with the fake URI naming it, by the JS engine.

But the JS engine already saves filenames for compilation units.  This suggests putting the eval'ed program source in the URI itself, which in turn suggests the data: idea Silver raised.  Then we don't need any Necko protocol handler.  All we need is for the JS engine and the XPConnect evalInSandbox native to generate the right data: URI and pass it into the right JS API calls instead of the eval* caller's filename.

Comments?  Are there limits on data: URIs that could hurt here?  The URI might look like data:application/javascript,... where the ... is a UTF-8 encoding of the program source, including newlines.  What am I missing?

/be

Comment 21

[Brendan Eich [:brendan]]

(In reply to comment #20)
> Comments?  Are there limits on data: URIs that could hurt here?  The URI might
> look like data:application/javascript,... where the ... is a UTF-8 encoding of
> the program source, including newlines.  What am I missing?

I left out the call site info:

data:application/javascript;called-from="<filename>:<lineno>",%s

looks like the best way to go, otherwise we'd have to hide called-from info in a comment at the end (gross), or invent a new URI scheme (undesirable).

Is there an API to recover MIME type parameters from the content-type, that the console code could use?

We wouldn't want to wire data: generation into SpiderMonkey, so we would need yet another optional (per runtime, no need to vary per context) callback.

Is this overkill?  It solves the full eval-source-citation problem as I understand it, but is there a simpler way?

/be

Comment 22

[Brendan Eich [:brendan]]

"eval-site" might be better than "called-from" -- comments welcome on this naming nit welcome too.

/be

Comment 23

[Aaron Boodman]

(In reply to comment #21)
> data:application/javascript;called-from="<filename>:<lineno>",%s

called-from is the location of the evalInSandbox() call, right? In that case, I think this idea loses the filename of the original source of the code which was eval'd. I understand that there won't always be such a source (random bits, etc), but when there is, it is probably more useful.

In other (clearer) words, where does "foo.user.js" go in this scheme?

Comment 24

[Brendan Eich [:brendan]]

(In reply to comment #23)
> (In reply to comment #21)
> > data:application/javascript;called-from="<filename>:<lineno>",%s
> 
> called-from is the location of the evalInSandbox() call, right? In that case, I
> think this idea loses the filename of the original source of the code which was
> eval'd. I understand that there won't always be such a source (random bits,
> etc), but when there is, it is probably more useful.
> 
> In other (clearer) words, where does "foo.user.js" go in this scheme?

It doesn't fit here, which may mean I should go find the old bug on line numbers in errors from eval being incorrect and reopen or resurrect it.  For what you want, mrbkap's proposal of optional 3rd and 4th args (filename and lineno) is sufficient -- except that it breaks JS console source citation again.

To solve both eval-source-citation and spoofed-eval-filename-lineno problems we need a new fake URI scheme, I think.

/be

Comment 25

[James Ross]

I don't think it would be a problem to have (over the existing evalInSandbox) a single optional parameter that got included in the generation, as maybe source="whatever" to allow a generic annotation, would it? I guess the line numbers start to go icky again...

I prefer eval-site over called-from, personally.

Comment 26

[Brendan Eich [:brendan]]

The //@line feature helps GM prepend code to user script and reset the first user script line's number to 1.  That's independent of this data: idea.

We don't need another URI scheme to spoof file and line of script, though -- we can just add a "file" content-type parameter:

data:application/javascript;eval-site="<file>:<line>";file="foo.user.js",...

The JS console implementation would get the "file" parameter and if supplied, use it in preference to eval-site's <file> substring.  But if, as with GM, there's no value in showing the eval-site's <file> anyway, because it is an implementation detail, we could eliminate eval-site in favor of file and optional line params:

data:application/javascript;file="<file>"[;line="<line>"],<source>

[] for optional stuff, <> for variables

No point in more backtrace file/line pairs jammed into MIME type parameters than necessary!  A JS backtrace from the JS console sounds like a separate RFE.

/be

Comment 27

[Boris Zbarsky [:bzbarsky]]

> Are there limits on data: URIs that could hurt here?

Not really.

> Is there an API to recover MIME type parameters from the content-type, that the
> console code could use?

Again, not really.

Comment 28

[Hixie (not reading bugmail)]

Note that in data: URIs, you should use %-encoded data for the parameter values, not "-quoted data. As in:

   data:text/javascript;file=x%2Cy.js;line=10,...       (GOOD)

...as opposed to:

   data:text/javascript;file="x,y.js";line="10",...     (BAD)

Comment 29

[Adam L. Peller]

I agree with James Ross that the bug I filed, bug 332176, is a duplicate.  However, I want to stress the severity of this in the case of a standard eval(), where the data returned for multi-line evals points the user at the wrong code and makes debugging extremely difficult.  The DojoToolkit, in particular, makes heavy use of this for their library loading and rumor has it that Microsoft's debugger handles this fine (just thought I'd razz you with that :-)

It seems that with the current behavior, the debug APIs provide the filename where the eval took place (useful information that arguably should still be provided somehow, just not here) as well as the line number within the eval'd code which seems to be offset by the line number of the eval itself.  I posted a simple example in bug 332176.

By default, it would be very nice to see "null" come back for the filename, along with the correct line number and a jsdIScript representing the eval'd script such that the source can be retrieved.

As an enhancement, being able to pass along a full or partial URL to represent the eval, which in the case of Dojo is a real URL fetched by XHR, would be a real plus.  In the case of Dojo, this would allow the debugger to correlate the script at runtime with a real resource and enable the establishment of breakpoints.  This metadata might be a third optional param to eval, or perhaps better yet a special comment syntax like the one mentioned for //@line (perhaps //@source)  The @line directive actually isn't of much interest here, at least in the context of an eval() in a webpage; the eval source should be assumed to start at line 1.  I'm not sure I followed the data: URI discussion, in particular whether it would be applied to a running application or if it's just an implementation detail.

Greasemonkey and Dojo users would rejoice if a solution were found to this problem.

Comment 30

[Brendan Eich [:brendan]]

Boris rightly points out that bug 332176 is not a dup of this one.  They're twins but not dups, and we can't fix the genome once now that fission has occurred ;-). So, two bugs.  Noting dependency.

/be

Comment 31

[John J. Barton]

Using the current jsd interface I found a way to debug eval() code (bug 332176). I wrote up the details:

http://www.almaden.ibm.com/cs/people/bartonjj/fireclipse/test/DynLoadTest/WebContent/DynamicJavascriptErrors.htm 

In my prototype implementation I used the data: uri scheme outlined here.  On the plus side, view-source from Javascript Console (error messages) just worked: errors are reported on the eval() buffer just like it was an http url.

On the minus side is size. The motivating examples are dojo code: these URIs are long, 20k say. This makes them annoying slow to print for debugging and they are not very useful un-decomposed in the UI. That's not too big of a problem. However, in my implementation I use them like this:
  var url = this.context.evalSourceURLByTag[frame.script.tag];
  this.executionFile = this.context.evalSourceFilesByURL[url]; 
where the 20k string is being used to index into the object. I don't have enough understanding of Javascript to tell if this is Truly Horrible or Just Fine.

Comment 32

[Justin Dolske [:Dolske]]

Confirmed, still exists with FF3 nightly and FB 1.2 tip.

Comment 33

[Brendan Eich [:brendan]]

(In reply to comment #31)
> In my prototype implementation I used the data: uri scheme outlined here.  On
> the plus side, view-source from Javascript Console (error messages) just
> worked: errors are reported on the eval() buffer just like it was an http url.

Cool -- as hoped for.

> On the minus side is size. The motivating examples are dojo code: these URIs
> are long, 20k say. This makes them annoying slow to print for debugging and
> they are not very useful un-decomposed in the UI. That's not too big of a
> problem.

You could work harder to pretty-print strings, elide overlong suffixes with options to expose more, etc. GDB is hardly great here, but I get by with such options.

> However, in my implementation I use them like this:
>   var url = this.context.evalSourceURLByTag[frame.script.tag];
>   this.executionFile = this.context.evalSourceFilesByURL[url]; 
> where the 20k string is being used to index into the object. I don't have
> enough understanding of Javascript to tell if this is Truly Horrible or Just
> Fine.

Should be fine, even better than fine in Firefox 3, which flags interned JS string descriptors so that their chars don't need to be re-hashed when used as computed property names.

/be

Comment 34

[John J. Barton]

(In reply to comment #33)
> (In reply to comment #31)
> > However, in my implementation I use them like this:
> >   var url = this.context.evalSourceURLByTag[frame.script.tag];
> >   this.executionFile = this.context.evalSourceFilesByURL[url]; 
> > where the 20k string is being used to index into the object. I don't have
> > enough understanding of Javascript to tell if this is Truly Horrible or Just
> > Fine.
> 
> Should be fine, even better than fine in Firefox 3, which flags interned JS
> string descriptors so that their chars don't need to be re-hashed when used as
> computed property names.
>
In the meantime I rewrote all of the code around eval in Firebug and replaced data url by MD5 for uniqueness tests.  After we tune the performance we could go back to data URL to see if it really makes any difference.

Comment 35

[Rob Campbell [:rc] (:robcee)]

is this no longer needed for Firebug? It seems like this would still be useful for SpiderMonkey to have. Is the URI scheme proposed in C#14 (and refined in C#26) still make sense?

Were the extra parameters ever added to evalInSandbox mentioned in C#9?

Comment 36

[Dave Webber]

(In reply to comment #35) 
> Were the extra parameters ever added to evalInSandbox mentioned in C#9?

Yes, by bug 445873, except they are optional 4th and 5th parameters (counting from 1), instead of 3rd and 4th; they follow a new optional 3rd parameter for JS version. I'm not sure about the extra parameters for the data: URI mentioned in comment 26.

Comment 37

[John J. Barton]

Ironically these extra parameters are now what prevent jetpack modules from being debuggable! jetpack's jsdIScript.fileName values are aren't urls or filenames :-(.

Before eval() was bogus in a predictable way.  Now if the dev sends bogus info there is no fix.  

The eval() arg should be an object that supports compiler and debugging calls, generated by a factory for simple cases.

Comment 38

[BMO Automation]

Per policy at https://wiki.mozilla.org/Bug_Triage/Projects/Bug_Handling/Bug_Husbandry#Inactive_Bugs. If this bug is not an enhancement request or a bug not present in a supported release of Firefox, then it may be reopened.