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

[Bug]: MV3 - Transactions - Sending a tx from a dapp triggers errors failed to create transaction notification and Lavamoat: property fetch of globalThis is inaccessible #24518

Closed
seaona opened this issue May 14, 2024 · 1 comment · Fixed by #24631
Assignees
Labels
MV3 release-11.16.6 Issue or pull request that will be included in release 11.16.6 release-12.0.0 Issue or pull request that will be included in release 12.0.0 Sev2-normal Normal severity; minor loss of service or inconvenience. team-extension-platform type-bug

Comments

@seaona
Copy link
Contributor

seaona commented May 14, 2024

Describe the bug

Whenever I send a transaction with a lower nonce, I see that MM popup stays loading for more than usual time, and the following background console errors appear:

  • failed to create transaction notification and
  • Lavamoat: property fetch of globalThis is inaccessible

Update: it seems this only happens with MV3 build
Update2: the errors seem to appear to all type of tx's (successful and not successful)

Expected behavior

No response

Screenshots/Recordings

Screenshot from 2024-05-14 15-04-43

failed-create-notification-global-this.mp4

The video below shows what is currently happening in prod: tx fails immediately, popup is closed and no errors above:

nonce-too-low-failed-immediately.mp4

Steps to reproduce

  1. Enable custom nonce from settings
  2. Trigger a tx from a dapp
  3. Set a nonce with lower value
  4. Accept the tx
  5. See how MM popup stays loading for several seconds
  6. See console errors

Error messages or log output

No response

Version

develop with MV3 build

Build type

None

Browser

Chrome

Operating system

Linux

Hardware wallet

No response

Additional context

No response

Severity

No response

@github-project-automation github-project-automation bot moved this to To be fixed in Bugs by severity May 14, 2024
@github-project-automation github-project-automation bot moved this to To be fixed in Bugs by team May 14, 2024
@seaona seaona added the Sev2-normal Normal severity; minor loss of service or inconvenience. label May 14, 2024
@seaona seaona changed the title [Bug]: Transactions - Sending a tx with lower nonce triggers errors failed to create transaction notification and Lavamoat: property fetch of globalThis is inaccessible [Bug]: MV3 - Transactions - Sending a tx with lower nonce triggers errors failed to create transaction notification and Lavamoat: property fetch of globalThis is inaccessible May 14, 2024
@seaona seaona changed the title [Bug]: MV3 - Transactions - Sending a tx with lower nonce triggers errors failed to create transaction notification and Lavamoat: property fetch of globalThis is inaccessible [Bug]: MV3 - Transactions - Sending a tx from a dapp triggers errors failed to create transaction notification and Lavamoat: property fetch of globalThis is inaccessible May 16, 2024
danjm added a commit that referenced this issue May 23, 2024
…mage util code when creating notifications (#24631)

## **Description**

MV3 bug that we needto fix:
#24518

In particular, we've got:
```
LavaMoat - property "fetch" of globalThis is inaccessible under scuttling mode.
at get (chrome-extension://lbnhnppncgilfagdfbanifbfolodkdaf/scripts/runtime-lavamoat.js:13190:11)
  at loadImageDataForServiceWorker (extensions::imageUtil:24:22)
  at loadImageData (extensions::imageUtil:111:5)
  at Object.loadAllImages (extensions::imageUtil:134:5)
  at replaceNotificationOptionURLs (extensions::notifications:89:13)
  at extensions::notifications:116:5
  at chrome-extension://lbnhnppncgilfagdfbanifbfolodkdaf/common-6.js:3:431145
  at new Promise (<anonymous>)
  at Proxy.<anonymous> (chrome-extension://lbnhnppncgilfagdfbanifbfolodkdaf/common-6.js:3:430825)
  at Object.apply (chrome-extension://lbnhnppncgilfagdfbanifbfolodkdaf/common-6.js:3:430244)
  at Object._showNotification (chrome-extension://lbnhnppncgilfagdfbanifbfolodkdaf/common-2.js:1:455026)
```

The problematic code is in not in our codebase but in chromium, here:
https://github.com/chromium/chromium/blob/main/extensions/renderer/resources/image_util.js#L23-L25

You can see that function is named loadImageDataForServiceWorker and it
is only used in mv3.

This breaks the browser notification feature.

As can be seen from the commit history and the comments in this PR, we
attemtped some solutions that would allow us to provide a modified and
tamed fetch to be used for this image fetching. However, there are
unknown problems - related to how sentry uses fetch and how sentry is
initialized - that interfere with the solutions we attempted. So for
now, as discussed with @weizman, the latest commit on the PR just adds a
scuttling exception for fetch (and for Offscreen Canvas, which is also
needed for the browser notification feature).

We will return to the root problem later, and aim to remove these
scuttling exceptions.

## **Related issues**

Fixes: #24518

## **Manual testing steps**

1. `yarn dist:mv3`
2. install, onboard
3. send a transaction
4. verify that a browser notification is shown once the transaction is
confirmed

## **Screenshots/Recordings**

<!-- If applicable, add screenshots and/or recordings to visualize the
before and after of your change. -->

### **Before**

<!-- [screenshots/recordings] -->

### **After**

<!-- [screenshots/recordings] -->

## **Pre-merge author checklist**

- [ ] I’ve followed [MetaMask Coding
Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md).
- [ ] I've completed the PR template to the best of my ability
- [ ] I’ve included tests if applicable
- [ ] I’ve documented my code using [JSDoc](https://jsdoc.app/) format
if applicable
- [ ] I’ve applied the right labels on the PR (see [labeling
guidelines](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/LABELING_GUIDELINES.md)).
Not required for external contributors.

## **Pre-merge reviewer checklist**

- [ ] I've manually tested the PR (e.g. pull and build branch, run the
app, test code being changed).
- [ ] I confirm that this PR addresses all acceptance criteria described
in the ticket it closes and includes the necessary testing evidence such
as recordings and or screenshots.
@github-project-automation github-project-automation bot moved this from To be fixed to Fixed in Bugs by severity May 23, 2024
@github-project-automation github-project-automation bot moved this from To be fixed to Fixed in Bugs by team May 23, 2024
danjm added a commit that referenced this issue May 31, 2024
…mage util code when creating notifications (#24631)

## **Description**

MV3 bug that we needto fix:
#24518

In particular, we've got:
```
LavaMoat - property "fetch" of globalThis is inaccessible under scuttling mode.
at get (chrome-extension://lbnhnppncgilfagdfbanifbfolodkdaf/scripts/runtime-lavamoat.js:13190:11)
  at loadImageDataForServiceWorker (extensions::imageUtil:24:22)
  at loadImageData (extensions::imageUtil:111:5)
  at Object.loadAllImages (extensions::imageUtil:134:5)
  at replaceNotificationOptionURLs (extensions::notifications:89:13)
  at extensions::notifications:116:5
  at chrome-extension://lbnhnppncgilfagdfbanifbfolodkdaf/common-6.js:3:431145
  at new Promise (<anonymous>)
  at Proxy.<anonymous> (chrome-extension://lbnhnppncgilfagdfbanifbfolodkdaf/common-6.js:3:430825)
  at Object.apply (chrome-extension://lbnhnppncgilfagdfbanifbfolodkdaf/common-6.js:3:430244)
  at Object._showNotification (chrome-extension://lbnhnppncgilfagdfbanifbfolodkdaf/common-2.js:1:455026)
```

The problematic code is in not in our codebase but in chromium, here:
https://github.com/chromium/chromium/blob/main/extensions/renderer/resources/image_util.js#L23-L25

You can see that function is named loadImageDataForServiceWorker and it
is only used in mv3.

This breaks the browser notification feature.

As can be seen from the commit history and the comments in this PR, we
attemtped some solutions that would allow us to provide a modified and
tamed fetch to be used for this image fetching. However, there are
unknown problems - related to how sentry uses fetch and how sentry is
initialized - that interfere with the solutions we attempted. So for
now, as discussed with @weizman, the latest commit on the PR just adds a
scuttling exception for fetch (and for Offscreen Canvas, which is also
needed for the browser notification feature).

We will return to the root problem later, and aim to remove these
scuttling exceptions.

## **Related issues**

Fixes: #24518

## **Manual testing steps**

1. `yarn dist:mv3`
2. install, onboard
3. send a transaction
4. verify that a browser notification is shown once the transaction is
confirmed

## **Screenshots/Recordings**

<!-- If applicable, add screenshots and/or recordings to visualize the
before and after of your change. -->

### **Before**

<!-- [screenshots/recordings] -->

### **After**

<!-- [screenshots/recordings] -->

## **Pre-merge author checklist**

- [ ] I’ve followed [MetaMask Coding
Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md).
- [ ] I've completed the PR template to the best of my ability
- [ ] I’ve included tests if applicable
- [ ] I’ve documented my code using [JSDoc](https://jsdoc.app/) format
if applicable
- [ ] I’ve applied the right labels on the PR (see [labeling
guidelines](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/LABELING_GUIDELINES.md)).
Not required for external contributors.

## **Pre-merge reviewer checklist**

- [ ] I've manually tested the PR (e.g. pull and build branch, run the
app, test code being changed).
- [ ] I confirm that this PR addresses all acceptance criteria described
in the ticket it closes and includes the necessary testing evidence such
as recordings and or screenshots.
danjm added a commit that referenced this issue May 31, 2024
…mage util code when creating notifications (#24631)

## **Description**

MV3 bug that we needto fix:
#24518

In particular, we've got:
```
LavaMoat - property "fetch" of globalThis is inaccessible under scuttling mode.
at get (chrome-extension://lbnhnppncgilfagdfbanifbfolodkdaf/scripts/runtime-lavamoat.js:13190:11)
  at loadImageDataForServiceWorker (extensions::imageUtil:24:22)
  at loadImageData (extensions::imageUtil:111:5)
  at Object.loadAllImages (extensions::imageUtil:134:5)
  at replaceNotificationOptionURLs (extensions::notifications:89:13)
  at extensions::notifications:116:5
  at chrome-extension://lbnhnppncgilfagdfbanifbfolodkdaf/common-6.js:3:431145
  at new Promise (<anonymous>)
  at Proxy.<anonymous> (chrome-extension://lbnhnppncgilfagdfbanifbfolodkdaf/common-6.js:3:430825)
  at Object.apply (chrome-extension://lbnhnppncgilfagdfbanifbfolodkdaf/common-6.js:3:430244)
  at Object._showNotification (chrome-extension://lbnhnppncgilfagdfbanifbfolodkdaf/common-2.js:1:455026)
```

The problematic code is in not in our codebase but in chromium, here:
https://github.com/chromium/chromium/blob/main/extensions/renderer/resources/image_util.js#L23-L25

You can see that function is named loadImageDataForServiceWorker and it
is only used in mv3.

This breaks the browser notification feature.

As can be seen from the commit history and the comments in this PR, we
attemtped some solutions that would allow us to provide a modified and
tamed fetch to be used for this image fetching. However, there are
unknown problems - related to how sentry uses fetch and how sentry is
initialized - that interfere with the solutions we attempted. So for
now, as discussed with @weizman, the latest commit on the PR just adds a
scuttling exception for fetch (and for Offscreen Canvas, which is also
needed for the browser notification feature).

We will return to the root problem later, and aim to remove these
scuttling exceptions.

## **Related issues**

Fixes: #24518

## **Manual testing steps**

1. `yarn dist:mv3`
2. install, onboard
3. send a transaction
4. verify that a browser notification is shown once the transaction is
confirmed

## **Screenshots/Recordings**

<!-- If applicable, add screenshots and/or recordings to visualize the
before and after of your change. -->

### **Before**

<!-- [screenshots/recordings] -->

### **After**

<!-- [screenshots/recordings] -->

## **Pre-merge author checklist**

- [ ] I’ve followed [MetaMask Coding
Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md).
- [ ] I've completed the PR template to the best of my ability
- [ ] I’ve included tests if applicable
- [ ] I’ve documented my code using [JSDoc](https://jsdoc.app/) format
if applicable
- [ ] I’ve applied the right labels on the PR (see [labeling
guidelines](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/LABELING_GUIDELINES.md)).
Not required for external contributors.

## **Pre-merge reviewer checklist**

- [ ] I've manually tested the PR (e.g. pull and build branch, run the
app, test code being changed).
- [ ] I confirm that this PR addresses all acceptance criteria described
in the ticket it closes and includes the necessary testing evidence such
as recordings and or screenshots.
danjm added a commit that referenced this issue May 31, 2024
…mage util code when creating notifications (#24631)

## **Description**

MV3 bug that we needto fix:
#24518

In particular, we've got:
```
LavaMoat - property "fetch" of globalThis is inaccessible under scuttling mode.
at get (chrome-extension://lbnhnppncgilfagdfbanifbfolodkdaf/scripts/runtime-lavamoat.js:13190:11)
  at loadImageDataForServiceWorker (extensions::imageUtil:24:22)
  at loadImageData (extensions::imageUtil:111:5)
  at Object.loadAllImages (extensions::imageUtil:134:5)
  at replaceNotificationOptionURLs (extensions::notifications:89:13)
  at extensions::notifications:116:5
  at chrome-extension://lbnhnppncgilfagdfbanifbfolodkdaf/common-6.js:3:431145
  at new Promise (<anonymous>)
  at Proxy.<anonymous> (chrome-extension://lbnhnppncgilfagdfbanifbfolodkdaf/common-6.js:3:430825)
  at Object.apply (chrome-extension://lbnhnppncgilfagdfbanifbfolodkdaf/common-6.js:3:430244)
  at Object._showNotification (chrome-extension://lbnhnppncgilfagdfbanifbfolodkdaf/common-2.js:1:455026)
```

The problematic code is in not in our codebase but in chromium, here:
https://github.com/chromium/chromium/blob/main/extensions/renderer/resources/image_util.js#L23-L25

You can see that function is named loadImageDataForServiceWorker and it
is only used in mv3.

This breaks the browser notification feature.

As can be seen from the commit history and the comments in this PR, we
attemtped some solutions that would allow us to provide a modified and
tamed fetch to be used for this image fetching. However, there are
unknown problems - related to how sentry uses fetch and how sentry is
initialized - that interfere with the solutions we attempted. So for
now, as discussed with @weizman, the latest commit on the PR just adds a
scuttling exception for fetch (and for Offscreen Canvas, which is also
needed for the browser notification feature).

We will return to the root problem later, and aim to remove these
scuttling exceptions.

## **Related issues**

Fixes: #24518

## **Manual testing steps**

1. `yarn dist:mv3`
2. install, onboard
3. send a transaction
4. verify that a browser notification is shown once the transaction is
confirmed

## **Screenshots/Recordings**

<!-- If applicable, add screenshots and/or recordings to visualize the
before and after of your change. -->

### **Before**

<!-- [screenshots/recordings] -->

### **After**

<!-- [screenshots/recordings] -->

## **Pre-merge author checklist**

- [ ] I’ve followed [MetaMask Coding
Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md).
- [ ] I've completed the PR template to the best of my ability
- [ ] I’ve included tests if applicable
- [ ] I’ve documented my code using [JSDoc](https://jsdoc.app/) format
if applicable
- [ ] I’ve applied the right labels on the PR (see [labeling
guidelines](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/LABELING_GUIDELINES.md)).
Not required for external contributors.

## **Pre-merge reviewer checklist**

- [ ] I've manually tested the PR (e.g. pull and build branch, run the
app, test code being changed).
- [ ] I confirm that this PR addresses all acceptance criteria described
in the ticket it closes and includes the necessary testing evidence such
as recordings and or screenshots.
danjm added a commit that referenced this issue May 31, 2024
…mage util code when creating notifications (#24631)

## **Description**

MV3 bug that we needto fix:
#24518

In particular, we've got:
```
LavaMoat - property "fetch" of globalThis is inaccessible under scuttling mode.
at get (chrome-extension://lbnhnppncgilfagdfbanifbfolodkdaf/scripts/runtime-lavamoat.js:13190:11)
  at loadImageDataForServiceWorker (extensions::imageUtil:24:22)
  at loadImageData (extensions::imageUtil:111:5)
  at Object.loadAllImages (extensions::imageUtil:134:5)
  at replaceNotificationOptionURLs (extensions::notifications:89:13)
  at extensions::notifications:116:5
  at chrome-extension://lbnhnppncgilfagdfbanifbfolodkdaf/common-6.js:3:431145
  at new Promise (<anonymous>)
  at Proxy.<anonymous> (chrome-extension://lbnhnppncgilfagdfbanifbfolodkdaf/common-6.js:3:430825)
  at Object.apply (chrome-extension://lbnhnppncgilfagdfbanifbfolodkdaf/common-6.js:3:430244)
  at Object._showNotification (chrome-extension://lbnhnppncgilfagdfbanifbfolodkdaf/common-2.js:1:455026)
```

The problematic code is in not in our codebase but in chromium, here:
https://github.com/chromium/chromium/blob/main/extensions/renderer/resources/image_util.js#L23-L25

You can see that function is named loadImageDataForServiceWorker and it
is only used in mv3.

This breaks the browser notification feature.

As can be seen from the commit history and the comments in this PR, we
attemtped some solutions that would allow us to provide a modified and
tamed fetch to be used for this image fetching. However, there are
unknown problems - related to how sentry uses fetch and how sentry is
initialized - that interfere with the solutions we attempted. So for
now, as discussed with @weizman, the latest commit on the PR just adds a
scuttling exception for fetch (and for Offscreen Canvas, which is also
needed for the browser notification feature).

We will return to the root problem later, and aim to remove these
scuttling exceptions.

## **Related issues**

Fixes: #24518

## **Manual testing steps**

1. `yarn dist:mv3`
2. install, onboard
3. send a transaction
4. verify that a browser notification is shown once the transaction is
confirmed

## **Screenshots/Recordings**

<!-- If applicable, add screenshots and/or recordings to visualize the
before and after of your change. -->

### **Before**

<!-- [screenshots/recordings] -->

### **After**

<!-- [screenshots/recordings] -->

## **Pre-merge author checklist**

- [ ] I’ve followed [MetaMask Coding
Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md).
- [ ] I've completed the PR template to the best of my ability
- [ ] I’ve included tests if applicable
- [ ] I’ve documented my code using [JSDoc](https://jsdoc.app/) format
if applicable
- [ ] I’ve applied the right labels on the PR (see [labeling
guidelines](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/LABELING_GUIDELINES.md)).
Not required for external contributors.

## **Pre-merge reviewer checklist**

- [ ] I've manually tested the PR (e.g. pull and build branch, run the
app, test code being changed).
- [ ] I confirm that this PR addresses all acceptance criteria described
in the ticket it closes and includes the necessary testing evidence such
as recordings and or screenshots.
danjm added a commit that referenced this issue Jun 4, 2024
…mage util code when creating notifications (#24631)

## **Description**

MV3 bug that we needto fix:
#24518

In particular, we've got:
```
LavaMoat - property "fetch" of globalThis is inaccessible under scuttling mode.
at get (chrome-extension://lbnhnppncgilfagdfbanifbfolodkdaf/scripts/runtime-lavamoat.js:13190:11)
  at loadImageDataForServiceWorker (extensions::imageUtil:24:22)
  at loadImageData (extensions::imageUtil:111:5)
  at Object.loadAllImages (extensions::imageUtil:134:5)
  at replaceNotificationOptionURLs (extensions::notifications:89:13)
  at extensions::notifications:116:5
  at chrome-extension://lbnhnppncgilfagdfbanifbfolodkdaf/common-6.js:3:431145
  at new Promise (<anonymous>)
  at Proxy.<anonymous> (chrome-extension://lbnhnppncgilfagdfbanifbfolodkdaf/common-6.js:3:430825)
  at Object.apply (chrome-extension://lbnhnppncgilfagdfbanifbfolodkdaf/common-6.js:3:430244)
  at Object._showNotification (chrome-extension://lbnhnppncgilfagdfbanifbfolodkdaf/common-2.js:1:455026)
```

The problematic code is in not in our codebase but in chromium, here:
https://github.com/chromium/chromium/blob/main/extensions/renderer/resources/image_util.js#L23-L25

You can see that function is named loadImageDataForServiceWorker and it
is only used in mv3.

This breaks the browser notification feature.

As can be seen from the commit history and the comments in this PR, we
attemtped some solutions that would allow us to provide a modified and
tamed fetch to be used for this image fetching. However, there are
unknown problems - related to how sentry uses fetch and how sentry is
initialized - that interfere with the solutions we attempted. So for
now, as discussed with @weizman, the latest commit on the PR just adds a
scuttling exception for fetch (and for Offscreen Canvas, which is also
needed for the browser notification feature).

We will return to the root problem later, and aim to remove these
scuttling exceptions.

## **Related issues**

Fixes: #24518

## **Manual testing steps**

1. `yarn dist:mv3`
2. install, onboard
3. send a transaction
4. verify that a browser notification is shown once the transaction is
confirmed

## **Screenshots/Recordings**

<!-- If applicable, add screenshots and/or recordings to visualize the
before and after of your change. -->

### **Before**

<!-- [screenshots/recordings] -->

### **After**

<!-- [screenshots/recordings] -->

## **Pre-merge author checklist**

- [ ] I’ve followed [MetaMask Coding
Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md).
- [ ] I've completed the PR template to the best of my ability
- [ ] I’ve included tests if applicable
- [ ] I’ve documented my code using [JSDoc](https://jsdoc.app/) format
if applicable
- [ ] I’ve applied the right labels on the PR (see [labeling
guidelines](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/LABELING_GUIDELINES.md)).
Not required for external contributors.

## **Pre-merge reviewer checklist**

- [ ] I've manually tested the PR (e.g. pull and build branch, run the
app, test code being changed).
- [ ] I confirm that this PR addresses all acceptance criteria described
in the ticket it closes and includes the necessary testing evidence such
as recordings and or screenshots.
@metamaskbot metamaskbot added the release-11.16.6 Issue or pull request that will be included in release 11.16.6 label Jun 4, 2024
@metamaskbot
Copy link
Collaborator

Missing release label release-11.16.6 on issue. Adding release label release-11.16.6 on issue, as issue is linked to PR #24631 which has this release label.

@gauthierpetetin gauthierpetetin added the release-12.0.0 Issue or pull request that will be included in release 12.0.0 label Jun 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
MV3 release-11.16.6 Issue or pull request that will be included in release 11.16.6 release-12.0.0 Issue or pull request that will be included in release 12.0.0 Sev2-normal Normal severity; minor loss of service or inconvenience. team-extension-platform type-bug
Projects
Archived in project
5 participants