Implement HostPromiseRejectionTracker (#2530) #4608

rhuanjl · 2018-01-27T16:27:23Z

Responding to #2530

This PR contains:

Internal CC machinery for HostPromiseRejectionTracker ecmaspec 25.4.1.9 - this should enable any host to implement a handler for uncaught promise rejections e.g. the method defined here WhatW spec 8.1.3.12
A very simplistic implementation in ch (behind a command line flag)
Test case

I'm expecting feedback/changes before this is merged but wanted to get other people's thoughts on it in this state. Please give me your thoughts/comments.

Cross ref: Edge uservoice request for this feature - note this PR will not put the feature in edge - it just creates the hooks the edge development team would have to track them through.

cc @liminzhu @saschanaz @fatcerberus @MSLaguana

Notes:
1. Not spec compliant for "await" EDIT: snip - re-read spec this comment was wrong what I've done in this PR is correct

2. The ch implementation I've done is pretty bad, it simply prints to the terminal whenever a notification is made, either handled or rejected - reasonable for testing that the interface is operational but certainly not what the WhatW spec would have a Host do.

3. Parameters passed to the callback function - per spec HostPromiseRejectionTracker should be passed the Promise and rejected/handled. I've gone for:
a) the promise
b) the promise's result - I know this could be retrieved from the promise object but I've added it for implementer's convenience
c) rejected/handled boolean - true for handled false for rejected - possibly should be an int or an enum of some kind

rhuanjl · 2018-01-27T18:28:13Z

So all the windows test/debug builds are failing on the test I've written. Please could someone tell me how to make a baseline file that will pass on windows? (Reading the output shows that it's printing the right text)

My guess is that this is something to do with line endings but I've tried setting my text editor to do CRLF line endings and it's still not working.

boingoing

This is a nice addition. Thanks a lot for taking the time to implement this support.

I gave it a look over and have a couple of cursory questions.

We only call SetIsHandled in JavascriptPromise::CreateThenPromise right now. Is that the only time we know that a promise is definitely handled?

I'm a little confused about the third argument (rejected) to the promise rejection tracker function. It looks like this indicates handled? All of the promises we pass to this function were rejected, yes?

Otherwise, seems sound. Don't worry too much about ch test hook quality - as long as it's on-par with other stuff in there shouldn't be a problem.

Your line-endings may be a problem. Ideally, try to make the baselines and commit them on windows. 😁 We have some settings in our gitconfig which may be messing up your files. I guess you could run unix2dos on them.

As for where to check-in, you retarget onto master. We won't be able to take this in release/1.8 and it's unlikely we'll cut more Node-ChakraCore releases using release/1.9.

I will give this a closer look and provide some more specific feedbacks but thanks again for digging in and contributing here.

rhuanjl · 2018-01-28T13:04:27Z

@boingoing thanks for the comments:

As far as I can see CreateThenPromise is the only place we need SetIsHandled other than possibly an extra point for async functions. Both .then and .catch go through CreateThenPromise. await (for async) also goes through it but too late - see my longer comment above.
The third argument should probably change or at least be reversed - it's currently true when an unhandled rejection is notified and false for a "handled" notification - should perhaps be an int to allow for more options in the future - wasn't sure how to do an enum that would work from within JavascriptLibrary.cpp and also be visible from ChakraCore.h - hence not using an enum for this.
Thanks for the tip - unix2dos seems to have done the trick with the baseline
I've retargeted and rebased to master as requested

fatcerberus · 2018-01-28T16:45:05Z

I'm a little confused about the third argument (rejected) to the promise rejection tracker function. It looks like this indicates handled? All of the promises we pass to this function were rejected, yes?

To answer that question, yes, all promises passed to the callback are rejected. The third parameter indicates whether the promise has just been rejected without a handler, or if it was rejected previously and is only now having a handler attached. It corresponds to the operation parameter in the spec here:
https://tc39.github.io/ecma262/#sec-host-promise-rejection-tracker

liminzhu · 2018-01-31T21:53:16Z

(only here to comment on the API surface change, not implementation)

Thanks for this very welcomed API addition @rhuanjl ! It looks nice and I appreciate it directly mirroring concepts in the spec. Couple of comments -

@bterlson from the ecma spec perspective, how stable is https://tc39.github.io/ecma262/#sec-host-promise-rejection-tracker and the likelihood/risk it will be modified? I'm asking for guidance for @rhuanjl 's early comment of whether the reject bool in JsHostPromiseRejectionTrackerCallback (mirroring operation in HostPromiseRejectionTracker) should be as-is or changed to perhaps a var for reflexibility but more complexity to read the value.
@rhuanjl can you move the APIs to ChakraCore.h? ChakraCommon is for APIs living in both inbox Chakra and ChakraCore. We do want to put it in Chakra some time, but to put it there we'll have to go through a review process internally with the Windows API champions which will take some time, so it's a good idea to put them in ChakraCore.h first where all ChakraCore-only APIs lie.
Just to be clear on what resolution is, if I have let p = Promise.reject(1), I shall see resolution with value 1?

rhuanjl · 2018-01-31T22:39:20Z

@liminzhu thanks for the comments:
2. I've moved it offline, will test and ensure nothing breaks and re-push later
3. That is correct

fatcerberus

Just a couple of naming/semantic issues and a copy/paste mishap. Other than that the change looks good, nice and clean. Works great in miniSphere, too. 🐖

fatcerberus · 2018-02-01T06:30:50Z

lib/Jsrt/ChakraCore.h

+///     A Promise Rejection Tracker callback may be set - which will then be called when this occurs.
+/// </remarks>
+/// <param name="promise">The promise object, represented as a JsValueRef.</param>
+/// <param name="resolution">The value/cause of the rejection, represented as a JsValueRef.</param>


resolution should be renamed, to either reason--or if you want something more generic, value. As far as promises go, "resolve" usually only refers to fulfillment (e.g. Promise.resolve() is never a rejection unless you specifically pass it a rejected promise) so the current name is maybe a bit confusing.

fatcerberus · 2018-02-01T06:33:09Z

lib/Jsrt/ChakraCore.h

+/// </remarks>
+/// <param name="promise">The promise object, represented as a JsValueRef.</param>
+/// <param name="resolution">The value/cause of the rejection, represented as a JsValueRef.</param>
+/// <param name="reject">Boolean - true if the promise has just been rejected with no handler, 


As discussed offline I'm still of the opinion that if we keep this as a boolean, the meaning should be reversed, i.e. it should be false for "rejected" and true for "handler attached".

fatcerberus · 2018-02-01T06:37:17Z

lib/Jsrt/ChakraCore.h

+///     Requires an active script context.
+///     </para>
+/// </remarks>
+/// <param name="callbackState"> The callback function being set.</param>


This should be

/// <param name="promiseRejectionTrackerCallback"> ... </param>

Correct?

rhuanjl · 2018-02-01T17:54:28Z

@liminzhu I've adjusted for my friend @fatcerberus 's comments which seemed sensible to me which included:

changing resolution to reason - as the value given as that parameter is the reason the promise was rejected per spec.
swapping the 3rd parameter so that true indicates rejectionHandled and false indicates promiseRejected

I have also moved the API to chakracore.h

@akroshg do you know when you will have a chance to review this?

CI failure appears to be unrelated (I can repro the fail with a clean download of master or 1.9 without applying this PR) I think CI on both master and 1.9 has been blocked since this PR: #4625

obastemur · 2018-02-01T19:28:44Z

#4632 should fix it

jackhorton

Some minor nitpicks. @boingoing one benefit of putting this in release/1.9 would be that we could fix Node-ChakraCore's feature gap around this sooner? Not sure if that matters.

jackhorton · 2018-02-01T21:51:20Z

lib/Runtime/Library/JavascriptLibrary.cpp

+    void JavascriptLibrary::SetNativeHostPromiseRejectionTrackerCallback(HostPromiseRejectionTrackerCallback function)
+    {
+        this->nativeHostPromiseRejectionTracker = function;
+        this->hostShouldTrackPromiseRejections = true;


Nit: set nativeHostPromiseRejectionTracker to nullptr upfront so that you can avoid this bool and just check if the function pointer is nullptr or not.

Agreed - though I've now added a callbackState in the place of this.

jackhorton · 2018-02-01T21:52:24Z

lib/Runtime/Library/JavascriptPromise.h

@@ -461,6 +461,8 @@ namespace Js
            PromiseStatusCode_HasRejection
        };

+        bool GetIsHandled() {return isHandled;}
+        void SetIsHandled() {isHandled = true;}


Nit: spacing

kfarnung · 2018-02-01T22:36:04Z

lib/Jsrt/ChakraCore.h

@@ -993,5 +1008,26 @@ CHAKRA_API
        _In_ JsValueRef object,
        _In_ JsValueRef key,
        _Out_ bool *hasOwnProperty);
+
+


nit: extra newline #Closed

kfarnung · 2018-02-01T22:37:25Z

lib/Runtime/Library/JavascriptLibrary.h

@@ -949,6 +953,9 @@ namespace Js
        JavascriptFunction* GetThrowerFunction() const { return throwerFunction; }

        void SetNativeHostPromiseContinuationFunction(PromiseContinuationCallback function, void *state);
+        void SetNativeHostPromiseRejectionTrackerCallback(HostPromiseRejectionTrackerCallback function);
+        void CallNativeHostPromiseRejectionTracker(Var promise, Var reason, bool handled);
+


nit: extra newline #Closed

kfarnung · 2018-02-01T22:41:30Z

bin/ch/ch.cpp

+        if(HostConfigFlags::flags.TrackRejectedPromises)
+        {
+            ChakraRTInterface::JsSetHostPromiseRejectionTracker(WScriptJsrt::PromiseRejectionTrackerCallback);
+        }
        len = strlen(fullPath);


nit: add a newline here #Closed

Between the closing curly and the next line #Closed

kfarnung · 2018-02-01T22:43:03Z

bin/ch/WScriptJsrt.cpp

+    Assert(reason != JS_INVALID_REFERENCE);
+    if(!handled)
+    {
+        wprintf(_u("Uncaught promise rejection\n"));


Should we also print out the message to ensure that the reason is being set correctly? #Closed

If I may add further commentary to this, it might make more sense to say "Promise rejected with no handler attached" rather than "Uncaught promise rejection". We don't know whether or not it will remain uncaught at this time.

I've updated to print the reason.

I've left it as "Uncaught promise rejection" - as at the time of notification it is uncaught - it may be handled later but it is not at the moment of notification.

kfarnung

LGTM with minor fixes

fatcerberus · 2018-02-01T23:22:39Z

bin/ChakraCore/ChakraCore.def

@@ -60,3 +60,5 @@ JsLessThan
 JsLessThanOrEqual

 JsCreateEnhancedFunction
+
+JsSetHostPromiseRejectionTracker


Should be a newline at end of file.

fatcerberus · 2018-02-01T23:26:02Z

bin/ch/ChakraRtInterface.h

@@ -356,6 +358,7 @@ class ChakraRTInterface
    static JsErrorCode WINAPI JsGetValueType(JsValueRef value, JsValueType *type) { return HOOK_JS_API(GetValueType(value, type)); }
    static JsErrorCode WINAPI JsSetIndexedProperty(JsValueRef object, JsValueRef index, JsValueRef value) { return HOOK_JS_API(SetIndexedProperty(object, index, value)); }
    static JsErrorCode WINAPI JsSetPromiseContinuationCallback(JsPromiseContinuationCallback callback, void *callbackState) { return HOOK_JS_API(SetPromiseContinuationCallback(callback, callbackState)); }
+    static JsErrorCode WINAPI JsSetHostPromiseRejectionTracker(JsHostPromiseRejectionTrackerCallback callback) { return HOOK_JS_API(SetHostPromiseRejectionTracker(callback)); }


Looking at the diff and seeing that literally all the other callbacks surrounding this get a callbackState argument, I think we should have it here for consistency too, even if I question its real-world utility in this case.

fatcerberus · 2018-02-01T23:28:40Z

bin/ch/WScriptJsrt.cpp

+    Assert(reason != JS_INVALID_REFERENCE);
+    if(!handled)
+    {
+        wprintf(_u("Uncaught promise rejection\n"));


If I may add further commentary to this, it might make more sense to say "Promise rejected with no handler attached" rather than "Uncaught promise rejection". We don't know whether or not it will remain uncaught at this time.

rhuanjl · 2018-02-02T00:14:54Z

Thanks for the reviews @kfarnung @jackhorton @fatcerberus
I think I've addressed everything. I haven't squashed yet in case anyone wants to check my new changes against their review points.

kfarnung · 2018-02-02T20:30:32Z

@curtisman @akroshg Can one of you take a look? #Closed

rhuanjl · 2018-02-04T16:27:02Z

@kfarnung sorry I didn't respond on Friday, I was away without a computer.

I've updated to latest master and squashed including your commit and adding some extra tests as requested by @MSLaguana

MSLaguana · 2018-02-05T17:16:40Z

Thanks for adding those additional tests, looks good to me!

kfarnung · 2018-02-05T19:35:41Z

test/es7/PromiseRejectionTracking.js

+
+let tests = [
+	{
+		name: "Reject promise with no reactions.",


super nit: looks like you used tabs here, can you switch over to spaces to match the majority of the files in the same directory? #Closed

Sorry about that, fixed.

rhuanjl · 2018-02-05T20:19:42Z

@dotnet-bot test Windows arm_test please

rhuanjl · 2018-02-07T20:00:59Z

@kfarnung any update on this?

kfarnung

A few more nit fixes

kfarnung · 2018-02-07T22:06:19Z

bin/ch/WScriptJsrt.cpp

+    JsValueRef strValue;
+    JsErrorCode error = ChakraRTInterface::JsConvertValueToString(reason, &strValue);
+
+    if(!handled)


nit: spacing between if and condition #Closed

kfarnung · 2018-02-07T22:06:39Z

bin/ch/ch.cpp

@@ -757,6 +757,11 @@ HRESULT ExecuteTest(const char* fileName)
            IfFailGo(E_FAIL);
        }

+        if(HostConfigFlags::flags.TrackRejectedPromises)


nit: space between if and condition #Closed

kfarnung · 2018-02-07T22:06:53Z

lib/Runtime/Library/JavascriptLibrary.cpp

+
+    void JavascriptLibrary::CallNativeHostPromiseRejectionTracker(Var promise, Var reason, bool handled)
+    {
+        if(nativeHostPromiseRejectionTracker != nullptr)


nit: space between if and condition #Closed

kfarnung · 2018-02-07T22:07:09Z

lib/Runtime/Library/JavascriptLibrary.cpp

+
+    void JavascriptLibrary::CallNativeHostPromiseRejectionTracker(Var promise, Var reason, bool handled)
+    {
+        if(nativeHostPromiseRejectionTracker != nullptr)


nit: this->nativeHostPromiseRejectionTracker #Closed

kfarnung · 2018-02-07T22:07:32Z

lib/Runtime/Library/JavascriptLibrary.cpp

+            BEGIN_LEAVE_SCRIPT(scriptContext);
+            try
+            {
+               nativeHostPromiseRejectionTracker(promise, reason, handled, this->nativeHostPromiseContinuationFunctionState);


nit: this->nativeHostPromiseRejectionTracker #Closed

kfarnung · 2018-02-07T22:08:01Z

lib/Runtime/Library/JavascriptPromise.cpp

@@ -660,6 +661,10 @@ namespace Js
        {
            reactions = this->GetRejectReactions();
            newStatus = PromiseStatusCode_HasRejection;
+            if(!GetIsHandled())


nit: space between if and condition #Closed

kfarnung · 2018-02-07T22:08:08Z

lib/Runtime/Library/JavascriptPromise.cpp

@@ -838,13 +843,19 @@ namespace Js
            EnqueuePromiseReactionTask(resolveReaction, sourcePromise->result, scriptContext);
            break;
        case PromiseStatusCode_HasRejection:
+            if(!sourcePromise->GetIsHandled())


nit: space between if and condition #Closed

kfarnung · 2018-02-07T22:10:00Z

Hey @rhuanjl, thanks for your patience. I'm still doing some local testing. I pushed a minor commit to exclude the new test from one of our suites. #Closed

rhuanjl · 2018-02-07T22:30:51Z

@kfarnung I fixed the new nits, I'll hold off on squashing for now as I assume there's more testing and at least one more review to come?

kfarnung · 2018-02-07T22:43:39Z

@liminzhu @bterlson Any concerns with the API surface for this change? I didn't see any follow up on the previous question:

@bterlson from the ecma spec perspective, how stable is https://tc39.github.io/ecma262/#sec-host-promise-rejection-tracker and the likelihood/risk it will be modified? I'm asking for guidance for @rhuanjl 's early comment of whether the reject bool in JsHostPromiseRejectionTrackerCallback (mirroring operation in HostPromiseRejectionTracker) should be as-is or changed to perhaps a var for reflexibility but more complexity to read the value.

@rhuanjl I don't see any other issues with this change, I'm just trying to make sure I cover all the bases. #Closed

rhuanjl · 2018-02-09T18:58:54Z

@kfarnung thanks very much for the help with this. Do you know if there is any chance of this getting landed soon.

I was hoping to prepare a PR for Promise.prototype.finally this weekend but it will be affecting several of the same files and nervous I'll end up with merge conflicts

kfarnung · 2018-02-09T19:16:26Z

I'm still waiting for feedback from @liminzhu and @akroshg before landing this. I don't have any concerns with the change and will land it as soon as I have approval from them. #Closed

akroshg · 2018-02-12T20:04:42Z

lib/Runtime/Library/JavascriptPromise.h

@@ -470,6 +472,7 @@ namespace Js
    protected:
        Field(PromiseStatus) status;
        Field(Var) result;
+        Field(bool) isHandled;


nit: move this up above var just to get better packing? instead of 40 bytes it can be in 32 bytes.

akroshg · 2018-02-12T20:07:48Z

test/es7/rlexe.xml

+      <files>PromiseRejectionTracking.js</files>
+      <compile-flags>-TrackRejectedPromises -args summary -endargs -nodeferparse</compile-flags>
+      <baseline>PromiseRejectionTracking.baseline</baseline>
+      <tags>exclude_jshost</tags>


exclude_jshost [](start = 12, length = 14)

Great. Thanks for covering with detailed test cases. @[email protected] should we be implementing this in jshost just to get consistency.

I was the one who added the tag. I think this is something we should do in the near future, it's just a matter of scheduling.

/cc @boingoing

akroshg · 2018-02-12T20:13:28Z

@rhuanjl thanks for PR. Looks good to me. (apologize for adding a late comment).

liminzhu · 2018-02-12T21:01:58Z

Looks good to me too!

1. Internal CC machinary for unhandled promise rejection. 2. Simplisitic implimentation in ch 3. Test case

rhuanjl · 2018-02-12T23:41:27Z

Fixed last point and updated to latest.

Thanks for reviews everyone; sorry for the number of style issues, I'll try and be more consistent in future.

@liminzhu

Merge pull request #4608 from rhuanjl:HostPromiseRejection Responding to #2530 This PR contains: 1. Internal CC machinery for HostPromiseRejectionTracker [ecmaspec 25.4.1.9](https://tc39.github.io/ecma262/#sec-host-promise-rejection-tracker) - this should enable any host to implement a handler for uncaught promise rejections e.g. the method defined here [WhatW spec 8.1.3.12](https://html.spec.whatwg.org/multipage/webappapis.html#unhandled-promise-rejections) 2. A very simplistic implementation in ch (behind a command line flag) 3. Test case I'm expecting feedback/changes before this is merged but wanted to get other people's thoughts on it in this state. Please give me your thoughts/comments. Cross ref: [Edge uservoice request for this feature](https://wpdev.uservoice.com/forums/257854-microsoft-edge-developer/suggestions/16665397-detect-unhandled-promise-rejection?tracking_code=4f4e218df62afae7db37fb5be5f4ff1c) - note this PR will not put the feature in edge - it just creates the hooks the edge development team would have to track them through. **cc** @liminzhu @saschanaz @fatcerberus @MSLaguana **Notes:** **1. Not spec compliant for "await" EDIT: snip - re-read spec this comment was wrong what I've done in this PR is correct** **2. The ch implementation I've done is pretty bad,** it simply prints to the terminal whenever a notification is made, either handled or rejected - reasonable for testing that the interface is operational but certainly not what the WhatW spec would have a Host do. **3. Parameters passed to the callback function** - per spec HostPromiseRejectionTracker should be passed the Promise and rejected/handled. I've gone for: a) the promise b) the promise's result - I know this could be retrieved from the promise object but I've added it for implementer's convenience c) rejected/handled boolean - true for handled false for rejected - possibly should be an int or an enum of some kind

kfarnung · 2018-02-13T00:30:42Z

@rhuanjl Thanks for your contribution!

liminzhu · 2018-02-13T00:35:03Z

Thanks @rhuanjl ! Now that this has been merged, can I ask you to send a quick PR to https://github.com/Microsoft/ChakraCore-wiki to update the docs as well (this should be an example for where to make the changes). I'm happy to do that as well so let me know.

rhuanjl force-pushed the HostPromiseRejection branch from 6d98ad6 to f2df324 Compare January 27, 2018 17:58

boingoing reviewed Jan 28, 2018

View reviewed changes

rhuanjl changed the base branch from release/1.8 to master January 28, 2018 12:52

rhuanjl force-pushed the HostPromiseRejection branch from 087e3fe to 4142600 Compare January 28, 2018 12:57

rhuanjl force-pushed the HostPromiseRejection branch from cfbd914 to dcdaf1f Compare January 28, 2018 15:52

boingoing requested a review from akroshg January 29, 2018 02:50

rhuanjl force-pushed the HostPromiseRejection branch from 4769f67 to 3a1dc62 Compare January 31, 2018 21:11

rhuanjl force-pushed the HostPromiseRejection branch 3 times, most recently from c1af310 to e7c64ab Compare February 1, 2018 00:15

fatcerberus suggested changes Feb 1, 2018

View reviewed changes

rhuanjl force-pushed the HostPromiseRejection branch 3 times, most recently from 8a15dc3 to abf54d1 Compare February 1, 2018 08:34

rhuanjl force-pushed the HostPromiseRejection branch from abf54d1 to fe5a34d Compare February 1, 2018 21:42

jackhorton reviewed Feb 1, 2018

View reviewed changes

kfarnung reviewed Feb 1, 2018

View reviewed changes

kfarnung approved these changes Feb 1, 2018

View reviewed changes

fatcerberus reviewed Feb 1, 2018

View reviewed changes

rhuanjl force-pushed the HostPromiseRejection branch from 3e050b8 to 87dc5ef Compare February 4, 2018 16:25

kfarnung reviewed Feb 5, 2018

View reviewed changes

rhuanjl force-pushed the HostPromiseRejection branch from 87dc5ef to 812e237 Compare February 5, 2018 19:41

rhuanjl force-pushed the HostPromiseRejection branch from 812e237 to 3b29e2f Compare February 7, 2018 07:22

kfarnung approved these changes Feb 7, 2018

View reviewed changes

rhuanjl mentioned this pull request Feb 7, 2018

Get at module export table in native code #3616

Closed

fatcerberus mentioned this pull request Feb 8, 2018

Throw in async function after await is not caught by debugger #4630

Open

rhuanjl force-pushed the HostPromiseRejection branch from d93de9e to dfff5a9 Compare February 8, 2018 17:36

akroshg reviewed Feb 12, 2018

View reviewed changes

HostPromiseRejection tracker:

0b61d16

1. Internal CC machinary for unhandled promise rejection. 2. Simplisitic implimentation in ch 3. Test case

rhuanjl force-pushed the HostPromiseRejection branch from dfff5a9 to 0b61d16 Compare February 12, 2018 23:22

chakrabot merged commit 0b61d16 into chakra-core:master Feb 13, 2018

rhuanjl deleted the HostPromiseRejection branch February 13, 2018 00:31

Implement HostPromiseRejectionTracker (#2530) #4608

Implement HostPromiseRejectionTracker (#2530) #4608

Conversation

rhuanjl commented Jan 27, 2018 • edited Loading

rhuanjl commented Jan 27, 2018 • edited Loading

boingoing left a comment • edited Loading

Choose a reason for hiding this comment

rhuanjl commented Jan 28, 2018 • edited Loading

fatcerberus commented Jan 28, 2018

liminzhu commented Jan 31, 2018 • edited Loading

rhuanjl commented Jan 31, 2018

fatcerberus 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

rhuanjl commented Feb 1, 2018 • edited Loading

obastemur commented Feb 1, 2018

jackhorton 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

kfarnung Feb 1, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kfarnung Feb 1, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kfarnung Feb 1, 2018 • edited Loading

Choose a reason for hiding this comment

kfarnung Feb 1, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kfarnung Feb 1, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kfarnung 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

rhuanjl commented Feb 2, 2018

kfarnung commented Feb 2, 2018 • edited Loading

rhuanjl commented Feb 4, 2018 • edited Loading

MSLaguana commented Feb 5, 2018

kfarnung Feb 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rhuanjl commented Feb 5, 2018

rhuanjl commented Feb 7, 2018

kfarnung left a comment

Choose a reason for hiding this comment

kfarnung Feb 7, 2018 • edited Loading

Choose a reason for hiding this comment

kfarnung Feb 7, 2018 • edited Loading

Choose a reason for hiding this comment

kfarnung Feb 7, 2018 • edited Loading

Choose a reason for hiding this comment

kfarnung Feb 7, 2018 • edited Loading

Choose a reason for hiding this comment

kfarnung Feb 7, 2018 • edited Loading

Choose a reason for hiding this comment

kfarnung Feb 7, 2018 • edited Loading

Choose a reason for hiding this comment

kfarnung Feb 7, 2018 • edited Loading

Choose a reason for hiding this comment

kfarnung commented Feb 7, 2018 • edited Loading

rhuanjl commented Feb 7, 2018

kfarnung commented Feb 7, 2018 • edited Loading

rhuanjl commented Feb 9, 2018

kfarnung commented Feb 9, 2018 • 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

rhuanjl commented Jan 27, 2018 •

edited

Loading

rhuanjl commented Jan 27, 2018 •

edited

Loading

boingoing left a comment •

edited

Loading

rhuanjl commented Jan 28, 2018 •

edited

Loading

liminzhu commented Jan 31, 2018 •

edited

Loading

rhuanjl commented Feb 1, 2018 •

edited

Loading

kfarnung Feb 1, 2018 •

edited

Loading

kfarnung Feb 1, 2018 •

edited

Loading

kfarnung Feb 1, 2018 •

edited

Loading

kfarnung Feb 1, 2018 •

edited

Loading

kfarnung Feb 1, 2018 •

edited

Loading

kfarnung commented Feb 2, 2018 •

edited

Loading

rhuanjl commented Feb 4, 2018 •

edited

Loading

kfarnung Feb 5, 2018 •

edited

Loading

kfarnung Feb 7, 2018 •

edited

Loading

kfarnung Feb 7, 2018 •

edited

Loading

kfarnung Feb 7, 2018 •

edited

Loading

kfarnung Feb 7, 2018 •

edited

Loading

kfarnung Feb 7, 2018 •

edited

Loading

kfarnung Feb 7, 2018 •

edited

Loading

kfarnung Feb 7, 2018 •

edited

Loading

kfarnung commented Feb 7, 2018 •

edited

Loading

kfarnung commented Feb 7, 2018 •

edited

Loading

kfarnung commented Feb 9, 2018 •

edited

Loading

liminzhu commented Feb 13, 2018 •

edited

Loading