Bugzilla

Reporter

Comment 1

•

16 years ago

This cannot happen under Windows when having a look at the source:
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/js/src/jsutil.c&rev=3.29&#54

timeless

Comment 2

•

16 years ago

eh? run it under a debugger, DebugBreak() will land you in the debugger. It's functionally equivalent.

Reporter

Comment 3

•

16 years ago

Oh right. But I forgot to say that no crash happens under Windows with the same versions of Minefield and Firebug.

Assignee

Updated

•

16 years ago

Assignee: general → brendan

Blocks: js-propcache

Flags: blocking1.9? → blocking1.9+

OS: Mac OS X → All

Priority: -- → P1

Hardware: PC → All

Target Milestone: --- → mozilla1.9beta4

Assignee

Comment 4

•

16 years ago

Henrik: could you please reproduce in a debugger and print op and js_CodeSpec[op]? Thanks,

/be

Reporter

Comment 5

•

16 years ago

Sure. Here the output of the debugger:

(gdb) frame 1
#1  0x0109d14e in js_GetIndexFromBytecode (script=0x40034ca0, pc=0x40034cdc "S", pcoff=0) at /Users/henrik/Projects/mozilla/source/mozilla/js/src/jsopcode.c:132
132	    JS_ASSERT(js_CodeSpec[op].length >= 1 + pcoff + UINT16_LEN);
Current language:  auto; currently c
(gdb) p op
$1 = JSOP_TRAP
(gdb) p js_CodeSpec[op]
$2 = {
  length = 1 '\001', 
  nuses = 0 '\0', 
  ndefs = 0 '\0', 
  prec = 0 '\0', 
  format = 0
}

Assignee

Updated

•

16 years ago

Status: NEW → ASSIGNED

Assignee

Comment 6

•

16 years ago

Attached patch proposed fix (obsolete) — Details — Splinter Review

Henrik, can you test this? Thanks,

/be

Attachment #303216 - Flags: review?(igor)

Comment 7

•

16 years ago

Comment on attachment 303216 [details] [diff] [review]
proposed fix

>Index: jsopcode.c
>===================================================================
>RCS file: /cvsroot/mozilla/js/src/jsopcode.c,v
>retrieving revision 3.290
>diff -p -u -8 -r3.290 jsopcode.c
>--- jsopcode.c	10 Feb 2008 03:30:08 -0000	3.290
>+++ jsopcode.c	14 Feb 2008 07:12:57 -0000
>@@ -124,16 +124,21 @@ GetJumpOffset(jsbytecode *pc, jsbytecode
> 
> uintN
> js_GetIndexFromBytecode(JSScript *script, jsbytecode *pc, ptrdiff_t pcoff)
> {
>     JSOp op;
>     uintN span, base;
> 
>     op = (JSOp)*pc;
>+    if (op == JSOP_TRAP) {
>+        op = JS_GetTrapOpcode(cx, script, pc);

This would not compile as js_GetIndexFromBytecode does not take cx. 

>+        if (op == JSOP_LIMIT)
>+            return 0;
>+    }

The callers of js_GetIndexFromBytecode expect that the function never fails. I think passing the bytecode itself to the function would fix the issue in a proper way.

Attachment #303216 - Flags: review?(igor) → review-

Assignee

Comment 8

•

16 years ago

Failure is not possible with a properly threaded debugger embedding, that pattern of checking for JSOP_LIMIT is just paranoia -- see JS_GetTrapOpcode:

JS_PUBLIC_API(JSOp) 
JS_GetTrapOpcode(JSContext *cx, JSScript *script, jsbytecode *pc)
{       
    JSTrap *trap;
    
    DBG_LOCK_EVAL(cx->runtime, trap = FindTrap(cx->runtime, script, pc));
    if (!trap) {
#ifdef JS_THREADSAFE
        /*
         * If we lost a race with another thread, return JSOP_LIMIT so our
         * caller can detect this case and do something sane.
         */
#else
        JS_ASSERT(0);   /* XXX can't happen */
#endif
        return JSOP_LIMIT;
    }
    return trap->op;
}

I would prefer to reparameterize js_GetIndexFromBytecode but keep it infallible. Igor, how does that sound?

/be

Comment 9

•

16 years ago

(In reply to comment #8)
> Failure is not possible with a properly threaded debugger embedding, that
> pattern of checking for JSOP_LIMIT is just paranoia -- see JS_GetTrapOpcode:
> 
> JS_PUBLIC_API(JSOp) 
> JS_GetTrapOpcode(JSContext *cx, JSScript *script, jsbytecode *pc)
> {       
>     JSTrap *trap;
> 
>     DBG_LOCK_EVAL(cx->runtime, trap = FindTrap(cx->runtime, script, pc));
>     if (!trap) {
> #ifdef JS_THREADSAFE
>         /*
>          * If we lost a race with another thread, return JSOP_LIMIT so our
>          * caller can detect this case and do something sane.
>          */

How that can be a paranoia if the code explicitly allows to fail?

Comment 10

•

16 years ago

(In reply to comment #8)
> JS_PUBLIC_API(JSOp) 
> JS_GetTrapOpcode(JSContext *cx, JSScript *script, jsbytecode *pc)
> {       
>     JSTrap *trap;
> 
>     DBG_LOCK_EVAL(cx->runtime, trap = FindTrap(cx->runtime, script, pc));
>     if (!trap) {
> #ifdef JS_THREADSAFE
>         /*
>          * If we lost a race with another thread, return JSOP_LIMIT so our
>          * caller can detect this case and do something sane.
>          */

I guess this is a bug in JS_GetTrapOpcode. It should never fail. Instead it can just check under the lock the value of *pc. If it is not [trap], then it should just return it.

Assignee

Comment 11

•

16 years ago

Right! For the longest time, the jsdbgapi.c code lacked any thread safety. You had to run one debugger in one thread, and not have racing trap-opcode setters or anything like that. But now that we lock, it's easy to fix. I will do this later today (feel free to steal this bug, though!).

/be

Assignee

Comment 12

•

16 years ago

Attached patch better proposed fix (obsolete) — Details — Splinter Review

Attachment #303216 - Attachment is obsolete: true

Assignee

Updated

•

16 years ago

Attachment #303653 - Flags: review?(igor)

Comment 13

•

16 years ago

Comment on attachment 303653 [details] [diff] [review]
better proposed fix

>diff -p -u -8 -r3.291 jsopcode.c
>--- jsopcode.c	16 Feb 2008 02:46:46 -0000	3.291
>+++ jsopcode.c	16 Feb 2008 03:12:53 -0000
>@@ -3804,27 +3805,27 @@ Decompile(SprintStack *ss, jsbytecode *p
>                         op = ((js_CodeSpec[op].format & JOF_PARENHEAD) ||
>                               ((js_CodeSpec[op].format & JOF_INVOKE) &&
>                                GET_ARGC(pc + len) == 1) ||
>                               (((op == JSOP_IFEQ || op == JSOP_IFEQX) &&
>                                (sn2 = js_GetSrcNote(outer, pc + len)) &&
>                                SN_TYPE(sn2) != SRC_COND)))
>                              ? JSOP_POP
>                              : JSOP_SETNAME;
>+                    }
> 
>                     /*
>                      * Stack this result as if it's a name, not an anonymous
>                      * function, so it doesn't get decompiled as a generator
>                      * function in a getter or setter context. The precedence
>                      * level is the same for JSOP_NAME and JSOP_ANONFUNOBJ.
>                      */
>                     LOCAL_ASSERT(js_CodeSpec[JSOP_NAME].prec ==
>                                  js_CodeSpec[saveop].prec);
>                     saveop = JSOP_NAME;
>-                    }


This fixes a bug an unrelated bug, doesn't it? I.e without the patch saveop would not be stacked, right?


r+ with this mystery resolved.

Assignee

Comment 14

•

16 years ago

Yes, I noticed the indentation wrongness and reasoned that the brace was misplaced -- want me to split it out into another bug? Could use your confirmation of my reasoning in any event.

/be

Comment 15

•

16 years ago

(In reply to comment #14)
> Yes, I noticed the indentation wrongness and reasoned that the brace was
> misplaced -- want me to split it out into another bug?

That would be ideal. I wish bugzilla would have an option like clone bug to quickly file stuff like that...

Brian Crowder

Comment 16

•

16 years ago

Look for "Clone This Bug" near the bottom of the page...  On the same line with the bold "BUG LIST:"

Assignee

Comment 17

•

16 years ago

Attached patch best proposed fix — Details — Splinter Review

Attachment #303653 - Attachment is obsolete: true

Attachment #303840 - Flags: review?(igor)

Attachment #303653 - Flags: review?(igor)

Updated

•

16 years ago

Attachment #303840 - Flags: review?(igor) → review+

Assignee

Updated

•

16 years ago

Attachment #303840 - Flags: approval1.9+

Assignee

Comment 18

•

16 years ago

Fixed:

js/src/jsdbgapi.c 3.129
js/src/jsopcode.c 3.293
js/src/jsopcode.h 3.59

/be

Status: ASSIGNED → RESOLVED

Closed: 16 years ago

Resolution: --- → FIXED