506018 - crash in js_GetClosureVar, regression from Trace JSOP_NAME for general closures

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Robert Sayre]

+++ This bug was initially created as a clone of Bug #496240 +++

clicking around http://www.viceland.com/index_us.php (occasionally nsfw)

Comment 1

[Robert Sayre]

0x0026daad js_GetClosureVar(JSContext*, JSObject*, unsigned int, unsigned int, unsigned int, double*) + 109
0x005ced07 0 + 6089991
0xbfffd858 0 + 3221215320
0x002815fe js_MonitorLoopEdge(JSContext*, unsigned int&) + 1342
0x001d9f37 js_Interpret + 45623 (jsinterp.cpp:3360)
0x001df3bb js_Execute + 427 (jsinterp.cpp:1644)
0x0018abfb JS_EvaluateUCScriptForPrincipals + 155 (jsapi.cpp:5156)

Comment 2

[Robert Sayre]

(gdb) call DumpJSStack()
0 anonymous() ["http://www.viceland.com/includes/shadowbox-build-3.0b/libraries/sizzle/sizzle.js":7]
    r = "matches"
    s = [object HTMLDivElement]
    e = [function]
    this = [object Window]
1 anonymous() ["http://www.viceland.com/includes/shadowbox-build-3.0b/libraries/sizzle/sizzle.js":7]
    g = undefined
    o = undefined
    h = undefined
    q = [function]
    m = [function]
    c = [function]
    a = [function]
    l = "PSEUDO"
    j = /:(nth|eq|gt|lt|first|last|even|odd)(?:\((\d*)\))?(?=[^-]|$)/
    f = [object Object]
    n = false
    d = [function]
    i = 0
    p = /((?:\((?:\([^()]+\)|[^()]+)+\)|\[(?:\[[^[\]]*\]|['"][^'"]*['"]|[^[\]'"]+)+\]|\\.|[^ >+~,(\[\\]+)+|[>+~])(\s*,\s*)?/g
    b = [function]
    this = [object Window]
2 <TOP LEVEL> ["http://www.viceland.com/includes/shadowbox-build-3.0b/libraries/sizzle/sizzle.js":7]
    this = [object Window]

Comment 3

[David Mandelin [:dmandelin]]

OK. I just duplicated it. I had to do a new build to do so, so it might be something even more recent that interacted with closures.

Comment 4

[David Mandelin [:dmandelin]]

I think I've fixed it. I still need to come up with a shell test case though.

Comment 5

[David Mandelin [:dmandelin]]

Attachment: Patch

The bug occurs when both the using and defining functions of the upvar are heavyweight. It turns out that I was implicitly assuming that:

  scdist(fp->scopeChain at use, defining func) + 1 == scdist(callee at use, def func)

where |scdist(a,b)| is the number of scope chain links from |a| to |b|. This assumption is valid only if the using function is not heavyweight. The assumption was a pure mistake. The patch computes the distance from callee at use to def at func in both places, so everything always matches.

Comment 6

[Brendan Eich [:brendan]]

Comment on attachment 390370 [details] [diff] [review]
Patch

>     for (uint32 i = 0; i < scopeIndex; ++i) {
>         call = OBJ_GET_PARENT(cx, call);
>     }

Overbraced nit.

>-                JSObject* objd = origobj;
>+                JSObject* objd = JSVAL_TO_OBJECT(cx->fp->argv[-2]);

Total nit again: canonical name for other such scope- and proto-chain index computations is tmp.

/be

Comment 7

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/a928ab3ec436

Comment 8

[Robert Sayre]

(I fixed brendan's nits)

Comment 10

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

Mass change: adding fixed1.9.2 keyword

(This bug was identified as a mozilla1.9.2 blocker which was fixed before the mozilla-1.9.2 repository was branched (August 13th, 2009) as per this query: http://is.gd/2ydcb - if this bug is not actually fixed on mozilla1.9.2, please remove the keyword. Apologies for the bugspam)