[MM-62709] Patched electron-context-menu to use WebContentsView, avoid using electron-dl #3291

devinbinnie · 2025-01-24T17:28:42Z

Summary

A feature of electron-context-menu was interfering with our normal download process, as electron-dl (an upstream dependency of electron-context-menu) was also performing the same functions as our internal downloading code.

To resolve this, I've patched electron-context-menu to just use the normal Electron download process. Also needed was a patch to allow for WebContentsView to be passed in as an entire view with a webContents property, as that was also causing a crash.

I have a follow-up ticket to create an E2E test for this: MM-62718
Which we can do later as it'll take a while to perfect it and this needs to be fixed/merged ASAP for the release.

Ticket Link

https://mattermost.atlassian.net/browse/MM-62709

Fixed an issue trying to download images using right-click -> Save As...

…d using electron-dl

marianunez · 2025-01-24T17:31:26Z

src/main/contextMenu.ts

@@ -52,7 +52,7 @@ export default class ContextMenu {
    reload = () => {
        this.dispose();

-        const options = {window: this.view.webContents, ...this.menuOptions};


What other areas that use context menu could this affect so we can keep in mind when we test this fix?

It's anywhere that you right-click in the app and you get that little menu. This includes right-clicking on links and such.

@DHaussermann heads up that we want to also smoke test other context menu to make sure there are no other issues.

DHaussermann · 2025-01-30T15:27:21Z

@devinbinnie I'm seeing issues with the build.

For me the initial problem is not solved when I context menu click and image click. To add clarity for reproduction here here, the crash is when I have the image open in the modal. It does not crash if I just right click the inline image from the thread without opening the larger modal view first.

Application: Mattermost 5.11.0-rc.1
Platform: Darwin 23.0.0 arm64
TypeError: Cannot read properties of undefined (reading 'session')
    at /Applications/Mattermost.app/Contents/Resources/app.asar/index.js:2:278077
    at new Promise (<anonymous>)
    at I (/Applications/Mattermost.app/Contents/Resources/app.asar/index.js:2:278009)
    at click (/Applications/Mattermost.app/Contents/Resources/app.asar/index.js:2:282005)
    at MenuItem.click (node:electron/js2c/browser_init:2:34816)
    at a._executeCommand (node:electron/js2c/browser_init:2:40231)

Additionally with some brief exploration I see a worse crash.
When I deleted a server from the list the app crashes. Then relaunch and quit options both trigger a crash loop that the OS cannot recover from. I must use Force Quit at the OS level to stop the loop.

Application: Mattermost 5.11.0-develop.1 [commit: 3d3ced4
]
Platform: Darwin 23.0.0 arm64
TypeError: e.isDestroyed is not a function
    at /private/var/folders/r4/bc0_cvt965n_b4n6ct_nrc800000gn/T/AppTranslocation/60D4F802-2423-4FCA-AC4E-A7156C778CA2/d/Mattermost.app/Contents/Resources/app.asar/index.js:2:282034
    at WebContents.<anonymous> (/private/var/folders/r4/bc0_cvt965n_b4n6ct_nrc800000gn/T/AppTranslocation/60D4F802-2423-4FCA-AC4E-A7156C778CA2/d/Mattermost.app/Contents/Resources/app.asar/index.js:2:282123)
    at Object.onceWrapper (node:events:634:26)
    at WebContents.emit (node:events:519:28)
    ```


Depending how extensive the scope is here, are there E2E tests or other smoke tests that are typically done from desktop QA? I'm a bit concerned that whoever normally does the release testing would need to revisit some this if the crashes can just happen in may areas.

devinbinnie · 2025-01-30T15:30:45Z

Application: Mattermost 5.11.0-rc.1
Platform: Darwin 23.0.0 arm64
TypeError: Cannot read properties of undefined (reading 'session')
    at /Applications/Mattermost.app/Contents/Resources/app.asar/index.js:2:278077
    at new Promise (<anonymous>)
    at I (/Applications/Mattermost.app/Contents/Resources/app.asar/index.js:2:278009)
    at click (/Applications/Mattermost.app/Contents/Resources/app.asar/index.js:2:282005)
    at MenuItem.click (node:electron/js2c/browser_init:2:34816)
    at a._executeCommand (node:electron/js2c/browser_init:2:40231)

This was reproduced on the wrong build, that's rc1, so I'd check that again if you could.

Application: Mattermost 5.11.0-develop.1 [commit: 3d3ced4
]
Platform: Darwin 23.0.0 arm64
TypeError: e.isDestroyed is not a function
    at /private/var/folders/r4/bc0_cvt965n_b4n6ct_nrc800000gn/T/AppTranslocation/60D4F802-2423-4FCA-AC4E-A7156C778CA2/d/Mattermost.app/Contents/Resources/app.asar/index.js:2:282034
    at WebContents.<anonymous> (/private/var/folders/r4/bc0_cvt965n_b4n6ct_nrc800000gn/T/AppTranslocation/60D4F802-2423-4FCA-AC4E-A7156C778CA2/d/Mattermost.app/Contents/Resources/app.asar/index.js:2:282123)
    at Object.onceWrapper (node:events:634:26)
    at WebContents.emit (node:events:519:28)
    ```

This one might be out of scope. Can we try the actual bug on the test build first?

Depending how extensive the scope is here, are there E2E tests or other smoke tests that are typically done from desktop QA? I'm a bit concerned that whoever normally does the release testing would need to revisit some this if the crashes can just happen in may areas.

cc @yasserfaraazkhan

devinbinnie · 2025-01-30T15:31:11Z

/update-branch

DHaussermann

Tested and passed

Confirmed the image save crash is now resolved
Tried other right click functions such as copying channel or team link
As suggested above I explored other context options such as getting attachment links, post action, emojis etc.
Ignored the crash when removing a server which will be addressed separately
No other issue found

LGTM!

DHaussermann · 2025-01-30T19:35:01Z

Thanks for the help @devinbinnie I see the fix now and put an approval comment.

I did a quick click through and explored a bit finding no other issues. As mentioned I unfortunately don't know much about what E2E is in place for desktop but some sanity checking for RC2 would help build confidence.

mattermost-build · 2025-01-30T20:36:13Z

Cherry pick is scheduled.

…d using electron-dl (#3291) Co-authored-by: Mattermost Build <build@mattermost.com> (cherry picked from commit 47cea9a)

…d using electron-dl (#3291) (#3307) Co-authored-by: Mattermost Build <build@mattermost.com> (cherry picked from commit 47cea9a) Co-authored-by: Devin Binnie <52460000+devinbinnie@users.noreply.github.com>

[MM-62709] Patched electron-context-menu to use WebContentsView, avoi…

3d3ced4

…d using electron-dl

devinbinnie added 2: Dev Review Requires review by a core committer 3: QA Review Requires review by a QA tester CherryPick/Approved Meant for the quality or patch release tracked in the milestone labels Jan 24, 2025

devinbinnie added this to the v5.11.0 milestone Jan 24, 2025

devinbinnie requested review from DHaussermann, a team and M-ZubairAhmed and removed request for a team January 24, 2025 17:28

mm-cloud-bot added the release-note label Jan 24, 2025

devinbinnie requested a review from marianunez January 24, 2025 17:28

marianunez approved these changes Jan 24, 2025

View reviewed changes

M-ZubairAhmed approved these changes Jan 28, 2025

View reviewed changes

amyblais removed the 2: Dev Review Requires review by a core committer label Jan 28, 2025

devinbinnie added the Build Apps for PR Builds signed builds for testing label Jan 28, 2025

devinbinnie removed the Build Apps for PR Builds signed builds for testing label Jan 30, 2025

Merge branch 'master' into MM-62709

85edf9a

devinbinnie added the Build Apps for PR Builds signed builds for testing label Jan 30, 2025

DHaussermann approved these changes Jan 30, 2025

View reviewed changes

DHaussermann added 4: Reviews Complete All reviewers have approved the pull request and removed 3: QA Review Requires review by a QA tester labels Jan 30, 2025

devinbinnie merged commit 47cea9a into master Jan 30, 2025
29 checks passed

devinbinnie deleted the MM-62709 branch January 30, 2025 20:36

mattermost-build pushed a commit that referenced this pull request Jan 30, 2025

[MM-62709] Patched electron-context-menu to use WebContentsView, avoi…

9d8cb7a

…d using electron-dl (#3291) Co-authored-by: Mattermost Build <build@mattermost.com> (cherry picked from commit 47cea9a)

mattermost-build mentioned this pull request Jan 30, 2025

Automated cherry pick of #3291 #3307

Merged

mattermost-build added CherryPick/Done Successfully cherry-picked to the quality or patch release tracked in the milestone and removed CherryPick/Approved Meant for the quality or patch release tracked in the milestone labels Jan 30, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MM-62709] Patched electron-context-menu to use WebContentsView, avoid using electron-dl #3291

[MM-62709] Patched electron-context-menu to use WebContentsView, avoid using electron-dl #3291

devinbinnie commented Jan 24, 2025 •

edited by jira bot

Loading

marianunez Jan 24, 2025

devinbinnie Jan 24, 2025

marianunez Jan 24, 2025

DHaussermann commented Jan 30, 2025 •

edited

Loading

devinbinnie commented Jan 30, 2025 •

edited

Loading

devinbinnie commented Jan 30, 2025

DHaussermann left a comment

DHaussermann commented Jan 30, 2025

mattermost-build commented Jan 30, 2025

[MM-62709] Patched electron-context-menu to use WebContentsView, avoid using electron-dl #3291

[MM-62709] Patched electron-context-menu to use WebContentsView, avoid using electron-dl #3291

Conversation

devinbinnie commented Jan 24, 2025 • edited by jira bot Loading

Summary

Ticket Link

marianunez Jan 24, 2025

Choose a reason for hiding this comment

devinbinnie Jan 24, 2025

Choose a reason for hiding this comment

marianunez Jan 24, 2025

Choose a reason for hiding this comment

DHaussermann commented Jan 30, 2025 • edited Loading

devinbinnie commented Jan 30, 2025 • edited Loading

devinbinnie commented Jan 30, 2025

DHaussermann left a comment

Choose a reason for hiding this comment

DHaussermann commented Jan 30, 2025

mattermost-build commented Jan 30, 2025

devinbinnie commented Jan 24, 2025 •

edited by jira bot

Loading

DHaussermann commented Jan 30, 2025 •

edited

Loading

devinbinnie commented Jan 30, 2025 •

edited

Loading