From 093400681700c7c441cde55f3a22900a5ebb5635 Mon Sep 17 00:00:00 2001
From: Luke Barnard <luke@matrix.org>
Date: Tue, 12 Jun 2018 14:13:09 +0100
Subject: [PATCH 01/17] Track decryption success/failure rate with piwik

Emit a piwik event when a decryption occurs in the category "E2E" with
the action "Decryption result" and the name either "failure" or
"success".

NB: This will cause Riot to a lot of networking when decrypting many
events. One HTTP request per decrypted event should be expected.
---
 src/components/structures/MatrixChat.js | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/src/components/structures/MatrixChat.js b/src/components/structures/MatrixChat.js
index bd8f66163f..d4969a8bf9 100644
--- a/src/components/structures/MatrixChat.js
+++ b/src/components/structures/MatrixChat.js
@@ -1308,6 +1308,15 @@ export default React.createClass({
             }
         });
 
+        // XXX: This will do a HTTP request for each Event.decrypted event
+        cli.on("Event.decrypted", (e) => {
+            if (e.isDecryptionFailure()) {
+                Analytics.trackEvent('E2E', 'Decryption result', 'failure');
+            } else {
+                Analytics.trackEvent('E2E', 'Decryption result', 'success');
+            }
+        });
+
         const krh = new KeyRequestHandler(cli);
         cli.on("crypto.roomKeyRequest", (req) => {
             krh.handleKeyRequest(req);

From 64b86108d06ceab1d24e46c5045e8fcd50aa88c8 Mon Sep 17 00:00:00 2001
From: Luke Barnard <luke@matrix.org>
Date: Wed, 13 Jun 2018 09:38:23 +0100
Subject: [PATCH 02/17] Only track decryption failures

---
 src/components/structures/MatrixChat.js | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/src/components/structures/MatrixChat.js b/src/components/structures/MatrixChat.js
index d4969a8bf9..b5c5efa230 100644
--- a/src/components/structures/MatrixChat.js
+++ b/src/components/structures/MatrixChat.js
@@ -1311,9 +1311,7 @@ export default React.createClass({
         // XXX: This will do a HTTP request for each Event.decrypted event
         cli.on("Event.decrypted", (e) => {
             if (e.isDecryptionFailure()) {
-                Analytics.trackEvent('E2E', 'Decryption result', 'failure');
-            } else {
-                Analytics.trackEvent('E2E', 'Decryption result', 'success');
+                Analytics.trackEvent('E2E', 'Decryption failure');
             }
         });
 

From 230de44071196151fbc247f0a90a9b9954286d38 Mon Sep 17 00:00:00 2001
From: Luke Barnard <luke@matrix.org>
Date: Wed, 13 Jun 2018 09:38:57 +0100
Subject: [PATCH 03/17] Adjust comment

---
 src/components/structures/MatrixChat.js | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/components/structures/MatrixChat.js b/src/components/structures/MatrixChat.js
index b5c5efa230..c94baa63ef 100644
--- a/src/components/structures/MatrixChat.js
+++ b/src/components/structures/MatrixChat.js
@@ -1309,6 +1309,7 @@ export default React.createClass({
         });
 
         // XXX: This will do a HTTP request for each Event.decrypted event
+        // if the decryption was a failure
         cli.on("Event.decrypted", (e) => {
             if (e.isDecryptionFailure()) {
                 Analytics.trackEvent('E2E', 'Decryption failure');

From 3cadbd3974a4ccc7aae395c96f63332dced9ff72 Mon Sep 17 00:00:00 2001
From: Luke Barnard <luke@matrix.org>
Date: Wed, 13 Jun 2018 11:21:26 +0100
Subject: [PATCH 04/17] Include decryption error in decryption failure metrics

---
 src/components/structures/MatrixChat.js | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/components/structures/MatrixChat.js b/src/components/structures/MatrixChat.js
index c94baa63ef..da729a1a2d 100644
--- a/src/components/structures/MatrixChat.js
+++ b/src/components/structures/MatrixChat.js
@@ -1312,7 +1312,7 @@ export default React.createClass({
         // if the decryption was a failure
         cli.on("Event.decrypted", (e) => {
             if (e.isDecryptionFailure()) {
-                Analytics.trackEvent('E2E', 'Decryption failure');
+                Analytics.trackEvent('E2E', 'Decryption failure', 'ev.content.body: ' + e.getContent().body);
             }
         });
 

From 62601d657da3afa34a392e3b02ba8800785f0000 Mon Sep 17 00:00:00 2001
From: Luke Barnard <luke@matrix.org>
Date: Fri, 15 Jun 2018 13:33:07 +0100
Subject: [PATCH 05/17] Implement DecryptionFailureTracker for less agressive
 tracking

Instead of pinging Analytics once per failed decryption, add the failure
to a list of failures and after a grace period, add it to a FIFO for
tracking. On an interval, track a single failure from the FIFO.
---
 src/DecryptionFailureTracker.js         | 124 +++++++++++++++++++++++
 src/components/structures/MatrixChat.js |  16 +--
 test/DecryptionFailureTracker-test.js   | 128 ++++++++++++++++++++++++
 3 files changed, 262 insertions(+), 6 deletions(-)
 create mode 100644 src/DecryptionFailureTracker.js
 create mode 100644 test/DecryptionFailureTracker-test.js

diff --git a/src/DecryptionFailureTracker.js b/src/DecryptionFailureTracker.js
new file mode 100644
index 0000000000..0f86093209
--- /dev/null
+++ b/src/DecryptionFailureTracker.js
@@ -0,0 +1,124 @@
+/*
+Copyright 2018 New Vector Ltd
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+class DecryptionFailure {
+    constructor(failedEventId) {
+        this.failedEventId = failedEventId;
+        this.ts = Date.now();
+    }
+}
+
+export default class DecryptionFailureTracker {
+    // Array of items of type DecryptionFailure. Every `CHECK_INTERVAL_MS`, this list
+    // is checked for failures that happened > `GRACE_PERIOD_MS` ago. Those that did
+    // are added to `failuresToTrack`.
+    failures = [];
+
+    // Every TRACK_INTERVAL_MS (so as to spread the number of hits done on Analytics),
+    // one DecryptionFailure of this FIFO is removed and tracked.
+    failuresToTrack = [];
+
+    // Spread the load on `Analytics` by sending at most 1 event per
+    // `TRACK_INTERVAL_MS`.
+    static TRACK_INTERVAL_MS = 1000;
+
+    // Call `checkFailures` every `CHECK_INTERVAL_MS`.
+    static CHECK_INTERVAL_MS = 5000;
+
+    // Give events a chance to be decrypted by waiting `GRACE_PERIOD_MS` before moving
+    // the failure to `failuresToTrack`.
+    static GRACE_PERIOD_MS = 5000;
+
+    constructor(fn) {
+        if (!fn || typeof fn !== 'function') {
+            throw new Error('DecryptionFailureTracker requires tracking function');
+        }
+
+        this.trackDecryptionFailure = fn;
+    }
+
+    eventDecrypted(e) {
+        if (e.isDecryptionFailure()) {
+            this.addDecryptionFailureForEvent(e);
+        } else {
+            // Could be an event in the failures, remove it
+            this.removeDecryptionFailuresForEvent(e);
+        }
+    }
+
+    addDecryptionFailureForEvent(e) {
+        this.failures.push(new DecryptionFailure(e.getId()));
+    }
+
+    removeDecryptionFailuresForEvent(e) {
+        this.failures = this.failures.filter((f) => f.failedEventId !== e.getId());
+    }
+
+    /**
+     * Start checking for and tracking failures.
+     * @return {function} a function that clears state and causes DFT to stop checking for
+     * and tracking failures.
+     */
+    start() {
+        const checkInterval = setInterval(
+            () => this.checkFailures(Date.now()),
+            DecryptionFailureTracker.CHECK_INTERVAL_MS,
+        );
+
+        const trackInterval = setInterval(
+            () => this.trackFailure(),
+            DecryptionFailureTracker.TRACK_INTERVAL_MS,
+        );
+
+        return () => {
+            clearInterval(checkInterval);
+            clearInterval(trackInterval);
+
+            this.failures = [];
+            this.failuresToTrack = [];
+        };
+    }
+
+    /**
+     * Mark failures that occured before nowTs - GRACE_PERIOD_MS as failures that should be
+     * tracked. Only mark one failure per event ID.
+     * @param {number} nowTs the timestamp that represents the time now.
+     */
+    checkFailures(nowTs) {
+        const failuresGivenGrace = this.failures.filter(
+            (f) => nowTs > f.ts + DecryptionFailureTracker.GRACE_PERIOD_MS,
+        );
+
+        // Only track one failure per event
+        const dedupedFailuresMap = failuresGivenGrace.reduce(
+            (result, failure) => ({...result, [failure.failedEventId]: failure}),
+            {},
+        );
+        const dedupedFailures = Object.keys(dedupedFailuresMap).map((k) => dedupedFailuresMap[k]);
+
+        this.failuresToTrack = [...this.failuresToTrack, ...dedupedFailures];
+    }
+
+    /**
+     * If there is a failure that should be tracked, call the given trackDecryptionFailure
+     * function with the first failure in the FIFO of failures that should be tracked.
+     */
+    trackFailure() {
+        if (this.failuresToTrack.length > 0) {
+            this.trackDecryptionFailure(this.failuresToTrack.shift());
+        }
+    }
+}
diff --git a/src/components/structures/MatrixChat.js b/src/components/structures/MatrixChat.js
index da729a1a2d..7884829a76 100644
--- a/src/components/structures/MatrixChat.js
+++ b/src/components/structures/MatrixChat.js
@@ -23,6 +23,7 @@ import PropTypes from 'prop-types';
 import Matrix from "matrix-js-sdk";
 
 import Analytics from "../../Analytics";
+import DecryptionFailureTracker from "../../DecryptionFailureTracker";
 import MatrixClientPeg from "../../MatrixClientPeg";
 import PlatformPeg from "../../PlatformPeg";
 import SdkConfig from "../../SdkConfig";
@@ -1308,14 +1309,17 @@ export default React.createClass({
             }
         });
 
-        // XXX: This will do a HTTP request for each Event.decrypted event
-        // if the decryption was a failure
-        cli.on("Event.decrypted", (e) => {
-            if (e.isDecryptionFailure()) {
-                Analytics.trackEvent('E2E', 'Decryption failure', 'ev.content.body: ' + e.getContent().body);
-            }
+        const dft = new DecryptionFailureTracker((failure) => {
+            // TODO: Pass reason for failure as third argument to trackEvent
+            Analytics.trackEvent('E2E', 'Decryption failure');
         });
 
+        const stopDft = dft.start();
+
+        // When logging out, stop tracking failures and destroy state
+        cli.on("Session.logged_out", stopDft);
+        cli.on("Event.decrypted", dft.eventDecrypted);
+
         const krh = new KeyRequestHandler(cli);
         cli.on("crypto.roomKeyRequest", (req) => {
             krh.handleKeyRequest(req);
diff --git a/test/DecryptionFailureTracker-test.js b/test/DecryptionFailureTracker-test.js
new file mode 100644
index 0000000000..9d3b035bf5
--- /dev/null
+++ b/test/DecryptionFailureTracker-test.js
@@ -0,0 +1,128 @@
+/*
+Copyright 2018 New Vector Ltd
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+import expect from 'expect';
+
+import DecryptionFailureTracker from '../src/DecryptionFailureTracker';
+
+import { MatrixEvent } from 'matrix-js-sdk';
+
+function createFailedDecryptionEvent() {
+    const event = new MatrixEvent({
+        event_id: "event-id-" + Math.random().toString(16).slice(2),
+    });
+    event._setClearData(
+        event._badEncryptedMessage(":("),
+    );
+    return event;
+}
+
+describe.only('DecryptionFailureTracker', function() {
+    it('tracks a failed decryption', function(done) {
+        const failedDecryptionEvent = createFailedDecryptionEvent();
+        let trackedFailure = null;
+        const tracker = new DecryptionFailureTracker((failure) => {
+            trackedFailure = failure;
+        });
+
+        tracker.eventDecrypted(failedDecryptionEvent);
+
+        // Pretend "now" is Infinity
+        tracker.checkFailures(Infinity);
+
+        // Immediately track the newest failure, if there is one
+        tracker.trackFailure();
+
+        expect(trackedFailure).toNotBe(null, 'should track a failure for an event that failed decryption');
+
+        done();
+    });
+
+    it('does not track a failed decryption where the event is subsequently successfully decrypted', (done) => {
+        const decryptedEvent = createFailedDecryptionEvent();
+        const tracker = new DecryptionFailureTracker((failure) => {
+            expect(true).toBe(false, 'should not track an event that has since been decrypted correctly');
+        });
+
+        tracker.eventDecrypted(decryptedEvent);
+
+        // Indicate successful decryption: clear data can be anything where the msgtype is not m.bad.encrypted
+        decryptedEvent._setClearData({});
+        tracker.eventDecrypted(decryptedEvent);
+
+        // Pretend "now" is Infinity
+        tracker.checkFailures(Infinity);
+
+        // Immediately track the newest failure, if there is one
+        tracker.trackFailure();
+        done();
+    });
+
+    it('only tracks a single failure per event, despite multiple failed decryptions for multiple events', (done) => {
+        const decryptedEvent = createFailedDecryptionEvent();
+        const decryptedEvent2 = createFailedDecryptionEvent();
+
+        let count = 0;
+        const tracker = new DecryptionFailureTracker((failure) => count++);
+
+        // Arbitrary number of failed decryptions for both events
+        tracker.eventDecrypted(decryptedEvent);
+        tracker.eventDecrypted(decryptedEvent);
+        tracker.eventDecrypted(decryptedEvent);
+        tracker.eventDecrypted(decryptedEvent);
+        tracker.eventDecrypted(decryptedEvent);
+        tracker.eventDecrypted(decryptedEvent2);
+        tracker.eventDecrypted(decryptedEvent2);
+        tracker.eventDecrypted(decryptedEvent2);
+
+        // Pretend "now" is Infinity
+        tracker.checkFailures(Infinity);
+
+        // Simulated polling of `trackFailure`, an arbitrary number ( > 2 ) times
+        tracker.trackFailure();
+        tracker.trackFailure();
+        tracker.trackFailure();
+        tracker.trackFailure();
+
+        expect(count).toBe(2, count + ' failures tracked, should only track a single failure per event');
+
+        done();
+    });
+
+    it('track failures in the order they occured', (done) => {
+        const decryptedEvent = createFailedDecryptionEvent();
+        const decryptedEvent2 = createFailedDecryptionEvent();
+
+        const failures = [];
+        const tracker = new DecryptionFailureTracker((failure) => failures.push(failure));
+
+        // Indicate decryption
+        tracker.eventDecrypted(decryptedEvent);
+        tracker.eventDecrypted(decryptedEvent2);
+
+        // Pretend "now" is Infinity
+        tracker.checkFailures(Infinity);
+
+        // Simulated polling of `trackFailure`, an arbitrary number ( > 2 ) times
+        tracker.trackFailure();
+        tracker.trackFailure();
+
+        expect(failures[0].failedEventId).toBe(decryptedEvent.getId(), 'the first failure should be tracked first');
+        expect(failures[1].failedEventId).toBe(decryptedEvent2.getId(), 'the second failure should be tracked second');
+
+        done();
+    });
+});

From 4a8442901d30e2721b7f581c1ab45db97a0ddd26 Mon Sep 17 00:00:00 2001
From: Luke Barnard <luke@matrix.org>
Date: Fri, 15 Jun 2018 14:45:11 +0100
Subject: [PATCH 06/17] Remove failures when marking them for tracking

---
 src/DecryptionFailureTracker.js | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/src/DecryptionFailureTracker.js b/src/DecryptionFailureTracker.js
index 0f86093209..9eadb332a8 100644
--- a/src/DecryptionFailureTracker.js
+++ b/src/DecryptionFailureTracker.js
@@ -98,9 +98,17 @@ export default class DecryptionFailureTracker {
      * @param {number} nowTs the timestamp that represents the time now.
      */
     checkFailures(nowTs) {
-        const failuresGivenGrace = this.failures.filter(
-            (f) => nowTs > f.ts + DecryptionFailureTracker.GRACE_PERIOD_MS,
-        );
+        const failuresGivenGrace = [];
+        const failuresNotReady = [];
+        while (this.failures.length > 0) {
+            const f = this.failures.shift();
+            if (nowTs > f.ts + DecryptionFailureTracker.GRACE_PERIOD_MS) {
+                failuresGivenGrace.push(f);
+            } else {
+                failuresNotReady.push(f);
+            }
+        }
+        this.failures = failuresNotReady;
 
         // Only track one failure per event
         const dedupedFailuresMap = failuresGivenGrace.reduce(

From ac0416af960da4f5c160314ea69d03b36e72a785 Mon Sep 17 00:00:00 2001
From: Luke Barnard <luke@matrix.org>
Date: Fri, 15 Jun 2018 14:48:20 +0100
Subject: [PATCH 07/17] Do not track previously tracked failures

---
 src/DecryptionFailureTracker.js       | 27 +++++++++++++++++++++++++--
 test/DecryptionFailureTracker-test.js | 24 ++++++++++++++++++++++++
 2 files changed, 49 insertions(+), 2 deletions(-)

diff --git a/src/DecryptionFailureTracker.js b/src/DecryptionFailureTracker.js
index 9eadb332a8..7bdfd6bfd0 100644
--- a/src/DecryptionFailureTracker.js
+++ b/src/DecryptionFailureTracker.js
@@ -21,6 +21,10 @@ class DecryptionFailure {
     }
 }
 
+function eventIdHash(eventId) {
+    return crypto.subtle.digest('SHA-256', eventId);
+}
+
 export default class DecryptionFailureTracker {
     // Array of items of type DecryptionFailure. Every `CHECK_INTERVAL_MS`, this list
     // is checked for failures that happened > `GRACE_PERIOD_MS` ago. Those that did
@@ -31,6 +35,11 @@ export default class DecryptionFailureTracker {
     // one DecryptionFailure of this FIFO is removed and tracked.
     failuresToTrack = [];
 
+    // Event IDs of failures that were tracked previously
+    eventTrackedPreviously = {
+        // [eventIdHash(eventId)]: true
+    };
+
     // Spread the load on `Analytics` by sending at most 1 event per
     // `TRACK_INTERVAL_MS`.
     static TRACK_INTERVAL_MS = 1000;
@@ -112,10 +121,24 @@ export default class DecryptionFailureTracker {
 
         // Only track one failure per event
         const dedupedFailuresMap = failuresGivenGrace.reduce(
-            (result, failure) => ({...result, [failure.failedEventId]: failure}),
+            (result, failure) => {
+                if (!this.eventTrackedPreviously[eventIdHash(failure.failedEventId)]) {
+                    return {...result, [failure.failedEventId]: failure};
+                } else {
+                    return result;
+                }
+            },
             {},
         );
-        const dedupedFailures = Object.keys(dedupedFailuresMap).map((k) => dedupedFailuresMap[k]);
+
+        const trackedEventIds = Object.keys(dedupedFailuresMap);
+
+        this.eventTrackedPreviously = trackedEventIds.reduce(
+            (result, eventId) => ({...result, [eventIdHash(eventId)]: true}),
+            this.eventTrackedPreviously,
+        );
+
+        const dedupedFailures = trackedEventIds.map((k) => dedupedFailuresMap[k]);
 
         this.failuresToTrack = [...this.failuresToTrack, ...dedupedFailures];
     }
diff --git a/test/DecryptionFailureTracker-test.js b/test/DecryptionFailureTracker-test.js
index 9d3b035bf5..34e2df2b6e 100644
--- a/test/DecryptionFailureTracker-test.js
+++ b/test/DecryptionFailureTracker-test.js
@@ -125,4 +125,28 @@ describe.only('DecryptionFailureTracker', function() {
 
         done();
     });
+
+    it('should not track a failure for an event that was tracked previously', (done) => {
+        const decryptedEvent = createFailedDecryptionEvent();
+
+        const failures = [];
+        const tracker = new DecryptionFailureTracker((failure) => failures.push(failure));
+
+        // Indicate decryption
+        tracker.eventDecrypted(decryptedEvent);
+
+        // Pretend "now" is Infinity
+        tracker.checkFailures(Infinity);
+
+        tracker.trackFailure();
+
+        // Indicate a second decryption, after having tracked the failure
+        tracker.eventDecrypted(decryptedEvent);
+
+        tracker.trackFailure();
+
+        expect(failures.length).toBe(1, 'should only track a single failure per event');
+
+        done();
+    });
 });

From cfe52a2888b1231f687e4d09420263eab7a96725 Mon Sep 17 00:00:00 2001
From: Luke Barnard <luke@matrix.org>
Date: Fri, 15 Jun 2018 15:13:59 +0100
Subject: [PATCH 08/17] Instead of passing dft.eventDecrypted, call it instead

So that `this` has the correct reference.
---
 src/components/structures/MatrixChat.js | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/components/structures/MatrixChat.js b/src/components/structures/MatrixChat.js
index 7884829a76..2a2fee1a21 100644
--- a/src/components/structures/MatrixChat.js
+++ b/src/components/structures/MatrixChat.js
@@ -1318,7 +1318,7 @@ export default React.createClass({
 
         // When logging out, stop tracking failures and destroy state
         cli.on("Session.logged_out", stopDft);
-        cli.on("Event.decrypted", dft.eventDecrypted);
+        cli.on("Event.decrypted", (e) => dft.eventDecrypted(e));
 
         const krh = new KeyRequestHandler(cli);
         cli.on("crypto.roomKeyRequest", (req) => {

From 011154396ce3f86a96f7070d4190be1bf821a73b Mon Sep 17 00:00:00 2001
From: Luke Barnard <luke@matrix.org>
Date: Fri, 15 Jun 2018 15:15:48 +0100
Subject: [PATCH 09/17] Add extra, useful expectation to test

---
 test/DecryptionFailureTracker-test.js | 1 +
 1 file changed, 1 insertion(+)

diff --git a/test/DecryptionFailureTracker-test.js b/test/DecryptionFailureTracker-test.js
index 34e2df2b6e..66bc6daf4b 100644
--- a/test/DecryptionFailureTracker-test.js
+++ b/test/DecryptionFailureTracker-test.js
@@ -120,6 +120,7 @@ describe.only('DecryptionFailureTracker', function() {
         tracker.trackFailure();
         tracker.trackFailure();
 
+        expect(failures.length).toBe(2, 'expected 2 failures to be tracked, got ' + failures.length);
         expect(failures[0].failedEventId).toBe(decryptedEvent.getId(), 'the first failure should be tracked first');
         expect(failures[1].failedEventId).toBe(decryptedEvent2.getId(), 'the second failure should be tracked second');
 

From f08274585e873607963de9822f27458c51423d61 Mon Sep 17 00:00:00 2001
From: Luke Barnard <luke@matrix.org>
Date: Fri, 15 Jun 2018 15:26:53 +0100
Subject: [PATCH 10/17] Persist tracked event ID hash using localStorage

---
 src/DecryptionFailureTracker.js         | 18 +++++++++++----
 src/components/structures/MatrixChat.js |  1 +
 test/DecryptionFailureTracker-test.js   | 30 +++++++++++++++++++++++++
 3 files changed, 45 insertions(+), 4 deletions(-)

diff --git a/src/DecryptionFailureTracker.js b/src/DecryptionFailureTracker.js
index 7bdfd6bfd0..2f3d286d55 100644
--- a/src/DecryptionFailureTracker.js
+++ b/src/DecryptionFailureTracker.js
@@ -36,7 +36,7 @@ export default class DecryptionFailureTracker {
     failuresToTrack = [];
 
     // Event IDs of failures that were tracked previously
-    eventTrackedPreviously = {
+    trackedEventHashMap = {
         // [eventIdHash(eventId)]: true
     };
 
@@ -59,6 +59,14 @@ export default class DecryptionFailureTracker {
         this.trackDecryptionFailure = fn;
     }
 
+    loadTrackedEventHashMap() {
+        this.trackedEventHashMap = JSON.parse(localStorage.getItem('mx-decryption-failure-event-id-hashes'));
+    }
+
+    saveTrackedEventHashMap() {
+        localStorage.setItem('mx-decryption-failure-event-id-hashes', JSON.stringify(this.trackedEventHashMap));
+    }
+
     eventDecrypted(e) {
         if (e.isDecryptionFailure()) {
             this.addDecryptionFailureForEvent(e);
@@ -122,7 +130,7 @@ export default class DecryptionFailureTracker {
         // Only track one failure per event
         const dedupedFailuresMap = failuresGivenGrace.reduce(
             (result, failure) => {
-                if (!this.eventTrackedPreviously[eventIdHash(failure.failedEventId)]) {
+                if (!this.trackedEventHashMap[eventIdHash(failure.failedEventId)]) {
                     return {...result, [failure.failedEventId]: failure};
                 } else {
                     return result;
@@ -133,11 +141,13 @@ export default class DecryptionFailureTracker {
 
         const trackedEventIds = Object.keys(dedupedFailuresMap);
 
-        this.eventTrackedPreviously = trackedEventIds.reduce(
+        this.trackedEventHashMap = trackedEventIds.reduce(
             (result, eventId) => ({...result, [eventIdHash(eventId)]: true}),
-            this.eventTrackedPreviously,
+            this.trackedEventHashMap,
         );
 
+        this.saveTrackedEventHashMap();
+
         const dedupedFailures = trackedEventIds.map((k) => dedupedFailuresMap[k]);
 
         this.failuresToTrack = [...this.failuresToTrack, ...dedupedFailures];
diff --git a/src/components/structures/MatrixChat.js b/src/components/structures/MatrixChat.js
index 2a2fee1a21..19c0c17d1d 100644
--- a/src/components/structures/MatrixChat.js
+++ b/src/components/structures/MatrixChat.js
@@ -1313,6 +1313,7 @@ export default React.createClass({
             // TODO: Pass reason for failure as third argument to trackEvent
             Analytics.trackEvent('E2E', 'Decryption failure');
         });
+        dft.loadEventTrackedPreviously();
 
         const stopDft = dft.start();
 
diff --git a/test/DecryptionFailureTracker-test.js b/test/DecryptionFailureTracker-test.js
index 66bc6daf4b..0ea710e5c7 100644
--- a/test/DecryptionFailureTracker-test.js
+++ b/test/DecryptionFailureTracker-test.js
@@ -150,4 +150,34 @@ describe.only('DecryptionFailureTracker', function() {
 
         done();
     });
+
+    it('should not track a failure for an event that was tracked in a previous session', (done) => {
+        // This test uses localStorage, clear it beforehand
+        localStorage.clear();
+
+        const decryptedEvent = createFailedDecryptionEvent();
+
+        const failures = [];
+        const tracker = new DecryptionFailureTracker((failure) => failures.push(failure));
+
+        // Indicate decryption
+        tracker.eventDecrypted(decryptedEvent);
+
+        // Pretend "now" is Infinity
+        // NB: This saves to localStorage specific to DFT
+        tracker.checkFailures(Infinity);
+
+        tracker.trackFailure();
+
+        // Simulate the browser refreshing by destroying tracker and creating a new tracker
+        const secondTracker = new DecryptionFailureTracker((failure) => failures.push(failure));
+        secondTracker.loadTrackedEventHashMap();
+        secondTracker.eventDecrypted(decryptedEvent);
+        secondTracker.checkFailures(Infinity);
+        secondTracker.trackFailure();
+
+        expect(failures.length).toBe(1, 'should track a single failure per event per session, got ' + failures.length);
+
+        done();
+    });
 });

From f22f2d7bd68b88d8b9ca200b0cdc2cff8481151a Mon Sep 17 00:00:00 2001
From: Luke Barnard <luke@matrix.org>
Date: Fri, 15 Jun 2018 15:49:33 +0100
Subject: [PATCH 11/17] Use a Map instead of Object to preserve failure
 ordering

---
 src/DecryptionFailureTracker.js | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/src/DecryptionFailureTracker.js b/src/DecryptionFailureTracker.js
index 2f3d286d55..e668884f64 100644
--- a/src/DecryptionFailureTracker.js
+++ b/src/DecryptionFailureTracker.js
@@ -129,17 +129,18 @@ export default class DecryptionFailureTracker {
 
         // Only track one failure per event
         const dedupedFailuresMap = failuresGivenGrace.reduce(
-            (result, failure) => {
+            (map, failure) => {
                 if (!this.trackedEventHashMap[eventIdHash(failure.failedEventId)]) {
-                    return {...result, [failure.failedEventId]: failure};
+                    return map.set(failure.failedEventId, failure);
                 } else {
-                    return result;
+                    return map;
                 }
             },
-            {},
+            // Use a map to preseve key ordering
+            new Map(),
         );
 
-        const trackedEventIds = Object.keys(dedupedFailuresMap);
+        const trackedEventIds = [...dedupedFailuresMap.keys()];
 
         this.trackedEventHashMap = trackedEventIds.reduce(
             (result, eventId) => ({...result, [eventIdHash(eventId)]: true}),
@@ -148,7 +149,7 @@ export default class DecryptionFailureTracker {
 
         this.saveTrackedEventHashMap();
 
-        const dedupedFailures = trackedEventIds.map((k) => dedupedFailuresMap[k]);
+        const dedupedFailures = dedupedFailuresMap.values();
 
         this.failuresToTrack = [...this.failuresToTrack, ...dedupedFailures];
     }

From 7489d7d5313b2f1aa59efd6a0332382a404a7f57 Mon Sep 17 00:00:00 2001
From: Luke Barnard <luke@matrix.org>
Date: Fri, 15 Jun 2018 16:50:52 +0100
Subject: [PATCH 12/17] Fix incorrect call to DFT

---
 src/components/structures/MatrixChat.js | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/components/structures/MatrixChat.js b/src/components/structures/MatrixChat.js
index 19c0c17d1d..818d058271 100644
--- a/src/components/structures/MatrixChat.js
+++ b/src/components/structures/MatrixChat.js
@@ -1313,7 +1313,7 @@ export default React.createClass({
             // TODO: Pass reason for failure as third argument to trackEvent
             Analytics.trackEvent('E2E', 'Decryption failure');
         });
-        dft.loadEventTrackedPreviously();
+        dft.loadTrackedEventHashMap();
 
         const stopDft = dft.start();
 

From c5252be4a86b52ef15074f45ab2accbc763aeefb Mon Sep 17 00:00:00 2001
From: Luke Barnard <luke@matrix.org>
Date: Fri, 15 Jun 2018 16:51:11 +0100
Subject: [PATCH 13/17] Test everything, not just DFT

---
 test/DecryptionFailureTracker-test.js | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/test/DecryptionFailureTracker-test.js b/test/DecryptionFailureTracker-test.js
index 0ea710e5c7..dc77b4f1d8 100644
--- a/test/DecryptionFailureTracker-test.js
+++ b/test/DecryptionFailureTracker-test.js
@@ -30,7 +30,7 @@ function createFailedDecryptionEvent() {
     return event;
 }
 
-describe.only('DecryptionFailureTracker', function() {
+describe('DecryptionFailureTracker', function() {
     it('tracks a failed decryption', function(done) {
         const failedDecryptionEvent = createFailedDecryptionEvent();
         let trackedFailure = null;

From 98ed93ee5bd9da1bd937e5c1c714f76f74a144bf Mon Sep 17 00:00:00 2001
From: Luke Barnard <luke@matrix.org>
Date: Fri, 15 Jun 2018 16:59:42 +0100
Subject: [PATCH 14/17] Don't hash the eventId (it's uneccessary)

---
 src/DecryptionFailureTracker.js | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/src/DecryptionFailureTracker.js b/src/DecryptionFailureTracker.js
index e668884f64..2a7f2a4e26 100644
--- a/src/DecryptionFailureTracker.js
+++ b/src/DecryptionFailureTracker.js
@@ -21,10 +21,6 @@ class DecryptionFailure {
     }
 }
 
-function eventIdHash(eventId) {
-    return crypto.subtle.digest('SHA-256', eventId);
-}
-
 export default class DecryptionFailureTracker {
     // Array of items of type DecryptionFailure. Every `CHECK_INTERVAL_MS`, this list
     // is checked for failures that happened > `GRACE_PERIOD_MS` ago. Those that did
@@ -37,7 +33,7 @@ export default class DecryptionFailureTracker {
 
     // Event IDs of failures that were tracked previously
     trackedEventHashMap = {
-        // [eventIdHash(eventId)]: true
+        // [eventId]: true
     };
 
     // Spread the load on `Analytics` by sending at most 1 event per
@@ -130,7 +126,7 @@ export default class DecryptionFailureTracker {
         // Only track one failure per event
         const dedupedFailuresMap = failuresGivenGrace.reduce(
             (map, failure) => {
-                if (!this.trackedEventHashMap[eventIdHash(failure.failedEventId)]) {
+                if (!this.trackedEventHashMap[failure.failedEventId]) {
                     return map.set(failure.failedEventId, failure);
                 } else {
                     return map;
@@ -143,7 +139,7 @@ export default class DecryptionFailureTracker {
         const trackedEventIds = [...dedupedFailuresMap.keys()];
 
         this.trackedEventHashMap = trackedEventIds.reduce(
-            (result, eventId) => ({...result, [eventIdHash(eventId)]: true}),
+            (result, eventId) => ({...result, [eventId]: true}),
             this.trackedEventHashMap,
         );
 

From edfc9a0841f8b23d3bb99744be692e99d9431364 Mon Sep 17 00:00:00 2001
From: Luke Barnard <luke@matrix.org>
Date: Fri, 15 Jun 2018 17:00:12 +0100
Subject: [PATCH 15/17] Fix bug with localStorage loading

---
 src/DecryptionFailureTracker.js | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/DecryptionFailureTracker.js b/src/DecryptionFailureTracker.js
index 2a7f2a4e26..337f4b08c3 100644
--- a/src/DecryptionFailureTracker.js
+++ b/src/DecryptionFailureTracker.js
@@ -56,7 +56,7 @@ export default class DecryptionFailureTracker {
     }
 
     loadTrackedEventHashMap() {
-        this.trackedEventHashMap = JSON.parse(localStorage.getItem('mx-decryption-failure-event-id-hashes'));
+        this.trackedEventHashMap = JSON.parse(localStorage.getItem('mx-decryption-failure-event-id-hashes')) || {};
     }
 
     saveTrackedEventHashMap() {

From 488cc416cf33fea9016fbb7dfa68130f7a0b91af Mon Sep 17 00:00:00 2001
From: Luke Barnard <luke@matrix.org>
Date: Fri, 15 Jun 2018 17:08:11 +0100
Subject: [PATCH 16/17] For now, shelve persistance across sessions

---
 src/DecryptionFailureTracker.js         | 16 +++++++++-------
 src/components/structures/MatrixChat.js |  5 ++++-
 test/DecryptionFailureTracker-test.js   |  6 ++++--
 3 files changed, 17 insertions(+), 10 deletions(-)

diff --git a/src/DecryptionFailureTracker.js b/src/DecryptionFailureTracker.js
index 337f4b08c3..069d06bbd1 100644
--- a/src/DecryptionFailureTracker.js
+++ b/src/DecryptionFailureTracker.js
@@ -55,13 +55,13 @@ export default class DecryptionFailureTracker {
         this.trackDecryptionFailure = fn;
     }
 
-    loadTrackedEventHashMap() {
-        this.trackedEventHashMap = JSON.parse(localStorage.getItem('mx-decryption-failure-event-id-hashes')) || {};
-    }
+    // loadTrackedEventHashMap() {
+    //     this.trackedEventHashMap = JSON.parse(localStorage.getItem('mx-decryption-failure-event-id-hashes')) || {};
+    // }
 
-    saveTrackedEventHashMap() {
-        localStorage.setItem('mx-decryption-failure-event-id-hashes', JSON.stringify(this.trackedEventHashMap));
-    }
+    // saveTrackedEventHashMap() {
+    //     localStorage.setItem('mx-decryption-failure-event-id-hashes', JSON.stringify(this.trackedEventHashMap));
+    // }
 
     eventDecrypted(e) {
         if (e.isDecryptionFailure()) {
@@ -143,7 +143,9 @@ export default class DecryptionFailureTracker {
             this.trackedEventHashMap,
         );
 
-        this.saveTrackedEventHashMap();
+        // Commented out for now for expediency, we need to consider unbound nature of storing
+        // this in localStorage
+        // this.saveTrackedEventHashMap();
 
         const dedupedFailures = dedupedFailuresMap.values();
 
diff --git a/src/components/structures/MatrixChat.js b/src/components/structures/MatrixChat.js
index 818d058271..8b36658de4 100644
--- a/src/components/structures/MatrixChat.js
+++ b/src/components/structures/MatrixChat.js
@@ -1313,7 +1313,10 @@ export default React.createClass({
             // TODO: Pass reason for failure as third argument to trackEvent
             Analytics.trackEvent('E2E', 'Decryption failure');
         });
-        dft.loadTrackedEventHashMap();
+
+        // Shelved for later date when we have time to think about persisting history of
+        // tracked events across sessions.
+        // dft.loadTrackedEventHashMap();
 
         const stopDft = dft.start();
 
diff --git a/test/DecryptionFailureTracker-test.js b/test/DecryptionFailureTracker-test.js
index dc77b4f1d8..c4f3116cba 100644
--- a/test/DecryptionFailureTracker-test.js
+++ b/test/DecryptionFailureTracker-test.js
@@ -151,7 +151,7 @@ describe('DecryptionFailureTracker', function() {
         done();
     });
 
-    it('should not track a failure for an event that was tracked in a previous session', (done) => {
+    xit('should not track a failure for an event that was tracked in a previous session', (done) => {
         // This test uses localStorage, clear it beforehand
         localStorage.clear();
 
@@ -171,7 +171,9 @@ describe('DecryptionFailureTracker', function() {
 
         // Simulate the browser refreshing by destroying tracker and creating a new tracker
         const secondTracker = new DecryptionFailureTracker((failure) => failures.push(failure));
-        secondTracker.loadTrackedEventHashMap();
+
+        //secondTracker.loadTrackedEventHashMap();
+
         secondTracker.eventDecrypted(decryptedEvent);
         secondTracker.checkFailures(Infinity);
         secondTracker.trackFailure();

From b0a277288927597dad8c7c6522e8b04a68377481 Mon Sep 17 00:00:00 2001
From: Luke Barnard <luke@matrix.org>
Date: Fri, 15 Jun 2018 17:58:43 +0100
Subject: [PATCH 17/17] Use more consistent start/stop pattern

---
 src/DecryptionFailureTracker.js         | 25 +++++++++++++++----------
 src/components/structures/MatrixChat.js |  4 ++--
 2 files changed, 17 insertions(+), 12 deletions(-)

diff --git a/src/DecryptionFailureTracker.js b/src/DecryptionFailureTracker.js
index 069d06bbd1..b1c6a71289 100644
--- a/src/DecryptionFailureTracker.js
+++ b/src/DecryptionFailureTracker.js
@@ -36,6 +36,10 @@ export default class DecryptionFailureTracker {
         // [eventId]: true
     };
 
+    // Set to an interval ID when `start` is called
+    checkInterval = null;
+    trackInterval = null;
+
     // Spread the load on `Analytics` by sending at most 1 event per
     // `TRACK_INTERVAL_MS`.
     static TRACK_INTERVAL_MS = 1000;
@@ -82,27 +86,28 @@ export default class DecryptionFailureTracker {
 
     /**
      * Start checking for and tracking failures.
-     * @return {function} a function that clears state and causes DFT to stop checking for
-     * and tracking failures.
      */
     start() {
-        const checkInterval = setInterval(
+        this.checkInterval = setInterval(
             () => this.checkFailures(Date.now()),
             DecryptionFailureTracker.CHECK_INTERVAL_MS,
         );
 
-        const trackInterval = setInterval(
+        this.trackInterval = setInterval(
             () => this.trackFailure(),
             DecryptionFailureTracker.TRACK_INTERVAL_MS,
         );
+    }
 
-        return () => {
-            clearInterval(checkInterval);
-            clearInterval(trackInterval);
+    /**
+     * Clear state and stop checking for and tracking failures.
+     */
+    stop() {
+        clearInterval(this.checkInterval);
+        clearInterval(this.trackInterval);
 
-            this.failures = [];
-            this.failuresToTrack = [];
-        };
+        this.failures = [];
+        this.failuresToTrack = [];
     }
 
     /**
diff --git a/src/components/structures/MatrixChat.js b/src/components/structures/MatrixChat.js
index 8b36658de4..2794e81788 100644
--- a/src/components/structures/MatrixChat.js
+++ b/src/components/structures/MatrixChat.js
@@ -1318,10 +1318,10 @@ export default React.createClass({
         // tracked events across sessions.
         // dft.loadTrackedEventHashMap();
 
-        const stopDft = dft.start();
+        dft.start();
 
         // When logging out, stop tracking failures and destroy state
-        cli.on("Session.logged_out", stopDft);
+        cli.on("Session.logged_out", () => dft.stop());
         cli.on("Event.decrypted", (e) => dft.eventDecrypted(e));
 
         const krh = new KeyRequestHandler(cli);