1921759 - browser_copy_query_without_tree.js and Windows CI Issues on upgrades

Status: RESOLVED FIXED

(Core :: Widget: Win32, defect, P2)

Description

[:jmoss]

I'm working with :jmaher to figure out why these bc1 tests continue to fail (https://treeherder.mozilla.org/jobs?repo=try&revision=1a2de9b71339a2add554b4873b755f658f564b3f&selectedTaskRun=O0biIHexR6Sbw2aM1yc7cg.0 & https://treeherder.mozilla.org/jobs?repo=try&revision=1a2de9b71339a2add554b4873b755f658f564b3f&selectedTaskRun=W-MNCn1TRiWTxIEMkG9euw.0) We had a similar issue with a windows VM in CI that had caused this bug https://bugzilla.mozilla.org/show_bug.cgi?id=1903163 but I don't know if that's related to this new error we're getting.

Comment 1

[:jmoss]

From Joel:

"While running on the current VM we have success, other clipboard tests seem to work, I wonder if because this is more "chrome" related that there are other window manager/focus issues at play?"

Comment 2

[:jmoss]

On our Windows CI virtual machines, we continue to disable the clipboard service upon each task run. This has been persistent for quite some time.

Comment 3

[Marco Bonardo [:mak]]

The logs don't clearly pinpoint a defect.
The strange thing is SimpleTest.promiseClipboardChange is failing at setting the clipboard to the initial value, that means SpecialPowers.clipboardCopyString is failing to set a simple string value in the clipboard for 5s, even retrying every 100ms. Hence the Timed out while polling clipboard for initialized data errors coming from here.
While the other failures may point to something wrong in the test, like copying or pasting too early, that failure is unlikely to be caused by the test, unless it's the test itself breaking the clipboard (but how?).
I'll see if I can reproduce locally, but iirc it was not possible also in the previous bug.

Comment 4

[Marco Bonardo [:mak]]

It may be useful to run a Try with --env "MOZ_LOG=clipboard:5" even if coverage seems low.
It may also be worth trying dom.events.testing.asyncClipboard = true, but it'd expect it to only be effecting in Content.

In debug builds I see some interesting assertions:

30:03     INFO - GECKO(7596) | [Parent 6348, Main Thread] ###!!! ASSERTION: Format for url flavor is <url> <linefeed> <page title>: 'lineIndex != -1', file /builds/worker/checkouts/gecko/widget/windows/nsDataObj.cpp:1943
[task 2024-09-26T18:30:03.285Z] 18:30:03     INFO - GECKO(7596) | #01: NS_DebugBreak(unsigned int, char const*, char const*, char const*, int) [xpcom/base/nsDebugImpl.cpp:493]
[task 2024-09-26T18:30:03.286Z] 18:30:03     INFO - GECKO(7596) | #02: nsDataObj::ExtractShortcutTitle(nsTString<char16_t>&) [widget/windows/nsDataObj.cpp:1942]
[task 2024-09-26T18:30:03.286Z] 18:30:03     INFO - GECKO(7596) | #03: nsDataObj::GetFileDescriptorInternetShortcutA(tagFORMATETC&, tagSTGMEDIUM&) [widget/windows/nsDataObj.cpp:1207]
[task 2024-09-26T18:30:03.287Z] 18:30:03     INFO - GECKO(7596) | #04: nsDataObj::GetData(tagFORMATETC*, tagSTGMEDIUM*) [widget/windows/nsDataObj.cpp:765]
[task 2024-09-26T18:30:03.287Z] 18:30:03     INFO - GECKO(7596) | #05: CreateDataCache [C:\Windows\System32\ole32.DLL + 0x89fa6]
[task 2024-09-26T18:30:03.288Z] 18:30:03     INFO - GECKO(7596) | #06: CreateDataCache [C:\Windows\System32\ole32.DLL + 0x8a9de]
[task 2024-09-26T18:30:03.289Z] 18:30:03     INFO - GECKO(7596) | #07: CreateDataCache [C:\Windows\System32\ole32.DLL + 0x8ab04]
[task 2024-09-26T18:30:03.290Z] 18:30:03     INFO - GECKO(7596) | #08: OleConvertOLESTREAMToIStorageEx2 [C:\Windows\System32\ole32.DLL + 0x353f4]
[task 2024-09-26T18:30:03.290Z] 18:30:03     INFO - GECKO(7596) | #09: DispatchMessageW [C:\Windows\System32\user32.dll + 0x182e1]
[task 2024-09-26T18:30:03.291Z] 18:30:03     INFO - GECKO(7596) | #10: DispatchMessageW [C:\Windows\System32\user32.dll + 0x17f9c]
[task 2024-09-26T18:30:03.291Z] 18:30:03     INFO - GECKO(7596) | #11: GetClassLongW [C:\Windows\System32\user32.dll + 0x230cd]
[task 2024-09-26T18:30:03.292Z] 18:30:03     INFO - GECKO(7596) | #12: KiUserCallbackDispatcher [C:\Windows\SYSTEM32\ntdll.dll + 0xa4174]
[task 2024-09-26T18:30:03.292Z] 18:30:03     INFO - GECKO(7596) | #13: NtUserGetClipboardData [C:\Windows\System32\win32u.dll + 0x3174]
[task 2024-09-26T18:30:03.293Z] 18:30:03     INFO - GECKO(7596) | #14: GetClipboardData [C:\Windows\System32\user32.dll + 0x8b21f]
[task 2024-09-26T18:30:03.293Z] 18:30:03     INFO - GECKO(7596) | #15: CreateDataCache [C:\Windows\System32\ole32.DLL + 0x888ed]
[task 2024-09-26T18:30:03.294Z] 18:30:03     INFO - GECKO(7596) | #16: DoDragDrop [C:\Windows\System32\ole32.DLL + 0x3d642]
[task 2024-09-26T18:30:03.295Z] 18:30:03     INFO - GECKO(7596) | #17: OleFlushClipboard [C:\Windows\System32\ole32.DLL + 0x342f]
[task 2024-09-26T18:30:03.295Z] 18:30:03     INFO - GECKO(7596) | #18: nsClipboard::SetNativeClipboardData(nsITransferable*, nsIClipboard::ClipboardType) [widget/windows/nsClipboard.cpp:519]

Comment 5

[:jmoss]

(In reply to Marco Bonardo [:mak] from comment #3)

The logs don't clearly pinpoint a defect.
The strange thing is SimpleTest.promiseClipboardChange is failing at setting the clipboard to the initial value, that means SpecialPowers.clipboardCopyString is failing to set a simple string value in the clipboard for 5s, even retrying every 100ms. Hence the Timed out while polling clipboard for initialized data errors coming from here.
While the other failures may point to something wrong in the test, like copying or pasting too early, that failure is unlikely to be caused by the test, unless it's the test itself breaking the clipboard (but how?).
I'll see if I can reproduce locally, but iirc it was not possible also in the previous bug.(In reply to Marco Bonardo [:mak] from comment #4)
It may be useful to run a Try with --env "MOZ_LOG=clipboard:5" even if coverage seems low.
It may also be worth trying dom.events.testing.asyncClipboard = true, but it'd expect it to only be effecting in Content.

In debug builds I see some interesting assertions:

30:03     INFO - GECKO(7596) | [Parent 6348, Main Thread] ###!!! ASSERTION: Format for url flavor is <url> <linefeed> <page title>: 'lineIndex != -1', file /builds/worker/checkouts/gecko/widget/windows/nsDataObj.cpp:1943
[task 2024-09-26T18:30:03.285Z] 18:30:03     INFO - GECKO(7596) | #01: NS_DebugBreak(unsigned int, char const*, char const*, char const*, int) [xpcom/base/nsDebugImpl.cpp:493]
[task 2024-09-26T18:30:03.286Z] 18:30:03     INFO - GECKO(7596) | #02: nsDataObj::ExtractShortcutTitle(nsTString<char16_t>&) [widget/windows/nsDataObj.cpp:1942]
[task 2024-09-26T18:30:03.286Z] 18:30:03     INFO - GECKO(7596) | #03: nsDataObj::GetFileDescriptorInternetShortcutA(tagFORMATETC&, tagSTGMEDIUM&) [widget/windows/nsDataObj.cpp:1207]
[task 2024-09-26T18:30:03.287Z] 18:30:03     INFO - GECKO(7596) | #04: nsDataObj::GetData(tagFORMATETC*, tagSTGMEDIUM*) [widget/windows/nsDataObj.cpp:765]
[task 2024-09-26T18:30:03.287Z] 18:30:03     INFO - GECKO(7596) | #05: CreateDataCache [C:\Windows\System32\ole32.DLL + 0x89fa6]
[task 2024-09-26T18:30:03.288Z] 18:30:03     INFO - GECKO(7596) | #06: CreateDataCache [C:\Windows\System32\ole32.DLL + 0x8a9de]
[task 2024-09-26T18:30:03.289Z] 18:30:03     INFO - GECKO(7596) | #07: CreateDataCache [C:\Windows\System32\ole32.DLL + 0x8ab04]
[task 2024-09-26T18:30:03.290Z] 18:30:03     INFO - GECKO(7596) | #08: OleConvertOLESTREAMToIStorageEx2 [C:\Windows\System32\ole32.DLL + 0x353f4]
[task 2024-09-26T18:30:03.290Z] 18:30:03     INFO - GECKO(7596) | #09: DispatchMessageW [C:\Windows\System32\user32.dll + 0x182e1]
[task 2024-09-26T18:30:03.291Z] 18:30:03     INFO - GECKO(7596) | #10: DispatchMessageW [C:\Windows\System32\user32.dll + 0x17f9c]
[task 2024-09-26T18:30:03.291Z] 18:30:03     INFO - GECKO(7596) | #11: GetClassLongW [C:\Windows\System32\user32.dll + 0x230cd]
[task 2024-09-26T18:30:03.292Z] 18:30:03     INFO - GECKO(7596) | #12: KiUserCallbackDispatcher [C:\Windows\SYSTEM32\ntdll.dll + 0xa4174]
[task 2024-09-26T18:30:03.292Z] 18:30:03     INFO - GECKO(7596) | #13: NtUserGetClipboardData [C:\Windows\System32\win32u.dll + 0x3174]
[task 2024-09-26T18:30:03.293Z] 18:30:03     INFO - GECKO(7596) | #14: GetClipboardData [C:\Windows\System32\user32.dll + 0x8b21f]
[task 2024-09-26T18:30:03.293Z] 18:30:03     INFO - GECKO(7596) | #15: CreateDataCache [C:\Windows\System32\ole32.DLL + 0x888ed]
[task 2024-09-26T18:30:03.294Z] 18:30:03     INFO - GECKO(7596) | #16: DoDragDrop [C:\Windows\System32\ole32.DLL + 0x3d642]
[task 2024-09-26T18:30:03.295Z] 18:30:03     INFO - GECKO(7596) | #17: OleFlushClipboard [C:\Windows\System32\ole32.DLL + 0x342f]
[task 2024-09-26T18:30:03.295Z] 18:30:03     INFO - GECKO(7596) | #18: nsClipboard::SetNativeClipboardData(nsITransferable*, nsIClipboard::ClipboardType) [widget/windows/nsClipboard.cpp:519]

I’m also happy to provide a loaner for local debugging on this environment if you’re unable to reproduce locally.

Comment 6

[Mark Banner (:standard8)]

To clarify, I believe these are only happening when various updates are applied. Hence I was wondering if this is something specific to windows.

(In reply to Marco Bonardo [:mak] from comment #4)

In debug builds I see some interesting assertions:

30:03     INFO - GECKO(7596) | [Parent 6348, Main Thread] ###!!! ASSERTION: Format for url flavor is <url> <linefeed> <page title>: 'lineIndex != -1', file /builds/worker/checkouts/gecko/widget/windows/nsDataObj.cpp:1943

We also see these on the current builders, though they could be related if the updates are being more strict or something.

Comment 7

[Marco Bonardo [:mak]]

(In reply to Mark Banner (:standard8) from comment #6)

To clarify, I believe these are only happening when various updates are applied.

How does one push to Try to trigger machines with these specific updates?
Can it also be done locally? I didn't have much success in the past with loaners and mochitest browser tests, mach was not working and I had to figure out commands and arguments to launch manually.

Comment 8

[:jmoss]

This is how we run OS integration tests where we found this:

mach try fuzzy --full --no-artifact -q 'windows' --tag os_integration --worker-suffix win11-64-2009=-alpha --worker-suffix win11-64-2009-gpu=-alpha --worker-suffix win11-64-2009-source=-alpha

I think you can use the second half of the command to override defaults to the -alpha pool for each worker pool.

Comment 9

[:jmoss]

Here is a try push with --env "MOZ_LOG=clipboard:5 added.

Comment 10

[Joel Maher ( :jmaher ) (UTC -8)]

I don't see anything in the gecko output that helps with logging, maybe the 'MOZ_LOG': 'clipboard:5' needs something else?

Comment 11

[Marco Bonardo [:mak]]

I suspect it may be uppercase "Clipboard:5"? I don't remember if MOZ_LOG is case sensitive, the docs don't tell, and I didn't test it.
I also see there's a "WidgetClipboard" logger with more info.
So it would probably be "Clipboard:5,WidgetClipboard:5".

Comment 12

[:jmoss]

Latest try push with updated MOZ_LOG ran as --env "MOZ_LOG=Clipboard:5,WidgetClipboard:5".

Comment 13

[:jmoss]

I've done an updated MOZ_LOG try push here with --env "MOZ_LOG=Clipboard:5,WidgetClipboard:5" to compare to comment 12.

Comment 14

[Marco Bonardo [:mak]]

The difference boils down to

GetDataFromDataObject
[task 2024-10-03T14:24:37.216Z] 14:24:37     INFO - GECKO(6680) | [Parent 3312: Main Thread]: D/WidgetClipboard GetNativeDataOffClipboard: overload taking IDataObject*.
[task 2024-10-03T14:24:37.216Z] 14:24:37     INFO - GECKO(6680) | [Parent 3312: Main Thread]: D/WidgetClipboard IDataObject::QueryGetData result : S_OK
[task 2024-10-03T14:24:37.217Z] 14:24:37     INFO - GECKO(6680) | [Parent 3312: Main Thread]: D/WidgetClipboard IDataObject::GetData result : 800401d0
[task 2024-10-03T14:24:37.217Z] 14:24:37     INFO - GECKO(6680) | [Parent 3312: Main Thread]: D/WidgetClipboard IDataObject::GetData result : 800401d0
[task 2024-10-03T14:24:37.218Z] 14:24:37     INFO - GECKO(6680) | [Parent 3312: Main Thread]: D/WidgetClipboard IDataObject::GetData result : 800401d0

That I think comes from RepeatedlyTryGetData. And the returned error is CLIPBRD_E_CANT_OPEN.

The code tries accessing the clipboard 3 times at 3ms intervals, it looks like it is failing all of them.
I wonder if the retry limits could be moved to prefs and those prefs increased for tests, I imagine relaxing the limits in tests should not be a problem?
Though first it may be worth trying to push a patch increasing these limits and see if it solves the problem.
I can try later, the code is here btw: https://searchfox.org/mozilla-central/rev/d56687458d4e6e8882c4b740e78413a0f0a69d59/widget/windows/nsClipboard.cpp#450-451

Comment 15

[Marco Bonardo [:mak]]

So, I tried to rise the limits to 10 tries with 10ms delay, still getting 10 consecutive CLIPBRD_E_CANT_OPEN errors.

At this point I think we need help on the platform integration side.

Comment 16

[:jmoss]

:smaug are you able to assist? I dug through some other bugs and found nsClipboard.cpp is referenced/modifed in Bug1726269.

Comment 17

[Ray Kraesig [:rkraesig]]

Have we been able to repro this issue interactively, or so far is it test-only?

(Triaging as P3/S3 for now, as a default.)

Comment 18

[Olli Pettay [:smaug][[e-mail]]]

I don't really know much about the windows side here. But is this an issue with the windows VM ? If the relevant function call never succeeds, Gecko side can't do much.

Comment 19

[Joel Maher ( :jmaher ) (UTC -8)]

I found an issue with some of the clipboard stuff while working on this locally, I ran into this failure:
TEST-UNEXPECTED-FAIL | browser/components/places/tests/browser/browser_bookmark_copy_folder_tree.js | 2 children - 1 == 2 -

this is fixed by calling clipboard.emptyClipboard() before the test, unfortunately this doesn't help with browser/components/places/tests/browser/browser_bookmark_copy_folder_tree.js.

The problem with browser/components/places/tests/browser/browser_bookmark_copy_folder_tree.js (code):

  library.PlacesOrganizer.selectLeftPaneBuiltIn("BookmarksToolbar");

  await promiseClipboard(function () {
    library.PlacesOrganizer._places.controller.copy();
  }, PlacesUtils.TYPE_X_MOZ_PLACE);

  library.PlacesOrganizer.selectLeftPaneBuiltIn("UnfiledBookmarks");

  await library.ContentTree.view.controller.paste();

  let toolbarCopyNode = library.ContentTree.view.view.nodeForTreeIndex(0);
  is(
    toolbarCopyNode.type,
    Ci.nsINavHistoryResultNode.RESULT_TYPE_FOLDER_SHORTCUT,
    "copy is still a folder shortcut"
  );

I cannot determine exactly where we fail, but I think we don't get back from the .paste() call and things get wonky from there.

Either way, I see 2 paths forward:

1. disable this test and accept the fact that the only test for this type of structured copy/paste testing will be disabled on windows. This will not be the only area in Firefox code that will have no coverage due to broken tests.
2. someone who knows more about clipboard and windows integration look into this. If this is an issue with our test setup, we are happy to change that, but this test setup has worked for years and many cycles of windows updates.

As Microsoft pushes updates out more aggressively on windows 11 than it did years in the past, it is likely by the end of the year that a majority of our end users on win11 will be running this patch or future patches. We will need to take option #1 sometime in the future (end of November) in order for us to move forward. In the rare case there is an urgent security update, we will have to upgrade ASAP which means option #1 would automatically happen.

Comment 20

[David Parks [:handyman]]

I've added some diagnostics to the logging when we fail to get the clipboard. (This should be permanently added.) They log the process that is holding the clipboard and preventing us from being able to read it. explorer.exe seems to be the culprit on try.

Comment 15 tried to resolve this by trying longer and it didn't work -- I've expanded it further from a 9ms total wait to a 500ms total wait (5 tries, 100ms apart) and still had no luck. And, since it seems to fail with this timeout, many times in a row, I don't think an even longer timeout would make a difference. But I did learn that the sleep_for call isn't sleeping (as seen by log time stamps) -- this is worth investigating separately. But replacing that call with a call to the Win32 SleepEx function, which does sleep, doesn't produce better results. This is even with the thread being alertable during the sleep, in case the clipboard needed to run a handler to update.

I haven't tried to determine if this actually is part of the test failure, but if it is, we probably won't have options beyond figuring out how to stop explorer from doing what its doing, or abandoning the test when explorer is doing this.

Comment 21

[Marco Bonardo [:mak]]

Fwiw, I've seen various users complaining about broken clipboard on Win11 on ms forums, and reporting either KB5043145 or KB5043076 seem to have solved the problem for them.

Comment 22

[Joel Maher ( :jmaher ) (UTC -8)]

both of those KB's are on the machine. I was hoping that would be a winner.

I am curious as :handyman mentioned explorer.exe is getting in the way. In our task setup, we do restart explorer.exe

this has been around for a long time. Maybe there are some issue with explorer.exe dependencies/timing with the latest win11 updates.

Comment 23

[Joel Maher ( :jmaher ) (UTC -8)]

I tried skipping the explorer.exe restart as well as clearing the clipboard and adding data to the clipboard in the task setup. Overall nothing is changing the behavior here, so I am not sure there is an easy win with our task bootstrapping.

Comment 24

[David Parks [:handyman]]

Killing Windows explorer is sometimes the most reasonable way to get it to relinquish clipboard control when it's not respecting the resource. It's brute-force, but we could probably stop or reduce the failures if we were restart explorer as a first step of this test. With admin privileges, it might work.

Comment 25

[Joel Maher ( :jmaher ) (UTC -8)]

I did 2 things for this try push:

• move restart explorer.exe to the last pre-task setup step
• remove all tests except the 1 test in question- this ensures that the test is running first with nothing else going on

this worked as you can see with a reasonable amount of retriggers.

the next push I did ran all the tests in the manifest, you can see here we fail as we did before.

So this means the other tests are influencing this. It would be realistic between test manifests to restart explorer.exe, but before this test? I would like to find another solution if possible.

I am going to do a few more try pushes and see if there is a specific test or set of tests that are causing the failure- i.e. they are affecting the browser or explorer.exe.

Comment 26

[Ray Kraesig [:rkraesig]]

Another possibility comes to mind: that explorer.exe doesn't start doing the problematic thing it's doing until a certain amount of time has passed, regardless of any other tests run. It might be worth trying a version that restarts Explorer, but then just sleeps for several minutes.

Comment 27

[David Parks [:handyman]]

Hopefully jmaher's bisections will find whatever is agitating explorer.exe but it indeed could be something uglier. I wrote a somewhat hacky patch to kill explorer.exe at the start of the test and the results are stranger than I anticipated, even with the assumption that restarting explorer at this point might do something crazy. The results show:

• The restart seems to do the trick for this test... on x86. Kind of. x86 now passes browser_bookmark_copy_folder_tree.js but fails for browser_bookmark_popup.js a few tests later. The failure is that the add bookmark doorhanger auto-closes when the mouse is over it and Ctrl-D (add bookmark) is pressed. The screenshot suggests it is closed.
• The x64 runs all timeout at the top of the test. I don't know what is causing the hang.

Comment 28

[Joel Maher ( :jmaher ) (UTC -8)]

I haven't been able to figure out anything with another day of further hacking around. I did waste a lot of time looking at the wrong test.

browser_bookmark_copy_folder_tree.js passes easier now that the clipboard.empty is called between tests (landed yesterday).

The problem is browser_copy_query_without_tree.js fails consistently and no tricks seem to help that.

Comment 29

[David Parks [:handyman]]

It turns out I was wrong earlier -- the failures do stop, but only if I kill explorer the right way. I'd tried killing it directly with KillProcess + re-running, and I got the failures from earlier. I also tried running the same powershell command as in mochitest, but using CreateProcess, and that didn't help. Only doing the "dumb" thing and using system from the standard library cleared it up. It wouldn't be too hard to determine what the difference is. Also, I have no idea how fragile this is, because I don't have an indication of why it helped.

Comment 30

[Joel Maher ( :jmaher ) (UTC -8)]

great stuff, I think this is progress.

I am not sure if one method of kill is delayed and might be similar to a SIGHUP vs a SIGKILL (hard kill).

If we were to do something like this, would there be risk of end users having similar issues? In this case Firefox hasn't changed, but windows has. It seems that no other test cases run into this and we have well over 100 test cases that do cut/paste operations. To me the risk seems small.

Assuming we want to move forward with a hack like this, can we put it behind a pref. We could even only add it on debug/asan/tsan and not allow the code in shippable- then skip the affected test cases on opt. I would prefer to add a key/value to manifests so we could support:

["browser_copy_query_without_tree.js"]
restart_explorer = true
skip-if = ["os == 'windows' && opt"] # restart explorer code is not available in opt

a lot of loose ends, but a few thoughts.

Comment 31

[David Parks [:handyman]]

I was able to do better than that -- I used jsctypes to call system in the test itself. No libxul changes needed, although I kept in the stuff that logs the process blocking the clipboard. When I run the test locally, the behavior is a fast explorer restart. I've got some concerns, but no other ideas, so I'll probably put this up for review today.

The risk that this could obscure something is real, but it's also not totally new. Even before this, we were killing explorer before the test suite -- presumably because it makes the test environment more stable. And we were seeing the failure jump across tests when we poked at it. It's a reasonable hack, given the behavior we have to deal with.

Another idea for improvement is to restart explorer only after something in the test fails because explorer was holding the clipboard. But that's not easy to implement.

Comment 32

[David Parks [:handyman]]

Attachment: Bug 1921759: Restart Windows Explorer at start of browser_bookmark_copy_folder_tree.js r=jmaher!,#win-reviewers!

In a new Windows update, explorer.exe is grabs the clipboard mutex and holds
it for a long time, which is causing test failures. Killing explorer.exe
restarts it and forces it to give up the mutex, allowing the test to pass.

Comment 33

[David Parks [:handyman]]

Attachment: Bug 1921759: Log the process that holds the clipboard mutex on Windows r=#win-reviewers!

When Windows keeps us from getting access to the clipboard because another
application is using it, this will report the exe that currently holds the
mutex.

Depends on D227741

Comment 34

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e02e9d6b2875
Restart Windows Explorer at start of browser_bookmark_copy_folder_tree.js r=jmaher,win-reviewers,places-reviewers,rkraesig,mak
https://hg.mozilla.org/integration/autoland/rev/9d0d5e7c1e3f
Log the process that holds the clipboard mutex on Windows r=win-reviewers,rkraesig
https://hg.mozilla.org/integration/autoland/rev/0d0028bf6cb6
apply code formatting via Lando

Comment 35

[Sandor Molnar[:smolnar]]

Backed out for causing build bustages @ nsClipboard.cpp

Backout link: https://hg.mozilla.org/integration/autoland/rev/97ddfa38796ba8723c3fa9a6d9cc8b3adc455342

Push with failures

Failure log -> ERROR - /builds/worker/checkouts/gecko/widget/windows/nsClipboard.cpp(416,26): error: use of undeclared identifier 'WinUtils'

Comment 36

[David Parks [:handyman]]

https://treeherder.mozilla.org/jobs?repo=try&selectedTaskRun=CDKPfgDDTZ2s6-lyhWfqEw.0&revision=a525239f2be68b3e34a5edb158f125e7a9870c83

Comment 37

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6c79dac12a0e
Restart Windows Explorer at start of browser_bookmark_copy_folder_tree.js r=jmaher,win-reviewers,places-reviewers,rkraesig,mak
https://hg.mozilla.org/integration/autoland/rev/59cf901e577a
Log the process that holds the clipboard mutex on Windows r=win-reviewers,rkraesig

Comment 38

[Serban Stanca [:SerbanS]]

https://hg.mozilla.org/mozilla-central/rev/6c79dac12a0e
https://hg.mozilla.org/mozilla-central/rev/59cf901e577a

Comment 39

[Joel Maher ( :jmaher ) (UTC -8)]

:handyman, are there any issues you can foresee with uplifting these patches to beta/release/esr?

Comment 40

[David Parks [:handyman]]

I don't but, unless it's important, I'd prefer to give it a few days in nightly before uplifts. The patches change the way we sleep when waiting for the clipboard lock, which shouldn't be interesting, but surprises are theoretically possible. Just ping me again if you would prefer it now.

Comment 41

[Nathan LaPré]

I think the patches landed for this bug break the MinGW build (tier 2) because of the addition of #include <Psapi.h> to WinUtils.cpp. See this run.

I landed two patches for bug 1901461 on the same day (yesterday) as the two patches associated with this bug re-landed. I think both landings broke the MinGW build in different ways, but the way my patches broke it (see bug 1929755) hid the Psapi.h issue. After fixing my breaks this morning, the Psapi.h issue has come up (see this comment). I don't think this is a huge deal, but I figured I should shoot you a heads-up. Unless I'm massively mistaken, I imagine there'll be a new tier 2 perma fail bug once I fix my error.

Comment 42

[Joel Maher ( :jmaher ) (UTC -8)]

lets give this a week to find any odd fallout or gain confidence there is none.

then we can work to uplift next week.

Comment 43

[David Parks [:handyman]]

(In reply to Nathan LaPré from comment #41)

I think the patches landed for this bug break the MinGW build (tier 2) because of the addition of #include <Psapi.h> to WinUtils.cpp.

Ah MinGW. The fix for this is easy -- the added logging code (and include) can be #ifdefed out for these builds. I'm pretty busy at the moment but I should be able to put that together tomorrow.

Comment 44

[Joel Maher ( :jmaher ) (UTC -8)]

:handyman, any concerns with uplifting this to other branches?

Comment 45

[David Parks [:handyman]]

Not anymore

Comment 46

[David Parks [:handyman]]

Comment on attachment 9434918 [details]
Bug 1921759: Restart Windows Explorer at start of browser_bookmark_copy_folder_tree.js r=jmaher!,#win-reviewers!

Beta/Release Uplift Approval Request

• User impact if declined/Reason for urgency: None. Tests fail in CI when some Windows updates are applied.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): The patch with the fix is test only. The patch that adds logging and improves the waiting is a minor change and hasn't shown any issues.
• String changes made/needed: N/A
• Is Android affected?: No

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: So that ESR can run tests on versions of Windows with the latest updates
• User impact if declined: None
• Fix Landed on Version: 134
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): The patch with the fix is test only. The patch that adds logging and improves the waiting is a minor change and hasn't shown any issues.

Comment 47

[Donal Meehan [:dmeehan]]

Comment on attachment 9434918 [details]
Bug 1921759: Restart Windows Explorer at start of browser_bookmark_copy_folder_tree.js r=jmaher!,#win-reviewers!

Fx133 rides the train to release next week, rejecting release uplift request.

Comment 48

[Donal Meehan [:dmeehan]]

Comment on attachment 9434919 [details]
Bug 1921759: Log the process that holds the clipboard mutex on Windows r=#win-reviewers!

Fx133 rides the train to release next week, rejecting release uplift request.

Comment 49

[Joel Maher ( :jmaher ) (UTC -8)]

I understand letting 134 ride to beta, but release will not have this fix for 4 more weeks, can we get this on release after 133 gets there?

Comment 50

[Donal Meehan [:dmeehan]]

(In reply to Joel Maher ( :jmaher ) (UTC -8) from comment #49)

I understand letting 134 ride to beta, but release will not have this fix for 4 more weeks, can we get this on release after 133 gets there?

I'll uplift this to beta for Fx133, then it will ride the train to release with Fx133 on Monday.

Comment 51

[Joel Maher ( :jmaher ) (UTC -8)]

that works, thanks!

Comment 52

[Donal Meehan [:dmeehan]]

(In reply to Joel Maher ( :jmaher ) (UTC -8) from comment #51)

that works, thanks!

sorry if I wasn't clear when I rejected the release uplifts

Comment 53

[David Parks [:handyman]]

(In reply to David Parks [:handyman] from comment #43)

(In reply to Nathan LaPré from comment #41)

I think the patches landed for this bug break the MinGW build (tier 2) because of the addition of #include <Psapi.h> to WinUtils.cpp.

Ah MinGW. The fix for this is easy -- the added logging code (and include) can be #ifdefed out for these builds. I'm pretty busy at the moment but I should be able to put that together tomorrow.

Looks like nrishel beat me to fixing this in bug 1930048 and that MinGW just didn't like the capital P.

Comment 54

[Donal Meehan [:dmeehan]]

Comment on attachment 9434918 [details]
Bug 1921759: Restart Windows Explorer at start of browser_bookmark_copy_folder_tree.js r=jmaher!,#win-reviewers!

Approved for 133.0 rc1

Comment 55

[Donal Meehan [:dmeehan]]

Comment on attachment 9434919 [details]
Bug 1921759: Log the process that holds the clipboard mutex on Windows r=#win-reviewers!

Approved for 133.0 rc1

Comment 56

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-beta/rev/5edda181fa1a
https://hg.mozilla.org/releases/mozilla-beta/rev/c82a98cf2f61

Comment 57

[Donal Meehan [:dmeehan]]

:jmaher/:handyman for esr128, only one of the patches has an esr128 uplift request?
Is https://bugzilla.mozilla.org/attachment.cgi?id=9434919 not needed on esr128?

Comment 58

[Joel Maher ( :jmaher ) (UTC -8)]

it is mostly diagnostics, but if it is in-tree we should add it to esr128. Also esr-115 will need the uplift as well. The primary reason is we are upgrading the machine pool and I forgot we had 2 ESR's going at once.

Comment 59

[Donal Meehan [:dmeehan]]

:handyman there are conflicts when grafting to esr128/esr115 caused by Bug 1904909.
Please attach a rebased patch that grafts cleanly

Comment 60

[David Parks [:handyman]]

on it

Comment 61

[David Parks [:handyman]]

Attachment: Bug 1921759: ESR 115 UPLIFT - Restart Windows Explorer at start of browser_bookmark_copy_folder_tree.js

Comment 62

[David Parks [:handyman]]

Attachment: Bug 1921759: ESR 115 UPLIFT - Log the process that holds the clipboard mutex on Windows

Comment 63

[David Parks [:handyman]]

Attachment: Bug 1921759: ESR 128 UPLIFT - Restart Windows Explorer at start of browser_bookmark_copy_folder_tree.js

Comment 64

[David Parks [:handyman]]

Attachment: Bug 1921759: ESR 128 UPLIFT - Log the process that holds the clipboard mutex on Windows

Comment 65

[David Parks [:handyman]]

Try pushes for esr 115 and esr 128

Hopefully that works.

Comment 66

[Donal Meehan [:dmeehan]]

Thanks :handyman, I saw the bugmail go by.
We are now in RC week, we won't take this uplift until next week.

Comment 67

[Donal Meehan [:dmeehan]]

Comment on attachment 9434919 [details]
Bug 1921759: Log the process that holds the clipboard mutex on Windows r=#win-reviewers!

Approved for 128.6esr

Comment 68

[Donal Meehan [:dmeehan]]

Comment on attachment 9434918 [details]
Bug 1921759: Restart Windows Explorer at start of browser_bookmark_copy_folder_tree.js r=jmaher!,#win-reviewers!

Approved for 128.6esr

Comment 69

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr128/rev/d216caf33faa
https://hg.mozilla.org/releases/mozilla-esr128/rev/9d48e0e99765

Comment 70

[Donal Meehan [:dmeehan]]

Comment on attachment 9434918 [details]
Bug 1921759: Restart Windows Explorer at start of browser_bookmark_copy_folder_tree.js r=jmaher!,#win-reviewers!

Approved for 115.19esr

Comment 71

[Donal Meehan [:dmeehan]]

Comment on attachment 9434919 [details]
Bug 1921759: Log the process that holds the clipboard mutex on Windows r=#win-reviewers!

Approved for 115.19esr

Comment 72

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr115/rev/52c7b6e01928
https://hg.mozilla.org/releases/mozilla-esr115/rev/950fa856c4ef