FX_MIGRATION_HOMEPAGE_IMPORTED probe is (incorrectly) always true.

RESOLVED FIXED in Firefox 49

Status

()

Firefox
Migration
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: Dolske, Assigned: Dolske)

Tracking

Trunk
Firefox 51
Points:
---

Firefox Tracking Flags

(firefox49 fixed, firefox50 fixed, firefox51 fixed)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

a year ago
Looking at the FX_MIGRATION_HOMEPAGE_IMPORTED telemetry probe on Beta-48 (https://mzl.la/2bJYfoH), it seems this always report "true".

The code was added as:

  Services.telemetry.getKeyedHistogramById("FX_MIGRATION_HOMEPAGE_IMPORTED")
                     .add(this._source, !!this._newHomePage);

Unfortunately, it looks like this._newHomePage has a value of "DEFAULT" to indicate that the user doesn't want to import their homepage:

http://searchfox.org/mozilla-central/rev/44f6964ba95b8ddd8ebf70c55b34cd2323afeef4/browser/components/migration/content/migration.js#315,329


So, need a patch to s/!!this._newHomePage/this._newHomePage != "DEFAULT"/

Should we rename the probe to make sure we're not looking at bogus data?
Comment hidden (mozreview-request)
(Assignee)

Updated

a year ago
Assignee: nobody → dolske
Comment on attachment 8785030 [details]
Bug 1298208 -  FX_MIGRATION_HOMEPAGE_IMPORTED probe is (incorrectly) always true.

https://reviewboard.mozilla.org/r/74334/#review72188
Attachment #8785030 - Flags: review?(MattN+bmo) → review+

Comment 3

a year ago
Pushed by jdolske@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8f30fd1bbbe1
FX_MIGRATION_HOMEPAGE_IMPORTED probe is (incorrectly) always true. r=MattN
(Assignee)

Comment 4

a year ago
Comment on attachment 8785030 [details]
Bug 1298208 -  FX_MIGRATION_HOMEPAGE_IMPORTED probe is (incorrectly) always true.

Approval Request Comment
[Feature/regressing bug #]: bug 731025
[User impact if declined]: No user impact, correctness fix for telemetry data
[Describe test coverage new/current, TreeHerder]: none, but QA tests feature for releases
[Risks and why]: Trivial fix, ultra low risk.
[String/UUID change made/needed]: N/A
Attachment #8785030 - Flags: approval-mozilla-beta?
Attachment #8785030 - Flags: approval-mozilla-aurora?
This managed to break all builds like https://treeherder.mozilla.org/logviewer.html#?job_id=2577571&repo=autoland

Backed out in https://hg.mozilla.org/integration/autoland/rev/20988ef014a1
Flags: needinfo?(dolske)
Imo, we should just completely remove homepage importing.
Most of the browsers today offer a default homepage with search capability and local contents, or an homepage that points to their corporate page (msn, apple...). Very few users change it.
Plus we are moving towards better interaction like ActivityStream.
This import code gives very small benefits to the user, and it's also (IIRC) one of the few remaining pieces that is synchronous. The cost largely overweight the benefit.
(Assignee)

Comment 7

a year ago
Bah. That's what I get for walking away at the end of the day before checking a local build. In my defense, histogram-whitelists.json was added after the original patch.

I fixed the original patch by updating histogram-whitelists.json (this was just a build-time check), and as penance added an extra patch to update the other migration telemetry probes with a bug number and email, allowing them to be removed from the whitelist. [Telemetry probes are now checked for mandatory fields, older probes missing these fields are grandfathered in via this whitelist.]

Build is fine locally now. :)
Flags: needinfo?(dolske)
(Assignee)

Comment 8

a year ago
(In reply to Marco Bonardo [::mak] from comment #6)
> Imo, we should just completely remove homepage importing.

I tend to agree, I think we were curious to first see if people were already opting-out / how often it had a page to import.
(Assignee)

Updated

a year ago
Attachment #8785030 - Attachment is obsolete: true
Attachment #8785030 - Flags: review+
Attachment #8785030 - Flags: approval-mozilla-beta?
Attachment #8785030 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 9

a year ago
Created attachment 8785368 [details] [diff] [review]
Fix probe (part 1, v.2)

(I can't figure out how to make mozreview actually attach patches again to this bug, so we're back to normal patches. Grr.)
Attachment #8785368 - Flags: review+
(Assignee)

Comment 10

a year ago
Created attachment 8785371 [details] [diff] [review]
Add metadata (Part 2, v.1)
Attachment #8785371 - Flags: review?(MattN+bmo)
Attachment #8785371 - Flags: review?(MattN+bmo) → review+
(Assignee)

Comment 11

a year ago
https://hg.mozilla.org/integration/fx-team/rev/65e93bfe39e501e15ab1d044d35b92c3ca7ada4e
Bug 1298208 -  FX_MIGRATION_HOMEPAGE_IMPORTED probe is (incorrectly) always true. r=mattn

https://hg.mozilla.org/integration/fx-team/rev/f1c8f78cd6f6e49a5f67b37567888473b898cb90
Bug 1298208 - add bug number and notification emails for migrator probes, removing them from exception whitelist. r=mattn
(Assignee)

Updated

a year ago
Attachment #8785368 - Attachment description: Fix probe (part 2, v.2) → Fix probe (part 1, v.2)
(Assignee)

Comment 12

a year ago
Comment on attachment 8785368 [details] [diff] [review]
Fix probe (part 1, v.2)

Approval Request Comment
[Feature/regressing bug #]: bug 731025
[User impact if declined]: No user impact, correctness fix for telemetry data
[Describe test coverage new/current, TreeHerder]: none, but QA tests feature for releases
[Risks and why]: Trivial fix, ultra low risk.
[String/UUID change made/needed]: N/A
Attachment #8785368 - Flags: approval-mozilla-beta?
Attachment #8785368 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 13

a year ago
https://hg.mozilla.org/integration/fx-team/rev/e6121d319f60b0e800d4b446ce8a0fc9cd48c21e
Bug 1298208 - followup fix for correct JS type. CLOSED TREE
(Assignee)

Comment 14

a year ago
I hate this bug. :(
Comment on attachment 8785368 [details] [diff] [review]
Fix probe (part 1, v.2)

Sounds like this is fairly crucial for us to know from telemetry. 
Let's take it for aurora and for beta 8. 
Andrei does your team test migration between beta 8 and release? A little bit of smoketest on migration might be good.
Flags: needinfo?(andrei.vaida)
Attachment #8785368 - Flags: approval-mozilla-beta?
Attachment #8785368 - Flags: approval-mozilla-beta+
Attachment #8785368 - Flags: approval-mozilla-aurora?
Attachment #8785368 - Flags: approval-mozilla-aurora+

Comment 16

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/65e93bfe39e5
https://hg.mozilla.org/mozilla-central/rev/f1c8f78cd6f6
https://hg.mozilla.org/mozilla-central/rev/e6121d319f60
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox51: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51

Comment 17

a year ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/47d0465d6f57
https://hg.mozilla.org/releases/mozilla-aurora/rev/5ed3f227fe0b
status-firefox50: --- → fixed

Comment 18

a year ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/280ba53d17e1
https://hg.mozilla.org/releases/mozilla-beta/rev/c8f9000e88f2
status-firefox49: --- → fixed
(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #15)
> Comment on attachment 8785368 [details] [diff] [review]
> Fix probe (part 1, v.2)
> 
> Sounds like this is fairly crucial for us to know from telemetry. 
> Let's take it for aurora and for beta 8. 
> Andrei does your team test migration between beta 8 and release? A little
> bit of smoketest on migration might be good.

We've spot-checked migration again using the latest beta tinderbox build (containing this fix)
on Windows 10 x64 [1], Ubuntu 16.04 x64 [2] and Mac OS X 10.11.6 [3]. 

Apart from a few already known bugs [4], there were no new issues identified during testing.

Please let me know if there's anything else we can help with here.


[1] http://archive.mozilla.org/pub/firefox/tinderbox-builds/mozilla-beta-win32/1472680282/
[2] http://archive.mozilla.org/pub/firefox/tinderbox-builds/mozilla-beta-linux64/1472680282/
[3] http://archive.mozilla.org/pub/firefox/tinderbox-builds/mozilla-beta-macosx64/1472680282/
[4] Known bugs - Windows 10 x64: https://mzl.la/2cbWqOq
               - Ubuntu 16.04 x64: https://mzl.la/2cbUvK6
               - Mac OS X 10.11.6: https://mzl.la/2cbUB47
Flags: needinfo?(andrei.vaida)
You need to log in before you can comment on or make changes to this bug.