1893458 - Taskbar pinning races shortcut creation propogating through Windows shell

Status: VERIFIED FIXED

(Firefox :: Installer, defect)

Description

[Bogdan Maris, Desktop Test Engineering]

Attachment: No notification on first start of nightly.gif

Found in

• Latest Nightly 127.0a1

Affected versions

• Latest Nightly 127.0a1

Tested platforms

• Affected platforms: Windows 11 Dev and Canary channels only - 24h2 (26080.1 and 26100.1)
• Unaffected platforms: MacOS and Ubuntu

Preconditions

• Have a clean Windows with no Firefox installed
• Clean the Windows is previous Firefoxes were installed by deleting the following:

Delete HKEY_CURRENT_USER\SOFTWARE\Mozilla
Delete HKEY_LOCAL_MACHINE\SOFTWARE\Mozilla
Delete HKEY_LOCAL_MACHINE\SOFTWARE\mozilla.org
Delete HKEY_LOCAL_MACHINE\WOW6432Node\Mozilla
Delete any folder with Mozilla from C:\Program Data
Delete all folders with Mozilla from C:\user\AppData\Local | Roaming | LocalLow

Steps to reproduce

1. Install latest Nightly (.exe)
2. Visit about:welcome page
3. Leave only Pin Nightly to taskbar checked
4. Click on the Save and continue button

Expected result

• A system notification is displayed asking the user to pin Firefox to taskbar.

Actual result

• No notification is displayed
• The notification will appear if user restarts Nightly and does steps 2-4 again OR by staying in the same session and doing steps 2-4 again.

Regression range

• Not a regression since this is a new implementation.

Additional notes

• Gif showing the issue attached

Comment 1

[Nipun Shukla]

This is known to be a bit inconsistent, I believe due to some issues with the Microsoft APIs we're using. I didn't test with a clean system but I did download the latest version of Nightly and try pinning from about:welcome. Everything worked correctly on my end.

Comment 2

[Michael Hughes]

This will only be an issue on the newer versions of Windows (Win 11 24H2, unreleased). It's not a priority to fix right now, but will definitely need to be fixed before Win11 24H2 ships. Rollout of that version of Windows will likely start rollout in September, 2024.

This does sound reasonably consistent for QA to reproduce (fails the first time on the newer versions of Windows).

Comment 3

[Michael Hughes]

Attachment: Bug 1893458 - attempt to fix tab pinning not working sometimes on the first time r=nalexander,nrishel

Depends on D207789

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2a32d2f7aa06
attempt to fix tab pinning not working sometimes on the first time r=nalexander,nrishel

Comment 5

[Cristian Tuns]

Backed out for causing build bustages in Windows11TaskbarPinning.cpp

• Backout link
• Push with failures
• Failure Log
• Failure line: /builds/worker/checkouts/gecko/browser/components/shell/Windows11TaskbarPinning.cpp(141,19): error: lambda capture 'count' is not used [-Werror,-Wunused-lambda-capture]

Comment 6

[Michael Hughes]

I've kicked off another landing. Setting to needs more information to Nicholas to try to reland it or assign to someone else if it gets rolled back again.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bfcfe75b775a
attempt to fix tab pinning not working sometimes on the first time r=nalexander,nrishel

Comment 8

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8b7820907458
Backed out 2 changesets (bug 1893458, bug 1890634) for causing bustages at TaskbarPinningMetricsTests.cpp. CLOSED TREE

Comment 9

[Atila Butkovits]

Backed out for causing bustages at TaskbarPinningMetricsTests.cpp.

Backout link: https://hg.mozilla.org/integration/autoland/rev/8b7820907458bb85eac02869790ef45fa1e2fcf9

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&selectedTaskRun=LMmyTvJdShOV2jVgfleW8g.0&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel&revision=29ad5906bfb007f51ff55b82ccba789e27de0695

Failure log: https://treeherder.mozilla.org/logviewer?job_id=456579360&repo=autoland&lineNumber=57020

Comment 10

[Nick Rishel [:nrishel]]

Attachment: Bug 1893458 - Migrate Windows Taskbar pinning to use MozPromises instead of callbacks and blocking. r=nrishel

As a tagalong this resolves Bug 1893458 by requesting LAF permissions and taskbar pinning on the same UI thread to remove race conditions.

Comment 11

[Nick Rishel [:nrishel]]

What is happening here is a race between the shortcut being created in the Start menu Programs folder and the attempt to pin the application (which depends on the former). In the Windows Sandbox this results in the old pinning shim to be displayed, but outside the sandbox this shim no longer runs on modern Windows, resulting in a failed attempt to pin.

This behavior only occurs when the user disables adding Firefox to the Start menu during install, because enough time passes to remove the race condition (probably the file being committed to disk).

Edit for more context:

When pinning to taskbar in Windows a shortcut needs to exist in one of a few well known locations. When those don't exist we need to create one, and allow its creation to percolate through the shell. Once it shows up in the virtual directory shell:appsfolder it's available. Until then pinning fails. For reference, the file is written almost immediately but it takes 2-4 seconds to appear in shell:appsfolder in a debug build. The wrinkle here is that shell:appsfolder is a virtual directory represented by a CLSID and no path, so normal folder watching affordances won't work here. Some things I've ruled out:

• SHNotifyChange with SHCNF_FLUSH.
• SHChangeNotifyRegister and waiting on shell:appsfolder.
• Sleep for 10 seconds after creating the shortcut (works, but not an acceptable solution).

Some open lines of investigation are COM's IShellFolder and IKnownFolder.

NI :molly in case you know off top of mind what WINAPI I should be using here.

Comment 12

[Molly Howell (she/her) (mostly inactive)]

SHChangeNotifyRegister not working for this is a bit shocking, to be honest. I have to wonder if they just never updated that mechanism to support FOLDERID_AppsFolder (although I found one SO answer which very implicitly claims that it does work). But if neither that nor SHNotifyChange is doing anything useful, I'm afraid I don't have a better idea than polling for the shortcut file's existence (and even that doesn't seem trivial, I suppose you'd have to SHCreateItemWithParent and then use IShellItem::GetAttributes or something in a loop).

Comment 13

[Nick Rishel [:nrishel]]

Thanks Molly! I'll give SHChangeNotifyRegister another shot; it's plausible I messed something up in my experiment between threading/apartments/messages/etc.

Comment 14

[Nick Rishel [:nrishel]]

Attachment: Bug 1893458 - Verify Start Menu shortcuts created at runtime have propagated to `shell:appsfolder` before attempting to pin them. r=#win-reviewers

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0f1534d062cc
Verify Start Menu shortcuts created at runtime have propagated to `shell:appsfolder` before attempting to pin them. r=win-reviewers,rkraesig

Comment 16

[Norisz Fay [:noriszfay]]

https://hg.mozilla.org/mozilla-central/rev/0f1534d062cc

Comment 17

[Bogdan Maris, Desktop Test Engineering]

Verified that using latest Windows 11 Dev and Canary builds and latest Nightly 134.0a1 this looks fixed, the pin notification appears from the start and it works as expected.