File upload is not always enabled

[SilverYoCha]

The file download is not always activated after the user logs in. Sometimes the download button appears and sometimes not.

To reproduce On my environment, this scenario seems reproducible (but not guaranteed):

1. Open ConverseJs with a user account
2. Perform page reloads (F5) until the button is no longer displayed (skip this step if the button is already not displayed).
3. Perform page reloads and check that the button is not displayed.
4. Close the browser, saving the tab context
5. Restart the browser
6. The download button appears

Expected behaviour The download button should always be displayed.

Environment:

• Desktop
• All browsers
• Converse.js (master branch and 9.1.1, not tried on other versions)

Additional context Functional ejabberd server.

Initialization options

 converse.initialize({
          'view_mode' : 'overlayed',
          'i18n' : 'a language',
          'assets_path' : '/myserver/chat/converse/',
          'sounds_path' : '/myserver/chat/converse/',
          'play_sounds' : false,
          'bosh_service_url' : 'bosh url',
          'allow_logout' : false,
          'auto_login' : true,
          'auto_reconnect' : true,
          'jid' : 'a jid',
          'clear_messages_on_reconnection' : true,
          'default_domain' : 'a domain',
          'domain_placeholder' : 'a domain',
          'password' : 'a password',
          'autocomplete_add_contact' : false,
          'notification_icon' : 'aLogo.png',
          'muc_domain' : 'conference.a.domain',
          'locked_muc_domain' : 'hidden',
          'muc_disable_slash_commands' : true,
          'locked_muc_nickname' : true,
          'nickname' : 'a nickname',
          'auto_register_muc_nickname' : true,
          'notify_all_room_messages' : true,
          'auto_join_on_invite' : false,
          'roster_groups' : false,
          'allow_adhoc_commands' : false,
          'allow_contact_removal' : false,
          'allow_contact_requests' : false,
          'allow_registration' : false,
          'show_controlbox_by_default' : false,
          'discover_connection_methods' : false
        });

[SilverYoCha]

If I add the code as shown in the following screenshot, it works better (but the problem appears sometimes, rarely though). Maybe a problem with the cache management? I can't find the problem clearly at the moment.

[jcbrand]

This bug has been reported a number of times: #2668, #1456, #1945.

It's a tricky bug to fix because it happens intermittently. I don't recall it ever really affecting me, but it clearly affects other users like yourself.

Given that it happens intermittently, my guess is that it is some kind of timing issue related to fetching the DiscoEntities collection from the cache and also writing to the collection.

IIRC when you write to a model or collection before it's been properly fetched, then the fetched values are discarded and you basically lose your cache.

I think that's what is happening here, the DiscoEntities are fetched but before the fetch is finished, something already writes to DiscoEntities, perhaps via calling .create on it.

Usually the way to fix these kinds of bugs is to make sure something uses await for some other process to finish, to fix the timing issue (where things happen in the wrong order, e.g. something writes to the DiscoEntities collection before fetchEntities has completed).

Looking at the code, I see in initializeDisco that it does await on fetchEntities and then calls .create (which should be fine I think because of the await before), and then triggers discoInitialized.

https://github.com/conversejs/converse.js/blob/1ad6de2dd6a0b56fc84a65a4bddf7647e595fba4/src/headless/plugins/disco/utils.js#L81

My guess is that some code somewhere else perhaps needs to await on discoInitialized before continuing and that would fix the timing issue.

Concerning passing ignore_cache, unless I'm missing something, I don't think passing ignore_cache to this.fetch is doing anything. this.fetch() calls a fetch method on the skeletor.js Collection and there's nothing in there that accepts or uses a ignore_cache flag.

https://github.com/conversejs/skeletor/blob/af329a269841d434f1445e35d67a3381a9186b11/src/collection.js#L491

[jcbrand]

Ok, I see that fetchFeatures looks at ignore_cache, and if that gets passed all the way through via options from fetchEntities I can see how it would make a difference.

https://github.com/conversejs/converse.js/blob/1ad6de2dd6a0b56fc84a65a4bddf7647e595fba4/src/headless/plugins/disco/entity.js#L104

File upload is a feature, so it makes sense.

[jcbrand]

Another possible cause is that the IQ stanza that fetches the features times out.

https://github.com/conversejs/converse.js/blob/1ad6de2dd6a0b56fc84a65a4bddf7647e595fba4/src/headless/plugins/disco/entity.js#L129

Perhaps you can verify whether this is happening or not.

[SilverYoCha]

Thank you @jcbrand for taking time to analyze again this issue.

Concerning the "ignore_cache" flag, I was preparing some screenshot to show you how I discovered the flag, but in a mean time, you have discovered it on your own!

I have verified the point about IQ stanza timeout when fetching features. I do not have this issue.

[joudinet]

Hi, I'm able to trigger this issue as well. I observed the difference in the websocket messages that are exchanged between the session where the button is present and the session where the button is missing.

In the one that works, there is a disco query request to upload.example.com with a reply that contains the urn:xmpp:http:upload:0 feature. In the session where the button is missing, this request is never sent. I guess it confirms @jcbrand's thoughts that for some (still unknown) reasons, the disco entities (and/or their features) are not fetched again from the server. I do not see any Timeout error messages in the console.

[joudinet]

While reading the source code for another reason, I found that according to this code in muc.js:

    async join (nick, password) {
        if (this.isEntered()) {
            // We have restored a groupchat from session storage,
            // so we don't send out a presence stanza again.
            return this;
        }
        // Set this early, so we don't rejoin in onHiddenChange
        this.session.save('connection_status', converse.ROOMSTATUS.CONNECTING);
        await this.refreshDiscoInfo();

It looks like the MUC join won't refresh the disco info if it is restoring from session storage. Since the issue occurs when a disco query to upload.example.com is missing, I suspect it should refresh all the time, shouldn't it? Unfortunately, I can't test it right now (as I'm using a compiled version of converse) but I report it for when I have time to look into it.

[joudinet]

More on this issue: I found a way to reproduce it all the time, as well as a way to fix it when it occurs. My settings are as follows:

{
"allow_contact_requests": false,
"allow_logout": false,
"allow_muc_invitations": false,
"allow_user_trust_override": false,
"assets_path": "/conversejs/",
"authentication": "anonymous",
"auto_login": true,
"auto_join_rooms": [ "[email protected]" ],
"clear_cache_on_logout": true,
"discover_connection_methods": false,
"enable_smacks": false,
"hide_muc_server": true,
"jid": "example.com",
"keepalive": false,
"locked_muc_nickname": true,
"loglevel": "debug",
"message_archiving": "never",
"muc_disable_slash_commands": true,
"notify_all_room_messages": [ "[email protected]" ],
"persistent_store": "sessionStorage",
"play_sounds": true,
"sounds_path": "/conversejs/sounds/",
"strict_plugin_dependencies": false,
"singleton": true,
"view_mode": "embedded",
"visible_toolbar_buttons": { "call": false, "spoiler": false,
			     "emoji": true, "toggle_occupants": true },
"websocket_url": "wss://foo.example.com/ws/"
}

The first time I log in, the upload button is visible. If I reload the page, the button disappears. If I clear the sessionStorage and reload the page, the button is visible again. So, this bug is related to how the source code is looking into the storage to decide if a feature is available or not. In the session storage, I do have an entry with { var: urn:xmpp:http:upload:0, ...} in a key that starts with converse-session/converse.features-upload.example.com-. When I debug api.disco.supports, I see it is called with feature = "urn:xmpp:http:upload:0" and jid = "example.com". Shouldn't jid be upload.example.com?

[joudinet]

I confirm that adding the prefix "upload." to _converse.domain in this function call fixes this issue: https://github.com/conversejs/converse.js/blob/1ad6de2dd6a0b56fc84a65a4bddf7647e595fba4/src/shared/chat/toolbar.js#L79

I guess the upload_jid should not be hardcoded but retrieve from elsewhere, but I do not know where :-/

[joudinet]

Still on this bug, actually the issue is elsewhere. When the storage is not empty, the queryForItems() function will not be called and the items property of the "example.com" entity is empty. Looking into the session storage, I see no key with disco-items in the name. I guess the disco plugin never save these items in the storage and that's explain this issue. A dirty fix is to add await entity.queryForItems(); just before this line: https://github.com/conversejs/converse.js/blob/a95c070c2bd67a447e88bfe50501c6f453a4a640/src/headless/plugins/disco/api.js#L265

A proper fix would be to save such disco items and restore them from the session storage but I do not know where to do that.

[jcbrand]

Hi @joudinet

Thanks a lot for the excellent work in figuring out the issue and making a PR.

There was still an issue with your PR which I addressed in a subsequent commit: https://github.com/conversejs/converse.js/commit/fd9e41a9170e938b87970fb68c66fe80aa6bcb5b

I think it should still work fine, but it would be great if you could double check.

[joudinet]

Sorry @jcbrand but your subsequent commit did break my patch. I've just double-checked it and find the trombone (aka. paperclip) still disappears with conversejs master branch. Let me add two screenshots to try to explain the issue. With my patch, when I reload the page, there are converse.disco-items elements in the session storage:

Unfortunately, with your patch, these items are missing, hence the missing trombone:

[licaon-kter]

What happens if you logout and login? Converse behaves strange at reloads afaik

[joudinet]

@licaon-kter the bug in the code is well identified. It is not related to login/logout but to saving the disco items in the browser storage. The function onDiscoItems, in src/headless/plugins/disco/entity.js, is responsible to save such items in the browser storage but fails to do it in the current version.

I fixed it in f9ac1442, but, if I understand correctly @jcbrand's next commit, it makes a test fail because this.items and _converse.disco_entities would not have a reference to the same model. Unfortunately, his patch reintroduces the same issue: disco items are not anymore saved in the browser storage.

[jcbrand]

The commit I made afterwards was to re-use the same entity, instead of creating a new one, because that entity gets identities attached to it as an attribute and if we create a new one, we don't have the identities attribute for both.

It was always intended that the same entity be re-used.

If the bug comes back by doing so, then I think that would mean that this sharing of the same entity is somehow problematic.

I've looked into it further, and I think I know why the sharing of a model between collections is problematic.

In Backbone, a model gets a collection attribute that points to the collection it belongs to. This implies that a model can only belong to one collection at a time.

So this idea of sharing a model between collections won't work.

However, simply using @joudinet's solution will likely also cause issues because there's this assumption in the Converse disco code that a model can be shared between collections.

That means some more thinking and potential refactoring might be needed here. I haven't worked with the disco code for a long time, so it'll take a while to wrap my head around it again and really understand what's going on.

[jcbrand]

What's still amazing to me, is that this bug doesn't happen for me, and I don't know why.

I'm using Prosody instead of Ejabberd, I wonder whether there is some subtle difference between the two that is somehow causing this issue.

@joudinet is there some way you can create a test account in an env where this bug can be reproduced?

[joudinet]

@jcbrand I use buildroot to make an image with ejabberd. Here is the instructions to get you a test environment to reproduce the bug:

• Download last stable release of buildroot: https://buildroot.org/download.html
• from a shell inside the extracted buildroot, run the following commands: make qemu_x86_64_defconfig make menuconfig
• Activate ejabberd dependencies: Toolchain > Enable C++ support Target packages > Interpreter languages and scripting > erlang Target packages > Libraries > Crypto > openssl binary
• Activate ejabberd: Target packages > Networking applications > ejabberd
• Compile the qemu image that contains an ejabberd server: make
• run the qemu system and redirect the 5443 port from your host to ejabberd: export PATH="/PATH/TO/buildroot/output/host/bin:${PATH}" cd output/images qemu-system-x86_64 -M pc -kernel bzImage \ -drive file=rootfs.ext2,if=virtio,format=raw \ -append "rootwait root=/dev/vda console=tty1 console=ttyS0" \ -serial stdio \ -nic user,model=virtio-net-pci,hostfwd=tcp::5443-:5443
• Log in as root user (no password): buildroot login: root
• Generate a self-signed certificate for ejabberd: # cd /etc/ssl/private # openssl req -new -newkey rsa:2048 -x509 -sha256 -days 365 \ -nodes -out ejabberd.crt -keyout ejabberd.key # chown ejabberd ejabberd.key
• Edit ejabberd configuration to enable anonymous authentication and to provide certificate files: # vi /etc/ejabberd/ejabberd.yml

auth_method: [internal, anonymous]
anonymous_protocol: both
disable_sasl_mechanisms: ["X-OAUTH2"]
certfiles:
  - /etc/ssl/private/ejabberd.crt
  - /etc/ssl/private/ejabberd.key

• restart ejabberd to use the new configuration: ejabberdctl restart

Now you have a fully running ejabberd server that listen on localhost:5443. Use a converse.js client to connect to it as an anonymous user via websocket and auto join a room. For example, my converse configuration:

{
	"allow_contact_requests": false,
	"allow_logout": false,
	"allow_muc_invitations": false,
	"allow_user_trust_override": "off",
	"assets_path": "/conversejs/",
	"authentication": "anonymous",
	"auto_login": true,
	"auto_join_rooms": [ "[email protected]" ],
	"clear_cache_on_logout": true,
	"discover_connection_methods": false,
	"enable_smacks": false,
	"hide_muc_server": true,
	"jid": "localhost",
	"keepalive": false,
	"locked_muc_nickname": true,
	"message_archiving": "never",
	"muc_disable_slash_commands": true,
	"notify_all_room_messages": [ "[email protected]" ],
	"persistent_store": "sessionStorage",
	"play_sounds": true,
	"sounds_path": "/conversejs/sounds/",
	"strict_plugin_dependencies": false,
	"singleton": true,
	"view_mode": "embedded",
	"visible_toolbar_buttons": { "call": false, "spoiler": false,
				     "emoji": true, "toggle_occupants": true },
	"websocket_url": "wss://localhost/ws/"
}

To reproduce the bug, simply refresh the webpage after being logged in the muc room, you will see the paperclip button disappearing.

[jcbrand]

Thanks @joudinet, I'll give it a try

[jcbrand]

Looks like I was wrong in saying you can't have one model in two collections: https://stackoverflow.com/questions/22838095/can-a-single-backbone-model-instance-be-in-two-collections-at-once

Which I guess is good news.

[SilverYoCha]

I am not understanding the point about that the assumption in the Converse disco code that a model can be shared between collections.

The collection of items managed by entity.js deals with browserStorage and the one of entities managed by utils.js too.

https://github.com/conversejs/converse.js/blob/659a69e7b750656852e69d2fc94f75d167714239/src/headless/plugins/disco/entity.js#L39-L41

https://github.com/conversejs/converse.js/blob/659a69e7b750656852e69d2fc94f75d167714239/src/headless/plugins/disco/utils.js#L65-L67

As the two browserStorages are different, after a Ctrl+F5, there is not any chance to have same entity references between the two collections.

[jcbrand]

As the two browserStorages are different, after a Ctrl+F5, there is not any chance to have same entity references between the two collections.

Indeed, looks like you're right. As soon as the page is reloaded the reference is severed.

I left this comment on your PR: https://github.com/conversejs/converse.js/pull/3070/files#r1017025519

After reading your last comment on this issue, I have now a better idea of what you were trying to do with your PR. You are re-adding all global entities so that the references are kept.

TBH, I'm not sure whether we really do have to share the entity object between the two collections, looks to me like it might be an optimization but not strictly necessary.

And I think the bug I fixed in this commit https://github.com/conversejs/converse.js/commit/a251608fc50390c7f46ae6ffbfbe9cd9774c8687 might have obfuscated that fact.

Would be good if you can check whether that commit fixes the issue for you, or whether we still need to do something like you do in your PR, which is to re-add entities from the global collection in order to retain references. I suspect we don't need that, which IIRC is something you thought all along :)

[SilverYoCha]

Unfortunately, commit https://github.com/conversejs/converse.js/commit/a251608fc50390c7f46ae6ffbfbe9cd9774c8687 is not solving the problem for me.

[citronalco]

I'm having this issue with Prosody. Sometimes there is an upload icon, sometimes not.

I have no clue how to debug Converse.js. If I should help out with some logs or so, I'd need some directions.

[jcbrand]

Should be fixed now based on this commit: https://github.com/conversejs/converse.js/commit/78a7841afb515780a7e67edf4675e06f240f5cd6

[SilverYoCha]

Yes @jcbrand, this is fixed in my environment.

To make things work, I had to implement something to trash the previous disco keys in storage.