1111701 - Remove E10S_AUTOSTART histogram

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect, P3)

Description

[Roberto Agostino Vitillo (:rvitillo)]

I have noticed some pings from Firefox 37.0a1 have a missing E10S_AUTOSTART histogram. I haven't digged deeper yet.

Comment 1

[Jim Mathies [:jimm]]

How serious of a problem do we have here? We're tempted to tracking- this since our telemetry data on this looks good in our control panels.

Comment 2

[Roberto Agostino Vitillo (:rvitillo)]

It's a very small fraction, about 3 every 100000 pings from Nightly 37.

Comment 3

[Georg Fritzsche [:gfritzsche]]

Roberto, did E10S_AUTOSTART provide any additional value over environment.settings.e10sEnabled or could we just remove this histogram?

Comment 4

[Roberto Agostino Vitillo (:rvitillo)]

I don't use E10S_AUTOSTART any longer and I am ok with removing it.

Comment 5

[Georg Fritzsche [:gfritzsche]]

Thanks, morphing.

Comment 6

[Alessio Placitelli [:Dexter]]

In order to remove this histogram, we need to:

- Remove the histogram definition from histograms.json [0].
- Remove its use from nsAppRunner.cpp [1].

In order to make sure that everything works correctly:

- ./mach build
- ./mach test toolkit/components/telemetry
- Run Firefox with ./mach run, then browse to about:telemetry and search for the E10S_AUTOSTART histogram in the Histograms section. It should not be there anymore.

[0] - https://dxr.mozilla.org/mozilla-central/rev/9d6ffc7a08b6b47056eefe1e652710a3849adbf7/toolkit/components/telemetry/Histograms.json
[1] - https://dxr.mozilla.org/mozilla-central/rev/9d6ffc7a08b6b47056eefe1e652710a3849adbf7/toolkit/xre/nsAppRunner.cpp

Comment 7

[Brendan Good]

(In reply to Alessio Placitelli [:Dexter] from comment #6)
> In order to remove this histogram, we need to:
> 
> - Remove the histogram definition from histograms.json [0].
> - Remove its use from nsAppRunner.cpp [1].
> 
> In order to make sure that everything works correctly:
> 
> - ./mach build
> - ./mach test toolkit/components/telemetry
> - Run Firefox with ./mach run, then browse to about:telemetry and search for
> the E10S_AUTOSTART histogram in the Histograms section. It should not be
> there anymore.
> 
> [0] -
> https://dxr.mozilla.org/mozilla-central/rev/
> 9d6ffc7a08b6b47056eefe1e652710a3849adbf7/toolkit/components/telemetry/
> Histograms.json
> [1] -
> https://dxr.mozilla.org/mozilla-central/rev/
> 9d6ffc7a08b6b47056eefe1e652710a3849adbf7/toolkit/xre/nsAppRunner.cpp

I'm starting to take a look at this and I would like just a bit of extra direction: 

1) I'm assuming that E10S_AUTOSTART and E10S_AUTOSTART_STATUS must both be removed from Histograms.json, am I correct in thinking this?

2) Could someone please go a bit more into detail about how to "remove [the E10s autostart histograms] from nsAppRunner.cpp"? As it stands right now, that is not entirely obvious to me. 

Sorry if these questions are a bit silly. Thanks!

Comment 8

[Alessio Placitelli [:Dexter]]

(In reply to Brendan from comment #7)

Hello and thanks for helping out on this!

> I'm starting to take a look at this and I would like just a bit of extra
> direction: 
> 
> 1) I'm assuming that E10S_AUTOSTART and E10S_AUTOSTART_STATUS must both be
> removed from Histograms.json, am I correct in thinking this?

No, E10S_AUTOSTART_STATUS should not be removed.

> 2) Could someone please go a bit more into detail about how to "remove [the
> E10s autostart histograms] from nsAppRunner.cpp"? As it stands right now,
> that is not entirely obvious to me. 

The E10S_AUTOSTART histogram is referenced one time in the nsAppRunner.cpp file: that line should be removed.

> Sorry if these questions are a bit silly. Thanks!

They are not ;-) Keep them coming!

Comment 9

[Brendan Good]

(In reply to Alessio Placitelli [:Dexter] from comment #8)
> (In reply to Brendan from comment #7)
> 
> Hello and thanks for helping out on this!
> 
> > I'm starting to take a look at this and I would like just a bit of extra
> > direction: 
> > 
> > 1) I'm assuming that E10S_AUTOSTART and E10S_AUTOSTART_STATUS must both be
> > removed from Histograms.json, am I correct in thinking this?
> 
> No, E10S_AUTOSTART_STATUS should not be removed.
> 
> > 2) Could someone please go a bit more into detail about how to "remove [the
> > E10s autostart histograms] from nsAppRunner.cpp"? As it stands right now,
> > that is not entirely obvious to me. 
> 
> The E10S_AUTOSTART histogram is referenced one time in the nsAppRunner.cpp
> file: that line should be removed.
> 
> > Sorry if these questions are a bit silly. Thanks!
> 
> They are not ;-) Keep them coming!

Thanks for the reply! It really helped! 

I have hit something of a snag, though. The changes themselves were fairly straightforward; however, when I ran the tests, /toolkit/components/telemetry/tests/unit/test_TelemetrySession.js and test_TelemetryEnvironment.js both failed. I suspected it may have been due to my changes so I reverted them locally and the tests still failed with the message of "application crashed [unknown top frame]"; how do I proceed? Thanks again.

Comment 10

[Alessio Placitelli [:Dexter]]

(In reply to Brendan from comment #9)
> (In reply to Alessio Placitelli [:Dexter] from comment #8)
> > (In reply to Brendan from comment #7)
> > 
> > Hello and thanks for helping out on this!
> > 
> > > I'm starting to take a look at this and I would like just a bit of extra
> > > direction: 
> > > 
> > > 1) I'm assuming that E10S_AUTOSTART and E10S_AUTOSTART_STATUS must both be
> > > removed from Histograms.json, am I correct in thinking this?
> > 
> > No, E10S_AUTOSTART_STATUS should not be removed.
> > 
> > > 2) Could someone please go a bit more into detail about how to "remove [the
> > > E10s autostart histograms] from nsAppRunner.cpp"? As it stands right now,
> > > that is not entirely obvious to me. 
> > 
> > The E10S_AUTOSTART histogram is referenced one time in the nsAppRunner.cpp
> > file: that line should be removed.
> > 
> > > Sorry if these questions are a bit silly. Thanks!
> > 
> > They are not ;-) Keep them coming!
> 
> Thanks for the reply! It really helped! 
> 
> I have hit something of a snag, though. The changes themselves were fairly
> straightforward; however, when I ran the tests,
> /toolkit/components/telemetry/tests/unit/test_TelemetrySession.js and
> test_TelemetryEnvironment.js both failed. I suspected it may have been due
> to my changes so I reverted them locally and the tests still failed with the
> message of "application crashed [unknown top frame]"; how do I proceed?
> Thanks again.

You're welcome!

The crash seems very odd, since it doesn't happen with your patch applied. Did you build again after removing the patch?

Could you post your test output to pastebin.mozilla.org and share the link?

Comment 11

[Brendan Good]

(In reply to Alessio Placitelli [:Dexter] from comment #10)
> (In reply to Brendan from comment #9)
> > (In reply to Alessio Placitelli [:Dexter] from comment #8)
> > > (In reply to Brendan from comment #7)
> > > 
> > > Hello and thanks for helping out on this!
> > > 
> > > > I'm starting to take a look at this and I would like just a bit of extra
> > > > direction: 
> > > > 
> > > > 1) I'm assuming that E10S_AUTOSTART and E10S_AUTOSTART_STATUS must both be
> > > > removed from Histograms.json, am I correct in thinking this?
> > > 
> > > No, E10S_AUTOSTART_STATUS should not be removed.
> > > 
> > > > 2) Could someone please go a bit more into detail about how to "remove [the
> > > > E10s autostart histograms] from nsAppRunner.cpp"? As it stands right now,
> > > > that is not entirely obvious to me. 
> > > 
> > > The E10S_AUTOSTART histogram is referenced one time in the nsAppRunner.cpp
> > > file: that line should be removed.
> > > 
> > > > Sorry if these questions are a bit silly. Thanks!
> > > 
> > > They are not ;-) Keep them coming!
> > 
> > Thanks for the reply! It really helped! 
> > 
> > I have hit something of a snag, though. The changes themselves were fairly
> > straightforward; however, when I ran the tests,
> > /toolkit/components/telemetry/tests/unit/test_TelemetrySession.js and
> > test_TelemetryEnvironment.js both failed. I suspected it may have been due
> > to my changes so I reverted them locally and the tests still failed with the
> > message of "application crashed [unknown top frame]"; how do I proceed?
> > Thanks again.
> 
> You're welcome!
> 
> The crash seems very odd, since it doesn't happen with your patch applied.
> Did you build again after removing the patch?
> 
> Could you post your test output to pastebin.mozilla.org and share the link?

I just did a fresh build (deleting the previous mozilla-central directory and then pulling again and then rebuilt it) and I still get this error. Here is the pastebin link which contains the test output for my build.

https://pastebin.mozilla.org/8856221

Thanks again.

Comment 12

[Alessio Placitelli [:Dexter]]

(In reply to Brendan from comment #11)
> I just did a fresh build (deleting the previous mozilla-central directory
> and then pulling again and then rebuilt it) and I still get this error. Here
> is the pastebin link which contains the test output for my build.
> 
> https://pastebin.mozilla.org/8856221
> 
> Thanks again.

As you found out, this doesn't look related to your patch:

 2:01.00 PROCESS_OUTPUT: Thread-41 (pid:27115) "FATAL ERROR: Non-local network connections are disabled and a connection attempt to lwttest.invalid (92.242.140.21) was made."
 2:01.00 PROCESS_OUTPUT: Thread-41 (pid:27115) "You should only access hostnames available via the test networking proxy (if running mochitests) or from a test-specific httpd.js server (if running xpcshell tests). Browser services should be disabled or redirected to a local server."
 2:01.01 PROCESS_OUTPUT: Thread-41 (pid:27115) "Hit MOZ_CRASH(Attempting to connect to non-local address!) at /home/brendan/mozilla-central/netwerk/base/nsSocketTransport2.cpp:1253"

Our tests do not allow non-local connections and it seems like your machine is resolving the "lwttest.invalid" address to 92.242.140.21. There's probably some relation with your network configuration and/or there's something going on with your DNS.

You can still check that the patch is working by running Firefox with ./mach run, then browse to about:telemetry and looking for for the E10S_AUTOSTART histogram in the Histograms section? It should not be there anymore.

If that works, please attach the patch here and ask for a review :)

Comment 13

[Brendan Good]

Attachment: Remove E10S_AUTOSTART histogram

Removed the autostart histogram from Histograms.json and associated line of
code from nsApprunner.cpp

Comment 14

[Alessio Placitelli [:Dexter]]

Comment on attachment 8706015 [details] [diff] [review]
Remove E10S_AUTOSTART histogram

Review of attachment 8706015 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good, thank you!

There's only one thing about the commit message. In general, having additional details on the next lines could be nice but, in this particular case, they don't add much information to the patch. Concentrating on the "why" some changes were made would be more interesting.

For this patch, I'd simply drop "Removed the autostart histogram from Histograms.json and associated line of
code from nsApprunner.cpp" from the commit message.

Comment 15

[Alessio Placitelli [:Dexter]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5c71b40ab4db

Comment 16

[Brendan Good]

(In reply to Alessio Placitelli [:Dexter] from comment #14)
> Comment on attachment 8706015 [details] [diff] [review]
> Remove E10S_AUTOSTART histogram
> 
> Review of attachment 8706015 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks good, thank you!
> 
> There's only one thing about the commit message. In general, having
> additional details on the next lines could be nice but, in this particular
> case, they don't add much information to the patch. Concentrating on the
> "why" some changes were made would be more interesting.
> 
> For this patch, I'd simply drop "Removed the autostart histogram from
> Histograms.json and associated line of
> code from nsApprunner.cpp" from the commit message.

That makes sense; I had doubts that the quoted part was unnecessary, but I thought I would play it safe; especially for a first patch. Thank you so much Alessio! I can't imagine how much more time you spent helping me through this as opposed to just doing this yourself (especially taking into account severe time zone differences). I can't thank you enough for your patience and dedication to helping me through this! Is there anything more work for me to do on this bug?

Comment 17

[Alessio Placitelli [:Dexter]]

(In reply to Brendan Good from comment #16)
> That makes sense; I had doubts that the quoted part was unnecessary, but I
> thought I would play it safe; especially for a first patch. Thank you so
> much Alessio! I can't imagine how much more time you spent helping me
> through this as opposed to just doing this yourself (especially taking into
> account severe time zone differences). I can't thank you enough for your
> patience and dedication to helping me through this! Is there anything more
> work for me to do on this bug?

Thank YOU very much for contributing, for your interest and kind words!

I would just need you to change the commit message to drop the two additional lines. I already pushed the patch to try and it looks good (see comment 15). After you attach the new patch with the changed commit message, I'll take care of landing this for you!

Comment 18

[Brendan Good]

Comment on attachment 8706015 [details] [diff] [review]
Remove E10S_AUTOSTART histogram

># HG changeset patch
># User brendan <[e-mail]>
># Date 1452357066 18000
>#      Sat Jan 09 11:31:06 2016 -0500
># Node ID 23b5d0813a3eefae838b33981446646dd047daee
># Parent  108e407489921aae013640db1e1ccc596fa3c73b
>Bug 1111701 - Remove E10S_AUTOSTART histogram; r?Dexter
>
>
>diff --git a/toolkit/components/telemetry/Histograms.json b/toolkit/components/telemetry/Histograms.json
>--- a/toolkit/components/telemetry/Histograms.json
>+++ b/toolkit/components/telemetry/Histograms.json
>@@ -8329,21 +8329,16 @@
>   },
>   "LOOP_ROOM_SESSION_WITHCHAT": {
>     "alert_emails": ["[e-mail]", "[e-mail]"],
>     "expires_in_version": "47",
>     "kind": "count",
>     "releaseChannelCollection": "opt-out",
>     "description": "Number of sessions where at least one chat message was exchanged"
>   },
>-  "E10S_AUTOSTART": {
>-    "expires_in_version": "never",
>-    "kind": "boolean",
>-    "description": "Whether a session is set to autostart e10s windows"
>-  },
>   "E10S_AUTOSTART_STATUS": {
>     "expires_in_version": "never",
>     "kind": "enumerated",
>     "n_values": 6,
>     "description": "Why e10s is enabled or disabled (0=ENABLED_BY_USER, 1=ENABLED_BY_DEFAULT, 2=DISABLED_BY_USER, 3=DISABLED_IN_SAFE_MODE, 4=DISABLED_FOR_ACCESSIBILITY, 5=DISABLED_FOR_MAC_GFX)"
>   },
>   "E10S_WINDOW": {
>     "expires_in_version": "never",
>diff --git a/toolkit/xre/nsAppRunner.cpp b/toolkit/xre/nsAppRunner.cpp
>--- a/toolkit/xre/nsAppRunner.cpp
>+++ b/toolkit/xre/nsAppRunner.cpp
>@@ -4739,17 +4739,16 @@ mozilla::BrowserTabsRemoteAutostart()
> 
>   // Uber override pref for manual testing purposes
>   if (Preferences::GetBool(kForceEnableE10sPref, false)) {
>     gBrowserTabsRemoteAutostart = true;
>     prefEnabled = true;
>     status = kE10sEnabledByUser;
>   }
> 
>-  mozilla::Telemetry::Accumulate(mozilla::Telemetry::E10S_AUTOSTART, gBrowserTabsRemoteAutostart);
>   mozilla::Telemetry::Accumulate(mozilla::Telemetry::E10S_AUTOSTART_STATUS, status);
>   if (Preferences::GetBool("browser.enabledE10SFromPrompt", false)) {
>     mozilla::Telemetry::Accumulate(mozilla::Telemetry::E10S_STILL_ACCEPTED_FROM_PROMPT,
>                                     gBrowserTabsRemoteAutostart);
>   }
>   if (prefEnabled) {
>     mozilla::Telemetry::Accumulate(mozilla::Telemetry::E10S_BLOCKED_FROM_RUNNING,
>                                     !gBrowserTabsRemoteAutostart);

Comment 19

[Brendan Good]

Comment on attachment 8706015 [details] [diff] [review]
Remove E10S_AUTOSTART histogram

># HG changeset patch
># User brendan <[e-mail]>
># Date 1452357066 18000
>#      Sat Jan 09 11:31:06 2016 -0500
># Node ID 23b5d0813a3eefae838b33981446646dd047daee
># Parent  108e407489921aae013640db1e1ccc596fa3c73b
>Bug 1111701 - Remove E10S_AUTOSTART histogram; r?Dexter
>
>diff --git a/toolkit/components/telemetry/Histograms.json b/toolkit/components/telemetry/Histograms.json
>--- a/toolkit/components/telemetry/Histograms.json
>+++ b/toolkit/components/telemetry/Histograms.json
>@@ -8329,21 +8329,16 @@
>   },
>   "LOOP_ROOM_SESSION_WITHCHAT": {
>     "alert_emails": ["[e-mail]", "[e-mail]"],
>     "expires_in_version": "47",
>     "kind": "count",
>     "releaseChannelCollection": "opt-out",
>     "description": "Number of sessions where at least one chat message was exchanged"
>   },
>-  "E10S_AUTOSTART": {
>-    "expires_in_version": "never",
>-    "kind": "boolean",
>-    "description": "Whether a session is set to autostart e10s windows"
>-  },
>   "E10S_AUTOSTART_STATUS": {
>     "expires_in_version": "never",
>     "kind": "enumerated",
>     "n_values": 6,
>     "description": "Why e10s is enabled or disabled (0=ENABLED_BY_USER, 1=ENABLED_BY_DEFAULT, 2=DISABLED_BY_USER, 3=DISABLED_IN_SAFE_MODE, 4=DISABLED_FOR_ACCESSIBILITY, 5=DISABLED_FOR_MAC_GFX)"
>   },
>   "E10S_WINDOW": {
>     "expires_in_version": "never",
>diff --git a/toolkit/xre/nsAppRunner.cpp b/toolkit/xre/nsAppRunner.cpp
>--- a/toolkit/xre/nsAppRunner.cpp
>+++ b/toolkit/xre/nsAppRunner.cpp
>@@ -4739,17 +4739,16 @@ mozilla::BrowserTabsRemoteAutostart()
> 
>   // Uber override pref for manual testing purposes
>   if (Preferences::GetBool(kForceEnableE10sPref, false)) {
>     gBrowserTabsRemoteAutostart = true;
>     prefEnabled = true;
>     status = kE10sEnabledByUser;
>   }
> 
>-  mozilla::Telemetry::Accumulate(mozilla::Telemetry::E10S_AUTOSTART, gBrowserTabsRemoteAutostart);
>   mozilla::Telemetry::Accumulate(mozilla::Telemetry::E10S_AUTOSTART_STATUS, status);
>   if (Preferences::GetBool("browser.enabledE10SFromPrompt", false)) {
>     mozilla::Telemetry::Accumulate(mozilla::Telemetry::E10S_STILL_ACCEPTED_FROM_PROMPT,
>                                     gBrowserTabsRemoteAutostart);
>   }
>   if (prefEnabled) {
>     mozilla::Telemetry::Accumulate(mozilla::Telemetry::E10S_BLOCKED_FROM_RUNNING,
>                                     !gBrowserTabsRemoteAutostart);

Comment 20

[Alessio Placitelli [:Dexter]]

Brendan, you should update the patch locally and attach the new file. I know one would be tempted to do that on bugzilla, but I'm afraid it doesn't work :(

Comment 21

[Brendan Good]

Attachment: Remove E10S_AUTOSTART histogram

Comment 22

[Brendan Good]

That makes sense, I tried to edit the attachment twice because I didn't see any change take effect. Thanks again for your help! Do you have any recommendations for a new bug to try? Searching for "good first bugs" is much easier than trying to search for "good second bugs". Also, do you have any ideas to try to figure out what is wrong with my network/dns settings that caused my problems earlier?

Comment 23

[Alessio Placitelli [:Dexter]]

https://hg.mozilla.org/integration/fx-team/rev/4107c88c8e3f1ed3321943e41d9737eac01946f6
Bug 1111701 - Remove E10S_AUTOSTART histogram. r=dexter

Comment 24

[Alessio Placitelli [:Dexter]]

(In reply to Brendan Good from comment #22)
> That makes sense, I tried to edit the attachment twice because I didn't see
> any change take effect. Thanks again for your help! Do you have any
> recommendations for a new bug to try? Searching for "good first bugs" is
> much easier than trying to search for "good second bugs". Also, do you have
> any ideas to try to figure out what is wrong with my network/dns settings
> that caused my problems earlier?

Yeah, looking for good next bugs right now could be a little tricky :-) I've flagged you on another bug that might be interesting!

AS for the Network/DNS problem, I don't know off the top of my head. You could try by dropping on IRC (irc.mozilla.org - #introduction) and asking there. Or, you may try to search the address you're resolving stuff to (92.242.140.21).

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4107c88c8e3f