Closed Bug 1364673 Opened 3 years ago Closed 2 years ago

Unwanted mouse throbber appears a few seconds after exiting Nightly when telemetry is enabled

Categories

(Toolkit :: Telemetry, defect, P1)

55 Branch
x86_64
Windows 7
defect

Tracking

()

VERIFIED FIXED
mozilla55
Tracking Status
firefox-esr52 --- unaffected
firefox53 --- unaffected
firefox54 --- unaffected
firefox55 + fixed

People

(Reporter: epinal99-bugzilla2, Assigned: gsvelto)

References

Details

(Keywords: regression, Whiteboard: [measurement:client])

Attachments

(1 file)

It's a remaining issue after fix in bug 1360819:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=2febf984044a9332f7f66b000b59d2dcee0b5515&tochange=1ee8d900197019dfdb84df0dbb861eb082ae00ce


With telemetry enabled, when the user has closed Nightly, the mouse throbber appears after a few seconds (sometimes less 1 sec) which is an unwanted behaviour making difficult the possibility for the user to identify the application perfoming some actions in the background.
Blocks: 1360819
Has Regression Range: --- → yes
Has STR: --- → yes
Flags: needinfo?(gsvelto)
Keywords: regression
Priority: -- → P1
Whiteboard: [measurement:client]
(In reply to Masatoshi Kimura [:emk] from comment #1)
> Maybe STARTF_FORCEOFFFEEDBACK will help?
> https://msdn.microsoft.com/en-us/library/windows/desktop/ms686331(v=vs.85).
> aspx

Looks like this could be added somewhere near http://searchfox.org/mozilla-central/rev/484d2b7f51b7aed035147bbb4a565061659d9278/nsprpub/pr/src/md/windows/ntmisc.c#629

I'm not sure if it will have any undesired effect on other consumers of the nsIProcess API.
(In reply to Alessio Placitelli [:Dexter] from comment #2)
> Looks like this could be added somewhere near
> http://searchfox.org/mozilla-central/rev/
> 484d2b7f51b7aed035147bbb4a565061659d9278/nsprpub/pr/src/md/windows/ntmisc.
> c#629
> 
> I'm not sure if it will have any undesired effect on other consumers of the
> nsIProcess API.

That's one way of fixing it but I'm afraid about disrupt other nsIProcess users. Another way to do this is to change the pingsender so that it doesn't appear to hang to the Windows WM. In a nutshell this involves running the actual HTTP code on a separate thread than the main one and having the main thread handle WM messages via GetMessage() since the WM detects an application as hung if it doesn't call GetMessage() for over 5 seconds.

I'll try this approach as soon as I'm done with the other bugs I'm dealing with.
I've looked into this again and apparently the amount of time the pingsender takes to run is irrelevant; the throbber appears not because the WM detects the pingsender as being run but just because it's being launched. Unfortunately nsIProcess is using ShellExecuteEx() and not CreateProcess() to launch the process so I can't use the STARTF_FORCEOFFFEEDBACK flag. I'm now toying with the other flags that function takes to see if I can solve this some other way.
Flags: needinfo?(gsvelto)
(In reply to Gabriele Svelto [:gsvelto] from comment #4)
> I've looked into this again and apparently the amount of time the pingsender
> takes to run is irrelevant; the throbber appears not because the WM detects
> the pingsender as being run but just because it's being launched.
> Unfortunately nsIProcess is using ShellExecuteEx() and not CreateProcess()
> to launch the process so I can't use the STARTF_FORCEOFFFEEDBACK flag. I'm
> now toying with the other flags that function takes to see if I can solve
> this some other way.

Thanks for looking into that.

As for the nsIProcess bit, are you sure we're using that code path? We actually have two paths for Windows in nsProcessCommon.cpp [1]:

- The first calling ShellExecuteEx if PROCESSMODEL_WINAPI is defined (which seems to be WinXP only [2]).
- The second calling PR_CreateProcess, which uses CreateProcess internally (see comment 2).

[1] - http://searchfox.org/mozilla-central/rev/9a7fbdee1d54f99cd548af95b81231d80e5f9ad1/xpcom/threads/nsProcessCommon.cpp#485,540
[2] - http://searchfox.org/mozilla-central/rev/9a7fbdee1d54f99cd548af95b81231d80e5f9ad1/xpcom/threads/nsProcess.h#11
I think I've just found the workaround we need: calling PeekMessage() when starting the pingsender didn't seem to do the trick, possibly because no message was posted into the message queue yet. On the other hand calling PostMessage()/GetMessage() with a dummy message seems to prevent Windows from showing the throbber/hourglass, possibly because it causes the WM to think that the application is already processing user input. Patch incoming.
(In reply to Alessio Placitelli [:Dexter] from comment #5)
> - The first calling ShellExecuteEx if PROCESSMODEL_WINAPI is defined (which
> seems to be WinXP only [2]).

XP_WIN is always defined on Windows; it's confusing but IIRC it's definition predates the launch of Windows XP, our codebase is *that* old :)
(In reply to Gabriele Svelto [:gsvelto] from comment #7)
> (In reply to Alessio Placitelli [:Dexter] from comment #5)
> > - The first calling ShellExecuteEx if PROCESSMODEL_WINAPI is defined (which
> > seems to be WinXP only [2]).
> 
> XP_WIN is always defined on Windows; it's confusing but IIRC it's definition
> predates the launch of Windows XP, our codebase is *that* old :)

Aargghh, you're right :)
PostMessage()/PeekMessage() pair may be better in case the post message got lost.
(In reply to Masatoshi Kimura [:emk] from comment #9)
> PostMessage()/PeekMessage() pair may be better in case the post message got
> lost.

Right, thanks. I did a bit of testing and it seems that this hides the throbber *most* of the time but not all the time. Sometimes it appears very briefly. An alternative solution is to turn the pingsender back into a console application and add an option to nsIProcess to make it use the SW_HIDE flag when starting it. Since I'm probably going to need this change for bug 1359326 too I'll try to do it that way; it feels more reliable.
Duplicate of this bug: 1366082
Given 1366082, I think we should bump this. Any progress on it, did you try the SW_HIDE trick?
Flags: needinfo?(gsvelto)
SW_HIDE works, I'm readying a patch for the other bug I need it for, so I'll have it done quickly.
Flags: needinfo?(gsvelto)
tracking for 55 as new regression.
Assignee: nobody → gsvelto
Status: NEW → ASSIGNED
Depends on: 1366711
This fix seems to work correctly once you have bug 1366711 applied; I still have to test that we're not regressing bug 1362024 though.
Comment on attachment 8872349 [details]
Bug 1364673 - Hide the mouse throbber when running the pingsender on Windows;

https://reviewboard.mozilla.org/r/143832/#review147570

This looks good, thanks. As you mentioned in comment 16, please make sure that this doesn't regress shutting down Windows when Firefox is opened.
Attachment #8872349 - Flags: review?(alessio.placitelli) → review+
I've tested the shutdown issue on Windows 10 and I can't repro, so this is looking good.
Pushed by gsvelto@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/62166eb25771
Hide the mouse throbber when running the pingsender on Windows; r=Dexter
https://hg.mozilla.org/mozilla-central/rev/62166eb25771
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Verified as fixed.
I tested this with shutdown ping feature in pre-beta testing.
Test runs are available here:https://testrail.stage.mozaws.net/index.php?/reports/view/363
or 
https://drive.google.com/a/mozilla.com/file/d/1gEB3CBrRdbcffEWa0uCjscHNEqV8jRINpiU57Lm5wsU1MP4qLOAdrQXvJPCeTMxMeL7SjaIT4ZtRnPx2/view?usp=sharing

Needinfo me if you have questions or feedback.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.