213841 - Crash on context destruction if 2 breakpoints set at same PC

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Eric Estievenart]

User-Agent:       Mozilla/4.0 (compatible; MSIE 6.0; Windows NT 5.1) Opera 7.11  [en]
Build Identifier: SpiderMonkey 1.5-rc5a

Calling JS_SetTrap() twice for the same program counter will generate
a crash when the JSContext is freed during the call to JS_ClearAllTraps().

Reproducible: Always

Steps to Reproduce:
See details. I could provide a sample code if needed.



In JS_SetTrap, if a trap is already found for the given pc,
the existing trap structure is linked to itself in the rt->trapList
linked list, thus generating a trap on either the context or runtime
destruction.

    trap = FindTrap(rt, script, pc);
    if (trap) {
        /* Restore opcode at pc so it can be saved again. */
        *pc = (jsbytecode)trap->op;
    } else {
        trap = (JSTrap *) JS_malloc(cx, sizeof *trap);
        if (!trap || !js_AddRoot(cx, &trap->closure, "trap->closure")) {
            if (trap)
                JS_free(cx, trap);
            return JS_FALSE;
        }
    }
    JS_APPEND_LINK(&trap->links, &rt->trapList);

I propose to correct that in the following manner :
- Always create another JSTrap (after all, the trap->closure information
  may be different for the 2 traps)

e.g. :

    trap = FindTrap(rt, script, pc);
    if (trap) {
        /* Restore opcode at pc so it can be saved again. */
        *pc = (jsbytecode)trap->op;
    }
    trap = (JSTrap *) JS_malloc(cx, sizeof *trap);
    if (!trap || !js_AddRoot(cx, &trap->closure, "trap->closure")) {
        if (trap)
            JS_free(cx, trap);
        return JS_FALSE;
    }
    JS_APPEND_LINK(&trap->links, &rt->trapList);
    ...

That way thee is no risk of crash. The only problem
is that if 2 breakpoints have been put at the same PC
(generally after the bp relocation due to empty source lines)
it is not easy to retrieve them individually.

Comment 1

[Phil Schwartau]

cc'ing Brendan, Mike -

Comment 2

[Brendan Eich [:brendan]]

Thanks for the bug.  Are you using jsdbgapi.h directly?  Just curious.  The trap
API was not designed to support more than one trap per pc, and more than a very
few traps (5 or 10) per runtime.

/be

Comment 3

[Eric Estievenart]

Yes, I'm using jsdbgapi.h directly.
Of course the engine is not designed to report more than one
trap per pc (would require to pass a list as parameter of the
debug hook) but this is not a really important issue.
Regarding the 5-10 trap limit per runtime, what does cause that
limit ? Performance issues when doing the lookup in the
trap list ? (which does not seem a real issue for me) or
something else I have missed ?
Thanks - Eric

Comment 4

[Brendan Eich [:brendan]]

If the engine were to support more than one trap per pc, then each trap's
handler would be called in a deterministic order -- there would not be a list of
traps passed to one handler.

The 10 limit is just to avoid O(n^2) growth adding to and searching a list
showing up for traps in inner loops.

Patch soon.

/be

Comment 5

[Brendan Eich [:brendan]]

Attachment: trivial patch

No need to create unreachable traps.  Just don't double-append.

/be

Comment 6

[Brendan Eich [:brendan]]

The 10 limit is just a design limit, it's not enforced.  Eric, if you could test
this patch I'd be grateful.

/be

Comment 7

[Brendan Eich [:brendan]]

Attachment: less minimal patch, but more efficient ever so slightly

This is the patch I'm going to land today.  It works for me in the js shell:

js> function f(a){var b=a*a; return b*a}
js> f

function f(a) {
    var b = a * a;
    return b * a;
}

js> dis(f)
main:
00000:	getarg 0
00003:	getarg 0
00006:	mul
00007:	setvar 0
00010:	pop
00011:	getvar 0
00014:	getarg 0
00017:	mul
00018:	return

Source notes:
  0:	 7 [   7] var
js> trap(f, 10, "print('hi')")
js> f(4)
hi
64
js> trap(f, 10, "print('bye')")
js> f(4)
bye
64
js> quit()

/be

Comment 8

[Brendan Eich [:brendan]]

Fixed.

/be

Comment 9

[Phil Schwartau]

Rubber-stamp vrfy

Comment 10

[Brendan Eich [:brendan]]

*** Bug 227261 has been marked as a duplicate of this bug. ***