repl: remove internal frames from runtime errors #15351

lance · 2017-09-12T00:54:40Z

When a user executes code in the REPLServer which generates an
exception, there is no need to display the REPLServer internal
stack frames.

Based on work by @princejwesley.

Should there be an option to display these stack frames for the purposes of Node.js core development, specifically on the REPL itself?

Ref: #9601

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

Affected core subsystem(s)

repl

bnoordhuis

Just an observation but the filtering approach in this PR is somewhat ad hoc.

If we are going to do more of this in the future, then perhaps we should revive parts of #2741: record script ids of builtin scripts and check stack frames against them.

v8::Exception::CreateMessage() (which that PR doesn't use) lets you do that for arbitrary exception objects.

bnoordhuis · 2017-09-12T08:45:13Z

lib/repl.js

+ if (err.stack) {
+ const regex = new RegExp(/\s+repl:/);
+ const stack = err.stack.split('\n');
+ const idx = stack.findIndex((frame) => regex.test(frame));


This filters only a single frame. Wouldn't it make more sense to do e.g.

err.stack = err.stack.split('\n').filter((s) => !/ repl:/.test(s)).join('\n');

This finds the index of repl: and on line 383 below, the stack is spliced from that point, so all frames after it are removed. If I am reading it correctly, I believe your suggestion would only eliminate lines that begin with repl:, but that's not all that we are looking for. Here is some before/after:

$ node --version v8.4.0 $ node > .load test/fixtures/repl-pretty-stack.js > 'use strict'; 'use strict' > function a() { ... b(); ... } undefined > function b() { ... c(); ... } undefined > function c() { ... throw new Error('Whoops!'); ... } undefined > a(); Error: Whoops! at c (repl:2:9) at b (repl:2:3) at a (repl:2:3) at repl:1:1 at ContextifyScript.Script.runInThisContext (vm.js:44:33) at REPLServer.defaultEval (repl.js:239:29) at bound (domain.js:301:14) at REPLServer.runBound [as eval] (domain.js:314:12) at REPLServer.onLine (repl.js:440:10) at emitOne (events.js:120:20) > $ ./node --version v9.0.0-pre $ ./node > .load test/fixtures/repl-pretty-stack.js 'use strict'; function a() { b(); } function b() { c(); } function c() { throw new Error('Whoops!'); } a(); Error: Whoops! at c (repl:9:13) at b (repl:6:5) at a (repl:3:3) >

As you can see, it takes care of stack frames including REPLServer, ContextifyScript, etc.

tniessen · 2017-09-12T09:06:13Z

lib/repl.js

@@ -374,6 +374,17 @@ function REPLServer(prompt,
 };
 }

+ function prettifyStackTrace(err) {
+ if (err.stack) {
+ const regex = new RegExp(/\s+repl:/);


I believe new RegExp is unnecessary in recent ES versions.

I had originally written this inline below as

const idx = stack.findIndex((frame) => /\s+repl:/.test(frame));

Which worked as expected. It just wreaked havoc on my syntax highlighting. 😜 I guess that's not the best reason to use new. But it's all I've got.

jasnell · 2017-09-12T09:41:18Z

lib/repl.js

@@ -209,9 +209,9 @@ function REPLServer(prompt,
 // preserve original error for wrapped command
 const error = wrappedErr || e;
 if (isRecoverableError(error, code))
- err = new Recoverable(error);
+ err = prettifyStackTrace(new Recoverable(error));


Why not just use Error.captureStackTrace()?

That's a good question. I guess I was just going with something similar to what @princejwesley did in #9601. But perhaps that's not the best approach. I'll try an impl with Error.captureStackTrace() and push it for review.

lance · 2017-09-12T21:13:05Z

@jasnell updated using Error.prepareStackTrace().

@bnoordhuis this change might address some of your concerns, since I'm no longer just parsing text but looking at frame.getFunctionName(). Still, I think this is a little problematic, since all I'm really doing is looking for a null function name starting from the bottom of the stack. This is the frame in which vm.runInContext() executes the user code within the body of REPLServer.defaultEval().

The fragility here is that this breaks if internals change and an anonymous function sneaks in between the time defaultEval() is called and when vm.runIn[This]Context() is called with the user code. I couldn't work out a better way to do it. I tried wrapping the user code in a __replInternalEval() function that I could use to split the stack, but that broke a lot of other tests across the board.

I like the idea of script ids for builtins. It would almost certainly make this implementation a little cleaner.

jasnell · 2017-09-14T19:41:17Z

lib/repl.js

@@ -374,6 +374,18 @@ function REPLServer(prompt,
 };
 }

+ Error.prepareStackTrace = function prepareStackTrace(error, structuredStack) {


hmmm... I do not think it is a good idea to be extending Error in this way. Why is the existing Error.captureStackTrace() not adequate?

@jasnell Error.captureStackTrace() captures the stack at the point where it is called. This would rarely be the stack a user is looking for, since that call would happen in the internals of REPLServer, after a call to vm.runInContext(). I think since it would be after the failed completion of an asynchronous execution, the user stack would be gone.

If I understand the v8 Error API documentation, Error.prepareStackTrace() is the recommended way to accomplish what I am trying to do.

Edit to add the relevant supporting doc

If you assign a different function value to Error.prepareStackTrace that function will be used to format stack traces. It will be passed the error object that it is preparing a stack trace for and a structured representation of the stack. User stack trace formatters are free to format the stack trace however they want and even return non-string values. It is safe to retain references to the structured stack trace object after a call to prepareStackTrace completes so that it is also a valid return value. Note that the custom prepareStackTrace function is only called once the stack property of Error object is accessed.

Ok. Lemme stew on this a bit more then :-)

I too had the feeling that doing this is somewhat wrong but by thinking about it further I agree with @lance that this is probably the right thing to do for the repl.

@lance User may override Error.prepareStackTrace in their code.
I suggest something like 👇

const prepareStackTrace = fn => (error, frames) => { // trim stack frames... let trimmedFrames = frames; if (fn) { // pass trimmed stack frame error.stack = <<trimmed stack string>> return fn(error, trimmedFrames); } return trimmedFrames; }; self.outputStream._write = self.outputStream.write; self.outputStream.write = (d) => { let pstrace = Error.prepareStackTrace; Error.prepareStackTrace = prepareStackTrace(pstrace); self.outputStream._write(d); Error.prepareStackTrace = pstrace; };

@princejwesley I thought about that as well but that can be seen as a advantage and as a disadvantage at the same time. Some users might want to know where the repl lines come from (probably only node-core and we could work around it but it is still something to consider). Therefore I did not complain about that.
So do we really want to prevent the user from being able to override the function? If he does that in the first place, the user can also filter the internal lines if he cares to do so.

@BridgeAR
May be setting process.stdout.write = process.stdout._write would disable the trimming feature(because we are binding the original function(outputstream.write) to same outputstream object instead of keeping it local to context).

Adding comment is sufficient, right?
Advantages:

User's choice(like formatting) is preserved.

repl stack is not leaked.

@BridgeAR we are not preventing user's printStackTrace function, Just decorating it with our printStackTrace which will trim the stack frames and pass it to user defined printStackTrace.

@princejwesley @BridgeAR I am a little conflicted about this, and the example here confuses me a little. What happens when self.outputStream._write(d); throws? Wouldn't Error.prepareStackTrace never get restored?

@lance wrap it with try/finally block. if self.outputstream._write(d) throws error, we can't use repl at all.

BridgeAR

Two small nits but otherwise LGTM. I can not think of a better way.

BridgeAR · 2017-09-19T19:49:58Z

test/parallel/test-repl.js

@@ -71,7 +71,7 @@ function clean_up() {
 function strict_mode_error_test() {
 send_expect([
 { client: client_unix, send: 'ref = 1',
- expect: /^ReferenceError:\sref\sis\snot\sdefined\n\s+at\srepl:1:5/ },
+ expect: /^ReferenceError:\sref\sis\snot\sdefined\n/ },


Is this the new end of the error? If yes, we might mark that in the RegExp and if not, I would just replace it with the new at.

BridgeAR · 2017-09-19T19:50:00Z

test/parallel/test-repl-pretty-stack.js

+
+SyntaxError: Unexpected identifier
+
+`


I am not a huge fan of the multi line template string and would rather see \n

Personally, I disagree. Template strings, to my mind, tend to make the test example easier to scan/read quickly.

As far as I know this was mainly discouraged so far and the code does not have any multi line template strings. I think it is more difficult to see whitespace with this. I do not want to block the PR with this comment though.

Yeah, I'm definitely not a fan of multi-line template strings and we've avoided them consistently in the past. It's definitely something that we can revisit. Ping @nodejs/tsc ... any feelings on this?

If it's possible to align the first line with the others (maybe with a tagged literal that removes the first line terminator), then I'm +1.

BridgeAR · 2017-09-19T19:52:48Z

Should this be semver-major though?

jasnell · 2017-09-20T08:08:29Z

will mark as major just to be safe. Ping @nodejs/tsc

When a user executes code in the REPLServer which generates an exception, there is no need to display the REPLServer internal stack frames.

lance · 2017-09-27T00:52:14Z

@princejwesley @BridgeAR PTAL - updated with a function similar to that suggested. Setting Error.prepareStackTrace just before write as suggested won't work, because by then the stack has already been accessed and it's only ever formatted once. I think interception on the 'error' event probably makes more sense here.

@BridgeAR I can take a look at template strings tomorrow. I'll defer to common wisdom, though I do still stand by my assertion that it's more readable, specifically for test output.

princejwesley · 2017-09-27T05:19:47Z

lib/repl.js

+ return fn(error, frames);
+ }
+ frames.push(error);
+ return frames.reverse().join('\n at ');


@lance return frames.reverse() is sufficient?

@princejwesley Error.prepareStacktrace() is used to format the stack trace. It’s not required to return a string, but that is the behavior of the default implementation, and I think it would be unexpected if users suddenly started getting an array from the Error.stack property.

BridgeAR · 2017-09-27T21:36:07Z

lib/repl.js

-  .replace(/^repl:\d+\r?\n/, '')
-  .replace(/^\s+at\s.*\n?/gm, '');
+ .replace(/^repl:\d+\r?\n/, '')
+ .replace(/^\s+at\s.*\n?/gm, '');


Is this still necessary with the prepareStackTrace function in place? And if yes, cant we just move that part in the prepareStackTrace function as well?

I can poke around on it more - the change in formatting is because that's what I was doing and missed noticing that on my local diff. It's tricky because of the way Error.prepareStackTrace() works. It's only ever called once for a given error, and the return value is by default (and typically in custom impls as well) a string. Because of the way errors are handled in the repl and domain, in the case of a SyntaxError specifically, it's a string before we have an opportunity to filter the stack frames in prepareStackTrace(). So I left it rather than rework more of the overall error handling.

@BridgeAR I spent the morning looking into removing this and dealing with it in Error.prepareStackTrace() but the ramifications were wide reaching, especially with regard to how we handle SyntaxError as a Recoverable object so that defaulting into editor mode on multi-line input works as expected. I do think there could be some consolidation and overall improvement of the error handling, but I don't think this is the PR to do that in.

BridgeAR

Still LGTM. I do not mind the template string that badly if you feel strongly about it, go ahead.

lance · 2017-09-29T17:59:09Z

Multi-line template strings have been removed, and I think I have addressed all other issues. As semver-major we will need more reviews on this for it to land. /cc @nodejs/tsc

lance · 2017-09-29T18:00:13Z

CI: https://ci.nodejs.org/job/node-test-pull-request/10337/

Edit: CI failures appear to be unrelated

jasnell

LGTM with green CI

lance · 2017-10-02T20:33:44Z

CI 2: https://ci.nodejs.org/job/node-test-pull-request/10384/

lance · 2017-10-04T18:16:55Z

Do we need another @nodejs/tsc LGTM for this to land?

lance · 2017-10-09T22:19:45Z

Pretty sure CI 2 was green, but didn't comment on it here. Just to be safe, running CI 3.

https://ci.nodejs.org/job/node-test-pull-request/10555/

EDIT
All good except one unrelated failure.

1,pi3-raspbian-jessie

FATAL: remote file operation failed: /home/iojs/build/workspace/node-test-binary-arm@tmp/secretFiles/2eb224a6-31a9-45bd-9b95-2d7b70d88a69 at hudson.remoting.Channel@44c3e069:JNLP4-connect connection from static-n49-176-213-208.sbr2.nsw.optusnet.com.au/49.176.213.208:48902: java.io.IOException: Unable to delete '/home/iojs/build/workspace/node-test-binary-arm@tmp/secretFiles/2eb224a6-31a9-45bd-9b95-2d7b70d88a69/.nfs000000000029f98f00000022'. Tried 3 times (of a maximum of 3) waiting 0.1 sec between attempts.
java.nio.file.FileSystemException: /home/iojs/build/workspace/node-test-binary-arm@tmp/secretFiles/2eb224a6-31a9-45bd-9b95-2d7b70d88a69/.nfs000000000029f98f00000022: Device or resource busy
	at sun.nio.fs.UnixException.translateToIOException(UnixException.java:91)
	at sun.nio.fs.UnixException.rethrowAsIOException(UnixException.java:102)
	at sun.nio.fs.UnixException.rethrowAsIOException(UnixException.java:107)
	at sun.nio.fs.UnixFileSystemProvider.implDelete(UnixFileSystemProvider.java:244)
	at sun.nio.fs.AbstractFileSystemProvider.deleteIfExists(AbstractFileSystemProvider.java:108)
	at java.nio.file.Files.deleteIfExists(Files.java:1165)
	at hudson.Util.tryOnceDeleteFile(Util.java:287)
	at hudson.Util.deleteFile(Util.java:243)
	at hudson.FilePath.deleteRecursive(FilePath.java:1209)
	at hudson.FilePath.deleteContentsRecursive(FilePath.java:1218)
	at hudson.FilePath.deleteRecursive(FilePath.java:1200)
	at hudson.FilePath.access$1000(FilePath.java:195)
	at hudson.FilePath$14.invoke(FilePath.java:1179)
	at hudson.FilePath$14.invoke(FilePath.java:1176)
	at hudson.FilePath$FileCallableWrapper.call(FilePath.java:2731)
	at hudson.remoting.UserRequest.perform(UserRequest.java:153)
	at hudson.remoting.UserRequest.perform(UserRequest.java:50)
	at hudson.remoting.Request$2.run(Request.java:336)
	at hudson.remoting.InterceptingExecutorService$1.call(InterceptingExecutorService.java:68)
	at java.util.concurrent.FutureTask.run(FutureTask.java:266)
	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1142)
	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:617)
	at hudson.remoting.Engine$1$1.run(Engine.java:94)
	at java.lang.Thread.run(Thread.java:745)
Caused: java.io.IOException: Unable to delete '/home/iojs/build/workspace/node-test-binary-arm@tmp/secretFiles/2eb224a6-31a9-45bd-9b95-2d7b70d88a69/.nfs000000000029f98f00000022'. Tried 3 times (of a maximum of 3) waiting 0.1 sec between attempts.
	at hudson.Util.deleteFile(Util.java:248)
	at hudson.FilePath.deleteRecursive(FilePath.java:1209)
	at hudson.FilePath.deleteContentsRecursive(FilePath.java:1218)
	at hudson.FilePath.deleteRecursive(FilePath.java:1200)
	at hudson.FilePath.access$1000(FilePath.java:195)
	at hudson.FilePath$14.invoke(FilePath.java:1179)
	at hudson.FilePath$14.invoke(FilePath.java:1176)
	at hudson.FilePath$FileCallableWrapper.call(FilePath.java:2731)
	at hudson.remoting.UserRequest.perform(UserRequest.java:153)
	at hudson.remoting.UserRequest.perform(UserRequest.java:50)
	at hudson.remoting.Request$2.run(Request.java:336)
	at hudson.remoting.InterceptingExecutorService$1.call(InterceptingExecutorService.java:68)
	at java.util.concurrent.FutureTask.run(FutureTask.java:266)
	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1142)
	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:617)
	at hudson.remoting.Engine$1$1.run(Engine.java:94)
	at java.lang.Thread.run(Thread.java:745)
	at ......remote call to JNLP4-connect connection from static-n49-176-213-208.sbr2.nsw.optusnet.com.au/49.176.213.208:48902(Native Method)
	at hudson.remoting.Channel.attachCallSiteStackTrace(Channel.java:1545)
	at hudson.remoting.UserResponse.retrieve(UserRequest.java:253)
	at hudson.remoting.Channel.call(Channel.java:830)
	at hudson.FilePath.act(FilePath.java:985)
Caused: java.io.IOException: remote file operation failed: /home/iojs/build/workspace/node-test-binary-arm@tmp/secretFiles/2eb224a6-31a9-45bd-9b95-2d7b70d88a69 at hudson.remoting.Channel@44c3e069:JNLP4-connect connection from static-n49-176-213-208.sbr2.nsw.optusnet.com.au/49.176.213.208:48902
	at hudson.FilePath.act(FilePath.java:992)
	at hudson.FilePath.act(FilePath.java:974)
	at hudson.FilePath.deleteRecursive(FilePath.java:1176)
	at org.jenkinsci.plugins.credentialsbinding.impl.UnbindableDir$UnbinderImpl.unbind(UnbindableDir.java:84)
	at org.jenkinsci.plugins.credentialsbinding.impl.SecretBuildWrapper$1.tearDown(SecretBuildWrapper.java:113)
	at hudson.model.Build$BuildExecution.doRun(Build.java:174)
	at hudson.model.AbstractBuild$AbstractBuildExecution.run(AbstractBuild.java:534)
	at hudson.model.Run.execute(Run.java:1728)
	at hudson.matrix.MatrixRun.run(MatrixRun.java:146)
	at hudson.model.ResourceController.execute(ResourceController.java:98)
	at hudson.model.Executor.run(Executor.java:405)

lance · 2017-10-10T01:39:34Z

@jasnell I'd like to land this if possible. Since it's semver-major it needs another @nodejs/tsc approval.

For the record, I'm questioning what makes this a major version bump. What is the scenario where users would be impacted? The only thing I can think of is if the repl module is used programmatically and somewhere in the code there is error handling that is doing manual parsing of a stack trace to achieve some outcome (e.g. report only the relevant information) from the error. Surely that's pretty unlikely don't you think? And in any case, the REPLServer's internal stack frames and structure aren't documented anywhere, and almost certainly changed over time with other non-major releases.

BridgeAR · 2017-10-10T01:50:07Z

I am fine to not have it as semver-major. I just thought it would be good to have some other opinions about it. Too bad there was no response from the @nodejs/tsc about that.
I remove it again as I was the person who brought it up in the first place.

targos · 2017-10-10T01:50:37Z

I don't think this needs to be semver-major

Fishrock123 · 2017-10-10T02:15:09Z

I'm not certain if any behavior within a repl context can truly be considered semver-major.

jasnell · 2017-10-10T02:34:28Z

I'm good with semver-patch

lance · 2017-10-10T21:49:20Z

Thanks all for the feedback. I will land this in the next day or so modulo work stuff.

lance · 2017-10-11T19:04:43Z

Landed in c5f54b1

When a user executes code in the REPLServer which generates an exception, there is no need to display the REPLServer internal stack frames. PR-URL: #15351 Reviewed-By: Prince John Wesley <princejohnwesley@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Refs: #9601

When a user executes code in the REPLServer which generates an exception, there is no need to display the REPLServer internal stack frames. PR-URL: nodejs/node#15351 Reviewed-By: Prince John Wesley <princejohnwesley@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Refs: nodejs/node#9601

MylesBorins · 2017-10-23T21:43:22Z

This does not land cleanly on v8.x, would someone be willing to backport?

lance · 2017-10-24T20:05:38Z

@MylesBorins I will take care of it.

Edit: PR submitted

MylesBorins · 2017-11-16T22:56:56Z

Should this be backported to v6.x-staging? If yes please follow the guide and raise a backport PR, if not let me know or add the dont-land-on label.

lance · 2017-11-16T23:06:26Z

@MylesBorins what determines if it should be backported. The backporting guide doesn't give much in the way of guidelines. I'm happy to do it if it should.

When a user executes code in the REPLServer which generates an exception, there is no need to display the REPLServer internal stack frames. PR-URL: #15351 Backport-PR-URL: #16457 Reviewed-By: Prince John Wesley <princejohnwesley@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Refs: #9601

lance · 2018-05-03T20:56:03Z

@gibfahn @MylesBorins I waited too long and v6.x is in LTS maintenance mode. This, and the fact that the v8 Error API was different enough in 5.1.281 that it makes the backport non-trivial.

The link to the V8 wiki does not adequately describe the behavior I am seeing in my attempts to backport, but it appears that the custom stack formatter is only called if Error.captureStackTrace is also called. I believe it is related to this change to eager capture of the stack.

I am going to remove the backport-requested-v6.x label and add dont-land-on.

lance added the repl Issues and PRs related to the REPL subsystem. label Sep 12, 2017

lance requested a review from princejwesley September 12, 2017 00:54

nodejs-github-bot added the repl Issues and PRs related to the REPL subsystem. label Sep 12, 2017

bnoordhuis reviewed Sep 12, 2017

View reviewed changes

tniessen reviewed Sep 12, 2017

View reviewed changes

jasnell reviewed Sep 12, 2017

View reviewed changes

jasnell reviewed Sep 14, 2017

View reviewed changes

BridgeAR approved these changes Sep 19, 2017

View reviewed changes

jasnell added the semver-major PRs that contain breaking changes and should be released in the next major version. label Sep 20, 2017

lance added 4 commits September 26, 2017 20:38

repl: remove internal frames from runtime errors

8febecb

When a user executes code in the REPLServer which generates an exception, there is no need to display the REPLServer internal stack frames.

repl: use Error.prepareStackTrace to make pretty

1dd23f6

repl: inline Error.prepareStackTrace function

59e2637

repl: allow user-defined Error.prepareStackTrace

99b2187

princejwesley reviewed Sep 27, 2017

View reviewed changes

princejwesley approved these changes Sep 27, 2017

View reviewed changes

BridgeAR reviewed Sep 27, 2017

View reviewed changes

BridgeAR approved these changes Sep 27, 2017

View reviewed changes

lance added 2 commits September 29, 2017 11:01

repl: remove templated strings from stack tests

63fc83e

repl: more precise error checking

f710db8

lance requested a review from a team September 29, 2017 17:57

BridgeAR approved these changes Oct 1, 2017

View reviewed changes

jasnell approved these changes Oct 2, 2017

View reviewed changes

lance self-assigned this Oct 9, 2017

BridgeAR removed the semver-major PRs that contain breaking changes and should be released in the next major version. label Oct 10, 2017

lance closed this Oct 11, 2017

MylesBorins added the backport-requested-v8.x label Oct 23, 2017

lance mentioned this pull request Oct 24, 2017

[v8.x backport] repl: remove internal frames from runtime errors #16457

Closed

3 tasks

jasnell mentioned this pull request Oct 30, 2017

Proposal: 2017-10-31, Version 9.0.0 (Current) #15135

Merged

MylesBorins added the backport-requested-v6.x label Nov 16, 2017

gibfahn mentioned this pull request Dec 20, 2017

v8.9.4 proposal #17774

Merged

lance removed the backport-requested-v8.x label May 2, 2018

lance added dont-land-on-v6.x and removed backport-requested-v6.x labels May 3, 2018

repl: remove internal frames from runtime errors #15351

repl: remove internal frames from runtime errors #15351

Conversation

lance commented Sep 12, 2017

Checklist

Affected core subsystem(s)

bnoordhuis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lance Sep 12, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lance commented Sep 12, 2017

Choose a reason for hiding this comment

lance Sep 14, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

princejwesley Sep 19, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

princejwesley Sep 19, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lance Sep 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BridgeAR left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BridgeAR commented Sep 19, 2017

jasnell commented Sep 20, 2017

lance commented Sep 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BridgeAR left a comment

Choose a reason for hiding this comment

lance commented Sep 29, 2017

lance commented Sep 29, 2017 • edited Loading

jasnell left a comment

Choose a reason for hiding this comment

lance commented Oct 2, 2017 • edited Loading

lance commented Oct 4, 2017

lance commented Oct 9, 2017 • edited Loading

lance commented Oct 10, 2017

BridgeAR commented Oct 10, 2017

targos commented Oct 10, 2017

Fishrock123 commented Oct 10, 2017

jasnell commented Oct 10, 2017

lance commented Oct 10, 2017

lance commented Oct 11, 2017

MylesBorins commented Oct 23, 2017

lance commented Oct 24, 2017 • edited Loading

MylesBorins commented Nov 16, 2017

lance commented Nov 16, 2017

lance commented May 3, 2018

lance Sep 12, 2017 •

edited

Loading

lance Sep 14, 2017 •

edited

Loading

princejwesley Sep 19, 2017 •

edited

Loading

princejwesley Sep 19, 2017 •

edited

Loading

lance Sep 21, 2017 •

edited

Loading

lance commented Sep 29, 2017 •

edited

Loading

lance commented Oct 2, 2017 •

edited

Loading

lance commented Oct 9, 2017 •

edited

Loading

lance commented Oct 24, 2017 •

edited

Loading