1357745 - Disable the shutdown ping sender

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect, P1)

Description

[Alessio Placitelli [:Dexter]]

Due to bug 1356673, we know that Firefox is crashing at shutdown due to the pingsender being invoked to send the shutdown ping.

Since we can't measure the volume of this crash, as it seems to happen after the crash reporter handler shuts down, we need to disable sending the shutdown ping using the pingsender as a precaution.

Comment 1

[Alessio Placitelli [:Dexter]]

Attachment: Bug 1357745 - Disable the shutdown ping sender.

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

Comment 2

[Alessio Placitelli [:Dexter]]

I've tested this manually on my machine and no shutdown ping is sent on shutdown anymore, but rather on the next Firefox restart.

Should we update the docs as well or leave them as they are since we'll hopefully fix the crash problem soon-ish?

Comment 3

[Alessio Placitelli [:Dexter]]

I think I've found the root cause of the crash from bug 1356673. Let's wait for the discussion there before moving on with this.

Comment 4

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8859592 [details]
Bug 1357745 - Disable the shutdown ping sender.

https://reviewboard.mozilla.org/r/131594/#review134450

In the future, lets use prefs to turn this on please.
That makes turning it off an easy pref change, which is trivial to approve/uplift.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/23199dc1bed3
Disable the shutdown ping sender. r=gfritzsche

Comment 6

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/23199dc1bed3

Comment 7

[Alessio Placitelli [:Dexter]]

Comment on attachment 8859592 [details]
Bug 1357745 - Disable the shutdown ping sender.

Approval Request Comment
[Feature/Bug causing the regression]: bug 1354482
[User impact if declined]: Firefox will crash on shutdown
[Is this code covered by automated tests?]: No, this only disables the feature.
[Has the fix been verified in Nightly?]: No
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: No
[Why is the change risky/not risky?]: It flips the pref that controls the feature, to disable it.
[String changes made/needed]: None

Comment 8

[Gerry Chang [:gchang]]

Comment on attachment 8859592 [details]
Bug 1357745 - Disable the shutdown ping sender.

Avoid Firefox crash on shutdown. Beta54+. Should be in 54 beta 2.

Comment 9

[Carsten Book [:Tomcat]]

needs rebasing

grafting 412816:23199dc1bed3 "Bug 1357745 - Disable the shutdown ping sender. r=gfritzsche"
merging toolkit/components/telemetry/TelemetrySession.jsm
warning: conflicts while merging toolkit/components/telemetry/TelemetrySession.jsm! (edit, then use 'hg resolve --mark')
abort: unresolved conflicts, can't continue
(use 'hg resolve' and 'hg graft --continue')

Comment 10

[Alessio Placitelli [:Dexter]]

(In reply to Carsten Book [:Tomcat] from comment #9)
> needs rebasing
> 
> grafting 412816:23199dc1bed3 "Bug 1357745 - Disable the shutdown ping
> sender. r=gfritzsche"
> merging toolkit/components/telemetry/TelemetrySession.jsm
> warning: conflicts while merging
> toolkit/components/telemetry/TelemetrySession.jsm! (edit, then use 'hg
> resolve --mark')
> abort: unresolved conflicts, can't continue
> (use 'hg resolve' and 'hg graft --continue')

Gah! Turns out I was wrong, bug 1354482 isn't on Beta yet, so this isn't needed there.

Comment 11

[Gerry Chang [:gchang]]

Hi :Dexter,
Does that mean 54 is not affected? I'll mark 54 as unaffected.

Comment 12

[Alessio Placitelli [:Dexter]]

(In reply to Gerry Chang [:gchang] from comment #11)
> Hi :Dexter,
> Does that mean 54 is not affected? I'll mark 54 as unaffected.

Yes, it isn't affected, sorry for the confusion.

Comment 13

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8859592 [details]
Bug 1357745 - Disable the shutdown ping sender.

Doesn't need uplift after all.