645160 - jsdIStackFrame is incorrectly truncated at indirect eval calls

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[John J. Barton]

This is a report against FF4.0, Gecko 2.0, not trunk.

1. Install Firebug 1.7.0
http://getfirebug.com/releases/firebug/1.7/firebug-1.7.0.xpi
2. Open http://nexapps.net/temp/dojo/
3. Open Firebug F12, enable all panels, reload,
4. OPen Firebug Tracing from Firebug Icon Menu upper left
5. Tracing option FBS_SRCUNITS
6. Select Script panel line 457, set a breakpoint, reload
7. Select Script side panel Stack, you should see:
(?)(scriptFragment="dojo.provide("dojo._bas...rc/dojo/./_base/lang.js")bootstrap.js (line 457)
_loadUri(uri="dojo-release-1.5.0-src/dojo/./_base/lang.js", cb=undefined)loader.js (line 113)
_loadUriAndCheck(uri="dojo-release-1.5.0-src/dojo/./_base/lang.js", moduleName="dojo._base.lang", cb=undefined)loader.js (line 141)
_loadPath(relpath="./_base/lang.js", module="dojo._base.lang", cb=undefined)loader.js (line 76)
require(moduleName="dojo._base.lang", omitModuleCheck=undefined)loader.js (line 411)
_base.js()_base.js (line 2)

This information is correct.
8. Clear the tracing console
9. Step-Over in Firebug. Firebug will be on line 113 of loader.js. 
10. In the tracing console click on the line that starts "No calling Frame for eval...". The stack trace begins with line 457. The earlier frames are missing, which breaks Firebug. Note thate an eval with no caller is impossible: the stack frame is incorrect.

Comment 1

[John J. Barton]

Honza, please confirm my steps.
Boris do you know if there could be some design decision about eval() involved here?

I'll try to come up with a friendly test case.

Comment 2

[John J. Barton]

Forgot: works in FF3.6

Comment 3

[Boris Zbarsky [:bzbarsky]]

No idea about eval....

Comment 4

[John J. Barton]

Reduced case:
http://getfirebug.com/tests/content/branches/1.8/script/debuggerKeyword/testPage.html
Click on the window.eval() button at the end.

The test case shows that
  window.eval(script) FAILS
  eval(script)        PASSES

Comment 5

[Jeff Walden [:Waldo]]

No comment on the bug itself, but:

(In reply to comment #0)
> Note thate an eval with no caller is impossible: the stack frame is incorrect.

I'm not sure whether you're speaking of what *is* or of what *should be*.  In case you're also speaking of what should be, note that the two aren't the same.  It should be possible to have eval without caller, but we don't support it, that's bug 602994.

Comment 6

[John J. Barton]

But as far a JS is concerned, native calls to eval() are not eval(). Unlike this bug, the call stack would not include eval() and as far as we could tell this would be a top level call like a script tag, depending on the jsdIScript object properties. (I have to say I don't understand why more compilation paths are being added when you can't support the ones you already have).

Comment 7

[Jeff Walden [:Waldo]]

Eval with no caller would behave identically to how indirect eval behaves (that is, the eval function when not called by a name "eval", e.g. |var x = eval; x()|).  Ideally, it's quite possible you wouldn't have to do anything special to handle this when it's implemented.

As for why it's being added, it's a spec requirement, nothing we can do about it.

Comment 8

[John J. Barton]

In practice:
    var x = eval;
    x(script);   // FAILS
in the same manner as window.eval() fails.  Is that this bug again or a different one.

From the perspective of the developer, x(script), window.eval(), and eval() should all have the same kind of stack structure: they call have a caller which we want to know about when debugging. For native eval() I would not expect to have a caller.

As for not doing anything special, I find it hard to believe. I currently set a breakpoint on pc=0 of the outer function of eval buffers so I can examine the call stack and determine the call site of the eval(). I have to assume my code would not be able to handle the native call case.

Comment 9

[John J. Barton]

Can we get any feedback on this bug? I put blocking2.0 request, even if I'm unsure that it will be seen by anyone.

Comment 10

[John J. Barton]

What can I do? This is serious regression that prevents Firebug 1.7 from debugging dojo 1.6 on FF4.0.

Comment 11

[David Rees]

As an end user of firebug and dojo, I can say this regression is definitely causing problems. At the very least if you would be great if someone could give us guidance/history on why the change happened and if there is some other way of approaching this?

Comment 12

[John J. Barton]

Just to clarify the test case from comment 4

function windowEvalScriptDebugger()
{
    var script = "function bar4()\n{\n   debugger;   /*@debuggerScriptRow - don't remove this comment*/\n}\nbar4();";
    window.eval(script);  // FAILS

    var x = eval;
    x(script);   // FAILS

    eval(script);
}

The debugger keyword hits for only the last line, not for either of the other cases.

Comment 13

[Daniel Veditz [:dveditz]]

This isn't going to get fixed if there's no assignee. Figuring out who broke this (by finding the regressing checkin) will help in identifying the problem and who needs to fix it.

Would love to approve a tested patch for this, but I can't realistically say we'd "block" a 4.0.1 release on the lack of a patch.

Comment 14

[Ingo]

This makes using Firebug for dojo development useless and hence makes FF4 as a development environment useless for me ....
Currently the choices open to me is to either stay on 3.6 (current choice) or move to get used to using Chrome's dev tools instead ...
Thanks
Ingo

Comment 15

[John J. Barton]

(In reply to comment #13)
> This isn't going to get fixed if there's no assignee. Figuring out who broke
> this (by finding the regressing checkin) will help in identifying the problem
> and who needs to fix it.

Regression testing is very unlikely to succeed here because the jsd system was completely non-functional during the three months between 4.0b7 and 4.0b10 when this bug is likely to have been introduced.

Comment 16

[MK]

Until the bug is fixed, I won't support FF4 in my projects. In the meantime, I will recommend other browsers, if my customers want more performance than FF 3.6.

Comment 17

[Steve Fink [:sfink] [:s:]]

(In reply to comment #15)
> 
> Regression testing is very unlikely to succeed here because the jsd system was
> completely non-functional during the three months between 4.0b7 and 4.0b10 when
> this bug is likely to have been introduced.

Can you point me to an environment (FF build and Firebug version) where this worked? I tried a trunk build from 2010-01-03 ("3.7a1pre") with Firebug1.6 and it only stopped on one of the 3 debugger; statements.

In particular, did this work in the 4.0b7-4.0b10 "dead zone" with methodjit disabled?

In looking into this, I thought I had something with cross-compartment calls creating an unexpected stack frame that is visible via Component.stack, which may be a real regression in createdScriptHasCaller but I'm not really sure, since the comment says "native interpret?" for that frame and so the cross-compartment frame may just be taking the place of whatever that was.

I'm still trying to track down what is eating the outer part of the stack in this case.

Comment 18

[Steve Fink [:sfink] [:s:]]

This was changed to conform to ES5 15.1.2.1. I'm guessing we'll need to come up with another mechanism to communicate the expected information. More details later.

Comment 19

[Steve Fink [:sfink] [:s:]]

Attachment: Part 1: add eval hook to debug hook set

So somebody tell me why it's a bad idea to just hit the problem with a big hammer:

Add an evalHook that is called with the stack of the code executing the eval, whether it is a direct or indirect eval. Also pass in the eval text for good measure, since I noticed that firebug is doing some crazy guessing that is getting it completely wrong in my test case. Throw in an 'evalType' param (direct eval vs indirect eval) for super extra bonus points.

Lightly tested, but basically works.

Comment 20

[Steve Fink [:sfink] [:s:]]

Attachment: Part 2: Add jsd.evalHook to idl and implement JSD side of eval hooks

Comment 21

[John J. Barton]

(In reply to comment #19)
> So somebody tell me why it's a bad idea to just hit the problem with a big
> hammer:
> 

The eval hook is a one-off version of
Bug 449464 - Implement jsdICompilationHook
so of course I won't think it's a bad idea. I just wish the you'd do or let me do the rest.

Comment 22

[David Rees]

I agree that Firebug (and some other tools that look at the stack) are currently forced to use some wizardry to figure how they really got there. 

+1 to include some of that basic eval info to help these tools out, especially since its "free" to include at eval time. Either using this approach or going the full monty with John's patch.

Comment 23

[Daniel Veditz [:dveditz]]

This is not going to make Macaw (building today) and we may not have another 2.0.x update. Please get this landed on mozilla-central and tested, and then we can evaluate whether it's safe enough to take in an interim update.

Comment 24

[Steve Fink [:sfink] [:s:]]

(In reply to comment #21)
> 
> The eval hook is a one-off version of
> Bug 449464 - Implement jsdICompilationHook
> so of course I won't think it's a bad idea. I just wish the you'd do or let me
> do the rest.

I like the general idea of a compilation unit, but even after reading through that whole thread for the 3rd time it feels like there are enough tricky edge cases that it needs to be baked into jsd2, not slipped into jsd1.

evalHook seems relatively innocuous, fixes the problem here, and removes some hairy firebug code that infers eval text with too little information and gets it wrong pretty easily. The main thing I don't like about it is that it's copying over arbitrarily large chunks of source code (because of an obnoxious internal type conversion.) And of course, it requires firebug changes rather than making the earlier mechanisms magically "work" again, but that seems unavoidable.

Comment 25

[Steve Fink [:sfink] [:s:]]

Attachment: Part 2: Add jsd.evalHook to idl and implement JSD side of eval hooks

Updated main uuid, made the enums match reality. Ready for review.

Comment 26

[Steve Fink [:sfink] [:s:]]

Attachment: Part 2: Add jsd.evalHook to idl and implement JSD side of eval hooks

Note to self: build first, *then* attach to bug.

Comment 27

[John J. Barton]

(In reply to comment #24)
> (In reply to comment #21)
> > 
> > The eval hook is a one-off version of
> > Bug 449464 - Implement jsdICompilationHook
> > so of course I won't think it's a bad idea. I just wish the you'd do or let me
> > do the rest.
> 
> I like the general idea of a compilation unit, but even after reading through
> that whole thread for the 3rd time it feels like there are enough tricky edge
> cases that it needs to be baked into jsd2, not slipped into jsd1.

They are just cases. Uninformed opinions do not necessarily turn them in to tricky edge cases. 

> 
> evalHook seems relatively innocuous, fixes the problem here, and removes some
> hairy firebug code that infers eval text with too little information and gets
> it wrong pretty easily. 

How are the calls to evalHook related to the calls to onScriptCreated? We still have to deal with all of the other compilation units through the onScriptCreated path so we have to know which ones are evals so we can skip them. 

> The main thing I don't like about it is that it's
> copying over arbitrarily large chunks of source code (because of an obnoxious
> internal type conversion.) 

There isn't any amount of source code copying you can do which will approach the amount of silly CPU we burn on the Firebug side for this case ;-)


> And of course, it requires firebug changes rather
> than making the earlier mechanisms magically "work" again, but that seems
> unavoidable.

Why unavoidable? All that needs to happen is to back out the change that caused this bug. It did not really succeed after all.

Don't get me wrong. This fix is a good step in the right direction, justified by expedience rather than how bad the other paths really are.

Comment 28

[Boris Zbarsky [:bzbarsky]]

> All that needs to happen is to back out the change that caused this bug. 

That would reintroduce a bug wrt observable JS behavior outside a debugger.  You _did_ read comment 18, right?

Comment 29

[John J. Barton]

Comment 18 refers to a hypothetical problem, that some code might depend upon the standard. Perhaps there are real problems that were solved, I don't know. But the standard is very new so I doubt that real code relies on it other than accidentally. Thus backing out the change and waiting for jsd2 (any day now right?) could be a reasonable option. At least in a world were developers were important this would not be automatically off the table.

Comment 30

[Steve Fink [:sfink] [:s:]]

(In reply to comment #27)
> (In reply to comment #24)
> > (In reply to comment #21)
> > > 
> > > The eval hook is a one-off version of
> > > Bug 449464 - Implement jsdICompilationHook
> > > so of course I won't think it's a bad idea. I just wish the you'd do or let me
> > > do the rest.
> > 
> > I like the general idea of a compilation unit, but even after reading through
> > that whole thread for the 3rd time it feels like there are enough tricky edge
> > cases that it needs to be baked into jsd2, not slipped into jsd1.
> 
> They are just cases. Uninformed opinions do not necessarily turn them in to
> tricky edge cases. 

Is there a good bug to discuss this? I guess we could use bug 449464 even though it's closed. I'd rather not continue the discussion in this bug, even though it's a viable alternative to the patch in this bug.

> > evalHook seems relatively innocuous, fixes the problem here, and removes some
> > hairy firebug code that infers eval text with too little information and gets
> > it wrong pretty easily. 
> 
> How are the calls to evalHook related to the calls to onScriptCreated? We still
> have to deal with all of the other compilation units through the
> onScriptCreated path so we have to know which ones are evals so we can skip
> them. 

Good point, and documentation I need to add to the patch. ...oh. Yes, rather than saying "Called before *and after* a script is eval'd", which is completely wrong.

Currently, the evalHook gets invoked first, just before the onScriptCreated. Reordering them would be a little tricky, because for an indirect eval the calling portion of the stack has already been blown away by the time onScriptCreated fires.

On the other hand, it makes the connection between evalHook and the corresponding onScriptCreated implicit, which lame. The created outer script may not exist yet with the current code, though, so I can't easily hand you a jsdIScript or a to-be-created tag.

Hm... this is a real problem. In particular, if you are evaling something that creates functions, you'll get

  evalHook(whole script)
  onScriptCreated(function 1)
  onScriptCreated(function 2)
  ...
  onScriptCreated(outer script)
  onScriptCreated(some unrelated script) (or another evalHook)

Ah! jjb gives the answer in bug 449464 comment 32: document and enforce that a functionName is null for and only for the outer script. I'll go look at the code and make sure that's reasonable, then update the comments. It's still lame that you have to rely on sequencing for the correspondence.

> > The main thing I don't like about it is that it's
> > copying over arbitrarily large chunks of source code (because of an obnoxious
> > internal type conversion.) 
> 
> There isn't any amount of source code copying you can do which will approach
> the amount of silly CPU we burn on the Firebug side for this case ;-)

That's probably true, though all the code that evals JSON worries me...

> > And of course, it requires firebug changes rather
> > than making the earlier mechanisms magically "work" again, but that seems
> > unavoidable.
> 
> Why unavoidable? All that needs to happen is to back out the change that caused
> this bug. It did not really succeed after all.

It succeeded in matching the spec. Backing it out would break conformance to the spec without a corresponding gain in *web* compatibility. And the spec isn't being totally arbitrary here; there's a good reason for the behavior.

I looked at what it would take to fake things so that the call stack would still be around for the hooks only, but it wasn't pretty.

Comment 31

[John J. Barton]

(In reply to comment #30)
> Is there a good bug to discuss this? 

I'm sorry I raised the issue, let's save our energy for jsd2.

> > 
> > How are the calls to evalHook related to the calls to onScriptCreated? We still
> > have to deal with all of the other compilation units through the
> > onScriptCreated path so we have to know which ones are evals so we can skip
> > them. 
> 
> Good point, and documentation I need to add to the patch. ...oh. Yes, rather
> than saying "Called before *and after* a script is eval'd", which is completely
> wrong.
> 
> Currently, the evalHook gets invoked first, just before the onScriptCreated.
> Reordering them would be a little tricky, because for an indirect eval the
> calling portion of the stack has already been blown away by the time
> onScriptCreated fires.
> 
> On the other hand, it makes the connection between evalHook and the
> corresponding onScriptCreated implicit, which lame. The created outer script
> may not exist yet with the current code, though, so I can't easily hand you a
> jsdIScript or a to-be-created tag.
> 
> Hm... this is a real problem. In particular, if you are evaling something that
> creates functions, you'll get
> 
>   evalHook(whole script)
>   onScriptCreated(function 1)
>   onScriptCreated(function 2)
>   ...
>   onScriptCreated(outer script)

At this point we always see the outer script run, nothing else can happen. So you just need to ensure that nothing can happen between evalHook and function 1, we'll do the rest.

>   onScriptCreated(some unrelated script) (or another evalHook)
> 
> Ah! jjb gives the answer in bug 449464 comment 32: document and enforce that a
> functionName is null for and only for the outer script. I'll go look at the
> code and make sure that's reasonable, then update the comments. It's still lame
> that you have to rely on sequencing for the correspondence.

To take the full half of the glass, this also means the fix is easy to slot into our current code, we just delete a bunch of lame code in our handler for the PC=0 breakpoint on the outer script.

Comment 32

[Luke Wagner [:luke]]

sfink: DirectEval and kind were recently cleaned up, could you rebase?

Comment 33

[Steve Fink [:sfink] [:s:]]

Attachment: Part 1: add eval hook to debug hook set

Sure enough. Sorry about that.

Comment 34

[Luke Wagner [:luke]]

Comment on attachment 526114 [details] [diff] [review]
Part 1: add eval hook to debug hook set

>diff --git a/js/src/jsprvtd.h b/js/src/jsprvtd.h
>+typedef enum EvalType {
>+    INDIRECT_EVAL,
>+    DIRECT_EVAL
>+} EvalType;

If this is in jsdbgapi, it should probably be prefixed with JS.

>     JSSourceHandler     sourceHandler;
>     void                *sourceHandlerData;
>+    JSEvalHook           evalHook;
>+    void                *evalHookData;
>     JSInterpreterHook   executeHook;

Indentation of 'evalHook'.

>+    JSEvalHook hook;
>+
>+    hook = cx->debugHooks->evalHook;
>+    if (hook) {
>+        AutoKeepAtoms keep(cx->runtime);
>+        hook(cx, source, evalType, cx->debugHooks->evalHookData);
>+    }

You're in C++ so you can:  "if (JSEvalHook hook = cx->debugHooks->evalHook)"

Also, do you happen to know the reason for AutoKeepAtoms?

>+extern void
>+js_CallEvalHook(JSContext *cx, JSString *source, EvalType evalType);

Could you include a comment explaining what the hook expects?  Like, is it ok that, after this hook is called, no eval may occur (if we recognize the string as JSON or hit an error)?

Other than that, I'm going to have to assume you know what you're doing with this thing :)

Comment 35

[David Rees]

John, will this help with properly detecting loadSubScript scripts as well? Essentially the problem I tried to work around in http://code.google.com/p/fbug/issues/detail?id=4317.

It feels like it will, but I'm asking in case it doesn't and there is tweak to the patch that will handle that case as well.

Comment 36

[John J. Barton]

Sorry David, comments 24 and 27 were answering this question "No".

The current eval() code in Firebug is based 100% on pure reverse engineering. I just tried a lot of cases and looked at the results. Cases I could figure out work. Cases I couldn't don't. That's the reason I can't apply your patch over on the fbug issue list: you fix some cases but break ones I know work. The patch here will take one case of the list but it does not tell us about the others.

Comment 37

[David Rees]

I hoping it might though with a little more work on my patch. I've been actually hoping that I might be able to fix it without this with more reverse engineering, but I haven't had a chance to dig back in again.

One last question, I'm assuming loadSubScript will not be considered an eval?

Comment 38

[Steve Fink [:sfink] [:s:]]

(In reply to comment #31)
> (In reply to comment #30)
> > Hm... this is a real problem. In particular, if you are evaling something that
> > creates functions, you'll get
> > 
> >   evalHook(whole script)
> >   onScriptCreated(function 1)
> >   onScriptCreated(function 2)
> >   ...
> >   onScriptCreated(outer script)
> 
> At this point we always see the outer script run, nothing else can happen. So
> you just need to ensure that nothing can happen between evalHook and function
> 1, we'll do the rest.

I don't think it's quite that simple. Unfortunately, an evalHook is not guaranteed to be followed with the corresponding onScriptCreate(outer script) because an eval can fail. Or it can be JSON, or whatever. So when you see onScriptCreated(outer script), it might be an unrelated, non-eval outer script that has nothing to do with the previous evalHook.

Argh.

One way forward would be to do your compilationBegin/compilationEnd pairs. But when in doubt, I'd like to match up with what JSD2 is planning, and that's to provide richer script origin information. I don't think there's much point in pushing that too far for JSD1, but maybe I can add a field to jsdIScript that tells you whether it's an eval script or not.

Then when you see an onScriptCreate for an eval script, you know it corresponds to the previous evalHook. Also, you only need to keep track of one "pending" evalHook; if you see another one before you've found an onScriptCreate with the eval flag set, then you can throw it out because it did not result in a script for whatever reason.

Comment 39

[Steve Fink [:sfink] [:s:]]

Comment on attachment 526062 [details] [diff] [review]
Part 2: Add jsd.evalHook to idl and implement JSD side of eval hooks

Canceling review request -- additional changes needed.

Comment 40

[David Rees]

(In reply to comment #38)
> ...
> One way forward would be to do your compilationBegin/compilationEnd pairs. But
> when in doubt, I'd like to match up with what JSD2 is planning, and that's to
> provide richer script origin information. I don't think there's much point in
> pushing that too far for JSD1, but maybe I can add a field to jsdIScript that
> tells you whether it's an eval script or not.
> ...

If you are adding a scriptType attribute already, it would be an added bonus if one of the flags is for loadSubScript also (not sure if that is readily available where you will be changing things or not). Right Firebug can't tell if something is a loadSubScript or an event (it assumes event).

Comment 41

[John J. Barton]

Just to point out faster repro steps:
1. Install http://getfirebug.com/releases/firebug/1.7X/firebug-1.7X.0b4.xpi
2. Open http://getfirebug.com/tests/content/branches/1.8/script/debuggerKeyword/testPage.html
3. Open Firebug F12, enable all panels, reload,
4. OPen Firebug Tracing from Firebug Icon Menu upper left
5. Tracing option FBS_SRCUNITS
6. Click debugger in window.eval()

Trace should have 35-ish lines

Comment 42

[Kevin Dangoor]

I'm going to nominate this for Firefox 5 as this problem makes Firebug unusable for a reasonably large swath of web developers (people using Dojo and similar).

Comment 43

[Oleg Mikheev]

(In reply to comment #42)
> I'm going to nominate this for Firefox 5

Does it mean that Firefox 4 is officially not usable for Dojo developers and likes and we will have to wait for Firefox 5?

Comment 44

[Kevin Dangoor]

The new Firefox rapid release process states that anything short of a serious security issue has to wait for Firefox 5 (which will be in beta next month and is due in June). The current phase of the release process is for backing out changes that aren't ready yet and correcting important bugs, hopefully including this one.

Comment 45

[Ingo]

You guys will loose a lot of developers.
Firebug is one of the tools which made me choose FireFox over others (or more correctly over Chrome - IE was not a real choice) as my main browser.
Well, Chrome here I come ....

Comment 46

[Jim Blandy :jimb]

(In reply to comment #43)
> (In reply to comment #42)
> > I'm going to nominate this for Firefox 5
> 
> Does it mean that Firefox 4 is officially not usable for Dojo developers and
> likes and we will have to wait for Firefox 5?

Yes. However, we've changed the way we're doing releases so hopefully this won't be that bad. Every six weeks we take what we've got and put it out as Aurora ("alpha", really), and disable features that have problems. Every six weeks we promote what's still working in Aurora to Beta, and finally Beta to Firefox N. So once the pipeline gets filled, we'll be doing a release every six weeks.

We won't be making a .x release for this fix, or any fix that's not a security problem. But it will appear on the Aurora channel soon, the Beta channel six weeks after that, and Firefox 5 will come out June 21.

Comment 47

[Jim Blandy :jimb]

I hid my main point: this fix will appear in Aurora much sooner than the release.

Comment 48

[Jim Blandy :jimb]

I think the real story in this bug is that it's absolutely crazy that we found out about this after the release. Period.

Unlike JSD, JSD2's tests are part of the SpiderMonkey test suite; nobody will be able to land a change on TraceMonkey (bad name; the main SpiderMonkey development area) that breaks JSD2 this badly. And we'll be writing extensive regression tests for JSD2.

So hopefully we will never have this sort of serious breakage at the lower debugging levels again.

Comment 49

[Steve Fink [:sfink] [:s:]]

(In reply to comment #43)
> (In reply to comment #42)
> > I'm going to nominate this for Firefox 5
> 
> Does it mean that Firefox 4 is officially not usable for Dojo developers and
> likes and we will have to wait for Firefox 5?

Yes. Or you could phrase it that Fx4 does not have Firebug support for Dojo developers and we will get it into the next possible release. Fx5 is the new Fx4.0.1 (or whatever).

And if you run Aurora for development, you'll get it much sooner. Sorry it's taken so long; it took me a while to narrow down and having it hit during some eventful weeks didn't help.

Note that although I have a patch, I am running into issues testing it because of an unrelated bug. (JSD is exposing function objects that do not descend from a global object, which breaks cross-compartment calls.)

Comment 50

[Steve Fink [:sfink] [:s:]]

Attachment: Part 2: Remember whether a script originated from an eval()

In order to associate an evalHook callback with an onScriptCreated, you need to know whether a script in onScriptCreated was created by an eval. Then the rule is simple: when onScriptCreated is called with a script created by an eval, then the preceding evalHook is for the same script. (evalHook fires for some things that don't end up creating scripts.)

I first attempted to add a parameter to js_CallNewScriptHook that gave this information, but XDR scripts will have forgotten where they came from.

This is the minimal patch for this purpose. For JSD2, we'll probably need some sort of 'origin' field with more detail.

Comment 51

[Jim Blandy :jimb]

(In reply to comment #50)
> This is the minimal patch for this purpose. For JSD2, we'll probably need some
> sort of 'origin' field with more detail.

That's bug 637572.

Comment 52

[Steve Fink [:sfink] [:s:]]

Attachment: Part 3: expose evalHook and fromEval flag through JSD

This is the JSD machinery for exposing eval information. Note that timeless has done one pass on it and I've updated with his comments, but he's going to be unavailable for a while so I'm redirecting to Brendan since we want this for Aurora.

Comment 53

[Steve Fink [:sfink] [:s:]]

Note that currently when I test with or without these patches, I run into bug 651214. I'm working around that for now, and my debugging output looks correct (evalHook is firing when expected, and the script type connected to the scripts on onScriptCreated are correct). I haven't tried updating Firebug to use the new APIs.

Comment 54

[Jason Orendorff [:jorendorff]]

Comment on attachment 527112 [details] [diff] [review]
Part 2: Remember whether a script originated from an eval()

Instead of bumping JSXDR_MAGIC_SCRIPT_CURRENT, I think the right thing is to bump JSXDR_BYTECODE_VERSION -- even though this is not a change to bytecode, per se. Brendan can tell us for sure.

Other than that, this is fine.

Comment 55

[Jason Orendorff [:jorendorff]]

The whole bug here is that indirect eval doesn't get the caller in ->prev. So this affects Error().stack too:

  function potatoMasher(obj, arg) { this.eval(arg); }
  potatoMasher(this, "var s = Error().stack");
  assertEq(/potatoMasher/.exec(s) instanceof Array, true);  // FAIL

This is a straight-up regression introduced with indirect eval. We should fix that. Adding new features to JSD is 100% fine by me, but note that fixing the regression will benefit not only Firebug but also existing JSD users, users of Error().stack, and JSD2. And we can do both.

Comment 56

[Steve Fink [:sfink] [:s:]]

(In reply to comment #55)
> The whole bug here is that indirect eval doesn't get the caller in ->prev. So
> this affects Error().stack too:
> 
>   function potatoMasher(obj, arg) { this.eval(arg); }
>   potatoMasher(this, "var s = Error().stack");
>   assertEq(/potatoMasher/.exec(s) instanceof Array, true);  // FAIL
> 
> This is a straight-up regression introduced with indirect eval. We should fix
> that. Adding new features to JSD is 100% fine by me, but note that fixing the
> regression will benefit not only Firebug but also existing JSD users, users of
> Error().stack, and JSD2. And we can do both.

Oh! I assumed that breaking the ->prev chain was necessary for doing the indirect eval in global scope. If it isn't, then yes, maintaining that chain seems way better than all of this, and should be much safer for Aurora as well.

Comment 57

[Boris Zbarsky [:bzbarsky]]

> You guys will loose a lot of developers.

Ingo, Oleg, I don't get something here.

This bug was first reported after Firefox 4 shipped (presumably because none of the developers who care about this cared to test the last betas or the RC).  There is no fix yet, and Firefox 4.0.1 is already frozen, on the "normal" 6-8 week maintenance release schedule we've had for several years now, and has been for a bit.  The next release after that will be in late June; that's exactly when 4.0.2 would have appeared on the old maintenance schedule.  It just happens that instead of calling that release "Firefox 4.0.2" it will be called "Firefox 5" and will include more than just security+regression fixes.  Note that "Firefox 5" _is_ the security update from "Firefox 4.0.1", so it's not like you have the choice of staying on 4.0.1 once 5 is released, unless you want a browser with security holes.

So I'm not sure what you're advocating.  Are you suggesting that this bug should deserve a separate release all of its own to fix it, separate from the normal maintenance releases?  Usually a bug has to hit a _really_ high bar to trigger an out-of-band release; the only cases I can think of are publicly-known security bugs that are being actively exploited and regressions from security fixes that broke sites that a large fraction of our user base uses.  This bug is neither.  It's a bug; we need to fix it.  But it's not something that's going to get a special release just to fix it.

Comment 58

[Boris Zbarsky [:bzbarsky]]

Or to be more clear, the "wait for Firefox 5" that comment 43 alludes to is no longer than the "wait for Firefox 4.0.2" would have been if we planned to do a 4.0.2.

Comment 59

[John J. Barton]

(In reply to comment #57)
> > You guys will loose a lot of developers.
> 
> Ingo, Oleg, I don't get something here.
> 
> This bug was first reported after Firefox 4 shipped (presumably because none of
> the developers who care about this cared to test the last betas or the RC).

In my opinion, FF4 shipped with this bug because jsd was completely inoperable for several months after FF4.0b7. At b10 we had to re-write Firebug's activation of jsd. Developers barely had a working beta before FF4 shipped. Why should developers care about Firefox if Firefox does not care about them? 
 
Jim's comment 48 is on target: this bug should never have happened.

In reality Ingo and Oleg are pretty special to take the time to complain. By now Web Inspector is feature complete and does not have this bug. Instead of playing catch up, we are arguing about startup time. I know this is not something anyone here can fix, but it's Very Frustrating.

Comment 60

[Ingo]

It's going to be released in FF5 now, accepted.
My mails were because 
a) I was frustrated it was not seen as important enough to go into the first maintenance release
b) I therefore wanted to make sure you guys understand how important Firebug's functionality has become for Firefox - it's my main reason I have so far refrained from making Chrome my main browser. You take (for the time being took) this away from me and my available choices are clear ....

Comment 61

[Boris Zbarsky [:bzbarsky]]

> a) I was frustrated it was not seen as important enough to go into the first
> maintenance release

The first maintenance release was code-frozen before there was a fix in this bug....

Comment 62

[Jason Orendorff [:jorendorff]]

We still don't have a fix, IMHO.

(In reply to comment #56)
> Oh! I assumed that breaking the ->prev chain was necessary for doing the
> indirect eval in global scope. If it isn't, then yes, maintaining that chain
> seems way better than all of this, and should be much safer for Aurora as well.

Well, it isn't necessary for *scoping* -- the scope chain and the dynamic call chain are separate concepts.

I think breaking the ->prev chain is needed for a security check. The fix, then, would be

  * set ->prev correctly in indirect eval
  * in the security check, to retain the current behavior,
    add code to detect the indirect eval case and stop walking the stack.

Maybe there is some reason this is impossible. Right now I'm pretty confused about it; I don't know which security check we're talking about or what the problem case is. I'll ask mrbkap or bz today.

Btw... Luke said on IRC that his view, after examining all the stack code in great detail, was that ->prev is "meaningless". I can see what might lead one to that conclusion, but I find it a little maddening all the same (no offense intended). Nulling it doesn't cause the engine to crash, but that does not imply that it's meaningless.

Comment 63

[John J. Barton]

(In reply to comment #62)
...
> I think breaking the ->prev chain is needed for a security check.

Just a suggestion to double check this idea. Having *less* information for a security check hardly makes sense.  It might be that the security check code is not expecting the rest of the stack, but that's bug for that code to fix.

Comment 64

[Luke Wagner [:luke]]

(In reply to comment #62)
> Btw... Luke said on IRC that his view, after examining all the stack code in
> great detail, was that ->prev is "meaningless". I can see what might lead one
> to that conclusion, but I find it a little maddening all the same (no offense
> intended). Nulling it doesn't cause the engine to crash, but that does not
> imply that it's meaningless.

I think you misunderstand my use of "meaningless".  Its not that it isn't important or that we can screw around with it.  Quite the opposite; its that 'prev' has no a priori specification with which I can evaluate whether indirect eval should or shouldn't have a null prev.  Instead, like many things in SM (but increasingly less so!) its defined in terms of "what are all the possible uses of 'prev', and what is correct for each one of those".  When meaning is defined in this way, all one can do in the face of a large codebase is preserve existing behavior (which I did; the tone of your comment seemed to imply that I introduced this; it seems NULL-prev came in with bug 519129 which is probably worth investigating to see why they did it).

On a positive note, as soon as security isn't using prev, I am very happy to *give* prev an a priori meaning in terms of, e.g., "what should the debugger see in a backtrace".

Comment 65

[Luke Wagner [:luke]]

Oh, I should also note that, as long as you understand why bug 519129 isn't going to reappear, I'm happy to spot-fix indirect eval right now.

Comment 66

[Jason Orendorff [:jorendorff]]

(In reply to comment #59)
> Jim's comment 48 is on target: this bug should never have happened.
> 
> In reality Ingo and Oleg are pretty special to take the time to complain.

(still catching up in this bug) Yeah, I totally agree.

It's maybe not entirely courteous to express how important a bug is to you by issuing an ultimatum, but hey, Mozilla hackers should have thick skins, and we should hear the message loud and clear: "this bug really hurts me!"

(In reply to comment #64)
> I think you misunderstand my use of "meaningless".  Its not that it isn't
> important or that we can screw around with it.  Quite the opposite; its that
> 'prev' has no a priori specification with which I can evaluate whether indirect
> eval should or shouldn't have a null prev.

Yeah, I totally misread you. The context was:

   <jorendorff> but anyhow, indirect eval is new. Why doesn't it link to the
                previous frame? [...]
         <luke> jorendorff: prev has so little meaning, why should or
                shouldn't it?

I took that as a rhetorical question.

Not much is "a priori" in the engine, but we have both been reverse-engineering (and forward-engineering, for that matter) the principles and invariants of the thing for years. We can and should do this for ->prev.

Anyway -- I certainly didn't mean to imply you introduced the null pointer.

The bug you linked to, bug 519129, is the key to the whole thing. Debugging now.

Comment 67

[John J. Barton]

Just to point out FF3.6 does not have this bug, so bug 519129 can't be the whole story.

Comment 68

[Steve Fink [:sfink] [:s:]]

Comment on attachment 527116 [details] [diff] [review]
Part 3: expose evalHook and fromEval flag through JSD

I'm taking my marbles and going home. Or going to bug 651214, anyway.

This bug is for fixing the original problem, presumably by restoring the ->prev chain. Bug 651214 is for adding the evalHook mechanism, which IMO is still desirable because it relieves Firebug of some contortions necessary to properly handle eval.

And this bug has now gone over my head. (I seem to have a knack for digging up unpleasant messes and dumping them on Luke to deal with...)

Comment 69

[Jeff Walden [:Waldo]]

(In reply to comment #66)
> The bug you linked to, bug 519129, is the key to the whole thing. Debugging
> now.

It looks to me like this.  To get a global eval there was one particular set of steps to take to make it happen.  We weren't doing all of them.  Bug 519129 moved us closer to doing all of them.  ("closer" only because I haven't investigated to be certain it took us all the way.)  Not because this was the clear and obvious step, following naturally from clear method specifications (or so it appears to me), but because it was simply more faithful aping, or "pretending hard enough" as that bug would have it.

If we can move away from "pretending" and even further toward "this is what you do for these semantics, that's what you do for others, etc.", that's good and will get us to a fix here in time.  In the meantime I have no expedient suggestions to avoid delays inherent in understanding and streamlining this code, unfortunately.  But I doubt we have time to move away slowly yet still see this fixed in the next release, so the short-term fix probably will be hairy.

Comment 70

[Jason Orendorff [:jorendorff]]

(In reply to comment #67)
> Just to point out FF3.6 does not have this bug, so bug 519129 can't be the
> whole story.

The patch in bug 519129 didn't go into FF3.6, only trunk, which turned into FF4.

To me this only underscores comment 48. The bug was on trunk for over a year, throughout all the betas, and stuff was so broken we didn't notice. :-P

Well, the past sucks. We will get this in shape.

Taking this bug.

Comment 71

[Jason Orendorff [:jorendorff]]

My understanding is that there are two issues here:

1. I can't just back out bug 519129; we actually need a new fix for it.

2. caps depends on ->prev being unhooked:
http://mxr.mozilla.org/mozilla-central/source/caps/src/nsScriptSecurityManager.cpp#2570
If I change the behavior of JS_FrameIterator, I need to also change that code to compensate. We really want the end-to-end behavior of that security check to remain exactly as it is.

(Other security code that examines the stack is not a problem because elsewhere we only look at the topmost script frame; such code doesn't care whether that frame has a ->prev or not.)

Comment 72

[Boris Zbarsky [:bzbarsky]]

Jason, I think that analysis is correct.

The CAPS code there can be as incestuous as the JSAPI will allow (if not more), imo.  Just adding something on JS stack frames that says "don't go past here" to CAPS and restoring ->prev would be perfectly sufficient, I'd think.  Note that CAPS already breaks out of the loop on principal mismatch; this would just be another loop break condition.

Comment 73

[Steve Fink [:sfink] [:s:]]

Argh, sorry. I've moved evalHook to bug 651568, not bug 651214. Bug 651214's something I'm running into constantly when stress-testing JSD/Firebug, but nobody else seems to notice it.

Comment 74

[Jason Orendorff [:jorendorff]]

Trying something:

http://tbpl.mozilla.org/?tree=Try&rev=827604627375
http://hg.mozilla.org/try/rev/827604627375

Comment 75

[John J. Barton]

While looking for a hackaround I discovered that the decompile result from test case described in comment 4 is incorrect. 
 var script = "function bar4()\n{\n debugger; /*@debuggerScriptRow - don't remove this comment*/\n}\nbar4();";
decompiles to
----------
function bar4() {
debugger;
}
function bar4() {
debugger;
}
bar4();

Comment 76

[Jason Orendorff [:jorendorff]]

(In reply to comment #75)
> While looking for a hackaround I discovered that the decompile result from test
> case described in comment 4 is incorrect.

I have noticed this too, while hacking on the debugger.

As far as I know, this currently can't be triggered from the JS shell or from a Web script. I think that's why we haven't noticed it before. It's observable through JSD... but honestly I'm not sure why. Ideally we should not be decompiling scripts for the debugger's benefit. I'd rather hack in an API to get the real original source than fix decompiler bugs. What do you think?

Comment 77

[John J. Barton]

(In reply to comment #76)

> Ideally we should not be
> decompiling scripts for the debugger's benefit. 

? FWIW without decompiling I probably could not have gotten Firebug to work on eval at all.

> I'd rather hack in an API to
> get the real original source than fix decompiler bugs. What do you think?
I think that is bug 651568.

Comment 78

[Steve Fink [:sfink] [:s:]]

(In reply to comment #77)
> (In reply to comment #76)
> 
> > Ideally we should not be
> > decompiling scripts for the debugger's benefit. 
> 
> ? FWIW without decompiling I probably could not have gotten Firebug to work on
> eval at all.

Right. But if you had a way to get scripts' source, you wouldn't need decompilation for that purpose, right?

I still think this should be fixed (in a separate bug), because decompilation is handy when dealing with minified source.

> > I'd rather hack in an API to
> > get the real original source than fix decompiler bugs. What do you think?
> I think that is bug 651568.

Bug 651568 will only get you the source for eval scripts. We need another mechanism (or a more general mechanism) to get the source for non-eval scripts. Is there a bug for that yet? (I need it for gdb/JIT integration, too.)

Comment 79

[Jason Orendorff [:jorendorff]]

(In reply to comment #77)
> (In reply to comment #76)
> 
> > Ideally we should not be
> > decompiling scripts for the debugger's benefit. 
> 
> ? FWIW without decompiling I probably could not have gotten Firebug to work on
> eval at all.

Feel free to intrepret my statement as saying the present situation is not ideal! :)

Comment 80

[Jason Orendorff [:jorendorff]]

The orange X was random. I re-ran the xpcshell tests and they all came up green.

http://tbpl.mozilla.org/?tree=Try&rev=b1b703bf4a17

Comment 81

[Jason Orendorff [:jorendorff]]

Attachment: v1

Blake, this patch does not contain a new fix for bug 519129, because setting fp->prev_ didn't cause any tests to fail. I looked through the uses of ->prev in the engine; it seems to be fine. Am I missing something?

Comment 82

[Johnathan Nightingale [:johnath]]

Hey Blake, this is on the tracking-firefox5 list which means it'll get poked a lot, but I'm guessing you're buried in reviews/security bugs - is there someone else who can pick this piece up?

Comment 83

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 529084 [details] [diff] [review]
v1

Global frames are guaranteed to be scripted, right? Which is why you don't have to change nsScriptSecurityManager::GetPrincipalAndFrame? This is a little worrisome in terms of implicit assumptions we might be breaking but let's get it in.

Sorry for the delay.

Comment 84

[Jason Orendorff [:jorendorff]]

Global frames are guaranteed to be scripted. But I don't know what nsScriptSecurityManager::GetPrincipalAndFrame is meant to do, so I'm not sure if it should change. A comment, at least, might be in order. Please ping me on IRC and let's get this hashed out. sayrer is anxious to get this landed on m-c.

Comment 85

[Jason Orendorff [:jorendorff]]

Blake explained on IRC. I'm convinced.

I will try to land this on m-c tonight.

Comment 86

[Steve Fink [:sfink] [:s:]]

(In reply to comment #85)
> Blake explained on IRC. I'm convinced.

...which I will now quote here without permission:

<jorendorff>
  I don't know anything about caps
  and I can't tell what GetPrincipalAndFrame is trying to do

<mrbkap>
  yeah, we're trying to kill it.
  It's basically trying to compute the subject principal of the currently-running code.

<jorendorff>
  huh. ok.
  so, how does indirect eval fit in here?
  it seems like... without my patch...

<mrbkap>
  The behavior with and without your patch is the same.
  (that's what I was weakly asserting in my comment)

<jorendorff>
  i can't tell

<mrbkap>
  before your patch, if we had an indirect eval frame, we were going to stop there no matter what because its "down" member was null.
  and in fact, the only case you changed was for scripted frames.
  and GetPrincipalAndFrame *always* stops at scripted frames.
  which makes your patch safe for that use-case.

<jorendorff>
  even in the case where we reach 'target' and then some other stuff happens?

<mrbkap>
  the target stuff can only cause us to terminate *earlier* than we otherwise would have.

<jorendorff>
  yes, out of the first loop
  but then there are a couple more calls to JS_FrameIterator
  though actually fp == target causes the first one to be skipped

<mrbkap>
  Yes.
  the problem is that we needed to return a stack frame in the second case.

<jorendorff>
  in fact both of them just hop back up to the topmost frame

<mrbkap>
  but we don't have one there.
  Oh, yes.

<jorendorff>
  which my patch also doesn't affect

<mrbkap>
  but in the case where your patch affects things...
  right

<jorendorff>
  great. ok. i believe you now :)

Comment 87

[Jason Orendorff [:jorendorff]]

http://hg.mozilla.org/mozilla-central/rev/2caec858311a

Doing fine in m-c.

Comment 88

[Johnathan Nightingale [:johnath]]

Jason - how risky is this? Patch is smallish, but not totally trivial-looking.

Comment 89

[Robert Sayre]

ping me for approval decision when ready.

Comment 90

[Jason Orendorff [:jorendorff]]

Risks:

The patch touches caps. This part I'm very confident about. The affected code is small and clear, and mrbkap reviewed the change.

The patch causes JS_FrameIterator to show the whole stack, when previously we truncated it. As mrbkap wrote, "This is a little worrisome in terms of implicit assumptions we might be breaking". But you know, I'm not sure what those could realistically be. Truncating the stack on some but not all API re-entry does not seem like a uesful security property at all, so it's a little hard to imagine that anything correct is depending on it. I did a random spot check of about a quarter of the places outside the engine that use JSStackFrame and JS_FrameIterator and didn't find anything suspicious (apart from the usual background radiation emitted by XPConnect code).

Benefits:

This fixes a pain point for Firebug users that need to debug code that contains an indirect eval, which is to say, |window.eval(code)|. So the bug does not hit everyone, but when it hits, the behavior is pretty cruel. One of the worst things a debugger can do is silently show an incorrect stack, and here everything above the indirect eval simply vanishes.

Comment 91

[Steve Fink [:sfink] [:s:]]

(In reply to comment #90)
> Benefits:

See also comment 42.

Comment 92

[LegNeato]

Please land this on releases/mozilla-aurora TODAY or approval will be rescinded.

Comment 93

[Jason Orendorff [:jorendorff]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/85a5fd71352b

Comment 94

[Sheila Mooney]

Can this land on Aurora as well?

Comment 95

[Sheila Mooney]

Sorry, I am an idiot...I didn't have the bug refreshed.