If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

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

VERIFIED FIXED in Firefox 55

Status

()

Toolkit
Telemetry
P1
major
VERIFIED FIXED
5 months ago
12 days ago

People

(Reporter: ehoogeveen, Assigned: gsvelto)

Tracking

55 Branch
mozilla55
All
Windows
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr45 unaffected, firefox-esr52 unaffected, firefox53 unaffected, firefox54 unaffected, firefox55 verified)

Details

(Whiteboard: [measurement:client])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

5 months ago
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.
Flags: needinfo?(alessio.placitelli)

Updated

5 months ago
Duplicate of this bug: 1360818

Comment 2

5 months ago
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

5 months ago
(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 :(
(Assignee)

Updated

5 months ago
Assignee: nobody → gsvelto
Status: NEW → ASSIGNED
(Assignee)

Comment 4

5 months ago
I'll take care of this, patch almost ready.
Flags: needinfo?(alessio.placitelli)
(Assignee)

Comment 5

5 months ago
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?
Attachment #8863234 - Flags: feedback?(ted)

Comment 6

5 months ago
(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)?
Priority: -- → P1
Whiteboard: [measurement:client]
Severity: normal → major
(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

5 months ago
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)
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?
Flags: needinfo?(gsvelto)
(Assignee)

Comment 10

5 months ago
I'm on it, just came out of paternity leave.
Flags: needinfo?(gsvelto)
(Assignee)

Updated

5 months ago
Attachment #8863234 - Attachment is obsolete: true
Attachment #8863234 - Flags: feedback?(ted)
Comment hidden (mozreview-request)

Comment 12

5 months ago
mozreview-review
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?
Attachment #8864442 - Flags: review?(alessio.placitelli) → review+
(Assignee)

Comment 13

5 months ago
(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.
(Assignee)

Comment 14

5 months ago
Looking good, let's land this.

Comment 15

5 months ago
Pushed by gsvelto@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7ab439a6b381
Do not show a console on Windows when launching the pingsender; r=Dexter

Comment 16

5 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/7ab439a6b381
Status: ASSIGNED → RESOLVED
Last Resolved: 5 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
(Assignee)

Comment 17

5 months ago
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
Flags: needinfo?(amasresha)

Comment 18

5 months ago
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
(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 :)
Status: RESOLVED → VERIFIED
status-firefox55: fixed → verified
Verified as FIXED starting from Mozilla Firefox Nightly 55.0a1 (2017-05-06).
+ clearing :ni
Flags: needinfo?(amasresha)
Version: Trunk → 55 Branch
status-firefox53: --- → unaffected
status-firefox54: --- → unaffected
status-firefox-esr45: --- → unaffected
status-firefox-esr52: --- → unaffected

Comment 21

5 months ago
Verified-fixed.
Test cases are here: https://public.etherpad-mozilla.org/p/1360819
(Assignee)

Comment 22

5 months ago
Thanks a lot Abe!

Updated

5 months ago
Blocks: 1362024
Duplicate of this bug: 1364145

Updated

4 months ago
Depends on: 1364673
You need to log in before you can comment on or make changes to this bug.