From 7ca470456bc1ab2142bcdbe858eabd551b8d589c Mon Sep 17 00:00:00 2001 From: dgp Date: Tue, 6 Sep 2016 20:30:07 +0000 Subject: [4dbdd9af14] Proposed fix for mem leak. --- generic/tclVar.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/generic/tclVar.c b/generic/tclVar.c index bdc64b7..55eb91c 100644 --- a/generic/tclVar.c +++ b/generic/tclVar.c @@ -4498,7 +4498,6 @@ TclDeleteNamespaceVars( Tcl_GetVariableFullName(interp, (Tcl_Var) varPtr, objPtr); UnsetVarStruct(varPtr, NULL, iPtr, /* part1 */ objPtr, NULL, flags); - Tcl_DecrRefCount(objPtr); /* free no longer needed obj */ /* * Remove the variable from the table and force it undefined in case @@ -4527,6 +4526,12 @@ TclDeleteNamespaceVars( } } } + + if (!TclIsVarUndefined(varPtr)) { + UnsetVarStruct(varPtr, NULL, iPtr, /* part1 */ objPtr, + NULL, flags); + } + Tcl_DecrRefCount(objPtr); /* free no longer needed obj */ VarHashRefCount(varPtr)--; VarHashDeleteEntry(varPtr); } -- cgit v0.12 From 41512fe1fa57f33743f10f5a1baa0444338693bc Mon Sep 17 00:00:00 2001 From: dgp Date: Wed, 7 Sep 2016 16:19:42 +0000 Subject: Improve the comments and add a test. --- generic/tclVar.c | 13 +++++++++++-- tests/var.test | 31 +++++++++++++++++++++++++++++++ 2 files changed, 42 insertions(+), 2 deletions(-) diff --git a/generic/tclVar.c b/generic/tclVar.c index 55eb91c..e95307e 100644 --- a/generic/tclVar.c +++ b/generic/tclVar.c @@ -4500,8 +4500,12 @@ TclDeleteNamespaceVars( NULL, flags); /* - * Remove the variable from the table and force it undefined in case - * an unset trace brought it back from the dead. + * We just unset the variable. However, an unset trace might + * have re-set it, or might have re-established traces on it. + * This namespace and its vartable are going away unconditionally, + * so we cannot let such things linger. That would be a leak. + * + * First we destroy all traces. ... */ if (TclIsVarTraced(varPtr)) { @@ -4527,6 +4531,11 @@ TclDeleteNamespaceVars( } } + /* + * ...and then, if the variable still holds a value, we unset it + * again. This time with no traces left, we're sure it goes away. + */ + if (!TclIsVarUndefined(varPtr)) { UnsetVarStruct(varPtr, NULL, iPtr, /* part1 */ objPtr, NULL, flags); diff --git a/tests/var.test b/tests/var.test index c852ca9..30e340e 100644 --- a/tests/var.test +++ b/tests/var.test @@ -22,6 +22,21 @@ if {[lsearch [namespace children] ::tcltest] == -1} { testConstraint testupvar [llength [info commands testupvar]] testConstraint testgetvarfullname [llength [info commands testgetvarfullname]] testConstraint testsetnoerr [llength [info commands testsetnoerr]] +testConstraint memory [llength [info commands memory]] +if {[testConstraint memory]} { + proc getbytes {} { + return [lindex [split [memory info] \n] 3 3] + } + proc leaktest {script {iterations 3}} { + set end [getbytes] + for {set i 0} {$i < $iterations} {incr i} { + uplevel 1 $script + set tmp $end + set end [getbytes] + } + return [expr {$end - $tmp}] + } +} catch {rename p ""} catch {namespace delete test_ns_var} @@ -540,6 +555,22 @@ test var-8.2 {TclDeleteNamespaceVars, "unset" traces on ns delete are called wit list [namespace delete test_ns_var] $::info } {{} {::test_ns_var::v {} u}} +test var-8.3 {TclDeleteNamespaceVars, mem leak} -constraints memory -setup { + proc ::t {a i o} { + set $a 321 + } +} -body { + leaktest { + namespace eval n { + variable v 123 + trace variable v u ::t + } + namespace delete n + } +} -cleanup { + rename ::t {} +} -result 0 + test var-9.1 {behaviour of TclGet/SetVar simple get/set} testsetnoerr { catch {unset u}; catch {unset v} list \ -- cgit v0.12 From 2bfb7b96c05182bfb3bcd913303aeb80ba0d2dcd Mon Sep 17 00:00:00 2001 From: dgp Date: Wed, 7 Sep 2016 18:31:58 +0000 Subject: Attempt to fix [7f02ff1efa]. Make trace-18.1 fail. Suspect test is an experiment that preserves the bug. --- generic/tclNamesp.c | 20 +++++++------------- generic/tclVar.c | 49 +++++++++++++++++++++++++++++++++---------------- 2 files changed, 40 insertions(+), 29 deletions(-) diff --git a/generic/tclNamesp.c b/generic/tclNamesp.c index 7f6ecf5..74dfaf8 100644 --- a/generic/tclNamesp.c +++ b/generic/tclNamesp.c @@ -382,19 +382,6 @@ Tcl_PopCallFrame( register CallFrame *framePtr = iPtr->framePtr; Namespace *nsPtr; - /* - * It's important to remove the call frame from the interpreter's stack of - * call frames before deleting local variables, so that traces invoked by - * the variable deletion don't see the partially-deleted frame. - */ - - if (framePtr->callerPtr) { - iPtr->framePtr = framePtr->callerPtr; - iPtr->varFramePtr = framePtr->callerVarPtr; - } else { - /* Tcl_PopCallFrame: trying to pop rootCallFrame! */ - } - if (framePtr->varTablePtr != NULL) { TclDeleteVars(iPtr, framePtr->varTablePtr); ckfree(framePtr->varTablePtr); @@ -422,6 +409,13 @@ Tcl_PopCallFrame( } framePtr->nsPtr = NULL; + if (framePtr->callerPtr) { + iPtr->framePtr = framePtr->callerPtr; + iPtr->varFramePtr = framePtr->callerVarPtr; + } else { + /* Tcl_PopCallFrame: trying to pop rootCallFrame! */ + } + if (framePtr->tailcallPtr) { TclSetTailcall(interp, framePtr->tailcallPtr); } diff --git a/generic/tclVar.c b/generic/tclVar.c index 48e09f6..0b371ee 100644 --- a/generic/tclVar.c +++ b/generic/tclVar.c @@ -5032,27 +5032,44 @@ TclDeleteVars( TclVarHashTable *tablePtr) /* Hash table containing variables to * delete. */ { - Tcl_Interp *interp = (Tcl_Interp *) iPtr; Tcl_HashSearch search; register Var *varPtr; - int flags; - Namespace *currNsPtr = (Namespace *) TclGetCurrentNamespace(interp); - - /* - * Determine what flags to pass to the trace callback functions. - */ - - flags = TCL_TRACE_UNSETS; - if (tablePtr == &iPtr->globalNsPtr->varTable) { - flags |= TCL_GLOBAL_ONLY; - } else if (tablePtr == &currNsPtr->varTable) { - flags |= TCL_NAMESPACE_ONLY; - } for (varPtr = VarHashFirstVar(tablePtr, &search); varPtr != NULL; varPtr = VarHashFirstVar(tablePtr, &search)) { - UnsetVarStruct(varPtr, NULL, iPtr, VarHashGetKey(varPtr), NULL, flags, - -1); + VarHashRefCount(varPtr)++; + + UnsetVarStruct(varPtr, NULL, iPtr, VarHashGetKey(varPtr), + NULL, TCL_TRACE_UNSETS, -1); + + if (TclIsVarTraced(varPtr)) { + Tcl_HashEntry *tPtr = Tcl_FindHashEntry(&iPtr->varTraces, varPtr); + VarTrace *tracePtr = Tcl_GetHashValue(tPtr); + ActiveVarTrace *activePtr; + + while (tracePtr) { + VarTrace *prevPtr = tracePtr; + + tracePtr = tracePtr->nextPtr; + prevPtr->nextPtr = NULL; + Tcl_EventuallyFree(prevPtr, TCL_DYNAMIC); + } + Tcl_DeleteHashEntry(tPtr); + varPtr->flags &= ~VAR_ALL_TRACES; + for (activePtr = iPtr->activeVarTracePtr; activePtr != NULL; + activePtr = activePtr->nextPtr) { + if (activePtr->varPtr == varPtr) { + activePtr->nextTracePtr = NULL; + } + } + } + + if (!TclIsVarUndefined(varPtr)) { + UnsetVarStruct(varPtr, NULL, iPtr, VarHashGetKey(varPtr), + NULL, TCL_TRACE_UNSETS, -1); + } + + VarHashRefCount(varPtr)--; VarHashDeleteEntry(varPtr); } VarHashDeleteTable(tablePtr); -- cgit v0.12 From 3833c42f545f3048c1274799da65766c9e76741b Mon Sep 17 00:00:00 2001 From: dgp Date: Thu, 8 Sep 2016 02:34:37 +0000 Subject: New test trace-18.5 for the bug. Updated trace-18.1 which was tuned to it. --- tests/trace.test | 23 ++++++++++++++++++++++- 1 file changed, 22 insertions(+), 1 deletion(-) diff --git a/tests/trace.test b/tests/trace.test index 1099f48..3b69d38 100644 --- a/tests/trace.test +++ b/tests/trace.test @@ -1227,7 +1227,7 @@ test trace-17.3 {traced variables must survive procedure exits} { test trace-18.1 {unset traces on procedure returns} { proc p1 {x y} {set a 44; p2 14} - proc p2 {z} {trace add variable z unset {traceCheck {lsort [uplevel 1 {info vars}]}}} + proc p2 {z} {trace add variable z unset {traceCheck {lsort [uplevel 2 {info vars}]}}} set info {} p1 foo bar set info @@ -1263,6 +1263,27 @@ test trace-18.4 {namespace delete / trace vdelete combo, Bug \#1338280} { rename doTrace {} set info } 1110 +test trace-18.5 {Bug 7f02ff1efa} -setup { + proc constant {name value} { + upvar 1 $name c + set c $value + trace variable c wu [list reset $value] + } + proc reset {v a i o} { + uplevel 1 [list constant $a $v] + } + proc demo {} { + constant pi 3.14 + } +} -body { + unset -nocomplain pi + demo + info exists pi +} -cleanup { + rename demo {} + rename reset {} + rename constant {} +} -result 0 # Delete arrays when done, so they can be re-used as scalars # elsewhere. -- cgit v0.12