Clicking a desktop notification crashes the browser

[saskakol]

- Did you search for similar issues before submitting this one?
Yes
- Describe the issue you encountered:
When clicking a desktop notification created by brave, the browser crashes.
- Platform (Win7, 8, 10? macOS? Linux distro?):
Windows 10
- Brave Version (revision SHA):
0.18.30 (795cc10)
- Steps to reproduce:
1. Get a desktop notification to pop up. (Can be tested using this tool)
2. Click the notification
- Actual result:
Browser crashes.
- Expected result:
The url linked to the notification opens.
- Will the steps above reproduce in a fresh profile? If not what other info can be added?
Not sure, I've only tested this using my regular setup.
- Is this an issue in the currently release
Yes
- Can this issue be consistently reproduced?
Yes

[LaurenWags]

+1 from Twitter: https://twitter.com/Schensel_/status/905884162921562112

[LaurenWags]

Could repro using 0.18.31 on Win10.
Could not repro on MacOS using 0.18.31.
Could not repro on Win10 using 0.18.29.

[LaurenWags]

STR on Win10
Navigate to http://www.bennish.net/web-notifications.html
Click Show.
Click Allow in Notifications bar.
Click on popup notification --> Brave crashes.

[LaurenWags]

+2 from community - https://community.brave.com/t/brave-browser-has-closed-constantly-without-displaying-error-message/5425/7
https://community.brave.com/t/brave-browser-has-closed-constantly-without-displaying-error-message/5425/9

[bridiver]

I think I have a fix, but I can't get notifications at all on win in dev

[ghost]

Hi ! +1 but for me it's not when I click on the notification. It's when it appears.

Brave: 0.18.30
rev: 795cc10
Muon: 4.3.17
libchromiumcontent: 61.0.3163.71
V8: 6.1.534.31
Node.js: 7.9.0
Update Channel: dev
OS Platform: Microsoft Windows
OS Release: 10.0.15063
OS Architecture: x64

[srirambv]

This seems to be fixed for me on Windows for 0.18.32. Both new profile and upgrade profile doesn't crash when clicking on notification from http://www.bennish.net/web-notifications.html

[bbondy]

close?

[srirambv]

Waiting for confirmation on Linux and macOS.

On Mon 11 Sep 2017 at 7:30 p.m., Brian R. Bondy ***@***.***> wrote: close? — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#10848 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AQONrj9obySS64CbtQiD09a3K8k2sBEsks5shTzvgaJpZM4PQDc0> .

-- -- Regards, Sriram BV

[bbondy]

it'll be re-opened if checked by qa per platform doesn't pass, so I'll just mark it as closed and re-open if that's not the case. Thanks.

[kjozwiak]

Using 0.18.30 rev: 795cc10, I made sure that I could reproduce the issue using the following platforms:

• Win 10 Pro x64 - Reproduced
• Win 7 Pro x86 - Notifications don't appear to be working
• Ubuntu 17.04 x64 - Reproduced (segmentation fault (core dumped))
• macOS 10.12.6 x64 - Couldn't Reproduce

Ensured that I couldn't reproduce the above issue using 0.18.32 rev: e94738d on the following platforms:

• Win 10 Pro x64 - PASSED
• Win 7 Pro x86 - Notifications don't appear to be working
• Ubuntu 17.04 x64 - PASSED
• macOS 10.12.6 x64 - PASSED

Test Cases Used:

• http://www.bennish.net/web-notifications.html
• https://pdehaan.github.io/push-notification-test/
• ensured that clicking on on the Notification under macOS's "Notifications Panel" didn't crash Brave
• ensured that clicking on on the Notification under Win 10's "Notifications Panel" didn't crash Brave

I'm going to install Win 10 x86 tomorrow to double check and make sure notifications are working on x86.... I'm not sure why they're not working on Win 7... Maybe Win 7 doesn't support the spec?

[bsclifton]

@kjozwiak that is correct- with Electron, desktop notifications are not supported on Windows 7.

A while after our fork (Muon) diverged from Electron, support was added (see electron/electron#8384 for more info)

[NejcZdovc]

so we could just pull in electron-archive/brightray#279 and have support for Windows 7?

[bsclifton]

@NejcZdovc I think it'll require some work to get that merged, but we totally could do it. I guess it depends on if we want to go through that effort for Windows 7
cc: @bbondy

Windows 7 mainstream support was EOLed in 2015- extended support EOL is still 3 years away
https://support.microsoft.com/en-us/help/13853/windows-lifecycle-fact-sheet

If this is something we'd like to do, we should create an issue 😄

[kjozwiak]

I guess it depends on if we want to go through that effort for Windows 7

I personally don't think it's worth the effort. I just wanted to double check and make sure that it wasn't a regression. As @bsclifton mentioned above, Win 7 mainstream support ended in 2015. I'm not sure that we want to get into the habit of supporting EOL platforms.

There's also a good argument of adding it as Win 7 still has a large % of the worldwide market share [1]. However, I'm not sure how accurate these stats are and how it aligns with that we're seeing in our metrics.

[1] http://gs.statcounter.com/os-version-market-share/windows/desktop/worldwide