422137 - assertion botch or bogus OOM when decompiling script with debugger trap on JOF_CALL bytecode

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[John J. Barton]

In FF3.0b4 I occasionally get an exception like this
Exception... "[JavaScript Error: "out of memory" {file: "chrome://firebug/content/reps.js" line: 381}]"  nsresult: "0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)"  location: "JS frame :: chrome://firebug/content/reps.js :: anonymous :: line 381"  data: yes]

The line in question is 
var str = obj.constructor.toString();

The exception does not trap in catch block. This makes my application fail.

The exception is not helpful because it provides me with no path prevent it.

Comment 1

[John J. Barton]

Justin, I am hoping that you can reproduce this out of memory error. I am on firebug/branches1.2 around R401. Run 
firebug/tests/script/SimpleIncludeJavascript.html
(This from svn, tests is sibling of branches).  Open firebug->ScriptPanel, Set a breakpoint on payloadCatsAndDucks.js, line 14, then reload the page. I can't say that it happens every time, but when it does the executable line will not highlight and the Watch panel will not update. The exception happens when building the watch panel.

I'm also running chromebug, but I don't think that it is involved.

By the way my FF memory is like 65M so the error message is bogus.

Comment 2

[Mike Shaver (:shaver emeritus)]

The challenge here is that if we're out of memory (and we should be, if we're getting that message), then we aren't going to be able to allocate the space to store the string information.  It's quite rare to see that error make it all the way to the Error Console, for that reason.  Wonder if we're hitting a stack limit or some such.  Is there a package I can install that'll correlate to this code?  Latest FB1.2 beta?

Justin: breaking on JS_ReportOutOfMemory will be helpful!

Comment 3

[John J. Barton]

We're not out of memory. We just have a message that says "out of memory".

Comment 4

[John J. Barton]

(In reply to comment #2)
> The challenge here is that if we're out of memory (and we should be, if we're
> getting that message), then we aren't going to be able to allocate the space to
> store the string information. 

I know that makes sense, but consider the possibilities:
  1) We have completely exhausted our resources. Ok then crash, we can't do something with nothing. I don't think that is the case, since we are able to recover and continue with "out of memory" at least.
  2) We have reached the allocation for a specific purpose. That limit is enforced in part to avoid #1. So what we really need is information on what limit was reached and sufficient introspection to figure out how to fix it.
 
The most likely scenario is a software bug, ergo the platform should help us debug it.  Even if the message is "I ran the GC and no free space came back" would help me understand that oh, i really did use it all up.

(Yes, there are two bugs here, one is the message and one is why it happens on FF3 today).

Comment 5

[timeless]

i think the idea is:
your js is expensive and creates garbage in the stack. it creates too much garbage and js runs out of memory. js by design throws an uncatchable exception. this kills your script. at this point js now has some garbage that it can free. xpconnect then gets the message and is able to report it.

anyone experiencing this is able to use a c++ debugger to attach to the process and find out what's wrong. here are links:

http://developer.mozilla.org/en/docs/How_to_get_a_stacktrace_with_WinDbg
http://bonsai.mozilla.org/cvsblame.cgi?&file=/mozilla/js/src/jsapi.c&mark=5522&rev=3.427#5522
http://bonsai.mozilla.org/cvsblame.cgi?&file=/mozilla/js/src/jscntxt.c&mark=872,787-881,883-886,889-890,901-904,906,910,911,915-916&rev=3.129#872

Note: assuming that reps is being called from inside a debugger hook, then iirc the jsd_xpc hook will return a failure message suppressing the second onError. (jsd_xpc knows your code and it aren't reentrant, and they assume that if your handler threw an exception handling something it wouldn't do a better job handling something else).

I'm not sure which bug you want caught. I don't know what you mean about "does not trap" if you mean that jsd_xpc intentionally skips the trap phase, then assuming I'm right, that's by design = jsd:WONTFIX. If you mean does not die, well, it dies. If you mean that js doesn't give jsd a chance to trap, it does, but while jsd_xpc is in its callback phase it uses jsdService::Pause (thread unsafely i might add, but that's not the subject of this bug) which means that when js looks for a hook, there isn't one = js:INVALID.

If you want JS to include one extra field indicating which kind of OOM is happening, that might be possible. however:

http://mxr.mozilla.org/seamonkey/search?string=JS_ReportOutOfMemory(&find=js%2Fsrc%2Fjs.*c&filter=\bJS_ReportOutOfMemory.[^J]

There are 70 callers in SpiderMonkey today (and a handful of callers outside which couldn't possibly be safely numbered). I think the only reasonably safe thing would be an integer error code or a four character fixed length char[] field which could be forcibly inserted into the error string.

For your purposes, I think it's better to just use windbg and find out what's happening, you only need to set one breakpoint (js3250!JS_ReportOutOfMemory).

"kp" or something in the debugger will easily give you enough information to figure out what's going on.

If you're using a debug build, you can try * **:
.call xpc3250!DumpJSStack();g

* this function may crash, deadlock, blow up your computer, etc., it might even work..., but it requires a console
** this unfortunately only works in debug builds (that's a bug I want to fix, but not this bug).

If not, you can us cx->fp | fp w/ fp->script, fp->down*, fp->pc, script->lineNo, script->fileName, script->main to build your own stack (examples may be found by searching bugzilla for "dt JSFunction" or something)
.hh dt (this has been brought to you by ".hh .hh")

Comment 6

[Igor Bukanov]

(In reply to comment #0)
> In FF3.0b4 I occasionally get an exception like this
> Exception... "[JavaScript Error: "out of memory" {file:
> "chrome://firebug/content/reps.js" line: 381}]"  nsresult: "0x80570021
> (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)"  location: "JS frame ::
> chrome://firebug/content/reps.js :: anonymous :: line 381"  data: yes]
> 
> The line in question is 
> var str = obj.constructor.toString();

Does it happen with earlier versions of Firefox? The bug can be in a bogus call js_ReportOutOfMemory in the engine when we are not, in fact, out of memory.

Comment 7

[John J. Barton]

(In reply to comment #6)

> Does it happen with earlier versions of Firefox?

This 'out of memory' message does not happen in FF2.

Comment 8

[John J. Barton]

(In reply to comment #5)
> i think the idea is:
> your js is expensive and creates garbage in the stack. it creates too much
> garbage and js runs out of memory. js by design throws an uncatchable
> exception. this kills your script. at this point js now has some garbage that
> it can free. xpconnect then gets the message and is able to report it.

But if js-by-design threw a catchable exception it would still have some garbage to collect, the objects between throw and catch, and I would have a chance to understand what was garbage and what was not.

> 
> anyone experiencing this is able to use a c++ debugger 

True of every JS error, but not really something we ought to consider adequate.

> (jsd_xpc knows your code and it aren't reentrant, and they assume that if your
> handler threw an exception handling something it wouldn't do a better job
> handling something else).

? The whole point of exception handling is to do a better job than "Error".

> 
> I'm not sure which bug you want caught. I don't know what you mean about "does
> not trap" if you mean that jsd_xpc intentionally skips the trap phase, then
> assuming I'm right, that's by design = jsd:WONTFIX. 

Well this I don't understand. If the exception is untrappable, then why throw it? It would be much better to just crash the application and force me to go use C++. As designed it just a big waste of time: I can't do anything with the app and the information given by the so-called error message doesn't help me fix it.  Based on the message "out of memory" what do you propose I do to fix it? Use C++ debugger? Then crash so I don't write more bug reports like this one ;-)

Anyway it seems unlikely that anything will change here, let's move on. There is a bug in FF3 that creates a bogus out-of-memory message.  Let's focus on fixing it.

Comment 9

[Mike Shaver (:shaver emeritus)]

The OOM site doesn't throw an exception; if it did you could catch it, I assure you, since there's no special handling of OOM exceptions anywhere in the interpreter.  (In part because there are no exceptions for OOM! ;) )  It simply triggers the error reporter and terminates the script.

What's happening here is that the script is terminating hard due to the OOM report (possibly spurious; see bug 422287 cloned from this one) and then XPConnect's error reporter is creating an exception from the error report so that it can be thrown to containing script, if this is a JS -> C++ -> JS layering (where we're now running on the second "->" as it unwinds).  With a stack trace via windbg for this case, we'd be able to see what that outer script is, I think, but it would only be of trivial interest.

Your (Firebug)'s error reporter should be getting called as well, from my reading of the code -- is it not in this case?

I'll try to get somehow set up with the right development environment to reproduce this, if nobody is willing or able to package it, but I'm out the rest of the week at funeral services and such, so please don't expect extremely rapid progress. :/

Comment 10

[Mike Shaver (:shaver emeritus)]

(In reply to comment #9)
> (possibly spurious; see bug 422287 cloned from this one)

Bug 422348, that is; apparently cloning a bug doesn't make the original reporter into a cc: member, which I must say I consider suboptimal behaviour!

Comment 11

[John J. Barton]

(In reply to comment #9)
> Your (Firebug)'s error reporter should be getting called as well, from my
> reading of the code -- is it not in this case?

Oops, you are quite right. The exception was being trapped and printed by my tracing code. But it wasn't in the place I was looking. The 'out of memory' exception prevents my code from dropping the UI into a breakpoint. So the app continues to run and it prints errors. So in my console I see "out of memory" + app errors; when I look at the bottom of the trace I see just the app errors.

I'll put this one on enhancement, it is still the case that I don't know how developers can use the error. But let's focus on 422348.

Comment 12

[timeless]

(this was supposed to be comment 9, but it got stuck in a debugger and lost a race with shaver and you, i'm committing it now without reading comment 9+, i'm sure they're valuable, but i don't have the energy to edit to remove duplicate content.)

[jsshell]
js> build()
built on Oct 30 2007 at 11:38:00
js> function z() {try {Array(1073741824).sort();} catch (e) {for (q in e) {print(q + ":" + e.q);}}print("hi");}
js> z()
typein:1: out of memory

[jsdb]
js> build()
built on Feb 24 2008 at 05:51:52
js> function z() {try {Array(1073741824).sort();} catch (e) {for (q in e) {print(q + ":" + e.q);}}print("hi");}
js> z()
............................
msg = out of memory
filename = typein
lineno = 1
lineBuf = null
tokenOffset = null
............................
[E]at [i]gnore [p]ass along [d]ebug ?d

hit debug break hook

 >line  1 of JSDScript: typein:z:1:1
  line  2 of JSDScript: typein:_TOP_LEVEL_:2:1

 >0001: z()
  0002: z()

jsdb1>resume()
typein:1: out of memory

shaver confirmed but i lost his explanation. in short it's a design decision (which I tried to explain earlier), oom is *not* catchable by scripts. instead, they're killed. However, a debugger can catch it if there is one (you can see that jsdb caught the oom). and again, if the code is the debugger according to jsd_xpc (jsdIService) then there is no debugger while the debugger is running, again a design decision.

This is not actually the case for jsdb where there are in fact 3 debuggers: the first debugger debugs the js>, the second debugger debugs the first jsdb1>, the last debugs the second jsdb2> as seen by jsdb3>. Eventually with enough work (and it's a *lot*), jsd_xpc should be able to offer some of these features. It will require rewriting most of the debugger consumers (they're currently very buggy in terms of api behavior) as well as at least one interface change (offForRuntime). But first, jsdb and jsd_xpc need to have threadsafety fixed, my patches for halves of that problem are baking...

To answer your question of why throw it, it's not "thrown", it's reported. The reporter in this case is xpconnect and as an implementation detail, it takes all reported errors and wraps them as exceptions which are then thrown. Whether it should actually do this for OOM is an interesting question. But you *do* want an OOM report to eventually end up in the error console so that people can know what file ran out of memory and that their script did indeed die for specific causes. This is different from some very rare but serious bugs where js just dies w/o any reports (typically these bugs are in liveconnect, talk to mossop if you are really interested).

But yes, other than some distant changes for jsd_xpc, none of this will change. To focus on the problem at hand, it's time to pull out windbg and breakpoint JS_ReportOutOfMemory.

Comment 13

[Igor Bukanov]

(In reply to comment #12)
> [jsshell]
> js> build()
> built on Oct 30 2007 at 11:38:00
> js> function z() {try {Array(1073741824).sort();} catch (e) {for (q in e)
> {print(q + ":" + e.q);}}print("hi");}
> js> z()
> typein:1: out of memory

The patch from bug 422348 fixes exactly this bogus OOM kind of problem. Now, one can still get an OOM after, say, Array(7 << 26).sort() which will try to allocate 7 << 29 bytes or 3.5GB which typically fails on 32 bits CPU. But at least this OOM is real.  

Comment 14

[Mike Shaver (:shaver emeritus)]

John: march 13's nightly should have the fix for 422348, but I'll leave this open for the general complaint that we don't allocate an error object when we're out of memory and throw it.

Comment 15

[Brian Crowder]

March 13 build will probably not work for John, because of the issue I describe in bug 420585, comment 7.

Comment 16

[Brian Crowder]

John:  This should be testable now, but I'm not seeing any reliable steps to reproduce in this bug.  Can you describe how you cause this OOM condition to occur?

Comment 17

[John J. Barton]

I incorrectly made this bug block firebug and chromebug reports.  Its not the lame error message (the subject here) that blocks me, but a bogus OOM message.  I'll try to make a test case.

Comment 18

[Brian Crowder]

You don't need to build a testcase for it if you can just walk be through the steps to reproduce using the browser with firebug, I have a working firebug1.2 setup.

Comment 19

[John J. Barton]

Ok I finally stumbled upon a case from a report by a user on a different problem.

Using Firebug 1.2a13X or SVN after R498, 
Set Console->Options->Show Chrome Errors + Show Chrome Messages
visit
http://speedmeter.de/speedtest/
first error will be a bogus OOM

Comment 20

[John J. Barton]

Now that Firebug 1.2 is starting to work again I am using it on more sites. I am seeing this OOM on many sites, seems like ones with fancy graphics. I escalated this bug: we need to at least understand the cause.  I see it in conjunction with other bizarre messages like bug 427164, but I don't know if these are coupled or independent so we need to sort this one first.

Comment 21

[Damon Sicore (:damons)]

If this is indeed simply a bogus error message, this isn't going to stop us from shipping.  Marking wanted1.9.0.x+.

Comment 22

[John J. Barton]

Well of course not, but personally I think you should know whether not its bogus before you ship, because we know from bug 427164 that its corrupting data structures and consequently makes firebug unusable.

Comment 23

[Brian Crowder]

In a debug build, while visiting http://speedmeter.de/speedmeter/ we're actually hitting the assertion at jsopcode.c:882

#0  JS_Assert (s=0x2d6a78 "top < ss->printer->script->depth", file=0x2d6824 "/Users/crowder/mozilla/js/src/jsopcode.c", ln=882) at /Users/crowder/mozilla/js/src/jsutil.c:63
#1  0x00265a6b in PushOff (ss=0xbfffbc60, off=13, op=JSOP_GETARG) at /Users/crowder/mozilla/js/src/jsopcode.c:882
#2  0x00274973 in Decompile (ss=0xbfffbc60, pc=0x19788f37 "T", nb=21, nextop=JSOP_NOP) at /Users/crowder/mozilla/js/src/jsopcode.c:4526
#3  0x00274b3b in DecompileCode (jp=0x1985bdf0, script=0x19788ef0, pc=0x19788f34 "S", len=21, pcdepth=0) at /Users/crowder/mozilla/js/src/jsopcode.c:4598
#4  0x002753c7 in js_DecompileFunction (jp=0x1985bdf0) at /Users/crowder/mozilla/js/src/jsopcode.c:4762
#5  0x001e9296 in JS_DecompileFunction (cx=0x1d863e20, fun=0x17fcdee0, indent=0) at /Users/crowder/mozilla/js/src/jsapi.c:4851
#6  0x00222d76 in fun_toStringHelper (cx=0x1d863e20, indent=0, argc=0, vp=0xd96634) at /Users/crowder/mozilla/js/src/jsfun.c:1513
#7  0x00222dcc in fun_toString (cx=0x1d863e20, argc=0, vp=0xd96634) at /Users/crowder/mozilla/js/src/jsfun.c:1523
#8  0x00249193 in js_Invoke (cx=0x1d863e20, argc=0, vp=0xd96634, flags=2) at jsinterp.c:1179
#9  0x002497f6 in js_InternalInvoke (cx=0x1d863e20, obj=0x17fcdee0, fval=394781624, flags=0, argc=0, argv=0x0, rval=0xbfffc010) at jsinterp.c:1352
In PushOff():
    JS_ASSERT(top < ss->printer->script->depth);

In release, this is reported as an OOM condition.

Comment 24

[Brian Crowder]

Interestingly, this seems to be browser-only.  The script-code which seems to break this on the speedtest site is the Prototype "packer" function.  Maybe firebug is trying to interpret a strict warning at this point?  The script causes several...  those warnings and the the decompilations that accompany them are correct in the shell, when I attempt the same test (with a thin layer of "DOM"-alike JS objects).

Comment 25

[Brian Crowder]

Even in debug, with the assertion removed, the browser survives this OOM condition, so I don't think it is terribly serious.  It does seem spurious, though.  Is there perhaps a better exception to be thrown at the point of this assert/OOM report?

Comment 26

[Brian Crowder]

Given that this issue is likely to be encountered by prototype+firebug users, we should probably address it.  It seems like a different exception wouldn't be too horrible here, I'm just not sure.

Hoping Brendan will weigh in, if he gets a moment.

Comment 27

[John J. Barton]

Another bit of weird info. I kept hitting an OOM in the debug loop for one test case. The OOM exception blew Firebug up. So I put a try/catch around the location reported by the OOM.  Not only did that stop the exception from ruining the debug session, it also stopped the exception altogether!?! Seems impossible...



 YAHOO.lang.isArray, YUI 2.2.2 June 2007
    isArray: function(obj) { // frames lose type, so test constructor string
    try {
        if (obj && obj.constructor &&
                   obj.constructor.toString().indexOf('Array') > -1) {
            return true;
        } else {
            return ((typeof obj == 'object') || (typeof obj == 'function')) && obj.constructor == Array;
        }
    } catch(exc) {
        FBTrace.dumpProperties("isArray FAILS:", exc);  /* Something weird: without the try/catch, OOM, with no exception?? */
        FBTrace.dumpProperties("isArray Fails on obj", obj);
    }
    },

Comment 28

[Mike Shaver (:shaver emeritus)]

OOM doesn't throw an exception; see comment 9.  Does the error reporter not get called when there's a try in play?  That would be an interesting bit of data.

If you're set up to do C++ debugging (via symbol server or your own build), getting a stack from js_ReportOutOfMemory would be interesting.

Comment 29

[Brian Crowder]

Talked with shaver about throwing a stack-quota error here, instead (catchable).  If we really -are- out of memory then the process of building the exception should fail and the OOM report should take precedence.  I'll try a patch for this in a moment.

Comment 30

[John J. Barton]

Perhaps I should start over on this one.

1) I don't believe there is any out of memory or similar. Something is broken in Firefox. I don't believe my code is filling up memory or recursive on the stack anything like that. The message is coming out because of some other problem. The problem could be entirely minor, I don't know.

2) Sometimes when this message comes out, jsd is corrupted. This is the reason we need to understand what causes the message.  I can not reproduce this, though I've not tried since the OOM come all the time.

is the speedmeter test case adequate?  Something else I can do?

Comment 31

[Brian Crowder]

John:  Please see comments 23 - 26.  And yes, the speedmeter test is fine, though if we had one we could attach to bugzilla that would be better.

Comment 32

[Mike Shaver (:shaver emeritus)]

If memory corruption is in play, it could easily be the _cause_ of this error, rather than a result of it, in which case making OOMs catchable doesn't really get us anywhere.  (Which won't happen soon if ever -- if that's still what this Firebug-blocking bug is about, per the Summary, then we're in a heap of trouble.)  What bug describes the jsd corruption?

Comment 33

[John J. Barton]

(In reply to comment #24)
> Interestingly, this seems to be browser-only.  The script-code which seems to
> break this on the speedtest site is the Prototype "packer" function.  Maybe
> firebug is trying to interpret a strict warning at this point?  

Sorry I don't know what you mean by interpreting a warning.

Comment 34

[John J. Barton]

(In reply to comment #26)
> Given that this issue is likely to be encountered by prototype+firebug users,
> we should probably address it.  It seems like a different exception wouldn't be
> too horrible here, I'm just not sure.

In my book if we out of memory on every page that includes something as small as prototype, it's horrible.

Comment 35

[John J. Barton]

(In reply to comment #32)
> If memory corruption is in play, it could easily be the _cause_ of this error,
> rather than a result of it, in which case making OOMs catchable doesn't really
> get us anywhere.  (Which won't happen soon if ever -- if that's still what this
> Firebug-blocking bug is about, per the Summary, then we're in a heap of
> trouble.)  What bug describes the jsd corruption?
> 

Bug 427164, but you won't learn anything there because the diagnosis was "the jsd problem is caused by out of memory" so its just a minor problem of the wrong error message.  

Comment 36

[Mike Shaver (:shaver emeritus)]

Sorry, I'm feeling dense today: I don't see a description of the corruption there.  What exactly is being corrupted?  (If we're getting memory corruption, I'd expect us to be crashy, not just exceptiony, but we could be getting lucky.)

Does something show up under valgrind for this, or something?

Comment 37

[John J. Barton]

Well I said jsd corruption, not memory corruption.  Fact is I don't know. What happens is  script.isLineExecutable fails with NS_ERROR_FAILURE. How can this happen? According to 427164, because of OOM. Then we are back here. 

Having jsdIscript functions randomly fail is a serious problem. I called it corruption for lack of understanding.

Comment 38

[Brian Crowder]

Attachment: my attempt at reproducing for the shell

run this as js -f test.txt -- I could not repro, though.

Comment 39

[Brian Crowder]

Attachment: don't use the patched "cs", keep cs = js_CodeSpec[JSOP_TRAP]

Shaver and I bashed our heads mightily on this for a while.  This seems to be the source of the bogus OOM-report, at least for the speedtest.de case, because we end up in a second PushOff(), even though we've already done DECOMPILE_CODE, thanks to the bogus codespec.

Comment 40

[Brendan Eich [:brendan]]

Comment on attachment 315884 [details] [diff] [review]
don't use the patched "cs", keep cs = js_CodeSpec[JSOP_TRAP]

>               case JSOP_TRAP:
>                 saveop = op = JS_GetTrapOpcode(cx, jp->script, pc);
>                 *pc = op;
>-                cs = &js_CodeSpec[op];
>-                len = cs->length;
>-                DECOMPILE_CODE(pc, len);
>+                len = js_CodeSpec[op].length;
>+                DECOMPILE_CODE(pc, js_CodeSpec[op].length);

Comment why cs is not set.

Do use len, not the repeated common expression js_CodeSpec[op].length loaded into len on the line before, in the actual parameter to DECOMPILE_CODE.

>@@ -4528,16 +4527,17 @@ Decompile(SprintStack *ss, jsbytecode *p
>         }
> 
>         if (cs->format & JOF_CALLOP) {
>             todo = Sprint(&ss->sprinter, "");
>             if (todo < 0 || !PushOff(ss, todo, saveop))
>                 return NULL;
>         }
> 
>+advance_pc:
>         pc += len;


Why is this label added?

/be

Comment 41

[Mike Shaver (:shaver emeritus)]

(In reply to comment #40)
> Comment why cs is not set.

I was going to argue that many of the switch's cases don't set cs, but the ones that check an op's length all seem to, so I guess that's the Roman element.  And I'm not going to go on record against additional comments in jsopcode.c, for sure. :)

Comment 42

[Brian Crowder]

Attachment: fixed

Another go.

Comment 43

[Mike Shaver (:shaver emeritus)]

Shell test, trivial in hindsight!

js> function f() { return a(); }
js> trap(f, 0, "print('trap')")
js> f
Assertion failure: top < ss->printer->script->depth, at jsopcode.c:882
Trace/BPT trap

Comment 44

[Brian Crowder]

https://build.mozilla.org/tryserver-builds/2008-04-15_18:[e-mail]-1208307545/

has TryServer builds made with the earlier of the two patches here (the functionality of the patch is the same, however).  Windows should be up soon.

Comment 45

[Brian Crowder]

Attachment: one more rev

Comment 46

[Brian Crowder]

Attachment: the real patch with the fixes.

Comment 47

[Brian Crowder]

Comment on attachment 315896 [details] [diff] [review]
the real patch with the fixes.

Except without the comment!

Comment 48

[Brian Crowder]

Attachment: there!

Sorry for bugmail spamming.

Comment 49

[Igor Bukanov]

The bug 397959 can be a dup of this as the test case there violates the assertion about top < ss->printer->script->depth.

Comment 50

[Igor Bukanov]

An alternative way to fix this once and for all is not to alter script->code with JSOP_TRAP. Rather that can go to a parallel bytecode vector that only js_Interpret will see. 

AFAICS the only complexity of this approach would come from the need to alter pc for an active frame if a debugger adds a trap during the execution of the frame. But this is rather straightforward now given that regs.pc is exposed to the debugger.

Comment 51

[Brian Crowder]

Igor:  I really like that idea, it's definitely weird that we mutate the script here.  That said, I think landing this and targeting a fix such as you suggest for 1.9.0.x is better.  Do you agree?

Comment 52

[Brendan Eich [:brendan]]

Allocating extra bytecode vectors while debugging could hurt too. Maybe not as badly as these bugs, but in combination with cx->fp->script->code and cx->fp->pc becoming unrelated, hrm....

/be

Comment 53

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 315897 [details] [diff] [review]
there!

a1.9=beltzner

Comment 54

[Brian Crowder]

Attachment: an alternative

This is an alternative patch that hoists the trap hackery outside of the switch.

Comment 55

[Brendan Eich [:brendan]]

Comment on attachment 316027 [details] [diff] [review]
an alternative

In some ways nicer but it could leave the trap removed on error. Avoidable loss of a breakpoint in the face of recovered-from error?

/be

Comment 56

[Brian Crowder]

Attachment: less hacky

This is less hacky than "an alternative", I think, but it also does not resolve bug 429264 comment 0.  It's not yet clear to me why.

Comment 57

[Brian Crowder]

Attachment: safer

This should handle error-recovery better.  Still trying to understand problem with 429264, which my "an alternative" patch handles properly.

Comment 58

[Mike Shaver (:shaver emeritus)]

Comment on attachment 316049 [details] [diff] [review]
safer

>+static jsbytecode *
>+Decompile(SprintStack *ss, jsbytecode *pc, intN nb, JSOp nextop)
>+{
>+    JSContext *cx;
>+    JSPrinter *jp;
>+    JSScript *oldscript, *patchedScript;
>+    jsbytecode *code;
>+
>+    cx = ss->sprinter.context;
>+    JS_CHECK_RECURSION(cx, return NULL);
>+
>+    jp = ss->printer;
>+    oldscript = jp->script;
>+    code = js_UntrapScriptCode(cx, oldscript);
>+    if (code != oldscript->code) {
>+        patchedScript = malloc(sizeof(*oldscript));
>+        memcpy(patchedScript, oldscript, sizeof(*oldscript));
>+        patchedScript->code = code;
>+        patchedScript->main = code;

Do we know that main always equals code?  Can't it be offset if there's prologue code, in which case we should offset it here as well (bumping like we do pc)?

>+        jp->script = patchedScript;
>+        pc = pc - oldscript->code + code;

Prefer
  pc = code + (pc - oldscript->code);
I think: base left, offset right.

>+    }
>+
>+    pc = DecompileBytecode(ss, pc, nb, nextop);
>+
>+    if (oldscript != jp->script) {
>+        JS_free(cx, jp->script->code);
>+        jp->script = oldscript;
>+    }
>     return pc;
> }

This leaks patchedScript's malloc if there are any traps.  Better to save the malloc and

JSScript *origscript, patchedscript;

origscript = jp->script;
code = js_UntrapScriptCode(cx, origscript);
if (code != origscript) {
    patchedscript = *jp->script;
    jp->script = &patchedscript;
    patchedscript.code = code;
    patchedscript.main = code + (origscript->main - origscript->code);
    pc = code + (pc - origscript->code);
}

pc = DecompileBytecode(ss, pc, nb, nextop);

if (jp->script == &patchedscript) {
   free stuff;
}

And then it looks like you're going to return a pc that points to freed memory?  You probably want to recompute pc based on the new offset before freeing patchedScript.code.

Comment 59

[Brian Crowder]

Attachment: fixing a couple bugs

Comment 60

[Brian Crowder]

Attachment: one more fix

Comment 61

[Brian Crowder]

Attachment: WIP

Going to try an experiment based on a suggestion from Igor next.  This is just a WIP and I might ditch it.

Comment 62

[Brian Crowder]

Comment on attachment 316089 [details] [diff] [review]
WIP

I think this is actually pretty close to what we need here.  Fixes both this bug and bug 429264.  It does NOT try to handle the case of a script setting a trap during a decompilation, and it is not threadsafe (neither of these are regressions from the previous behavior, though)

Comment 63

[Brian Crowder]

Comment on attachment 316089 [details] [diff] [review]
WIP

Cancelling review until Igor posts his proposed fix.

Comment 64

[Igor Bukanov]

Attachment: generic big fix

I was wrong about the minimality of the fix. It is definitely not small. On the other hand it fixes 4 more missing trap checks in js_GetIndexFromBytecode, js_GetPropertyHelper, obj_eval and js_LookupPropertyWithFlags.

This is untested beyound compilation ability.

Comment 65

[Brian Crowder]

Comment on attachment 316089 [details] [diff] [review]
WIP

If it is believed that we cannot stomach Igor's "correct" patch for 1.9.0, then we should take this one (upon review, of course).  Igor's should become its own bug, and we should take it as soon as possible.

Comment 66

[Brian Crowder]

Attachment: One quick nit-pick of my own

Comment 67

[Igor Bukanov]

(In reply to comment #65)
> (From update of attachment 316089 [details] [diff] [review])
> If it is believed that we cannot stomach Igor's "correct" patch for 1.9.0, then
> we should take this one (upon review, of course).  Igor's should become its own
> bug, and we should take it as soon as possible.

I filed that as an enhancement bug 429615. I think without JSScript.main -> JSScript.prologLength changes (they went into the separated bug 429616) the updated patch does not look particularly big or scary.

Comment 68

[Igor Bukanov]

(In reply to comment #67)
> I filed that as an enhancement bug 429615. I think without JSScript.main ->
> JSScript.prologLength changes (they went into the separated bug 429616) the
> updated patch does not look particularly big or scary.

That idea with JSScript.origCode does not work in presence of a debugger that sets the traps from another thread. Access to JSScript.code/JSScript.origCode is inherently racy. Without taking locks at least to access JSScript.origCode there is always a chance that after the code read JSScript.origCode that was shared with JSScript.code a debugger sets the trap to JSScript.code and copies JSScript.origCode into another array.

What can work to fix JSOP_TRAP-related bugs once and for all is to remove JSOP_TRAP. Instead JSScript can have a bitset indicating which pc has associated trap. 

For threaded interpreter the code will change jumpTable to point to interruptJumpTable, not normalJumpTable, as long as there are traps in the the script. Then under the interrupt: lebel the code would check for both the interrupt handler and a possible trap for the current pc.

For non-threaded interpreter the idea is to change the current if/switch check,

        if (interruptHandler) {
...
        }

        switch (op) {
        }

into something like:

        switchOp = op | interruptMask;
      do_switch:
        switch (switchOp) {
          255:
            check for interrupt and traps;
            switchOp = op;
            goto do_switch;

Here interruptMask would be set to 255 as long as an interrupt handler is set or the script has associated traps. Otherwise it would 0.

Now, to allow immediate setting of traps, jumpTable/interruptMask can be added to JSFrameRegs. Then JS_SetTrap can enumerate all the frames on all contexts in the runtime and update JSFrameRegs.jumpTable/interruptMask when JSStackFrame.script matches the script for which the trap is set.

Comment 69

[Mike Shaver (:shaver emeritus)]

Does this patch make our threadsafety story _worse_?  If not, I think we should look at it; we need a story for the JSOP_TRAP decompilation problems that plague Firebug, even if we don't yet reach perfection.

If the big patch does give us new races, then I think we should review crowder's latest and get it in, because it fixes a bunch of trap-and-decompile bugs that are likely to be very painful for debugger users.

jsdbgapi would love some attention post-1.9, I'm sure.

Comment 70

[Igor Bukanov]

(In reply to comment #69)
> If the big patch does give us new races, then I think we should review
> crowder's latest and get it in

Both crowder's patch and that so called "big patch" makes our thread-safety situation worse since both patches adds more places that would race with the debugger. But since crowder's patch is small and does fix firebug issues for browser's cases, we should take it. Proper threadsafe solutions can wait.

Comment 71

[Mike Shaver (:shaver emeritus)]

Comment on attachment 316262 [details] [diff] [review]
One quick nit-pick of my own

Igor: can you review this, then?  Thanks!

Comment 72

[Mike Shaver (:shaver emeritus)]

Comment on attachment 316262 [details] [diff] [review]
One quick nit-pick of my own

a=shaver

Comment 73

[Mike Shaver (:shaver emeritus)]

Marking checkin-needed, in case crowder doesn't overlap with a green tree-time.  Let's get some of these bits into the test suite, too.

John: this is a very old bug, and could quite possibly have been crashing Firebug in various ways in older versions of Firefox.

Comment 74

[Brian Crowder]

jsdbgapi.c: 3.145
jsopcode.c: 3.311

Comment 75

[Bob Clary [:bc] (inactive)]

thanks shaver!

/cvsroot/mozilla/js/tests/js1_5/extensions/regress-422137.js,v  <--  regress-422137.js
initial revision: 1.1

Comment 76

[Bob Clary [:bc] (inactive)]

v 1.9.0

Comment 77

[John J. Barton]

(In reply to comment #73)

> John: this is a very old bug, and could quite possibly have been crashing
> Firebug in various ways in older versions of Firefox.
> 

Has or will this make it into FF2? One of the most serious Firebug issues on FF2 was decompile-related crashes.  We disabled a lot of decompiling in Firebug 1.1 for that reason. In Firebug 1.2 that stuff is all enabled.  So absent improvements in FF2, Firebug 1.2 on FF2 will crash often I suppose. 

Comment 78

[Mike Shaver (:shaver emeritus)]

I don't know if these patches will make a Firefox 2 update.  (There have been a handful of them chasing down decompilation-with-breakpoints, so we'd need to figure out which ones apply, and get a unified version for test.) 

They only apply to decompiling where there are breakpoints set, though, so a workaround might be to just remove the breakpoints around decompilation.