1360819 - On Windows Nightly, PingSender shows terminal while browser is shutting down

Status: VERIFIED FIXED

(Toolkit :: Telemetry, defect, P1)

Description

[Emanuel Hoogeveen [:ehoogeveen]]

Just updated to the latest Nightly and got a bit worried when I saw a program popping up as I shut down the browser.

My guess is that PingSender is built as a console application (/SUBSYSTEM:CONSOLE) when it should be a Windows application (/SUBSYSTEM:WINDOWS); this will prevent it from spawning a console unless you do it manually.

I'm guessing this is a regression from the change in bug 1356673; maybe the way we spawned it before avoided this problem.

Comment 2

[DJ-Leith]

In Bug 1336360 "Use the PingSender for sending pings when the browser shuts down"

See: https://bugzilla.mozilla.org/show_bug.cgi?id=1336360#c39

Alessio Placitelli [:Dexter] 2017-03-06 02:55:10 PST wrote:

> Gabriele, I've noticed something interesting when trying the patch manually
> on Windows.
> 
> If I run and then shut down Firefox, the ping sender indeed gets called
> (nothing odd here). However, a black command line prompt shows up for
> just a second. Are you aware of any way to suppress that?

This looks like a very similar issue.

DJ-Leith

Comment 3

[Alice0775 White]

(In reply to DJ-Leith from comment #2)
> In Bug 1336360 "Use the PingSender for sending pings when the browser shuts
> down"
> 
> See: https://bugzilla.mozilla.org/show_bug.cgi?id=1336360#c39
> 
> Alessio Placitelli [:Dexter] 2017-03-06 02:55:10 PST wrote:
> 
> > Gabriele, I've noticed something interesting when trying the patch manually
> > on Windows.
> > 
> > If I run and then shut down Firefox, the ping sender indeed gets called
> > (nothing odd here). However, a black command line prompt shows up for
> > just a second. Are you aware of any way to suppress that?
> 
> This looks like a very similar issue.
> 
> DJ-Leith


Aha,  It seems Mozilla made the same mistake. No automate test is provided :(

Comment 4

[Gabriele Svelto [:gsvelto]]

I'll take care of this, patch almost ready.

Comment 5

[Gabriele Svelto [:gsvelto]]

Attachment: [PATCH WIP] Make the pingsender a GUI application on Windows

I've taken this from the crashreporter; it's not pretty but it works. Would there be a better way to do this? Possibly something related to bug 1223530?

Comment 6

[Ziga Seilnacht]

(In reply to Gabriele Svelto [:gsvelto] from comment #5)
> Created attachment 8863234 [details] [diff] [review]
> [PATCH WIP] Make the pingsender a GUI application on Windows
> 
> I've taken this from the crashreporter; it's not pretty but it works. Would
> there be a better way to do this? Possibly something related to bug 1223530?

Could you maybe use CommandLineToArgvW (https://msdn.microsoft.com/en-us/library/windows/desktop/bb776391.aspx)?

Comment 7

[Alessio Placitelli [:Dexter]]

(In reply to Gabriele Svelto [:gsvelto] from comment #5)
> Created attachment 8863234 [details] [diff] [review]
> [PATCH WIP] Make the pingsender a GUI application on Windows
> 
> I've taken this from the crashreporter; it's not pretty but it works. Would
> there be a better way to do this? Possibly something related to bug 1223530?

I'm not sure if this breaks anything else, but I found that adding the following line to pingsender_win.cpp (without any other change)

> #pragma comment(linker, "/SUBSYSTEM:windows /ENTRY:mainCRTStartup")

solves the problem in a cleaner way, as it doesn't force us to define a WinMain function. I'm not sure if this introduces any other regression though, I quickly tested this locally on Windows and it seems to behave correctly.

Comment 8

[Gabriela [:gaby2300]]

Windows 7 64 bit and today's Nightly: Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:55.0) Gecko/20100101 Firefox/55.0

I can reproduce this (the black command line prompt showing up for just a second in a window titled ping sender)

Comment 9

[Alessio Placitelli [:Dexter]]

Gabriele, I caught up with Ted over IRC. Looks like the solution in comment 7 is ok. Would you mind producing a patch for that?

Comment 10

[Gabriele Svelto [:gsvelto]]

I'm on it, just came out of paternity leave.

Comment 11

[Gabriele Svelto [:gsvelto]]

Attachment: Bug 1360819 - Do not show a console on Windows when launching the pingsender;

Review commit: https://reviewboard.mozilla.org/r/136118/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/136118/

Comment 12

[Alessio Placitelli [:Dexter]]

Comment on attachment 8864442 [details]
Bug 1360819 - Do not show a console on Windows when launching the pingsender;

https://reviewboard.mozilla.org/r/136118/#review139130

This looks good to me, thanks. Would you mind flagging QA to verify this on the different supported Windows platforms?

Comment 13

[Gabriele Svelto [:gsvelto]]

(In reply to Alessio Placitelli [:Dexter] from comment #12)
> Comment on attachment 8864442 [details]
> Bug 1360819 - Do not show a console on Windows when launching the pingsender;
> 
> https://reviewboard.mozilla.org/r/136118/#review139130
> 
> This looks good to me, thanks. Would you mind flagging QA to verify this on
> the different supported Windows platforms?

Sure, I'm waiting for the try run to finish first though as there's a Windows failure that looks like an intermittent but I want to be 100% sure before landing.

Comment 14

[Gabriele Svelto [:gsvelto]]

Looking good, let's land this.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7ab439a6b381
Do not show a console on Windows when launching the pingsender; r=Dexter

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7ab439a6b381

Comment 17

[Gabriele Svelto [:gsvelto]]

Abe could you test this on Windows alone? The test case is:

- Open Firefox, wait at least 1 minute;
- Close Firefox
- No console/terminal windows should be shown

Comment 18

[Gabriela [:gaby2300]]

FWI, I still see the console/terminal windows when closing Nightly using Windows 7 64 bit, User Agent Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:55.0) Gecko/20100101 Firefox/55.0, Build ID  20170505030252

Comment 19

[Alessio Placitelli [:Dexter]]

(In reply to Gabriela [:gaby2300] from comment #18)
> FWI, I still see the console/terminal windows when closing Nightly using
> Windows 7 64 bit, User Agent Mozilla/5.0 (Windows NT 6.1; Win64; x64;
> rv:55.0) Gecko/20100101 Firefox/55.0, Build ID  20170505030252

This fix didn't make it into that Nightly (see https://hg.mozilla.org/mozilla-central/file/0b255199db9d6a6f189b89b7906f99155bde3726/toolkit/components/telemetry/pingsender/pingsender_win.cpp ). It will be in the next Nightly!

Thanks for checking this out though :)

Comment 20

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

Verified as FIXED starting from Mozilla Firefox Nightly 55.0a1 (2017-05-06).
+ clearing :ni

Comment 21

[Abe - QA (:Abe_LV)]

Verified-fixed.
Test cases are here: https://public.etherpad-mozilla.org/p/1360819

Comment 22

[Gabriele Svelto [:gsvelto]]

Thanks a lot Abe!