Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Generate User-Agent when app starts, thus avoiding storing it on disk #929

Merged
merged 1 commit into from
Apr 26, 2020

Conversation

manavmehta
Copy link
Collaborator

@manavmehta manavmehta commented Apr 12, 2020

Fixes: #921

What's this PR do?
Fetch and send UA from main process and use ipc call to catch it in renderer process: system-util.ts and use it elsewhere in the app as required, avoiding storing it on disk.

Any background context you want to provide?
Please refer #921

You have tested this PR on:

  • Linux/Ubuntu
  • macOS Catalina 10.15.2

@manavmehta
Copy link
Collaborator Author

@andersk @timabbott Can you please have a look? If this feels right, we can refine it :)

@timabbott
Copy link
Sponsor Member

This certainly looks plausible. It's probably a good idea to test this against a Zulip development server configured with a Python print() statement to print the User-Agent it received in both GET /server_settings and GET /events to confirm it's sending the right data in both contexts.

@akashnimare
Copy link
Member

@manavmehta can you test this against a zulip test server and note down the UA in the following cases -
a) When user add a new server
b) API requests from the renderer process

@manavmehta
Copy link
Collaborator Author

manavmehta commented Apr 25, 2020

null_UA_on_adding_server
The UA is correct in /server_settings and /events
But it is sent as null (or the default value) while adding a server, thereafter, it is correct in renderer API calls.

Reason: The new-server-form calls domain-util which calls system-utils afresh.

Workaround as suggested by @abhigyank ebb1238
Change the default value of UA
This would work because:

  1. This is the only part we use to recognize Desktop app
  2. A user adds server least often.

@timabbott @andersk any fixes in your mind or should we go with this?
(documenting this a TODO so that someone could come up with a fix)

@andersk
Copy link
Member

andersk commented Apr 25, 2020

A more robust approach would be for the renderer process to ask the main process for the user-agent with ipcRenderer.invoke (caching the resulting Promise) or ipcRenderer.sendSync (caching the resulting string).

@manavmehta manavmehta changed the title [WIP] Generate User-Agent when app starts, thus avoiding storing it on disk Generate User-Agent when app starts, thus avoiding storing it on disk Apr 25, 2020
@manavmehta
Copy link
Collaborator Author

manavmehta commented Apr 25, 2020

@andersk That was a great suggestion.
It is ready for review and merge from my side.
cc: @akashnimare

P.S. If you want, I can send you the log where I printed a bunch of UAs received by the test server

Copy link
Collaborator

@kanishk98 kanishk98 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright, this looks good to me, except for a few comments I've left that you may want to consider.
Great work, @manavmehta!

app/renderer/js/utils/system-util.ts Outdated Show resolved Hide resolved
app/renderer/js/main.ts Outdated Show resolved Hide resolved
app/main/index.ts Outdated Show resolved Hide resolved
@andersk
Copy link
Member

andersk commented Apr 25, 2020

The goal is to avoid the race condition where we see ZulipElectron/5.0.0 for some time at startup instead of the full ZulipElectron/5.0.0 Mozilla/5.0 blah/blah blah string. Your code still doesn’t do that.

Either use ipcRenderer.invoke this way:

const userAgentPromise: Promise<str> = ipcRenderer.invoke('fetch-user-agent');

export async function getUserAgent(): string {
    return `ZulipElectron/${app.getVersion()} ${await userAgentPromise}`;
}

(which requires adjusting its callers to be async as well), or use ipcRenderer.sendSync.

@manavmehta
Copy link
Collaborator Author

@andersk I tested 37c420a against Zulip test server and logging both. Couldn't find a single instance of the race condition.

But since theoretically, the condition is possible, I think we may go with ipcRenderer.sendSync
Tested it and it works fine. Plus, since it fetches UA synchronously, I don't think there's a possibility for the race condition.

Though it adds a delay, which is probably lesser than a millisecond and better than the race condition anyway.
Thanks :)

app/main/index.ts Outdated Show resolved Hide resolved
app/renderer/js/utils/system-util.ts Outdated Show resolved Hide resolved
Fixes: zulip#921.

Co-authored-by: Anders Kaseorg <anders@zulipchat.com>
@andersk
Copy link
Member

andersk commented Apr 26, 2020

I edited this to use ses.setUserAgent to set the User-Agent once for the entire session rather than individually for each webview. I also squashed all the commits together.

@andersk andersk merged commit 16f0af8 into zulip:master Apr 26, 2020
@manavmehta manavmehta deleted the user-agent branch April 27, 2020 11:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

User-Agent is incorrectly cached in settings.json, with stale value remembered on upgrades
6 participants