332176 - eval still uses call site line number as offset for eval'ed code in the year 2014

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P3)

Description

[Adam L. Peller]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.12) Gecko/20050915 Firefox/1.0.7
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.12) Gecko/20050915 Firefox/1.0.7

If code is run through an eval() which defines functions, then those functions are  called up in the debugger, such as at a breakpoint in a stack frame, the code reference is wrong.  It will point to the file in which the eval took place, and the line number will be the line number of the eval, offset by the line position within the contents of the eval string.  See the attached example.

This is a problem with the dojo toolkit, which relies on XHR + eval to bootstrap its libraries.  Workarounds are available, but they deviate from the actual code path and only work under certain circumstances.  The Microsoft JS debugger apparently is able to bring the eval references up properly.

While this bug can be demonstrated in Venkman, I believe it is really a problem in the jsd.  I am trying to write a debugger directly to the jsd APIs and have the same problem.


Reproducible: Always

Steps to Reproduce:
1. start up venkman
2. load evaltest.html
3. observe the first breakpoint.  Notice that the first breakpoint is at line 26  (eval is line 24 + offset of 2 in the "foo" function script to the debugger statement)  This has nothing to do with the line of code at line 26.
4. continue and watch the second breakpoint at line 27.  Again, this is at line 24 + an offset of 3 in the "bar" function script.  There isn't even any code at line 27. 
etc.




Expected Results:  
I guess it would be best to get a null back for the filename and have to look at the script object to get the actual String representing the executing code, and the line should be relative to that String.  Then, Venkman and other debuggers could display the proper code and offset.

Comment 1

[Adam L. Peller]

Attachment: JS example which defines functions in eval() and causes breakpoints in the debugger

Comment 2

[James Ross]

This is more likely to be a Spidermonkey issue than JSD, as that is where all the stack and line info is from. See bug 307984, which is exactly the same issue, and to which this is probably just a DUP.

Comment 3

[Adam L. Peller]

Yep, thanks for the pointer, and I'd be okay with that, except I think this is a fairly important issue, especially with AJAX toolkits and vanilla "eval()".  I'd argue that the default behavior ought to be line counts beginning with '1' in the eval source, and a filename of null or something specified by metadata (a 3rd argument for file on eval() would be fine, but I don't see the need for a 4th)  I didn't totally follow the data: URI or how it would be used.

Comment 4

[James Ross]

Don't worry, I don't follow half the suggestions on bug 307984 either!

I'm not sure there is anything either Venkman or JSD can do themselves to help the situation though, which is why I suggested marking it a duplicate of the Spidermonkey bug.

For historical amusement, the stack is show as (for foo):
  |> foo            attachment.cgi, line 26, pc 67
     __toplevel__   attachment.cgi, line 26, pc 67

For bar:
  |> bar            attachment.cgi, line 27, pc 5
     __toplevel__   attachment.cgi, line 28, pc 75

For baz:
  |> baz            attachment.cgi, line 28, pc 14
     __toplevel__   attachment.cgi, line 30, pc 83

The function calls are on lines 26, 28 and 30, and the eval is on 24.

It's clear that it is the PC to line map that's causing the mess (the PC values look fine for their position within the eval strings). As was mentioned (I think) in bug 307984 there's never going to be a way to identify the string source itself, but I guess it should at least always point at the eval line.

Comment 5

[Adam L. Peller]

Strange.  The pc looks fine except for 'foo'?

Yes, I agree, it's mostly or entirely a SpiderMonkey issue, especially wrt the line number, which I think is just off by a fixed offset, the line position of the eval.  Hopefully that part is easy to fix.  I would hope that the JSD would provide enough information for me to know this is an eval and the file is different than where it's eval'd. (Although the information about where the eval took place is also useful and might be nice to pass through a debugging API, I don't think it should be the primary debugging reference)  I'd hope that the JSD would make the eval contents available as a jsdIScript such that I could grab the contents as a string, of course with the help of SpiderMonkey.

I'll try to put together a comment for the other bug and hopefully get the conversation moving again.

Comment 6

[Boris Zbarsky [:bzbarsky]]

This is not a duplicate of bug 307984 -- the two entry points into the JS engine are totally different, and they would need to be fixed separately, with rather different fixes.

Comment 7

[James Ross]

(In reply to comment #5)
> Strange.  The pc looks fine except for 'foo'?

That was a typo, the pc was 0 for the top stack item IIRC.

Comment 8

[James Ross]

Boris, they may enter via slightly different routes, but they still both end up in exactly the same place - JS_CompileUCScriptForPrincipals. Also, even though the code that sets the start line number is outside this function, it is actually esentially the same code in both routes and should be fixed the same way IMHO.

Anyway, this is a bug in the eval function, so moving to Spidermonkey.

Comment 9

[Brendan Eich [:brendan]]

This seems like a very low-numbered bug, possibly WONTFIXed.  I'll try to dig it up later today.

No matter what happened in the old days, we should fix this bug.  Just using the line number of the eval call for all lines in the eval'ed program source is not ideal for diagnostics, but better for debuggers.

In some bug, I had proposed a source URI scheme that could encode both the eval-call-line-number and 1-based-line-number-within-the-eval'ed-program-source.  Does this seem worth pursuing?

/be

Comment 10

[James Ross]

Some bug is probably bug 307984 comment 14. :)

From a debugging point of view, the stack item for the code being eval'ed would have the line number of the eval'ed source (not offset) and a fake filename (possibly just 'eval'). That would make the stack sane, at least (as the 2nd item will point at the eval call site). We'd need nsIConsoleService2 to make error reporting sane for it in the browser, however.

Comment 11

[James Ross]

Actually, I take that nsIConsoleService2 line back - it would, I think, be possible to report the error as occuring at stack position 2's location if stack position 1 is 'eval', without affecting the stack itself.

Comment 12

[Adam L. Peller]

Rather than a fake filename, from the debugger standpoint anyways, I'd rather see something unambiguous like a null, or perhaps Brendan's URI scheme, so that I can identify it as an eval.  It's important that the source of the evaluated script can optionally be specified somehow in metadata, perhaps in a comment scheme (//@source-reference:...) such that a debugger could tie in the right resources and make it possible to set breakpoints, etc.  If the location and line number of the eval were encoded in there as well, I suppose that's a bonus.  It's not relevant in a stack trace, but is certainly good debugging info.

Would it be possible to get to the actual scripting object (jsdIScript) from the stack frame as well, such that I could get the source as a string?

Comment 13

[Blake Kaplan (:mrbkap) (inactive)]

->me so I don't lose this again.

Comment 14

[John J. Barton]

Attachment: Fireclipse/Firebug screeenshot on Adam Peller's example

Fireclipse/Firebug:
http://www.almaden.ibm.com/u/bartonjj/fireclipse/index.html

Comment 15

[John J. Barton]

I attached a screenshot of Adam's eval() test code running under my modified Firebug.

Comment 16

[timeless]

adam: there is no distinction between a fake filename and a fake url scheme, the js api only has one field, it's the "name" of the thing that contains the script. in gecko, that's basically a url. in some other consumer it could probably be a file path.

from my perspective, such a thing would be ideal, however, i wonder how best to control the naming. perhaps it doesn't matter. as long as js tells jsd that a new thing is being compiled and jsd can dig up the stack trace to recognize that it's from an eval.

Comment 17

[John J. Barton]

This is just an update about how Firebug 1.2 does eval() debugging.

Firebug 1.1 implemented the double hook scheme I outlined in the ref on comment
14. In addition to being very hacky, 1.1 could be extremely slow. 

For 1.2 I did some performance tests, mostly around different ways of 'naming'
an eval buffer so one can determine that a new eval() is the same as an old
eval() that a user has set a breakpoint in. Firebug 1.1 used data URLs, which
have some advantages, but then some wise guys started with their 40,000 lines
of Javascript, which makes the data URL extremely long.  Even if we decide the
data url compute time is tolerable, storing the URL-encoded source solely in
case we need to compare it for equality seems wasteful. (Memory consumption was
an issue for Firebug 1.1).

After some trials I ended up selecting MD5 hash for the equality comparison.
Its certain, compact, and available in javascript. Computing the hash is still
a significant contributor to Firebug performance issues, and if we implement
other kinds of fixes to jsd, then the hash will rise to the top.

(The performance analysis also convinced me that Firebug's rendering was a
large part of the problem, one only connected to eval() by coincidence: more
Ajax, larger Javascript, and more eval()s now than a few years ago).

We have a lot of experience with eval debugging now in Firebug. We could decide
to push the hash computation into jsd, but it would not be the highest priority
unless we know that the computation can be a lot faster somehow.

Comment 18

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

MD5 is a heavy weapon for this purpose; would a much-cheaper CRC32 perhaps suffice?  MD5 in C++ might be faster than CRC32 in JS -- for now! -- but if we put the CRC32 in C++ inside jsd you would likely win pretty big.

Even CRC32 is a big hammer, though: maybe simply a string hash function as may be exposed by |hashCode| in future JS versions -- you can chain to handle collisions, right?

Comment 19

[John J. Barton]

You are right. MD5 was selected for one very simple reason: nsICryptoHash exists. 

Considering CRC32. Is this right: If we have 2^12 evals() and 2^32 bits, we have 1 in 2^20 probability of collision, or one in a million. Well, two in a million since if we hash to the correct match its a good thing, only if hash to the wrong one do we lose. If all 7 million Firebug users are debugging apps with 4k evals we get four failures a day. Pretty extreme numbers so I think CRC32 is fine.

No, I don't think we can handle collisions because I am trying to find a match between a long string I have (the new eval) and one I do not (the previous eval). Even if I keep the previous eval I don't want to handle collisions because that will be the common case (every time I want a match) so I'll end up in the slow path any way.

Comment 20

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

since Firebug's eval handling has changed over the last couple of versions, is this still a problem?

Comment 21

[Adam L. Peller]

Rob, Firebug works around this problem, but it still exists in the jsd and surfaces in Firefox, for example when exceptions appear directly in the console.

Comment 22

[John J. Barton]

Adam, Bug 449464 would solve this problem and all similar ones like new Function, browser generated functions, HTML inlined Javascript, sandbox evaled javascript, and document.write javascript.

Comment 24

[Jason Orendorff [:jorendorff]]

I'm interested in going after this.

The specific change I'd like to make is to change the filename/URL of eval scripts to "(eval code)" and change the line numbers to start at 1, at least by default. This would affect both the debugger APIs and the .stack attribute of Error objects. It would affect eval, Function, and evalInSandbox.

This seems like a very easy change to make. Any objections? Firebug works around the weird existing behavior. Would fixing the bug break Firebug's workaround in horrible ways?

Comment 25

[John J. Barton]

I guess that the line numbering change will break Firebug. If the script.fileName is not XUL and if the script.functionName is not blank and if the script.baseLineNumber == 1, then Firebug guesses that the script is a browser generated event handler or some kind of code generated from a .xml file. It sets a breakpoint on PC=0 and when the breakpoint hits Firebug looks to see if there is a caller. If not, then it guessed right; else it treats the script as an eval(). (As you can see this is all ridiculous).

I wonder if viewsource or other tools will be confused by many identical buffers names "(eval code)"; also lots of code related to script.fileName assumes the value is a URL.  Maybe a data URL that encodes "eval from <callerURL>@<line>;<serial>" where serial starts at 1 and increase for each data URL would be better? Maybe some eval cases would just work then, if the order of eval happens to be constant.

If the script.fileName is changed in a way that cannot be confused with any existing fileName, then we can change Firebug to look for the special names.

Also note that Bug 449464 would remove all of the funky code in Firebug so then this kind of change could be done easily.

Comment 26

[Jason Orendorff [:jorendorff]]

(In reply to comment #25)

Thanks for the quick reply.

> I wonder if viewsource or other tools will be confused by many identical
> buffers names "(eval code)"; also lots of code related to script.fileName
> assumes the value is a URL.

Adding a serial number and the original calling url/line seems like a good idea.

The thing about URLs is a downer though. The data URL idea sounds quite bogus. Hmm.

Comment 27

[Jason Orendorff [:jorendorff]]

(In reply to comment #25)
> If the script.fileName is changed in a way that cannot be confused with any
> existing fileName, then we can change Firebug to look for the special names.

This sounds better than making data urls. I'll give it a try.

Comment 28

[James Ross]

How about e.g. "x-eval-code:<serial number>?file=<file>&line=<line>"?

Comment 29

[Jason Orendorff [:jorendorff]]

Attachment: WIP 1 - line numbers only

This isn't complete. There are 9 places where I need to change the
filename as well as the line number.

I adjusted places where these APIs were being called passing line number
information from the JS caller:

JS_Evaluate*, JSD_Evaluate*, jsd_Evaluate*, EvaluateString,
EvaluateStringWithValue, JS_Compile*, CompileScript,
CompileEventHandler, CompileFunction, nsXULPrototypeScript::Compile.

I also changed places where the line number was hard-coded to 0. This
is always a mistake. It should be 1.

A few places pass the line number where a bit of script appears within a
larger file (not the caller's line number). This is correct; that's what
the parameter is actually for. So I left those places alone. They are:
nsScriptLoader.cpp, content/xbl/src/nsXBLProtoImpl*.cpp, nsXULContentSink,
and the places where CompileEventHandler is called.

Running the JS test suite now to see if we have any tests that want the old behavior. :)

Comment 30

[timeless]

Attachment: updated to compile and cover bug 566446

the nsScriptLoader initializes its line number to 0 and there isn't necessarily a caller which changes it, hence the extra code to pass 1

Comment 31

[rauldmiller]

(In reply to comment #19)
> Considering CRC32. Is this right: If we have 2^12 evals() and 2^32 bits, we
> have 1 in 2^20 probability of collision, or one in a million.

I doubt this is still relevant, but just in case: if you have 4096 distinct evals in your debug session, you then have 8386560 distinct pairs of evals, and your odds of collision would be at least 1-(1-2^-32)^8386560 or something like 0.2% on one page.

See also http://en.wikipedia.org/wiki/Combination

Comment 32

[John J. Barton]

I don't think we need the patch from comment 30 to fix this bug, but it may solve the very important problem on bug 566446

Comment 33

[David Flanagan [:djf]]

Attachment: simple patch that partially solves the problem

This is a simple patch that I proposed for Bug 667514 (which I've now marked as a duplicate of this one).  It defines a fake filename for the text passed to eval() and Function(). It doesn't address the issue of distinct strings being passed to eval() from the same line, however.

Comment 35

[Patrick Walton (:pcwalton)]

Should we get this reviewed? Seems like a nice stopgap fix to me.

Comment 36

[David Flanagan [:djf]]

Attachment: updated patch

modified patch so it applies cleanly and changed one %d to %u

Comment 37

[David Flanagan [:djf]]

Attachment: testcase that shows what this patch does

Comment 38

[David Flanagan [:djf]]

(In reply to Patrick Walton (:pcwalton) from comment #35)
> Should we get this reviewed? Seems like a nice stopgap fix to me.

Patrick: I've updated the patch, and I've added a test case.
Have you read the comments in bug 667514 where I originally submitted this patch? I have concerns about allocating a big static buffer of text on the stack. Another concern was about not wanting to change this too frequently if a better fix (sourcemap related?) is in the pipeline.

Do you still think I should try to get this checked in?

Comment 39

[David Flanagan [:djf]]

Attachment: updated patch

Yet another version of the patch. This time with hg queue goodness.

Comment 40

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Just had a conversation on IRC with :jimb, :jorendorff, and :djvj.

To summarize:

* The script created from the call to eval should have a 0 offset

* But we still need to store the eval callsite's offset somewhere so we can expose it on Debugger.Source#introductionOffset

* We should maintain the filename ":" lineNumber syntax in stack traces. :jimb proposed this format for filenames in stack traces:

    foo.js line 34 > eval line 19 > eval:4

* :djvj agreed to take this bug. (Thanks!)

Comment 41

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

(In reply to Nick Fitzgerald [:fitzgen] from comment #40)
> * But we still need to store the eval callsite's offset somewhere so we can
> expose it on Debugger.Source#introductionOffset

"somewhere" being one of ScriptSource or ScriptSourceObject

Comment 42

[Kannan Vijayan [:djvj]]

So the patch as described is done.  Will post it shortly.. but I ran into some behaviour which is similar to the behaviour we're trying to fix in this bug.  I stumbled across it when implementing the pcOffset forwarding for |introductionOffset| support.

Turns out that the same concept applies to ScriptSources introduced by |new Function("blah")| as well as,
strangely.. |new (function *(){}.constructor)("blah")|.   (Thanks to :jorendorff for that second one).

Both of these introduce new ScriptSources logically associated with a bytecodeOffset in a caller script, similar to eval.  Do we want to adopt a similar syntactic convention for this?

foo.js:
  // Line 1
  new Function("throw new Error()"); // Line 2

The ScriptSource for the function's script would have a filename like:
  "foo.js line 2 > new Function()"

Or if using generator's string constructor:
  "foo.js line 2 > new GeneratorFunction()"


Do we want to do this?

Comment 43

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

(In reply to Kannan Vijayan [:djvj] from comment #42)
> So the patch as described is done.  Will post it shortly.. but I ran into
> some behaviour which is similar to the behaviour we're trying to fix in this
> bug.  I stumbled across it when implementing the pcOffset forwarding for
> |introductionOffset| support.
> 
> Turns out that the same concept applies to ScriptSources introduced by |new
> Function("blah")| as well as,
> strangely.. |new (function *(){}.constructor)("blah")|.   (Thanks to
> :jorendorff for that second one).
> 
> Both of these introduce new ScriptSources logically associated with a
> bytecodeOffset in a caller script, similar to eval.  Do we want to adopt a
> similar syntactic convention for this?
> 
> foo.js:
>   // Line 1
>   new Function("throw new Error()"); // Line 2
> 
> The ScriptSource for the function's script would have a filename like:
>   "foo.js line 2 > new Function()"
> 
> Or if using generator's string constructor:
>   "foo.js line 2 > new GeneratorFunction()"
> 
> 
> Do we want to do this?

Yes, I believe we do.

Comment 44

[Kannan Vijayan [:djvj]]

Attachment: fix-eval-filenames.patch

Does what's described by :fitzgen in comment 40.  Green on try: https://tbpl.mozilla.org/?tree=Try&rev=545503be8cbb

Doesn't implement added behaviour suggested in comment 42.  Holding off on review request until that question has been resolved.

Comment 45

[Jim Blandy :jimb]

(In reply to Kannan Vijayan [:djvj] from comment #42)
> Do we want to do this?

Oh, I love it.

Comment 46

[Kannan Vijayan [:djvj]]

Attachment: fix-eval-filenames.patch

Update patch.  Seems like previous patch didn't quite make it through try fully green.  I think it may be because the new filename code was a bit too broad and captured debugger.eval(...) scripts as well, which should not have been happening.

New patch fixes the filename behaviour modification on debugger.eval(), cleans up and refactors some of the code, and adds support for "new Function()" and "new GeneratorFunction".

Comment 47

[Kannan Vijayan [:djvj]]

Waiting on new try: https://tbpl.mozilla.org/?tree=Try&rev=d18095708de1

Comment 48

[Kannan Vijayan [:djvj]]

The actual try results (after build fix): https://tbpl.mozilla.org/?tree=Try&rev=28086a0d7dac

There are some browser mochitest failures and some XPCShell test failures.  The problem is with different behaviour (now that eval-ed code presents a different source, the tests do not match up).

I started going through and fixing these.. got through a few of the mochitests today.  Patch is still good for review while I fix up the tests.

Comment 49

[Kannan Vijayan [:djvj]]

Attachment: fix-eval-filenames.patch

Updated patch which fixes earlier build failure and some mochitest failures.

Comment 50

[Jim Blandy :jimb]

Comment on attachment 826965 [details] [diff] [review]
fix-eval-filenames.patch

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

Still reviewing; some thoughts as I read:

::: js/src/jsapi.h
@@ +3389,5 @@
>  
> +    enum Introducer {
> +        EVAL,
> +        NEW_FUNCTION,
> +        NEW_GENERATOR

The introduction function is going to be a pretty open-ended set; for example, Firefox is going to have the Worker constructor and the worker importScripts function, and probably a bunch of other random things.

Would it make sense to represent the Introducer simply as a 'const char *'? (Or I guess we can use u"foo" now, and get 'const jschar *'?) Then these would be "eval", "Function", and "GeneratorFunction" (right???) I think it'll be fine for the forseeable future for CompileOptions's API to simply require that it be a statically allocated string, to keep allocation simple.

::: js/src/jsscript.cpp
@@ +1365,5 @@
> +    // Len = strlen(callerFilename) + strlen(" line ") +
> +    //       strlen(toStr(callerLineno)) + strlen(" > ") + strlen(introducer);
> +    char linenoBuf[15];
> +    size_t filenameLen = strlen(callerFilename);
> +    size_t linenoLen = snprintf(linenoBuf, 15, "%u", callerLineno);

It seems like the behavior C99 specifies for snprintf (return the number of characters that *would* have been written, regardless of whether they actually fit in the buffer) would have been really useful here, but MSVC docs say its snprintf doesn't behave that way. Too bad.

@@ +1376,5 @@
> +                 1 /* \0 */;
> +    filename_ = cx->pod_malloc<char>(len);
> +    if (!filename_)
> +        return false;
> +    snprintf(filename_, len, "%s line %s > %s", callerFilename, linenoBuf, introducer);

Could we assert that this snprintf produces the expected number of characters?

Eventually, the filenames ought to become jschars instead of chars; when that happens, we could use StringBuffer here. In the mean time, this code is ugly, but it's localized ugliness, and I don't know of a better way, so we'll go with it.

Comment 51

[Kannan Vijayan [:djvj]]

Attachment: eval-lineno-fix-1.patch

Full fix for eval filenames.  This change preserves |getURL| so that it returns the original filename of the script (not the modified "filename line X > eval" string, so that debugger mochitests can still largely pass).

Comment 52

[Kannan Vijayan [:djvj]]

Attachment: eval-lineno-fix-2-jittests.patch

jit-tests fixes.

Comment 53

[Kannan Vijayan [:djvj]]

Attachment: eval-lineno-fix-3-mochitests.patch

Mochitests-bc fixes

Comment 54

[Kannan Vijayan [:djvj]]

Green try run for mochitests-bc: https://tbpl.mozilla.org/?tree=Try&rev=53aefeba5bbc

Comment 55

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

Kannan, are you still waiting for review on this?

jimb, ping?

Comment 56

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

(are these patches for jsd1?)

Comment 57

[Jim Blandy :jimb]

I'm getting back to reviewing this. Should be today or tomorrow.

Comment 58

[Jim Blandy :jimb]

(In reply to Rob Campbell [:rc] (:robcee) from comment #56)
> (are these patches for jsd1?)

No, they affect error reporting throughout SpiderMonkey, and make more accurate information available to Debugger as well --- although, if I'm reading correctly, the patch doesn't change Debugger's behavior, to avoid breaking lots of tests.

Comment 59

[Jim Blandy :jimb]

So there'll be Debugger followup work necessary.

Comment 60

[Jim Blandy :jimb]

I spent today looking this over; I have some comments, but I'm really aiming to just finish the review tomorrow, so I'm not going to post them yet.

Comment 61

[Jim Blandy :jimb]

Comment on attachment 8343080 [details] [diff] [review]
eval-lineno-fix-1.patch

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

Hi, Kannan. Here are some initial comments. I want to finish this review this afternoon, and post final comments.

::: js/src/builtin/Eval.cpp
@@ +312,5 @@
>                                                              : NOT_CALLED_FROM_JSOP_EVAL);
>  
> +        const char *introducerFilename = filename;
> +        if (callerScript && callerScript->scriptSource()->introducerFilename())
> +            introducerFilename = callerScript->scriptSource()->introducerFilename();

We discussed this on IRC:

It was confusing to me that we call CurrentScriptFileLineOrigin to determine where we're being called from --- but then use callerScript instead of script.

We agreed that (excluding surprising cases like self-hosted code calling direct eval) callerScript and script would always be the same. So I think we should just use 'script' here, so that CurrentScriptFileLineOrigin is the single authoritative source for this data.

@@ +403,5 @@
>                 .setNoScriptRval(false)
>                 .setPrincipals(principals)
> +               .setOriginPrincipals(originPrincipals)
> +               .setIntroductionInfo(introducerFilename, "eval", lineno, pcOffset,
> +                                    cumulativeLine);

Could this and the similar code in EvalKernel and FunctionConstructor be abstracted out into their own function, to which the CompileOptions gets passed, instead of repeating the logic?

::: js/src/jsscript.cpp
@@ +1289,5 @@
>  ScriptSource::destroy()
>  {
>      JS_ASSERT(ready());
>      adjustDataSize(0);
> +    if (introducerFilename_ && introducerFilename_ != filename_)

Since it's okay to pass nullptr to js_free, this could simply be:

if (introducerFilename_ != filename_)

@@ +1453,5 @@
> +        size_t introducerLen = strlen(introducerFilename) + 1;
> +        introducerFilename_ = cx->pod_malloc<char>(introducerLen);
> +        if (!introducerFilename_)
> +            return false;
> +        js_memcpy(introducerFilename_, introducerFilename, introducerLen);

I think this could be a call to js_strdup(ExclusiveContext *, const char *).

@@ +1472,5 @@
> +                 1 /* \0 */;
> +    filename_ = cx->pod_malloc<char>(len);
> +    if (!filename_) {
> +        if (introducerFilename_)
> +            js_free(introducerFilename_);

If we take this error exit, and introducerFilename_ is non-null, then we're left with a ScriptSource pointing to a freed string, and ScriptSource::destroy will be unhappy.

We could zero introducerFilename_ here, but it seems just as good to simply omit the whole "if...js_free..." entirely and let ScriptSource::destroy clean it up.

::: js/src/jsscript.h
@@ +345,5 @@
>      } data;
>      uint32_t refs;
>      uint32_t length_;
>      uint32_t compressedLength_;
> +    char *introducerFilename_;

It would be clearer if introducerFilename_ and cumulativeIntroductionLine_ were grouped together, with a common comment explaining that they're only needed to preserve the old "attribute eval'ed code to call site" behavior, for Debugger's sake.

We should also document that it can alias filename_.

@@ +355,5 @@
> +    // bytecode offset in caller script that generated this code.
> +    // This is present for eval-ed code, as well as "new Function(...)"-introduced
> +    // scripts.
> +    uint32_t introductionOffset_;
> +    uint32_t cumulativeIntroductionLine_;

I may be just misreading the patch --- but I can't figure out where this cumulative introduction line value is ever actually *used*. I mean, we do propagate the data from introducer to introducee, but it never comes out and gets used anywhere. Could we just drop it?

::: js/src/vm/Debugger.cpp
@@ +2585,5 @@
> +                strcmp(script->scriptSource()->introducerFilename(), urlCString.ptr()) == 0)
> +            {
> +                gotSourceURL = true;
> +            }
> +            if (!gotFilename && !gotSourceURL)

Isn't this the same as:

  if ((script->filename() &&
       strcmp(script->filename(), urlCString.ptr()) == 0) ||
      (script->scriptSource()->introducerFilename() &&
       strcmp(script->scriptSource()->introducerFilename(), urlCString.ptr()) == 0))
    return;

I find the temporary variables confusing, at least partially because they suggest that something more complicated than a normal pile-of-&&-and-|| condition.

Comment 62

[Kannan Vijayan [:djvj]]

Most comments addressed.  Feedback on others:

(In reply to Jim Blandy :jimb from comment #61)
> Comment on attachment 8343080 [details] [diff] [review]
> @@ +403,5 @@
> >                 .setNoScriptRval(false)
> >                 .setPrincipals(principals)
> > +               .setOriginPrincipals(originPrincipals)
> > +               .setIntroductionInfo(introducerFilename, "eval", lineno, pcOffset,
> > +                                    cumulativeLine);
> 
> Could this and the similar code in EvalKernel and FunctionConstructor be
> abstracted out into their own function, to which the CompileOptions gets
> passed, instead of repeating the logic?

I don't see much value in doing this.  The abstracted function will just have a number of different arguments, which will all be passed in at both callsites, so instead of the above, we'll instead have:
    set(/* noScriptRval = */ false,
        /* principals = */ principals,
        /* originPrincipals = */ originPrincipals,
        /* introducerFilename = */ introducerFilname,
        ...)

The only advantage is that required parameters will be more strictly checked if we refactor this call into its own function, but I am not sure how significant that advantage is.  Factoring this call out to a function may add cruft instead of removing it.

> @@ +355,5 @@
> > +    // bytecode offset in caller script that generated this code.
> > +    // This is present for eval-ed code, as well as "new Function(...)"-introduced
> > +    // scripts.
> > +    uint32_t introductionOffset_;
> > +    uint32_t cumulativeIntroductionLine_;
> 
> I may be just misreading the patch --- but I can't figure out where this
> cumulative introduction line value is ever actually *used*. I mean, we do
> propagate the data from introducer to introducee, but it never comes out and
> gets used anywhere. Could we just drop it?

I think you're right.  I'll confirm that it's stale and drop it.

> 
> ::: js/src/vm/Debugger.cpp
> @@ +2585,5 @@
> > +                strcmp(script->scriptSource()->introducerFilename(), urlCString.ptr()) == 0)
> > +            {
> > +                gotSourceURL = true;
> > +            }
> > +            if (!gotFilename && !gotSourceURL)
> 
> Isn't this the same as:
> 
>   if ((script->filename() &&
>        strcmp(script->filename(), urlCString.ptr()) == 0) ||
>       (script->scriptSource()->introducerFilename() &&
>        strcmp(script->scriptSource()->introducerFilename(),
> urlCString.ptr()) == 0))
>     return;
> 
> I find the temporary variables confusing, at least partially because they
> suggest that something more complicated than a normal pile-of-&&-and-||
> condition.

I'm partial to the existing variant.  The last conditional simply describes itself (if we got neither a filname or a sourceURL, then return).  The bool variables serve to label each of the previous conditionals that set them to true.

The &&/|| construction of all conditions as a single expression doesn't seem to convey the reasoning behind the logic very well.

Comment 63

[Jim Blandy :jimb]

Okay. The 'abstract this out' and 'pile-of-&&-and-||' comments are certainly open to interpretation.

Comment 64

[Jim Blandy :jimb]

Comment on attachment 8343080 [details] [diff] [review]
eval-lineno-fix-1.patch

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

Some further comments. Still reviewing.

::: js/src/jsapi.h
@@ +3476,5 @@
> +    const char *introducer;
> +    unsigned introductionLineno;
> +    uint32_t introductionOffset;
> +    uint32_t cumulativeIntroductionLine;
> +    bool hasIntroductionInfo;

These new ReadOnlyCompileOptions members need to be handled as the rest are in the list here:
https://hg.mozilla.org/integration/mozilla-inbound/file/1e9ac0371578/js/src/jsapi.cpp#l4334

@@ +3552,5 @@
> +    OwningCompileOptions &setIntroductionInfo(const char *introducerFn, const char *intro,
> +                                              unsigned line, uint32_t offset,
> +                                              uint32_t cumulativeLine)
> +    {
> +        introducerFilename_ = introducerFn;

The idea of OwningCompileOptions is that its lifetime doesn't have any necessary relationship with that of the strings, etc. that you pass to its setters. This supports off-thread compilation and some other random cases. See: https://hg.mozilla.org/integration/mozilla-inbound/file/1e9ac0371578/js/src/jsapi.h#l3481

This is why, for example, OwningCompileOptions has a bunch of fallible setters: they need to make copies of their arguments, and we might OOM.

So you're going to need a setter for 'introducerFilename' like this:
https://hg.mozilla.org/integration/mozilla-inbound/file/1e9ac0371578/js/src/jsapi.cpp#l4391

And you'll need to extend the copy function here:
https://hg.mozilla.org/integration/mozilla-inbound/file/1e9ac0371578/js/src/jsapi.cpp#l4386

And you'll need to extend the destructor here:
https://hg.mozilla.org/integration/mozilla-inbound/file/1e9ac0371578/js/src/jsapi.cpp#l4374

If you document the restriction that 'introducer' must be a statically allocated string, then I think it's okay to leave that case simple.

Comment 65

[Jim Blandy :jimb]

Comment on attachment 8343080 [details] [diff] [review]
eval-lineno-fix-1.patch

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

If you have a rebased version of the patch, could you attach it to the bug? I'd like to try to write test cases that show where I think this is not behaving correctly.

::: js/src/builtin/Eval.cpp
@@ +312,5 @@
>                                                              : NOT_CALLED_FROM_JSOP_EVAL);
>  
> +        const char *introducerFilename = filename;
> +        if (callerScript && callerScript->scriptSource()->introducerFilename())
> +            introducerFilename = callerScript->scriptSource()->introducerFilename();

I'm still confused...

We only retain ScriptSource::introducerFilename_ because, for Debugger's sake, we want to continue to claim that eval'd code's URL is that of its introducer. But doesn't that mean that introducerFilename should *always* be, simply, 'filename' here? Why would we want to reach out to the introducer's introducer --- isn't that what this does?

::: js/src/jsapi.h
@@ +3618,5 @@
>      CompileOptions &setCanLazilyParse(bool clp) { canLazilyParse = clp; return *this; }
>      CompileOptions &setSourcePolicy(SourcePolicy sp) { sourcePolicy = sp; return *this; }
> +    CompileOptions &setIntroductionInfo(const char *introducerFn, const char *intro,
> +                                        unsigned line, uint32_t offset,
> +                                        uint32_t cumulativeLine)

Note that plain 'CompileOptions' is specified to only refer to resources owned elsewhere --- so this setter is okay.

Comment 66

[Jim Blandy :jimb]

Comment on attachment 8343080 [details] [diff] [review]
eval-lineno-fix-1.patch

Since there are several revisions needed to the patch, I'm clearing review. I will put this at the front of my queue again if you re-request review.

Comment 67

[Jim Blandy :jimb]

(In reply to Jim Blandy :jimb from comment #65)
> I'm still confused...
> 
> We only retain ScriptSource::introducerFilename_ because, for Debugger's
> sake, we want to continue to claim that eval'd code's URL is that of its
> introducer. But doesn't that mean that introducerFilename should *always*
> be, simply, 'filename' here? Why would we want to reach out to the
> introducer's introducer --- isn't that what this does?

Oh, wait, I think I see why this is like this. When we have evals within evals within evals, we need to always propagate the innermost "real" filename inwards, regardless of what synthetic filenames we accumulate on our way in.

Comment 68

[Kannan Vijayan [:djvj]]

Addressed comments, but try run of updated patch is orange on a few tests.  Might be bitrot, might be caused by changes to patch, or maybe new tests.  Will get it green again and post updated patch.

Comment 69

[Jim Blandy :jimb]

Okay, looking forward to it.

Comment 70

[Kannan Vijayan [:djvj]]

Attachment: eval-lineno-fix-1-update.patch

Updated patch with comments addressed, applies to tip (m-i revision eca43063ea0f).

The oranges introduced by the changes were because of the |callerScript| to |script| change.  There are some cases where eval can be called direclty from the browser (e.g. div.onclick = eval).

In these cases, the CurrentScriptFileLineOrigin returns a bogus |script|.  The existing implementation is not affected because it passes |callerScript| to frontend::CompileScript, and |callerScript| is properly null in that case.

Reverting the change from |callerScript| to |script| fixed those oranges.  Final try run after removing |cumulativeIntroductionLine|:

https://tbpl.mozilla.org/?tree=Try&rev=9244018dff2f

Comment 71

[Kannan Vijayan [:djvj]]

Comment on attachment 8368630 [details] [diff] [review]
eval-lineno-fix-1-update.patch

Try is green again.  Putting updated patch up for review.

Comment 72

[Kannan Vijayan [:djvj]]

Comment on attachment 8368630 [details] [diff] [review]
eval-lineno-fix-1-update.patch

Whoops.  Wrong :j nickname :)

Comment 73

[Jim Blandy :jimb]

Ah, I forgot a spot:
https://hg.mozilla.org/mozilla-central/file/735a648bca0d/js/src/jsapi.cpp#l4374

That needs to copy over the introduction filename too, I think.

Comment 74

[Jim Blandy :jimb]

The plumbing around CompileOptions is baroque... but I can explain exactly how it all got to be that way, and it feels like it makes sense. It's just that the consequences are so complicated. I wonder if it should all simply be replaced with something that truly owns all its stuff.

Comment 75

[Kannan Vijayan [:djvj]]

Attachment: eval-lineno-fix-1-update2.patch

Part of the update got rolled into the wrong patch in the mq patchqueue.  Posting proper update patch.

Comment 76

[Jim Blandy :jimb]

If the Function constructor now uses the script returned by CurrentScriptFileLineOrigin, and that script is sometimes wrong (as mentioned in comment 70) then we may need to just fix CurrentScriptFileLineOrigin before we can proceed.

Comment 77

[Jim Blandy :jimb]

Comment on attachment 8368630 [details] [diff] [review]
eval-lineno-fix-1-update.patch

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

This looks good. Two comments on comments:

::: js/src/jsapi.cpp
@@ +4441,5 @@
> +        if (!copy)
> +            return false;
> +    }
> +
> +    // OwningCompileOptions always owns sourceMapURL_, so this cast is okay.

'introducerFilename_', not 'sourceMapURL_'

::: js/src/jsscript.h
@@ +386,5 @@
> +    // scripts.
> +    uint32_t introductionOffset_;
> +
> +    // Original "raw" filename of top-level script that introduced the eval (or
> +    // chain of evals).

Permit me a nit: I think we want more detail here, since what we're doing is bizarre and sees only restricted use:

    // In the past, dynamic scripts were attributed to the filename in
    // which the eval/Function/etc. call appeared. Debugger needs this
    // behavior preserved for the time being. So, even though we produce
    // informative filenames for dynamic scripts describing where the call
    // occurred, like "foo.js line 30 > eval line 30 > Function", for
    // Debugger we must also preserve the original outermost filename, like
    // "foo.js".

Comment 78

[Jim Blandy :jimb]

Comment on attachment 8368755 [details] [diff] [review]
eval-lineno-fix-1-update2.patch

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

Okay, this all looks good. However, we need to fix the Function-constructor-versus-CurrentScriptFileLineOrigin issue, and Debugger.Source.prototype needs an accessor for introductionType. With those issues addressed, r=me.

Thanks very much for taking this on! I think people will really appreciate the better filenames in the backtraces.

Comment 79

[Jim Blandy :jimb]

Comment on attachment 8343081 [details] [diff] [review]
eval-lineno-fix-2-jittests.patch

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

::: js/src/jit-test/tests/basic/spread-call-eval.js
@@ +19,5 @@
>  let line0 = Error().lineNumber;
>  try {             // line0 + 1
>    eval(...["("]); // line0 + 2
>  } catch (e) {
> +  assertEq(e.lineNumber, 1);

Can't we just delete all the 'line0' related stuff? It's not used any more.

Comment 80

[Kannan Vijayan [:djvj]]

(In reply to Jim Blandy :jimb from comment #79)
> Comment on attachment 8343081 [details] [diff] [review]
> eval-lineno-fix-2-jittests.patch
> 
> Review of attachment 8343081 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/jit-test/tests/basic/spread-call-eval.js
> @@ +19,5 @@
> >  let line0 = Error().lineNumber;
> >  try {             // line0 + 1
> >    eval(...["("]); // line0 + 2
> >  } catch (e) {
> > +  assertEq(e.lineNumber, 1);
> 
> Can't we just delete all the 'line0' related stuff? It's not used any more.

It still seems useful to assert that the line-numbers of exceptions raised within eval scripts carry the right relative values.

Comment 81

[Jim Blandy :jimb]

Comment on attachment 8343082 [details] [diff] [review]
eval-lineno-fix-3-mochitests.patch

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

This looks good. One question, at bottom.

It turns out that almost all those 'eval' statements in the tests are hold-overs from before we had Debugger.prototype.findScripts working the way we needed to, and the only way the tools could find out about scripts was via onNewScript events. (Eval generates such events; code loaded before the global became a debuggee does not.) So they can come out without hurting the intention of the tests.

It would certainly be much better to remove the evals and have the tests continue to check for meaningful line numbers, instead of just '1'. But I think we can leave that for a follow-up.

Do you remember why some of the tests changed the value returned by gEditor.getSelection and isEditorSel?

Comment 82

[Jim Blandy :jimb]

Comment on attachment 8343082 [details] [diff] [review]
eval-lineno-fix-3-mochitests.patch

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

r=me; explained on IRC:

<jimb> djvj: Do you remember why some of the tests changed the value returned
       by gEditor.getSelection?  [12:08]
<jimb> and isEditorSel  [12:09]
<djvj> jimb: those tests simulate particular series of debugger UI actions.  I
       think in some cases, it does a search, then focuses the pane, and then
       checks the selected text for the search.  However, the eval filename
       changes changed specifics of how that interface responded.  [12:14]
<djvj> jimb: I'd have to re-run the specific test to identify exactly what the
       difference is.  Gimme a bit and I'll do that.
<djvj> jimb: originally, the eval filename changes caused different file tabs
       to be present on the left side of the debugger UI - new tabs for the
       "new filenames" since they weren't the same as the containing script
       anymore.
<djvj> this caused the specific sequence of focusing events to be affected

Comment 83

[Kannan Vijayan [:djvj]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2c84be838689

Comment 84

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2c84be838689

Comment 85

[Jason Orendorff [:jorendorff]]

It would be nice to document the augmented Error.stack format here:
  https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Error/Stack

The new syntax is just "URL line NUMBER > eval" or "URL line NUMBER > Function" can occur at the beginning of any line of error.stack.

This means that that stack frame was in eval code (or a function created using the Function constructor). The "URL line NUMBER" part tells the location of the call to eval or Function. This is useful because the fact that an error happened at line 1234 of some eval code is not much use by itself. It's better to at least have a little extra information about how we ended up in eval.

Comment 86

[Jason Orendorff [:jorendorff]]

Slightly more detailed description of the new Error.stack syntax:
  http://esdiscuss.org/topic/standardizing-error-stack-or-equivalent#content-13

Comment 87

[Florian Scholz (Open Web Docs)]

Noted here https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Error/Stack#Stack_of_eval%27ed_code

and in the release notes for Firefox 30
https://developer.mozilla.org/en-US/Firefox/Releases/30#JavaScript