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

DEVICE_EVENT in Connect fails to trigger for certain users #4869

Open
SebastienGllmt opened this issue Aug 19, 2020 · 11 comments
Open

DEVICE_EVENT in Connect fails to trigger for certain users #4869

SebastienGllmt opened this issue Aug 19, 2020 · 11 comments
Labels
code Code improvements connect Connect API related (ie. fee calculation) connect-webextension

Comments

@SebastienGllmt
Copy link

Our app (Yoroi) essentially does the following:

trezor.on(DEVICE_EVENT, () => {...})
trezor.on(UI_EVENT, () => {...})

await trezor.cardanoGetPublicKey({ ... })

However, we've recently had reports from users saying Yoroi fails to load connect their Trezor device.

This is what the logs SHOULD look like

TrezorConnectStore::_init called
TrezorConnectStore:: UI_EVENT: iframe-loaded
TrezorConnectStore:: DEVICE_EVENT: device-connect
TrezorConnectStore:: DEVICE_EVENT: device-changed
TrezorConnectStore:: UI_EVENT: ui-cancel-popup-request
TrezorConnectStore:: UI_EVENT: ui-close_window

However, for users that run into this issue, they get

TrezorConnectStore::_init called
TrezorConnectStore:: UI_EVENT: iframe-loaded
TrezorConnectStore:: UI_EVENT: ui-cancel-popup-request
TrezorConnectStore:: UI_EVENT: ui-close_window

Note the two device events are missing, but ui events arrived correctly.

Expected behavior: once the user selects "allow once for this session", a DEVICE_EVENT is fired
Observed behavior: in some cases, no DEVICE_EVENT is fired

What's strange is we've had people affected report weird things like:

  • It works on one build of Yoroi, then doesn't work on the next
  • It doesn't work, but if they select "don't ask me again" for allowing Trezor connections, it works again

It's strange that it would work for some users, but not others and that the UI_EVENT gets called but not the DEVICE_EVENT. Any idea what would cause this? Worst case we will just have to call trezor.getFeatures({}) every time somebody connects Trezor to our wallet to get it working reliably.

Trezor-connect version: 8.1.10

@szymonlesisz
Copy link
Contributor

Hi @SebastienGllmt,
event emitting logic wasn't changed for a long time now, so i'm quite surprised that this issue comes up recently
I did try it in multiple builds (Web and webextensions) and it works as expected, at least on my machine :)

DEVICE_EVENT is filtered and it's shouldn't be emitted before user permission (neither allow once for this session or don't ask me again)

i've look up at your code and there are certain lines that bothers me and maybe they are the reason (idk)

https://github.com/Emurgo/yoroi-frontend/blob/5d37177e4f4dfff534d11044e31ed22fb8309fe9/app/stores/lib/TrezorWrapper.js#L12

  • this is not true, every single function requires an iframe presence because iframe is a response provider

https://github.com/Emurgo/yoroi-frontend/blob/5d37177e4f4dfff534d11044e31ed22fb8309fe9/app/stores/lib/TrezorWrapper.js#L49

  • calling dispose will remove an iframe from your project, so next time you'll call a connect function this iframe will be recreated and all given temporary permissions will be revoked.
    So basically if user will not check don't ask me again (which stores given permissions in local storage) all "temporary permissions" (allow once for this session) and all event listeners (DEVICE_EVENT) are gone at this point

could you pls be more specific and provide an example where it doesn't work for you?

@szymonlesisz
Copy link
Contributor

plus a tip for that:
https://github.com/Emurgo/yoroi-frontend/blob/5d37177e4f4dfff534d11044e31ed22fb8309fe9/app/stores/lib/TrezorWrapper.js#L17

you can call TrezorConnect.init with lazyLoad: true once, at the beginning of your app livecycle
This option will "prepare" connect-lib to work (settings, manifest, etc), but it will NOT inject an iframe and will not affect NON-TREZOR-USER until you call some TrezorConnect.method

@szymonlesisz
Copy link
Contributor

also after a couple of tries with Yoroi extension i've noticed that you are affected by this issue: trezor/connect#593

and user is asked for a passphrase multiple times :(
we will try to fix it soon

@SebastienGllmt
Copy link
Author

It's entirely possible the trick to open the iframe just-in-time is what is causing the instability. The behavior was only introduces about a month before the hardfork and so it's possible it doesn't happen for many people and we had a spike of Trezor users after we announced Shelley support causing a few affected people to speak up about the issue.

I'll try and play around with the logic, but I can't reproduce the issue myself so I don't have much hope for being able to say it's definitely fixed anytime soon.

@keraf
Copy link
Contributor

keraf commented Sep 8, 2020

Looking at the merged PR in your project, this seems to be resolved. I'm going to close this issue for now. If that's not the case, please re-open this issue. If you have any other problems with trezor-connect, feel free to open new issues 🙂

@keraf keraf closed this as completed Sep 8, 2020
@SebastienGllmt
Copy link
Author

Unfortunately the error persists. After many attempts, I managed to get a build on my machine that reliably reproduces the issue for me but strangely installing the same build on a different machine doesn't reproduce the error. This is seemingly the same as some users reporting sometimes the build works for them, sometimes it doesn't.

I tried to put a logpoint here: https://github.com/trezor/connect/blob/876e55e5a884507f94d2a87b2aec6f81301ef1bf/src/js/env/browser/index.js#L77

and I got the following:

MessageEvent
data: {
    event: "UI_EVENT"
    type: "iframe-bootstrap"
}

MessageEvent
data: {
    event: "UI_EVENT"
    payload: undefined
    type: "iframe-loaded"
}

MessageEvent
data: {
    event: "UI_EVENT"
    payload: undefined
    type: "ui-cancel-popup-request"
}

MessageEvent
data: {
    event: "UI_EVENT"
    payload: undefined
    type: "ui-close_window"
}

MessageEvent
data: {
    event: "RESPONSE_EVENT"
    id: 1
    payload: { ... }
    success: true
    type: "RESPONSE_EVENT"
}

So it seems that even Trezor Connect itself doesn't see the DeviceEvent.

Maybe this could somehow be linked to having multiple copies of our extension downloaded at the same time? (since we ship for a mainnet build and a "nightly" build). We had the same issue with our Ledger iframe (which is what the PR mentioned earlier fixes for Ledger)

@keraf
Copy link
Contributor

keraf commented Sep 9, 2020

As @szymonlesisz mentioned in this issue, DEVICE_EVENT is not supposed to be emitted if the user didn't permit it:

DEVICE_EVENT is filtered and it's shouldn't be emitted before user permission (neither allow once for this session or don't ask me again)

If you want to reproduce this, you can clear trezorconnect_permissions in localStorage on https://connect.trezor.io/ or navigate to it using incognito mode. If you look at the value of trezorconnect_permissions, you can see that permissions are set per source. The source will differ between both extensions (having different IDs).

@SebastienGllmt
Copy link
Author

As @szymonlesisz mentioned in this issue, DEVICE_EVENT is not supposed to be emitted if the user didn't permit it

That's fine. The problem I'm having is that the DEVICE_EVENT is sometimes not fired even if the user did allow it. You can tell from the example yesterday that clearly I allowed it because my debug logs contain the RESPONSE_EVENT for the request, and yet the DEVICE_EVENT never fired.

To be very precise:

For all users (affected by this bug or not),
https://github.com/trezor/connect/blob/0661a940afb45f0a8270c83af5fa61023a20a7e5/src/js/popup/view/common.js#L101 fires the following

{ 
    event: "UI_EVENT",
    payload: {
        granted: true,
        remember: false,
    },
    type: "ui-receive_permission"
}

However, only for users affected by this issue, the handleMessage function doesn't receive the DeviceEvent (only receiving UI events) https://github.com/trezor/connect/blob/876e55e5a884507f94d2a87b2aec6f81301ef1bf/src/js/popup/PopupManager.js#L222

@keraf
Copy link
Contributor

keraf commented Sep 10, 2020

I'll have to dig deeper into that one. I'll re-open the issue for now.

In the meantime, maybe we can figure out a workaround for this. What exactly are you trying to achieve?

@keraf keraf reopened this Sep 10, 2020
@SebastienGllmt
Copy link
Author

When a user exports their public key through Yoroi, we look for the DEVICE_EVENT to get the device id. We plan to use in the future to be able to group accounts that belong to the same Trezor device in our UI (we don't have this feature, but hopefully coming this year)

Workarounds from people that have been affected by this are as I mentioned in the original post:

  • Run the same build on a different browser (Chrome -> Brave or Brave -> Chrome)
  • One user reported selecting "don't ask me again" when approving permission made it work for them

I can help debug this in any way I can, but I think the code between PopupManager.js and common.js are happening on the Trezor bridge so I don't have a way to debug into it.

@mark-stopka
Copy link

@szymonlesisz I can confirm this issue on Mac OS X Brave browser, I have solved it as @SebastienGllmt suggested by using don't ask me again...

Yes, and the Trezor was brand new...

@tsusanka tsusanka transferred this issue from trezor/connect Feb 4, 2022
@tsusanka tsusanka added code Code improvements connect Connect API related (ie. fee calculation) labels Feb 4, 2022
@tsusanka tsusanka changed the title DEVICE_EVENT fails to trigger for certain users DEVICE_EVENT in Connect fails to trigger for certain users Feb 4, 2022
@hynek-jina hynek-jina added the LOW label Feb 11, 2022
@hynek-jina hynek-jina removed the LOW label Jun 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code Code improvements connect Connect API related (ie. fee calculation) connect-webextension
Projects
Status: No status
Development

No branches or pull requests

7 participants