1614025 - Persist data over multiple restarts

Status: RESOLVED FIXED

(Chat Core :: Matrix, defect)

Description

[Patrick Cloke [:clokep]]

Currently the MatrixClient object that gets created stores data in memory instead of storing it to disk, etc. etc. When repeatedly connection you end up with multiple "devices" connected to your Matrix account, additionally I believe this is what causes the full history to be downloaded each connection.

The createClient docs show how an options object can be passed in to configure the store. I think we want to use the IndexDBStore instead, which uses IndexedDB under the hood.

Comment 1

[Khushil Mistry [:khushil324]]

Attachment: WIP-Bug-1614025_persist-data-over-multiple-restarts.patch

await opts.store.startup();

The code is not moving ahead from this part.
My concern is if we are importing the right IndexedDB.

Comment 2

[Patrick Cloke [:clokep]]

Hey Khushil, I might have steered you wrong. It seems that IndexedDB.jsm "provides Promise-based wrappers around ordinarily IDBRequest-based IndexedDB methods and classes."

I think we want to use the following instead: Cu.importGlobalProperties(["indexedDB"]); (see https://searchfox.org/comm-central/rev/0c6472bec30280a0648e5101ffacaf72907e4cc2/mozilla/toolkit/modules/IndexedDB.jsm#15-18).

Comment 3

[Khushil Mistry [:khushil324]]

Attachment: Bug-1614025_persist-data-over-multiple-restarts-0.patch

There is still an issue: if you click on previous conversation(Today, Yesterday, Last week), you will see the same messages multiple times. It's UI related issue only. I will solve that in follow-up bug.

Comment 4

[Khushil Mistry [:khushil324]]

Attachment: Bug-1614025_persist-data-over-multiple-restarts-1.patch

Comment 5

[Khushil Mistry [:khushil324]]

Apply after patches from Bug 1347533 and Bug 1348038.

Comment 6

[Khushil Mistry [:khushil324]]

Attachment: Bug-1614025_persist-data-over-multiple-restarts-2.patch

Comment 7

[Patrick Cloke [:clokep]]

Comment on attachment 9137070 [details] [diff] [review]
Bug-1614025_persist-data-over-multiple-restarts-2.patch

Review of attachment 9137070 [details] [diff] [review]:
-----------------------------------------------------------------

Protocol code shouldn't reach out to anything to do with UI, it should be considered backend only.

::: chat/protocols/matrix/matrix.jsm
@@ +199,5 @@
>    __proto__: GenericAccountPrototype,
>    observe(aSubject, aTopic, aData) {},
> +  remove() {
> +    let dbName = this.getString("server") + this.imAccount.name;
> +    let mail3PaneWindow = Services.wm.getMostRecentWindow("mail:3pane");

This code should not be calling into anything to do with windows code. Does the SDK not provide a way to clean-up a database?

@@ +210,2 @@
>    unInit() {},
> +  async connect() {

I dont think we can just make this async. The calling code doesn't treat it as such.

@@ +217,5 @@
> +    // mail3PaneWindow will not load in time so we need to wait for some time.
> +    await new Promise(r => setTimeout(r, 5000));
> +    let win = tabmail.tabInfo[0].linkedBrowser.contentWindow;
> +    let indexedDB = win.indexedDB;
> +    let localStorage = win.localStorage;

I don't think we want these attached to the window. This code should have no concept of windows. We should be able to instantiate these directly, I think.

@@ +227,2 @@
>  
> +    if (indexedDB && localStorage) {

Do we ever expect these to not exist? Should we error in that case?

Comment 8

[Khushil Mistry [:khushil324]]

Attachment: Bug-1614025_persist-data-over-multiple-restarts-3.patch

Comment 9

[Khushil Mistry [:khushil324]]

(In reply to Patrick Cloke [:clokep] from comment #7)

Protocol code shouldn't reach out to anything to do with UI, it should be
considered backend only.

indexedDB from Cu.importGlobalProperties(["indexedDB"]); is working so changed the logic from window.indexedDB to indexedDB.

Comment 10

[Khushil Mistry [:khushil324]]

Attachment: Bug-1614025_persist-data-over-multiple-restarts-4.patch

Comment 11

[Khushil Mistry [:khushil324]]

Apply topic patch first from bug 1348038.

Comment 12

[Patrick Cloke [:clokep]]

Comment on attachment 9140525 [details] [diff] [review]
Bug-1614025_persist-data-over-multiple-restarts-4.patch

Review of attachment 9140525 [details] [diff] [review]:
-----------------------------------------------------------------

I think this is on the right track but needs a bit more work. The main piece that scares me is the async connect function.

::: chat/protocols/matrix/matrix.jsm
@@ +196,5 @@
>  MatrixAccount.prototype = {
>    __proto__: GenericAccountPrototype,
>    observe(aSubject, aTopic, aData) {},
> +  remove() {
> +    for (let room of Object.values(this._roomList)) {

I think we also want to clear out `this._roomList` to reset the state. See https://searchfox.org/comm-central/rev/2131bede09ff0e7273bc77f669150491cd0d7685/chat/protocols/irc/irc.jsm#2258-2262

@@ +199,5 @@
> +  remove() {
> +    for (let room of Object.values(this._roomList)) {
> +      room.close();
> +    }
> +    this.disconnect();

The core code guarantees the account gets disconnected: https://searchfox.org/comm-central/source/chat/components/src/imAccounts.jsm#760-762

@@ +206,2 @@
>    unInit() {},
> +  async connect() {

I don't think we can just make this async -- the caller doesn't know it is async so won't properly await.

@@ +208,2 @@
>      this.reportConnecting();
> +    let dbName = this.getString("server") + this.imAccount.name;

I'm not convinced this is the best thing to use here... I think we should use something based on the account ID instead. This means that even if someone deletes the account and recreates it we are guaranteed to be using a different storage.

Maybe something like: `dbName = "chat:matrix:" + this.imAccount.id;`? I think that will do something like "chat:matrix:1". Looking at how we generate entries for the password manager might be useful.

@@ +210,4 @@
>      let baseURL = this.getString("server") + ":" + this.getInt("port");
>  
> +    const opts = {
> +      useAuthorizationHeader: true,

What does this change do?

@@ +254,5 @@
>          case "PREPARED":
>            this.reportConnected();
>            break;
>          case "STOPPED":
>            // XXX Report disconnecting here?

Do you know if this comment is still valid?

@@ +297,5 @@
>            } else if (event.getType() == "m.room.topic") {
>              conv.setTopic(event.getContent().topic, event.sender.name);
>            } else if (conv && event.getType() == "m.room.power_levels") {
>              conv.notifyObservers(null, "chat-update-topic");
> +            conv.writeMessage(

Is this just for debugging or do we still need it?

@@ +380,5 @@
>    disconnect() {
>      if (this._client) {
>        this._client.stopClient();
>      }
> +    if (!this.disconnected) {

When will calling stopClient not result in using disconnecting?

Comment 13

[Khushil Mistry [:khushil324]]

(In reply to Patrick Cloke [:clokep] from comment #12)

I think we also want to clear out this._roomList to reset the state. See
https://searchfox.org/comm-central/rev/
2131bede09ff0e7273bc77f669150491cd0d7685/chat/protocols/irc/irc.jsm#2258-2262

Right.

The core code guarantees the account gets disconnected:
https://searchfox.org/comm-central/source/chat/components/src/imAccounts.
jsm#760-762

Ohh, got it. This multiple calls of reportDisconnected were causing some error so needed the check of if (this.disconnected) further in the code.

I don't think we can just make this async -- the caller doesn't know it is
async so won't properly await.

The caller doesn't need to wait since the "connect" function is not returning anything. We just need to call this.reportConnected() whenever we are prepared. We are doing it here. Without await, the store will not startup on time and createClient function will throw an error. We just need synchronous behavior in the "connect" function itself, we don't need to worry about the caller function.

I'm not convinced this is the best thing to use here... I think we should
use something based on the account ID instead. This means that even if
someone deletes the account and recreates it we are guaranteed to be using a
different storage.

Maybe something like: dbName = "chat:matrix:" + this.imAccount.id;? I
think that will do something like "chat:matrix:1". Looking at how we
generate entries for the password manager might be useful.

Okay cool.

What does this change do?

[opts.useAuthorizationHeader = false] Set to true to use Authorization header instead of query param to send the access token to the server.

Do you know if this comment is still valid?

Yes, we don't need it now.

Is this just for debugging or do we still need it?

It will generate a message and we want to show it to the user. That's why. This all will changes once we update this writeConv function and show the right message according to the respective event.

Comment 14

[Patrick Cloke [:clokep]]

(In reply to Khushil Mistry [:khushil324] from comment #13)

(In reply to Patrick Cloke [:clokep] from comment #12)

I don't think we can just make this async -- the caller doesn't know it is
async so won't properly await.

The caller doesn't need to wait since the "connect" function is not returning anything. We just need to call this.reportConnected() whenever we are prepared. We are doing it here. Without await, the store will not startup on time and createClient function will throw an error. We just need synchronous behavior in the "connect" function itself, we don't need to worry about the caller function.

What happens if we need to cancel the connection before it is complete or something of that nature?

What does this change do?

[opts.useAuthorizationHeader = false] Set to true to use Authorization header instead of query param to send the access token to the server.

OK, but why do we need to make this change?

Is this just for debugging or do we still need it?

It will generate a message and we want to show it to the user. That's why. This all will changes once we update this writeConv function and show the right message according to the respective event.

So this should have been part of bug 1348038?

Comment 15

[Khushil Mistry [:khushil324]]

Attachment: Bug-1614025_persist-data-over-multiple-restarts-5.patch

Comment 16

[Khushil Mistry [:khushil324]]

OK, but why do we need to make this change?

The documentation recommends it. Plus we may need it with SSO.

So this should have been part of bug 1348038?

Maybe, this is getting changed in the next patch so it will not matter much here.

Comment 17

[Patrick Cloke [:clokep]]

Comment on attachment 9140807 [details] [diff] [review]
Bug-1614025_persist-data-over-multiple-restarts-5.patch

Review of attachment 9140807 [details] [diff] [review]:
-----------------------------------------------------------------

Looks pretty close! A few minor comments.

::: chat/protocols/matrix/matrix.jsm
@@ +199,5 @@
> +  remove() {
> +    for (let room of Object.values(this._roomList)) {
> +      room.close();
> +    }
> +    delete this.room;

This is incorrect, it should be `delete this._roomList` (although you might actually need to instantiate it as a `new Map()`).

@@ +200,5 @@
> +    for (let room of Object.values(this._roomList)) {
> +      room.close();
> +    }
> +    delete this.room;
> +    this._client.clearStores();

Can you add a comment about what this is doing?

@@ +216,2 @@
>  
> +    opts.store = new MatrixSDK.IndexedDBStore({

Can we just include this in the definition of opts?

opts = {
  useAuthorizationHeader: true,
  baseUrl: baseURL,
  store = new MatrixSDK.IndexedDBStore(...),
}

@@ +351,5 @@
>  
>      // Get the list of joined rooms on the server and create those conversations.
>      this._client.getJoinedRooms().then(response => {
>        for (let roomId of response.joined_rooms) {
> +        if (this._roomList[roomId]) {

Can you add a comment above this saying why it exists?

Comment 18

[Khushil Mistry [:khushil324]]

(In reply to Patrick Cloke [:clokep] from comment #17)

This is incorrect, it should be delete this._roomList (although you might
actually need to instantiate it as a new Map()).

I am changing how we use _roomList in the upcoming patch. Here, we are using _roomList[id] instead of has(), get() and set() which is the wrong usage of Map. So I will just put delete this._roomList here.

Comment 19

[Khushil Mistry [:khushil324]]

Attachment: Bug-1614025_persist-data-over-multiple-restarts-6.patch

Comment 20

[Patrick Cloke [:clokep]]

Comment on attachment 9141519 [details] [diff] [review]
Bug-1614025_persist-data-over-multiple-restarts-6.patch

Changes look good!

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/9585630b0575
Persist data over multiple restarts. r=clokep