Closed Bug 458020 Opened 16 years ago Closed 15 years ago

Evaluating variables returns incorrect results

Categories

(Core :: JavaScript Engine, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla1.9.1b2

People

(Reporter: Fallen, Assigned: mrbkap)

Details

(Keywords: fixed1.9.1, regression)

Attachments

(1 file, 2 obsolete files)

When debugging javascript with venkman 0.9.87.4 on thunderbird built from comm-central (hg tip), evaluating variables gives totally different results:


STR:
1. Set a breakpoint in a random function and make the application stop there.
2. Evaluate a variable

Results, for example:

function saveReminder(item) { /* breakpoint inside */ }

* |item| /should/ be a calIItemBase
* Evaluating |item| gives:
   $[0] = [Array] [class: Array] {0}
* Evaluating |arguments[0]| (correctly) gives:
   $[1] = [XPComponent] [class: XPCWrappedNative_NoHelper] {0}
* Setting |item| to the evaluation object and evaluating this works fine.


I've heard this issue reported by other developers too, seems noone has filed a bug yet though. Its really hard to use venkman at all with this bug. Sometimes the variables evaluated are also jsd stack frames, integers, and so on.
Related to bug 449766?
Possibly the same cause, yes. Not limited to "undefined" though.
This bug here is a regression though.
Keywords: regression
Great. Do you know when it regressed? And/or are you already trying to figure out?
Not exactly. Old IRC logs show that the first complaints were around 2008/09/09. I wrote at that time:
<mcsmurf> looks like Venkman is now quite broken on trunk :|
<mcsmurf> the scope window in Venkman reports other variable values and types than the interactive command shell

Dunno though which output of those were wrong or if both were wrong.
(In reply to comment #5)
> Not exactly. Old IRC logs show that the first complaints were around
> 2008/09/09. I wrote at that time:
> <mcsmurf> looks like Venkman is now quite broken on trunk :|
> <mcsmurf> the scope window in Venkman reports other variable values and types
> than the interactive command shell
> 
> Dunno though which output of those were wrong or if both were wrong.

Funny. Bug 446120 is about the Watches window and Locals view disagreeing (and a bunch of errors popping up for whatever you do...

Would be good if someone could determine a regression range, and/or reliable, complete steps to reproduce...
Hrm, this seems to work fine here again. Maybe Fallen can do some tests...or maybe provide a reliable way to reproduce this.
STR:

* Download/compile a thunderbird nightly
* Install venkman 0.9.87.4
* Open both, don't exclude browser files
* Open mailCore.js
* Set a breakpoint in the first possible line of CustomizeMailToolbar
* Use the context menu to trigger the breakpoint
* evaluate |toolboxId| in the interactive session

Result:
< 0001: toolboxId
> $[0] = [Array] [class: Array] {0}

Expected:
< 0001: toolboxId
> $[0] = [string] "mail-toolbox"

I'll try to find a regression range later on
I think I got a regression range now, SeaMonkey build 2008-09-08 01:33:36 works fine, build from 2008-09-09 00:23:43 does not. I tested this by breaking in the function "saveImageURL" and then evaluating the imageCache var in the interactive command shell (the Local Variables view displayed the correct type/value). In the older build it displayed the usual xpconnect object, in the newer build it displayed something like "[integer] 214". 214 was the line number Venkman was currently at. When moving on with "Step Over" one line, the var evaluated to "[integer] 215".
Check-ins in that time frame: http://hg.mozilla.org/mozilla-central/pushloghtml?startdate=2008-09-08+01%3A33%3A36&enddate=2008-09-09+00%3A23%3A43
(In reply to comment #9)
> I think I got a regression range now, SeaMonkey build 2008-09-08 01:33:36 works
> fine, build from 2008-09-09 00:23:43 does not.
> Check-ins in that time frame:
> http://hg.mozilla.org/mozilla-central/pushloghtml?startdate=2008-09-08+01%3A33%3A36&enddate=2008-09-09+00%3A23%3A43

Ugh, at least half of SpiderMonkey was touched in that timeframe, there were quite a few checkins. Right now I'm suspecting http://hg.mozilla.org/mozilla-central/rev/71c48ddc9e8c (bug 454266). Cc-ing Brendan Eich. Brendan, could you please comment as to the likelihood this breakage is caused by that patch (and/or perhaps one of the other checkins in that timeframe? Thanks a lot!
It looks like Bug 454266 is not the cause, tested by local back-out.
BTW: I tested with a FF build, build id 2008-09-08 09:17:24 and there it was already broken. So I guess candidates for causing this bug here are Bug 446386, Bug 454163 (unlikely), Bug 453133, Bug 330237 (very unlikey).
Backing out Bug 446386 locally did not have any effect, backing out Bug 453133 locally did not work, too many changes in code since that one was checked in.
Igor: Could you possibly take a look at this bug here? I think it is caused by your check-in from Bug 453133. It's the only bug left from the regression range I have, I could not back it out locally though as there were too many check-ins in that code already.
If you want/need to reproduce, I suggest you either download/compile a SeaMonkey trunk build or a Firefox trunk build and install the Venkman extension (you need to hack the install.rdf file to include the Firefox trunk version). Then you open Venkman, make sure the check box under Debug->Exclude Browser Files is not checked, double-click on the file contentAreaUtils.js on the left and set a breakpoint in the function "saveImageURL" (click on the function and then click on the "-" before the line in the code view on the right). Now try to save any image on a web page by right clicking on it and clicking on "Save Image As...". Now click on "Step Over" until the "var imageCache = Components....." line has been executed. Then enter "imageCache" or "eval(imageCache)" in the command line and observe the wrong output. In the "Local Variables" window on the left the correct output should be displayed. When you enter the command, the code under http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/extensions/venkman/resources/content/venkman-commands.js&rev=1.47&mark=781#781 gets executed AFAIK.
We need a reduced testcase. Comment 0 talks about a formal paramter named item evaluating to an Array instead of the expected XPConnect-wrapped native. I have no idea what would cause that, but it's probably not bug 453133 or anything like that bug.

/be
You're right. I tried backing out again locally, Bug 446386 caused this bug here. Tested by reverting check-ins with hg revert --all -r till I found that one.
Igor, can you take a look? Thanks,

/be
Assignee: rginda → general
Component: JavaScript Debugger → JavaScript Engine
Product: Other Applications → Core
QA Contact: venkman → general
Target Milestone: --- → mozilla1.9.1b2
Assignee: general → igor
Priority: -- → P2
Igor: Already had a chance to look into this?
(In reply to comment #18)
> Igor: Already had a chance to look into this?

I will look at this later this week.
any update here?
This bug makes venkman **totally** unusable to xulrunner-based app developers
and this is a major blocker for many of us. Igor, please give a few cycles to
this bug, we really need a fix.
Flags: blocking1.9.1?
Short term fix: fix this bug.

Long term fix: regression tests for venkman/jsd.
Flags: blocking1.9.1? → blocking1.9.1+
fwiw, ime window.eval(...) can be used as a workaround.

from memory, that's a way i've worked around the loss of frames because of previous things. (a couple of years ago, some other flag)
Attached patch patch v1 (obsolete) — Splinter Review
The problem here is that in between the script executing, a venkman script with the same staticDepth executes and continues to execute through the call to JS_EvaluateInStackFrame, so when we compile the DEBUGGER frame, we properly compute the upvar depth, but cx->display is wrong.

This patch is correct for the time being since we only do upvars for the immediate script, and so we don't have to worry about cx->display[fp->script->staticDepth+...]. I don't know if brendan's new upvar work in bug 452498 would fix this as well.
Assignee: igor → mrbkap
Status: NEW → ASSIGNED
Attachment #354366 - Flags: review?(brendan)
Oh, and another workaround is to, instead of evaluating |someVar| to evaluate |with ({}) someVar| to stymie the emitter's upvar optimization.
(In reply to comment #24)
> Created an attachment (id=354366) [details]
> patch v1
> 
> The problem here is that in between the script executing, a venkman script with
> the same staticDepth executes and continues to execute through the call to
> JS_EvaluateInStackFrame, so when we compile the DEBUGGER frame, we properly
> compute the upvar depth, but cx->display is wrong.
> 
> This patch is correct for the time being since we only do upvars for the
> immediate script, and so we don't have to worry about
> cx->display[fp->script->staticDepth+...]. I don't know if brendan's new upvar
> work in bug 452498 would fix this as well.

Works absolutely fine. I patched my tree with it and at least Venkman is
now usable again until a fix gets into the trunk. Thanks a lot Blake !
Comment on attachment 354366 [details] [diff] [review]
patch v1

Common script and test

    if (script->staticDepth < JS_DISPLAY_SIZE)

around display indexing, and r=me.

/be
Attached patch patch v2 (obsolete) — Splinter Review
I wasn't sure if it was worth commoning the extra oldscript->staticDepth comparison.
Attachment #354366 - Attachment is obsolete: true
Attachment #354904 - Flags: review?(brendan)
Attachment #354366 - Flags: review?(brendan)
Attachment #354904 - Flags: review?(brendan) → review+
Comment on attachment 354904 [details] [diff] [review]
patch v2

>     script = js_CompileScript(cx, scobj, fp, JS_StackFramePrincipals(cx, fp),
>                               TCF_COMPILE_N_GO |
>                               TCF_PUT_STATIC_DEPTH(fp->script->staticDepth + 1),
>                               chars, length, NULL,
>                               filename, lineno);
>     if (!script)
>         return JS_FALSE;
> 
>+    /* Ensure that the display is up to date for this particular stack frame. */
>+    oldscript = fp->script;

Common oldscript fully -- I see fp->script->... above in the js_CompileScript call's params.

>+    if (oldscript->staticDepth < JS_DISPLAY_SIZE) {
>+        disp = &cx->display[fp->script->staticDepth];
>+        displaySave = *disp;
>+        *disp = fp;
>+    }
>     ok = js_Execute(cx, scobj, script, fp, JSFRAME_DEBUGGER | JSFRAME_EVAL,
>                     rval);
>+    if (oldscript->staticDepth < JS_DISPLAY_SIZE)
>+        *disp = displaySave;

Is GCC gonna whine about disp being used but not always initialized? It genuinely can't assume oldscript->staticDepth is const, even though it is (C++ can help here; later).

One way out: set disp = NULL in an else clause for the first if (oldscript->staticDepth < JS_DISPLAY_SIZE) and just test if (disp) in the second if-then.

r=me with these.

/be
Attached patch patch v3Splinter Review
This should be warning free and correct.

Daniel, does the offer of French chocolate[1] stand for MoCo employees as well? :)

[1] http://www.glazman.org/weblog/dotclear/index.php?post/2008/12/11/Hard-times-for-Venkman-users-on-mozilla-central
Attachment #354904 - Attachment is obsolete: true
Attachment #355461 - Flags: review+
(In reply to comment #30)

> Daniel, does the offer of French chocolate[1] stand for MoCo employees as well?
> :)

Of course !-) Let me check tomorrow the US FDA requirements. I don't want
the chocolates to be blocked at customs and destroyed...
Keywords: checkin-needed
http://hg.mozilla.org/mozilla-central/rev/965b2068588d and http://hg.mozilla.org/mozilla-central/rev/7658f9a1f671 ... I'll get this in on 1.9.1 in a couple of days.
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Flags: in-testsuite-
Flags: in-litmus-
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: