1500054 - The specific icon has a black background and the "A3710B8EBB50CD3" string is wrongly displayed on the web notification when using zip package instead of installer stub

Status: VERIFIED FIXED

(Toolkit Graveyard :: Notifications and Alerts, defect, P3)

Description

[Cosmin Muntean [:cmuntean], Ecosystem QA]

Attachment: rec of the issue.gif

[Affected versions]:
- Nightly 64.0a1, Build ID: 20181017101626
- Screenshots 35.0.0
- Screenshots 33.0.0

[Affected Platforms]:
- Windows 10

[Steps to reproduce]:
1. Open the latest Nightly (64.0a1) build and navigate to https://www.wikipedia.org/ website.
2. Click the "Take a Screenshot" option from the "Page Actions" menu.
3. Perform o selection on the page.
4. Click the "Copy" or "Save" button.
5. Observe the web notification.

[Expected result]:
- The icon from the web notification is correctly displayed.

[Actual results]:
- The background of the icon from the web notification is black and a string code is displayed under the text.

[Regression]:
- This issue is not reproducible with Nightly (64.0a1) builds older than 2018/10/03. But it is reproducible with the Nightly builds started from 2018/10/04.
- Unfortunately I haven't managed to find the exact regression range since the issue is not reproducible with the builds downloaded using the mozregression. However, we suspect that bug 1155505 introduced this issue.

[Notes]:
- Attached a screen recording with the issue.

Comment 1

[Cosmin Muntean [:cmuntean], Ecosystem QA]

Makoto, can you please take a look over this?

Comment 2

[Edouard Oger [:eoger]]

If I remember correctly I think this problem is triggered by the App ID not being present when Firefox is not "installed" with the installation wizard.

Comment 3

[Makoto Kato [:m_kato]]

(In reply to Edouard Oger [:eoger] from comment #2)
> If I remember correctly I think this problem is triggered by the App ID not
> being present when Firefox is not "installed" with the installation wizard.

Yes.  I think that TaskBarID has garbage value.  This is related to bug 1496650.

Comment 4

[Makoto Kato [:m_kato]]

How do you install Firefox?  From zip? or installer's exe?

Comment 5

[Cosmin Muntean [:cmuntean], Ecosystem QA]

I' using a Nightly zip. I extract the file and I'm running Nightly from there.

Comment 6

[Makoto Kato [:m_kato]]

(In reply to Cosmin Muntean [:CosminMCG], Experiments QA from comment #5)
> I' using a Nightly zip. I extract the file and I'm running Nightly from
> there.

Thanks. When using zip package, AppID isn't generated. But I think, when using updater.exe to update newest Firefox even if using zip package, updater will generate AppID and write it to registry without creating shortcut.

Hmm, I think that we should check shortcuts_log.ini, then if there is no shortcut, we shouldn't allow toast notification.

Comment 7

[Jared Wein [:jaws] (please needinfo? me)]

Changing to P5 since this requires running Firefox from a zip.

Comment 8

[Bruce]

Attachment: Black icon but correct app name.png

(In reply to Jared Wein [:jaws] (Regression Engineering Owner for 65) (please needinfo? me) from comment #7)
> Changing to P5 since this requires running Firefox from a zip.

IIUC there are two issues here: (1) the icon of the notification is not visible / is black, and (2) notification doesn't say it's coming from Firefox and instead has some garbled value.

Apparently, issue#2 is caused by Firefox running from a zip, but at least issue#1 is reproducible even if Firefox is properly installed. Please see attached screenshot, it is from Nightly v65.0a1 (2018-11-14) installed on Windows 1809.

Requesting re-prioritization and summary correction. I believe that the reason behind black icons is the presence of `fill="context-fill"` in the svg being used. E.g. Screenshots uses this[1] svg. If I replace `fill="context-fill"` with say `fill="magenta"`, the icon becomes visible[2] (although it still looks bad because it has a solid black background instead of the Acrylic background of the notification).

[1]https://raw.githubusercontent.com/mozilla-services/screenshots/master/webextension/icons/copied-notification.svg
[2]https://i.imgur.com/iHPHIiZ.png

Comment 9

[Makoto Kato [:m_kato]]

(In reply to Bruce from comment #8)
> Created attachment 9025121 [details]
> Black icon but correct app name.png
> 
> (In reply to Jared Wein [:jaws] (Regression Engineering Owner for 65)
> (please needinfo? me) from comment #7)
> > Changing to P5 since this requires running Firefox from a zip.
> 
> IIUC there are two issues here: (1) the icon of the notification is not
> visible / is black, and (2) notification doesn't say it's coming from
> Firefox and instead has some garbled value.

Icon issue may be bug 1502772.  Please file a new bug with reproduce step if possible

Comment 10

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Bruce from comment #8)
> Requesting re-prioritization and summary correction. I believe that the
> reason behind black icons is the presence of `fill="context-fill"` in the
> svg being used. E.g. Screenshots uses this[1] svg. If I replace
> `fill="context-fill"` with say `fill="magenta"`, the icon becomes visible[2]
> (although it still looks bad because it has a solid black background instead
> of the Acrylic background of the notification).

This could be changed to `fill="context-fill black"`, such that `black` will be the fall-back color if one is not provided. But I believe that bug 1502772 is the main issue with the icon being all black here, since even your `fill="magenta"` screenshot looks wrong.

I'll move this back to P3.

Comment 11

[Bruce]

(In reply to Makoto Kato [:m_kato] from comment #9)
> Icon issue may be bug 1502772.  Please file a new bug with reproduce step if
> possible
I have filed bug 1508167.

Comment 12

[Nick Alexander :nalexander [he/him]]

There is a lot going on here; let me capture some notes while I investigate.

First, Windows native notifications require an AUMID. At one time, that AUMID could be arbitrary, but per this SO answer as of the Fall Creators Update of roughly October 2017, the AUMID needs to be "real". The Windows native notification functionality was first added to Firefox in March 2018, so this restriction has always been in place.

Second, what "real" means, exactly, we need to get to the bottom of. Not only does the AUMID need to be registered in the registry, but I believe a shortcut (.lnk) needs to be present in one of the well-known Windows locations (see this classifying code), but it may be that only some of the locations are accepted by Windows. This explains why the existing check to see that an AUMID is known is not sufficient: Firefox will happily generate an AUMID that is not present in the registry and has no corresponding shortcut.

Together, I think this means that packaged installations (those not installed by the installer) will not work at this time. We may have latitude to create shortcuts in order to make this work, but that itself is fraught -- see Bug 1672957 for the type of problems that can arise creating shortcuts.

My preference is to not create shortcuts but instead to

• disable the native notification backend when we're going to get a visible AUMID/no icon/etc, and
• to message to the user when this is the case.

Messaging could mean new about:preferences UI, or a browser console message, or popping an additional XUL notification about the fallback in this case. This is consonant with our taskbar pinning code, which doesn't try to create new shortcuts. I will note that we do have code to create shortcuts that is currently unused, so with a little work we could arrange for a shortcut to be created. But I think I'd rather message the user to pin Firefox to the taskbar in order for native notifications to work than try to keep these fiddly global shortcuts fresh.

Comment 13

[Nick Alexander :nalexander [he/him]]

(In reply to Nick Alexander :nalexander [he/him] from comment #12)

There is a lot going on here; let me capture some notes while I investigate.

First, Windows native notifications require an AUMID. At one time, that AUMID could be arbitrary, but per this SO answer as of the Fall Creators Update of roughly October 2017, the AUMID needs to be "real". The Windows native notification functionality was first added to Firefox in March 2018, so this restriction has always been in place.

Second, what "real" means, exactly, we need to get to the bottom of. Not only does the AUMID need to be registered in the registry, but I believe a shortcut (.lnk) needs to be present in one of the well-known Windows locations (see this classifying code), but it may be that only some of the locations are accepted by Windows. This explains why the existing check to see that an AUMID is known is not sufficient: Firefox will happily generate an AUMID that is not present in the registry and has no corresponding shortcut.

This also explains why running post-update, which can write AUMIDs to the registry, isn't sufficient for native notifications to display correctly.

Together, I think this means that packaged installations (those not installed by the installer) will not work at this time. We may have latitude to create shortcuts in order to make this work, but that itself is fraught -- see Bug 1672957 for the type of problems that can arise creating shortcuts.

My preference is to not create shortcuts but instead to

• disable the native notification backend when we're going to get a visible AUMID/no icon/etc, and

I'll note that this is equivalent to Makoto-san's suggestion to check the shortcut log, although I would prefer to re-use or modify the enumeration code agashlin added for taskbar pinning.

Comment 14

[Nick Alexander :nalexander [he/him]]

My instinct was that we need not prioritize packaged installations (as opposed to installed-by-the-installer installations), but in fact the data seems to show about 15-25% of our Windows users are not running from an installation that ran our installer: see this query (MoCo-only). This means we should accommodate packaged builds explicitly.

Comment 15

[Nick Rishel [:nrishel]]

Some observations:
The garbage string is the aumid, which for zip builds is the install path hash.
Installed msix builds (including those in the Microsoft Store) have the same issue, albeit the string is a different form: Mozilla.Firefox[Windows app hash]![Install or Profile hash?].
We're using the aumid for the notification id, but afaict providing a non aumid string to https://searchfox.org/mozilla-central/source/widget/windows/ToastNotificationHandler.cpp#404 doesn't seem to cause any problem. I'm guessing we should instead provide a branding string here.

Comment 16

[Nick Rishel [:nrishel]]

Attachment: Bug 1500054 - Pre: Normalize branding short and long names for Firefox desktop builds. r=nalexander

Depends on D142357

Comment 17

[Nick Rishel [:nrishel]]

Attachment: Bug 1500054 - Part 1: Using Firefox branding instead of AppUserModelID for Windows native notifications id. r=nalexander

This switch fixes cases where aumid is not a friendly name, e.g. when Firefox is run from a zip and the aumid is the install hash.

Depends on D142358

Comment 18

[Nick Rishel [:nrishel]]

Resolved by https://phabricator.services.mozilla.com/D151739.

Comment 19

[Cosmin Muntean [:cmuntean], Ecosystem QA]

I'm cleaning up the old issues.
I have re-verified this issue with the latest Nightly 116.0a1 zip build (Build ID: 20230703204942) on Windows 10 x64 and Windows 11 x64.

• The icon for the "Copy Screenshot" notification is correctly displayed and the string code is no longer displayed.

Considering this I will mark this issue as verified - fixed.