diff --git a/packages/salesforcedx-apex-replay-debugger/src/core/heapDumpService.ts b/packages/salesforcedx-apex-replay-debugger/src/core/heapDumpService.ts index 1ee9fbde30..cfde33138c 100644 --- a/packages/salesforcedx-apex-replay-debugger/src/core/heapDumpService.ts +++ b/packages/salesforcedx-apex-replay-debugger/src/core/heapDumpService.ts @@ -134,7 +134,8 @@ export class HeapDumpService { const updatedVariable = this.createVariableFromReference( symbolName, refVar, - new Map() + new Map(), + new Array() ); if (updatedVariable) { frameInfo.locals.set(localVar.name, updatedVariable); @@ -173,7 +174,8 @@ export class HeapDumpService { const updatedVariable = this.createVariableFromReference( symbolName, refVar, - new Map() + new Map(), + new Array() ); if (updatedVariable) { statics.set(staticVar.name, updatedVariable); @@ -210,7 +212,8 @@ export class HeapDumpService { const updatedVariable = this.createVariableFromReference( symName, refVar, - new Map() + new Map(), + new Array() ); if (updatedVariable) { frameInfo.globals.set(symName, updatedVariable); @@ -298,7 +301,8 @@ export class HeapDumpService { // cloned into a new variable that has a variable name. private copyReferenceContainer( refContainer: ApexVariableContainer, - varName: string + varName: string, + createVarRef = true ) { const tmpContainer = new ApexVariableContainer( varName, @@ -311,9 +315,15 @@ export class HeapDumpService { // set so it can be expanded in the variables window. Note this check // is kind of superfluous but it's better to be safe than sorry if (!this.isPrimitiveType(tmpContainer.type)) { - tmpContainer.variablesRef = this.logContext - .getVariableHandler() - .create(tmpContainer); + if (createVarRef) { + tmpContainer.variablesRef = this.logContext + .getVariableHandler() + .create(tmpContainer); + } else { + // If the variable is being cloned for a name change then + // it needs to use the same variablesRef as the parent + tmpContainer.variablesRef = refContainer.variablesRef; + } } return tmpContainer; } @@ -327,7 +337,8 @@ export class HeapDumpService { // 1: '2' // 2: '3' private createStringFromVarContainer( - varContainer: ApexVariableContainer + varContainer: ApexVariableContainer, + visitedSet: Set ): string { // If the varContainer isn't a reference or is a string references if ( @@ -336,36 +347,55 @@ export class HeapDumpService { ) { return varContainer.value; } - // For a list or set the name string is going to end up being (,...: {=,...=} - const isListOrSet = - varContainer.type.toLowerCase().startsWith('list<') || - varContainer.type.toLowerCase().startsWith('set<'); - // The live debugger, for collections, doesn't include their type in variable name/value - const containerType = this.isCollectionType(varContainer.type) - ? '' - : `${varContainer.type}:`; - let returnString = isListOrSet ? '(' : `${containerType}{`; - let first = true; - // Loop through each of the container's variables to get the name/value combinations, calling to create - // the string if another collection is found. - for (const entry of Array.from(varContainer.variables.entries())) { - const valueAsApexVar = entry[1] as ApexVariableContainer; - if (!first) { - returnString += ', '; - } - if (valueAsApexVar.ref) { - // if this is also a ref then create the string from that - returnString += this.createStringFromVarContainer(valueAsApexVar); + + // If the varContainer is a ref and it's already been visited then return the string + if (varContainer.ref) { + if (visitedSet.has(varContainer.ref)) { + return 'already output'; } else { - // otherwise get the name/value from the variable - returnString += isListOrSet - ? valueAsApexVar.value - : `${valueAsApexVar.name}=${valueAsApexVar.value}`; + visitedSet.add(varContainer.ref); + } + } + let returnString = ''; + try { + // For a list or set the name string is going to end up being (,...: {=,...=} + const isListOrSet = + varContainer.type.toLowerCase().startsWith('list<') || + varContainer.type.toLowerCase().startsWith('set<'); + // The live debugger, for collections, doesn't include their type in variable name/value + const containerType = this.isCollectionType(varContainer.type) + ? '' + : `${varContainer.type}:`; + returnString = isListOrSet ? '(' : `${containerType}{`; + let first = true; + // Loop through each of the container's variables to get the name/value combinations, calling to create + // the string if another collection is found. + for (const entry of Array.from(varContainer.variables.entries())) { + const valueAsApexVar = entry[1] as ApexVariableContainer; + if (!first) { + returnString += ', '; + } + if (valueAsApexVar.ref) { + // if this is also a ref then create the string from that + returnString += this.createStringFromVarContainer( + valueAsApexVar, + visitedSet + ); + } else { + // otherwise get the name/value from the variable + returnString += isListOrSet + ? valueAsApexVar.value + : `${valueAsApexVar.name}=${valueAsApexVar.value}`; + } + first = false; + } + returnString += isListOrSet ? ')' : '}'; + } finally { + if (varContainer.ref) { + visitedSet.delete(varContainer.ref); } - first = false; } - returnString += isListOrSet ? ')' : '}'; return returnString; } @@ -577,7 +607,10 @@ export class HeapDumpService { } // If the reference doesn't contain any child references then it's value can set here. if (!hasInnerRefs) { - refContainer.value = this.createStringFromVarContainer(refContainer); + refContainer.value = this.createStringFromVarContainer( + refContainer, + new Set() + ); } } @@ -597,24 +630,44 @@ export class HeapDumpService { public createVariableFromReference( varName: string, refVariable: ApexVariableContainer, - visitedMap: Map + visitedMap: Map, + updateAfterVarCreation: ApexVariableContainer[] ): ApexVariableContainer | undefined { // if this isn't a reference? if (!refVariable.ref) { return undefined; } - // If this reference has already been seen, return + // If this reference has already been seen, then there's + // a good chance that the variable is still being created + // and we can't reset set the value now. if (visitedMap.has(refVariable.ref)) { + const visitedVar = visitedMap.get( + refVariable.ref + ) as ApexVariableContainer; + if (visitedVar !== null) { + if (visitedVar.name !== varName) { + const updatedNameVarContainer = this.copyReferenceContainer( + visitedVar, + varName, + false + ); + updateAfterVarCreation.push(updatedNameVarContainer); + return updatedNameVarContainer; + } else { + return visitedVar; + } + } return undefined; } try { - visitedMap.set(refVariable.ref, null); - // First, clone the reference into the named variable const namedVarContainer = this.copyReferenceContainer( refVariable, varName ); + // Create the visitedMap entry with what will be the actual + // variable. + visitedMap.set(refVariable.ref, namedVarContainer); // If the value hasn't been set yet, then we have to walk through all of the children and update // any child references on the variable with a recursive call @@ -641,7 +694,8 @@ export class HeapDumpService { const updatedKeyVarContainer = this.createVariableFromReference( KEY_VALUE_PAIR_KEY, keyRef!, - visitedMap + visitedMap, + updateAfterVarCreation ); if (updatedKeyVarContainer) { keyName = updatedKeyVarContainer.value; @@ -670,7 +724,8 @@ export class HeapDumpService { const updatedValueVarContainer = this.createVariableFromReference( KEY_VALUE_PAIR_VALUE, valueRef!, - visitedMap + visitedMap, + updateAfterVarCreation ); if (updatedValueVarContainer) { valueVal = updatedValueVarContainer.value; @@ -710,7 +765,8 @@ export class HeapDumpService { const updatedChildContainer = this.createVariableFromReference( childVarName, childVarRefContainer!, - visitedMap + visitedMap, + updateAfterVarCreation ); // update the child variable in the map if (updatedChildContainer) { @@ -727,7 +783,8 @@ export class HeapDumpService { } } namedVarContainer.value = this.createStringFromVarContainer( - namedVarContainer + namedVarContainer, + new Set() ); } @@ -735,6 +792,18 @@ export class HeapDumpService { } finally { // Ensure the current reference is removed from the visited map visitedMap.delete(refVariable.ref); + // If the visited map is empty that means the variable is done being hooked up + // If there are any items in the updateAfterVarCreation array then now is the + // time to update them + if (visitedMap.size === 0) { + updateAfterVarCreation.forEach(element => { + const varContainer = element as ApexVariableContainer; + varContainer.value = this.createStringFromVarContainer( + varContainer, + new Set() + ); + }); + } } } diff --git a/packages/salesforcedx-apex-replay-debugger/test/unit/adapter/apexReplayDebugVariablesHandling.test.ts b/packages/salesforcedx-apex-replay-debugger/test/unit/adapter/apexReplayDebugVariablesHandling.test.ts index 1d704a8dc2..24fe54ee4f 100644 --- a/packages/salesforcedx-apex-replay-debugger/test/unit/adapter/apexReplayDebugVariablesHandling.test.ts +++ b/packages/salesforcedx-apex-replay-debugger/test/unit/adapter/apexReplayDebugVariablesHandling.test.ts @@ -25,6 +25,7 @@ import { HeapDumpService } from '../../../src/core/heapDumpService'; import { MockApexReplayDebug } from './apexReplayDebug.test'; import { createHeapDumpResultForTriggers, + createHeapDumpWithCircularRefs, createHeapDumpWithNestedRefs, createHeapDumpWithNoStringTypes, createHeapDumpWithStrings @@ -310,36 +311,26 @@ describe('Replay debugger adapter variable handling - unit', () => { }); it('Should not create string refs if there are not any in the heapdump', () => { - // Create the heap dump and pass the overlay result into createStringRefsFromHeapdump - // Verify with that the ref's map is empty const heapdump = createHeapDumpWithNoStringTypes(); heapDumpService.createStringRefsFromHeapdump( heapdump.getOverlaySuccessResult()! ); - // There should be no strings in the refsMap expect(refsMap.size).to.be.eq(0); }); it('Should only create string refs if there are not any in the heapdump', () => { - // Create the heap dump and pass the overlay result into createStringRefsFromHeapdump - // Verify with that the ref's map is empty const heapdump = createHeapDumpWithNoStringTypes(); heapDumpService.createStringRefsFromHeapdump( heapdump.getOverlaySuccessResult()! ); - // There should be no strings in the refsMap expect(refsMap.size).to.be.eq(0); }); it('Should create string refs if there are any in the heapdump', () => { - // Create the heap dump and pass the overlay result into createStringRefsFromHeapdump - // Verify with that the ref's map is empty. const heapdump = createHeapDumpWithStrings(); heapDumpService.createStringRefsFromHeapdump( heapdump.getOverlaySuccessResult()! ); - // There should be 2 strings in the refsMap, verify their values which should - // contain quotes expect(refsMap.size).to.be.eq(2); let tempStringVar = refsMap.get('0x47a32f5b') as ApexVariableContainer; expect(tempStringVar.value).to.be.eq( @@ -350,7 +341,6 @@ describe('Replay debugger adapter variable handling - unit', () => { }); it('Should not follow reference chain when creating leaf variables except strings', () => { - // Create a heapdump const heapdump = createHeapDumpWithNestedRefs(); getHeapDumpForThisLocationStub = sinon .stub(LogContext.prototype, 'getHeapDumpForThisLocation') @@ -361,10 +351,7 @@ describe('Replay debugger adapter variable handling - unit', () => { heapDumpService.replaceVariablesWithHeapDump(); expect(createStringRefsFromHeapdumpSpy.called).to.be.true; expect(updateLeafReferenceContainerSpy.called).to.be.true; - // with no local, static or global variables to update this shouldn't be called expect(createVariableFromReferenceSpy.called).to.be.false; - - // there should be 4 references in the map, 2 strings and 2 objects expect(refsMap.size).to.be.eq(4); // NonStaticClassWithVariablesToInspect has an inner class of the same type. @@ -399,10 +386,8 @@ describe('Replay debugger adapter variable handling - unit', () => { 'innerVariable' ) as ApexVariableContainer; expect(innerApexRefVar.ref).to.be.eq('0x55260a7a'); - // There should be no children on the inner variable since it's a reference expect(innerApexRefVar.variables.size).to.be.eq(0); - // Verify there is an entry in the ref's for the inner var, verify everything is set on it tempApexVar = refsMap.get('0x55260a7a') as ApexVariableContainer; expect(tempApexVar.variables.size).to.be.eq(7); expect( @@ -432,13 +417,11 @@ describe('Replay debugger adapter variable handling - unit', () => { }); it('Should follow reference chain when creating instance variables from references', () => { - // Create the heapdump const heapdump = createHeapDumpWithNestedRefs(); getHeapDumpForThisLocationStub = sinon .stub(LogContext.prototype, 'getHeapDumpForThisLocation') .returns(heapdump); - // Create a ref variable to update const localRefVariable = new ApexVariableContainer( 'foo', '', @@ -448,10 +431,8 @@ describe('Replay debugger adapter variable handling - unit', () => { const id = frameHandler.create(frameInfo); topFrame.id = id; frameInfo.locals.set(localRefVariable.name, localRefVariable); - // Update it with the heap dump heapDumpService.replaceVariablesWithHeapDump(); - // Verify the variable was updated const updatedLocRefVariable = frameInfo.locals.get( localRefVariable.name ) as ApexVariableContainer; @@ -488,14 +469,13 @@ describe('Replay debugger adapter variable handling - unit', () => { ).to.be.eq( "'This is a longer string that will certainly get truncated until we hit a checkpoint and inspect it_extra'" ); - // Get the innerVariable from the updatedLocRefVariable which should be fully populated + const innerApexRefVar = updatedLocRefVariable.variables.get( 'innerVariable' ) as ApexVariableContainer; expect(innerApexRefVar.ref).to.be.eq('0x55260a7a'); - // The innerVariable should have 7 children (no further references), verify everything has been set correctly - expect(innerApexRefVar.variables.size).to.be.eq(7); + expect(innerApexRefVar.variables.size).to.be.eq(7); expect( (innerApexRefVar.variables.get('MyBoolean') as ApexVariableContainer) .value @@ -572,15 +552,12 @@ describe('Replay debugger adapter variable handling - unit', () => { expect(createStringRefsFromHeapdumpSpy.calledOnce).to.be.true; expect(updateLeafReferenceContainerSpy.calledOnce).to.be.false; expect(createVariableFromReferenceSpy.calledOnce).to.be.false; - // The variable should have been updated and have a value of 5 const updatedNonRefVariable = frameInfo.locals.get( nonRefVariable.name ) as ApexVariableContainer; expect(updatedNonRefVariable.value).to.be.eq('5'); }); - // Variables, whether local, static or trigger all use the same code - // to update. Having this in there for statics is just for sanity it('Should update a non-reference static variable', () => { const heapdump = new ApexHeapDump('some ID', 'Foo', '', 10); heapdump.setOverlaySuccessResult({ @@ -630,12 +607,66 @@ describe('Replay debugger adapter variable handling - unit', () => { expect(createStringRefsFromHeapdumpSpy.calledOnce).to.be.true; expect(updateLeafReferenceContainerSpy.calledOnce).to.be.false; expect(createVariableFromReferenceSpy.calledOnce).to.be.false; - // The variable should have been updated and have a value of 5 const updatedNonRefVariable = frameInfo.statics.get( nonRefVariable.name ) as ApexVariableContainer; expect(updatedNonRefVariable.value).to.be.eq('5'); }); + + it('Should correctly deal with circular references and variable values', () => { + const heapdump = createHeapDumpWithCircularRefs(); + getHeapDumpForThisLocationStub = sinon + .stub(LogContext.prototype, 'getHeapDumpForThisLocation') + .returns(heapdump); + + const localRefVariable = new ApexVariableContainer( + 'cf1', + '', + 'CircularReference', + '0x717304ef' + ); + const frameInfo = new ApexDebugStackFrameInfo(0, 'Foo'); + const id = frameHandler.create(frameInfo); + topFrame.id = id; + frameInfo.locals.set(localRefVariable.name, localRefVariable); + heapDumpService.replaceVariablesWithHeapDump(); + + // Verify the variable was updated and that there is a cicular reference that + // was set up correctly. The local variable cf1, of type CircularReference contains + // a field that's a List. After creating cf1 we add cf1 to its own + // list. + const expectedVariableValue = + 'CircularReference:{(already output), someInt=5}'; + const expectedListVarValue = + '(CircularReference:{already output, someInt=5})'; + + const updatedLocRefVariable = frameInfo.locals.get( + localRefVariable.name + ) as ApexVariableContainer; + expect(updatedLocRefVariable.value).to.be.eq(expectedVariableValue); + + expect(updatedLocRefVariable.variables.size).to.be.eq(2); + expect( + (updatedLocRefVariable.variables.get( + 'someInt' + ) as ApexVariableContainer).value + ).to.be.eq('5'); + const listChildVar = updatedLocRefVariable.variables.get( + 'cfList' + ) as ApexVariableContainer; + + expect(listChildVar.value).to.be.eq(expectedListVarValue); + expect(listChildVar.variables.size).to.be.eq(1); + + const listElementVar = listChildVar.variables.get( + '0' + ) as ApexVariableContainer; + expect(listElementVar.value).to.be.eq(expectedVariableValue); + expect( + (listElementVar.variables.get('cfList') as ApexVariableContainer) + .value + ).to.be.eq(expectedListVarValue); + }); }); // Describe replaceVariablesWithHeapDump describe('heapDumpTriggerContextVariables', () => { @@ -712,19 +743,14 @@ describe('Replay debugger adapter variable handling - unit', () => { .stub(LogContext.prototype, 'getVariableHandler') .returns(variableHandler); - // In this test we're not running a trigger isRunningApexTriggerStub.returns(false); const frameInfo = new ApexDebugStackFrameInfo(0, 'Foo'); const id = frameHandler.create(frameInfo); topFrame.id = id; - // There should be no globals before the heap dump processing expect(frameInfo.globals.size).to.eq(0); - heapDumpService.replaceVariablesWithHeapDump(); - - // There should be no globals after the heap dump processing expect(frameInfo.globals.size).to.eq(0); }); @@ -739,24 +765,16 @@ describe('Replay debugger adapter variable handling - unit', () => { .stub(LogContext.prototype, 'getVariableHandler') .returns(variableHandler); - // In this test we are running a trigger isRunningApexTriggerStub.returns(true); const frameInfo = new ApexDebugStackFrameInfo(0, 'Foo'); const id = frameHandler.create(frameInfo); topFrame.id = id; - // There should be no globals before the heap dump processing expect(frameInfo.globals.size).to.eq(0); - heapDumpService.replaceVariablesWithHeapDump(); - // There should be 8 globals after the heap dump processing - // 6 Trigger.is* boolean values - // 1 Trigger.new - List - // 1 Trigger.newmap - Map expect(frameInfo.globals.size).to.eq(8); - // Verify the false Trigger booleans expect( (frameInfo.globals.get( EXTENT_TRIGGER_PREFIX + 'isbefore' @@ -777,7 +795,6 @@ describe('Replay debugger adapter variable handling - unit', () => { EXTENT_TRIGGER_PREFIX + 'isupdate' ) as ApexVariableContainer).value ).to.eq('false'); - // Verify the True Trigger booleans expect( (frameInfo.globals.get( EXTENT_TRIGGER_PREFIX + 'isafter' @@ -789,17 +806,12 @@ describe('Replay debugger adapter variable handling - unit', () => { ) as ApexVariableContainer).value ).to.eq('true'); - // Verify the Trigger.new map const triggerNew = frameInfo.globals.get( EXTENT_TRIGGER_PREFIX + 'new' ) as ApexVariableContainer; expect(triggerNew.type).to.be.eq('List'); - // The variablesRef should be set as part of the variable processing. 0 is - // the default, if the reference is set it'll be greater than 0 expect(triggerNew.variablesRef).to.be.greaterThan(0); - // There should be 3 items in the Trigger.new list expect(triggerNew.variables.size).to.be.eq(3); - // Verify the entries in the array expect( (triggerNew.variables.get('0') as ApexVariableContainer).ref ).to.eq('0x5f163c72'); @@ -810,18 +822,13 @@ describe('Replay debugger adapter variable handling - unit', () => { (triggerNew.variables.get('2') as ApexVariableContainer).ref ).to.eq('0x76e9852b'); - // Verify the Trigger.newmap which is a Map const triggerNewmap = frameInfo.globals.get( EXTENT_TRIGGER_PREFIX + 'newmap' ) as ApexVariableContainer; expect(triggerNewmap.type).to.be.eq('Map'); - // The variablesRef should be set as part of the variable processing. 0 is - // the default, if the reference is set it'll be greater than 0 expect(triggerNewmap.variablesRef).to.be.greaterThan(0); expect(triggerNewmap.variables.size).to.be.eq(3); - // Verify the key/value pairs in the map - // There should be a key name with the type of Id and value sane as the kvp name - // There should be a value of type Account whose ref is the account ref + let tempKeyValPairApexVar = triggerNewmap.variables.get( 'key0_value0' ) as ApexVariableContainer; diff --git a/packages/salesforcedx-apex-replay-debugger/test/unit/adapter/heapDumpTestUtil.ts b/packages/salesforcedx-apex-replay-debugger/test/unit/adapter/heapDumpTestUtil.ts index 16fb63376b..f2be1f74a6 100644 --- a/packages/salesforcedx-apex-replay-debugger/test/unit/adapter/heapDumpTestUtil.ts +++ b/packages/salesforcedx-apex-replay-debugger/test/unit/adapter/heapDumpTestUtil.ts @@ -836,3 +836,78 @@ export function createHeapDumpWithNestedRefs(): ApexHeapDump { } as ApexExecutionOverlayResultCommandSuccess); return heapdump; } + +// Partial heapdump with a circular reference +export function createHeapDumpWithCircularRefs(): ApexHeapDump { + const heapdump = new ApexHeapDump('some ID', 'Foo', '', 10); + heapdump.setOverlaySuccessResult({ + HeapDump: { + className: 'CircularRefTest', + extents: [ + { + collectionType: null, + count: 1, + definition: [ + { + name: 'cfList', + type: 'List' + }, + { + name: 'someInt', + type: 'Integer' + } + ], + extent: [ + { + address: '0x717304ef', + isStatic: false, + size: 12, + symbols: ['cf1'], + value: { + entry: [ + { + keyDisplayValue: 'cfList', + value: { + value: '0x614edc98' + } + }, + { + keyDisplayValue: 'someInt', + value: { + value: 5 + } + } + ] + } + } + ], + totalSize: 12, + typeName: 'CircularReference' + }, + { + collectionType: 'CircularReference', + count: 1, + definition: [], + extent: [ + { + address: '0x614edc98', + isStatic: false, + size: 8, + symbols: null, + value: { + value: [ + { + value: '0x717304ef' + } + ] + } + } + ], + totalSize: 8, + typeName: 'List' + } + ] + } + } as ApexExecutionOverlayResultCommandSuccess); + return heapdump; +}