1312182 - MOZ_TELEMETRY_REPORTING is "dangerous"

Status: VERIFIED FIXED

(Toolkit :: Telemetry, defect, P1)

Description

[Marco Bonardo [:mak]]

I'd like to understand how much of MOZ_TELEMETRY_REPORTING is still needed in frontend code.

We have only a few points in browser code using it, and that may cause more problems than benefits imo, because the code that we (developers) run differs from the code that we ship.
For example this caused bug 1306639, where I thought we regressed telemetry, but in reality I always tested with my own build where MOZ_TELEMETRY_REPORTING was not defined, and thus it was missing a notification observer.

Having sliced out code in developer builds is scary, it may either hide bugs or create them.

This is the points where this is defined:
http://searchfox.org/mozilla-central/search?q=MOZ_TELEMETRY_REPORTING&path=

I think at least some of the uses from browser/ should go away, do we plan to ship browser without telemetry? Is this for non official builds?
If so, I think it would be ok if we only hide the UI parts, but we should not remove code like the one in nsBrowserGlue that is already excluded by the telemetry backend.

I'm not sure if there's any other point that we could remove or cleanup, and that's why I'm filing this under telemetry.
Could you please audit the callpoints and tell what can go away?

Comment 1

[Alessio Placitelli [:Dexter]]

(In reply to Marco Bonardo [::mak] from comment #0)
> I think at least some of the uses from browser/ should go away, do we plan
> to ship browser without telemetry? Is this for non official builds?
> If so, I think it would be ok if we only hide the UI parts, but we should
> not remove code like the one in nsBrowserGlue that is already excluded by
> the telemetry backend.

In bug 1200164, we made MOZ_TELEMETRY_REPORTING control the upload of the Telemetry pings. Long story short, without this definition telemetry should still be gathered (e.g. for dev builds) but must not be sent.

With that context, I think that what you found is a bug: we should not be defending the call sites in nsBrowserGlue.js with MOZ_TELEMETRY_REPORTING.

> I'm not sure if there's any other point that we could remove or cleanup, and
> that's why I'm filing this under telemetry.
> Could you please audit the callpoints and tell what can go away?

The rest of the call sites are for the Preferences UI. I think these should be changed as well, as we would always want to show the UI if Telemetry is recording. Otherwise, we end up with bugs like bug 1284010.

Georg, any opinion on the points above?

[1] - http://searchfox.org/mozilla-central/rev/84075be5067b68dc6cb3b89f999645650e68c05b/toolkit/components/telemetry/Telemetry.cpp#1898

Comment 2

[Alessio Placitelli [:Dexter]]

Attachment: bug1312182.patch

After discussing this a bit, I've attempted to get rid of all the references to MOZ_TELEMETRY_REPORTING in the front end code.

As mentioned in comment 1, MOZ_TELEMETRY_REPORTING controls if pings get uploaded to our servers, not the recording itself. For this reason, it should not be used to enable or disable UI controls.

This patch does 2 things:

- Removes the MOZ_TELEMETRY_REPORTING wrapping for the keyword-search feature in nsBrowserGlue.js
- Always enables the Preferences entries for Telemetry, but makes them "disabled" if MOZ_TELEMETRY_REPORTING is not defined, as the preferences explicitly mention "sharing" and "sending", so they refer to ping uploads.

Georg, do these changes make sense for you? We should probably also require somebody else to review the code changes, especially the ones for the Preferences panel.

Comment 3

[Alessio Placitelli [:Dexter]]

Mh, I wonder if we could simply use |telemetry.isOfficialTelemetry| for enabling/disabling the UI parts instead of relying solely on MOZ_TELEMETRY_REPORTING. We're relying in the former for sending [1].

[1] - https://dxr.mozilla.org/mozilla-central/rev/79feeed4293336089590320a9f30a813fade8e3c/toolkit/components/telemetry/TelemetrySend.jsm#1031

Comment 4

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8811183 [details] [diff] [review]
bug1312182.patch

Jared, can you also take a look over the advanced.js changes?

I think we should probably have a localized message there when the data settings are disabled, e.g. "Data reporting is not enabled for this build configuration."
Otherwise it is a bit confusing as to why this is greyed out / disabled.

Comment 5

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8811183 [details] [diff] [review]
bug1312182.patch

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

::: browser/components/preferences/in-content/advanced.js
@@ +246,5 @@
>    {
> +    this._setupLearnMoreLink("toolkit.telemetry.infoURL", "telemetryLearnMore");
> +    // If we're not sending any Telemetry, disble The telemetry one as well.
> +    if (!AppConstants.MOZ_TELEMETRY_REPORTING) {
> +      document.getElementById("submitTelemetryBox").setAttribute("disabled", "true");

I agree with Georg, we should have a localizable message here that is displayed when these controls are disabled.

Comment 6

[Alessio Placitelli [:Dexter]]

Attachment: bug1312182.patch

@gfritzsche: I've added a localized string as requested by you and jaws. Do we need to bring someone from the UX on board for checking that? I've attached a screenshot to see how it looks like.

@flod: would you kindly check the localization bits? This patch adds a new string that is only shown on builds that are not sending any Telemetry.

Comment 7

[Alessio Placitelli [:Dexter]]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #5)
> Comment on attachment 8811183 [details] [diff] [review]
> bug1312182.patch
> 
> Review of attachment 8811183 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/components/preferences/in-content/advanced.js
> @@ +246,5 @@
> >    {
> > +    this._setupLearnMoreLink("toolkit.telemetry.infoURL", "telemetryLearnMore");
> > +    // If we're not sending any Telemetry, disble The telemetry one as well.
> > +    if (!AppConstants.MOZ_TELEMETRY_REPORTING) {
> > +      document.getElementById("submitTelemetryBox").setAttribute("disabled", "true");
> 
> I agree with Georg, we should have a localizable message here that is
> displayed when these controls are disabled.

Thanks :jaws!

Comment 8

[Francesco Lodolo [:flod]]

Comment on attachment 8818535 [details] [diff] [review]
bug1312182.patch

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

The l10n bits look good (I don't see a screenshot though).

::: browser/locales/en-US/chrome/browser/preferences/advanced.dtd
@@ +29,5 @@
>  <!ENTITY checkSpelling.accesskey         "t">
>  
>  <!ENTITY dataChoicesTab.label            "Data Choices">
>  
> +<!ENTITY healthReportingDisabled.label   "Data reporting is not enabled for this build configuration">

I wonder if "available" is more suitable for this case?

Comment 9

[Alessio Placitelli [:Dexter]]

Attachment: datareporting.png

Telemetry supported vs Telemetry unsupported screenshot.

Comment 10

[Alessio Placitelli [:Dexter]]

(In reply to Francesco Lodolo [:flod] (mostly out of office until Dec 19) from comment #8)
> Comment on attachment 8818535 [details] [diff] [review]
> bug1312182.patch
> 
> Review of attachment 8818535 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The l10n bits look good (I don't see a screenshot though).
> 
> ::: browser/locales/en-US/chrome/browser/preferences/advanced.dtd
> @@ +29,5 @@
> >  <!ENTITY checkSpelling.accesskey         "t">
> >  
> >  <!ENTITY dataChoicesTab.label            "Data Choices">
> >  
> > +<!ENTITY healthReportingDisabled.label   "Data reporting is not enabled for this build configuration">
> 
> I wonder if "available" is more suitable for this case?

Ouch, I forgot to attach the screenshot. Thank you for catching that. I'll update the message once I get the review from Georg as well!

Comment 11

[Francesco Lodolo [:flod]]

Side question (coffee didn't fully kick in yet…): is this easily reproducible on a standard build? My guess would be not, in which case maybe we should add a note. Example:

<!-- LOCALIZATION NOTE (healthReportingDisabled.label): This message is
displayed above disabled data sharing options in special builds. -->

Comment 12

[Alessio Placitelli [:Dexter]]

(In reply to Francesco Lodolo [:flod] (mostly out of office until Dec 19) from comment #11)
> Side question (coffee didn't fully kick in yet…): is this easily
> reproducible on a standard build? 

If you clone m-c and build without providing MOZ_TELEMETRY_REPORTING in the .mozconfig file, you'd see the message. This is also the case for third party packaged builds with no Telemetry support (Fedora, Debian, archlinux, etc.).

Basically, you should be able to see this message unless it's an official build by us.

But I think it makes sense to leave a note there, I'll do that.

Comment 13

[Alessio Placitelli [:Dexter]]

Comment on attachment 8818580 [details]
datareporting.png

:phlsa, flagging you on this one as I'm not sure who's the best person to ask :)

Background: since builds compiled directly from mozilla-central and third party builds (Firefox packed in Fedora, ArchLinux, etc.) cannot send Telemetry, we're disabling the options in the Preferences and showing a clear message explaining why.

From an UX perspective, does the new look (below, in the screenshot) look reasonable?

Comment 14

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8818535 [details] [diff] [review]
bug1312182.patch

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

Generally this looks fine to me, i left some comments to follow up on below.

::: browser/base/content/baseMenuOverlay.xul
@@ -60,5 @@
>                    oncommand="openHelpLink('keyboard-shortcuts')"
>                    onclick="checkForMiddleClick(this, event);"
>                    label="&helpKeyboardShortcuts.label;"
>                    accesskey="&helpKeyboardShortcuts.accesskey;"/>
> -#ifdef MOZ_TELEMETRY_REPORTING

We should not remove this, but change it to MOZ_SERVICES_HEALTHREPORT, as that is the flag that controls whether we have about:healthreport:
https://dxr.mozilla.org/mozilla-central/rev/2785aaf276ba29fb2e1f5607d90d441fee42efb4/browser/base/jar.mn#49

::: browser/components/preferences/in-content/advanced.js
@@ +252,5 @@
>     */
>    initTelemetry: function()
>    {
> +    this._setupLearnMoreLink("toolkit.telemetry.infoURL", "telemetryLearnMore");
> +    // If we're not sending any Telemetry, disble The telemetry one as well.

"disable the Telemetry"
Also, do you mean the "Telemetry upload checkbox" or so? "the Telemetry one" is vague.

@@ +254,5 @@
>    {
> +    this._setupLearnMoreLink("toolkit.telemetry.infoURL", "telemetryLearnMore");
> +    // If we're not sending any Telemetry, disble The telemetry one as well.
> +    if (!AppConstants.MOZ_TELEMETRY_REPORTING) {
> +      document.getElementById("submitTelemetryBox").setAttribute("disabled", "true");

When, using a dev build with toolkit.telemetry.enabled;true, i open preferences and this triggers...
Will this trigger setTelemetrySectionEnabled() and disable my extended Telemetry recording?

Ditto with all the checkbox setting below.

Comment 15

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

(In reply to Alessio Placitelli [:Dexter] from comment #13)
> Comment on attachment 8818580 [details]
> datareporting.png
> 
> :phlsa, flagging you on this one as I'm not sure who's the best person to
> ask :)
> 
> Background: since builds compiled directly from mozilla-central and third
> party builds (Firefox packed in Fedora, ArchLinux, etc.) cannot send
> Telemetry, we're disabling the options in the Preferences and showing a
> clear message explaining why.
> 
> From an UX perspective, does the new look (below, in the screenshot) look
> reasonable?

Sorry, took me forever to respond...
Yes, looks reasonable!

Comment 16

[Alessio Placitelli [:Dexter]]

Attachment: bug1312182.patch

Comment 17

[Alessio Placitelli [:Dexter]]

Comment on attachment 8826139 [details] [diff] [review]
bug1312182.patch

(In reply to Georg Fritzsche [:gfritzsche] from comment #14)
> @@ +254,5 @@
> >    {
> > +    this._setupLearnMoreLink("toolkit.telemetry.infoURL", "telemetryLearnMore");
> > +    // If we're not sending any Telemetry, disble The telemetry one as well.
> > +    if (!AppConstants.MOZ_TELEMETRY_REPORTING) {
> > +      document.getElementById("submitTelemetryBox").setAttribute("disabled", "true");
> 
> When, using a dev build with toolkit.telemetry.enabled;true, i open
> preferences and this triggers...
> Will this trigger setTelemetrySectionEnabled() and disable my extended
> Telemetry recording?
> 
> Ditto with all the checkbox setting below.

I've updated the patch with the changes suggested by :Flod.

I've also manually checked that the prefs behave consistently in developer builds, as follows:

- I've manually set the uploadEnabled and telemetry.enabled prefs to true.
- Opened the about:preferences
- Navigated to the data choices section (it showed the first checkbox as locked & untick, the share additional data locked & ticked)
- Closed the page
- The value in the prefs did not change. That's because we're calling |initSubmitHealthReport| after checking if the pref is locked or MOZ_TELEMETRY_REPORTING is not set. But we return if that's the case.

This leaves us with an inconsistent pref synchronization between UI and pref value: we're manually unticking the first checkbox (even if the pref is true), but leaving the second untouched (and so ticked if the pref is true).

Should we care about that? Maybe a better option would be to just lock the options without actually ticking/unticking them if telemetry is not available.

Comment 18

[Julien Cristau [:jcristau]]

Mass wontfix for bugs affecting firefox 52.

Comment 19

[Alessio Placitelli [:Dexter]]

Comment on attachment 8826139 [details] [diff] [review]
bug1312182.patch

Cancelling the review: I'll rebase this and break it into UI and Telemetry specific changes to ease reviewing.

Comment 20

[Alessio Placitelli [:Dexter]]

Attachment: nightly_prefs.PNG

Comment 21

[Alessio Placitelli [:Dexter]]

Attachment: Bug 1312182 - Consolidate the uses of MOZ_TELEMETRY_REPORTING in the code.

The MOZ_TELEMETRY_REPORTING define does not control wheter or not Telemetry is enabled
but rather if it will send the gathered data to Mozilla servers. We still want to
display the about:preferences options and let developer know about this behaviour.
Please note that this patch is not changing any behaviour: it's only making it explicit
by showing the options as disabled rather than hiding them.

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

Comment 22

[Alessio Placitelli [:Dexter]]

I've un-bitrot the patch and pushed it back on MozReview. The behaviour from comment 17 is preserved, there's only one major change: while the Telemetry prefs are now in privacy.xul, the localization strings are still in advanced.dtd, so I've added the new one there as well.

Comment 23

[Alessio Placitelli [:Dexter]]

Comment on attachment 8872555 [details]
Bug 1312182 - Consolidate the uses of MOZ_TELEMETRY_REPORTING in the code.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/144078/diff/1-2/

Comment 24

[Francesco Lodolo [:flod]]

Comment on attachment 8872555 [details]
Bug 1312182 - Consolidate the uses of MOZ_TELEMETRY_REPORTING in the code.

https://reviewboard.mozilla.org/r/144078/#review147798

Strings-wise it looks good. With prefs reorg, strings have been moved all over the place, so it's expected to find strings for privacy still in advanced.dtd, moving them around would create a lot of churn for localizers.

Talking about prefs reorg: do you need this also in preferences-old, since the reorg is not moving to Beta with 55?

Comment 25

[Alessio Placitelli [:Dexter]]

Comment on attachment 8872555 [details]
Bug 1312182 - Consolidate the uses of MOZ_TELEMETRY_REPORTING in the code.

https://reviewboard.mozilla.org/r/144078/#review147798

Mh, I don't think that's needed: this change is only helpful for developers who build from mozilla-central.

Comment 26

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8872555 [details]
Bug 1312182 - Consolidate the uses of MOZ_TELEMETRY_REPORTING in the code.

https://reviewboard.mozilla.org/r/144078/#review149622

Thanks, this looks good.

Can we get a safety QA pass scheduled on this for the "official" build configuration?

::: browser/components/preferences/in-content/privacy.xul:763
(Diff revision 2)
> -#ifdef MOZ_TELEMETRY_REPORTING
> -<groupbox id="historyGroup" data-category="panePrivacy" data-subcategory="reports" hidden="true">
> +#ifdef MOZ_DATA_REPORTING
> +<groupbox id="telemetryGroup" data-category="panePrivacy" data-subcategory="reports" hidden="true">

Nit: Keep the empty line, it seems to match the `#endif` style.

::: browser/locales/en-US/chrome/browser/preferences/advanced.dtd:34
(Diff revision 2)
>  <!ENTITY dataChoicesTab.label            "Data Choices">
>  
> +<!-- LOCALIZATION NOTE (healthReportingDisabled.label): This message is displayed above
> +disabled data sharing options in developer builds or builds with no Telemetry support
> +available. -->
> +<!ENTITY healthReportingDisabled.label   "Data reporting is not available for this build configuration">

Maybe "Data reporting is disabled for ..."?

Comment 27

[Alessio Placitelli [:Dexter]]

Comment on attachment 8872555 [details]
Bug 1312182 - Consolidate the uses of MOZ_TELEMETRY_REPORTING in the code.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/144078/diff/2-3/

Comment 28

[Alessio Placitelli [:Dexter]]

Comment on attachment 8872555 [details]
Bug 1312182 - Consolidate the uses of MOZ_TELEMETRY_REPORTING in the code.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/144078/diff/3-4/

Comment 29

[Alessio Placitelli [:Dexter]]

Ouch, had to do some fun rebasing due to 1363850 which renames some directories :)

Comment 30

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8872555 [details]
Bug 1312182 - Consolidate the uses of MOZ_TELEMETRY_REPORTING in the code.

https://reviewboard.mozilla.org/r/144078/#review149734

Please update browser/components/preferences/in-content/\* as well.

::: browser/components/preferences/in-content-new/privacy.js:1462
(Diff revision 4)
>  
>      let checkbox = document.getElementById("submitHealthReportBox");
>  
> -    if (Services.prefs.prefIsLocked(PREF_UPLOAD_ENABLED)) {
> +    // Telemetry is only sending data if MOZ_TELEMETRY_REPORTING is defined.
> +    // We still want to display the preferences panel if that's not the case, but
> +    // we want it to be disabled and unckecked.

spelling error, "unchecked" instead of "unckecked"

Comment 31

[Alessio Placitelli [:Dexter]]

Comment on attachment 8872555 [details]
Bug 1312182 - Consolidate the uses of MOZ_TELEMETRY_REPORTING in the code.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/144078/diff/4-5/

Comment 32

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8872555 [details]
Bug 1312182 - Consolidate the uses of MOZ_TELEMETRY_REPORTING in the code.

https://reviewboard.mozilla.org/r/144078/#review153038

Thanks!

Comment 33

[Alessio Placitelli [:Dexter]]

Hi Madalin! 

Would you kindly do a QA pass on this before we land it? The builds ara available here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=45bba2ca0841ab5d45fbb1678cbba5c609eb4559&selectedJob=106941703

We want to make sure that:

- Developer builds (no MOZ_TELEMETRY_REPORTING defined when building) still show the Telemetry options in about:preferences (currently they don't), but that they look as in the attached screenshot;
- The state of the options in about:preferences reflect the state of the telemetry prefs, toolkit.telemetry.enabled and datareporting.healthreport.uploadEnabled;
- There is no regression on non-developer/official builds: the UI prefs are still shown and control the preferences.

Please let me know if you need more input on this.

Comment 34

[Alessio Placitelli [:Dexter]]

Mh, you know what? let's land this anyway, it shouldn't bite us on official builds.

Comment 35

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7f1e5af6138b
Consolidate the uses of MOZ_TELEMETRY_REPORTING in the code. r=flod,gfritzsche,jaws

Comment 36

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for failing e.g. browser-chrome's browser_storagePressure_notification.js because of entity parsing error for healthReportingDisabled.label:

https://hg.mozilla.org/integration/autoland/rev/ffe216fab0e7a8449b328b181d8523aaabd91dfe

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=7f1e5af6138bbcbd7c3ef73d37e50b91c52e18e1&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable

Failure log browser_storagePressure_notification.js: https://treeherder.mozilla.org/logviewer.html#?job_id=107058097&repo=autoland

[task 2017-06-14T15:26:23.727717Z] 15:26:23     INFO - TEST-PASS | browser/base/content/test/general/browser_storagePressure_notification.js | Should display storage pressure notification - 
[task 2017-06-14T15:26:23.729694Z] 15:26:23     INFO - Console message: [JavaScript Error: "Not a number"]
[task 2017-06-14T15:26:23.733059Z] 15:26:23     INFO - Console message: [JavaScript Error: "XML Parsing Error: undefined entity
[task 2017-06-14T15:26:23.736104Z] 15:26:23     INFO - Location: about:preferences#advanced
[task 2017-06-14T15:26:23.738308Z] 15:26:23     INFO - Line Number 1099, Column 20:" {file: "about:preferences#advanced" line: 1099 column: 20 source: "      <description>&healthReportingDisabled.label;</description>"}]
[task 2017-06-14T15:26:23.743106Z] 15:26:23     INFO - Buffered messages finished
[task 2017-06-14T15:26:23.748203Z] 15:26:23     INFO - TEST-UNEXPECTED-FAIL | browser/base/content/test/general/browser_storagePressure_notification.js | Test timed out - 

More browser-chrome tests ran and failed for https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=4180fb6074b11fa29119cdef6a90432be01f0036&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable

Comment 37

[Francesco Lodolo [:flod]]

> [task 2017-06-14T15:26:23.738308Z] 15:26:23     INFO - Line Number 1099,
> Column 20:" {file: "about:preferences#advanced" line: 1099 column: 20
> source: "      <description>&healthReportingDisabled.label;</description>"}]

I'm failing to spot anything that justifies this one.

Comment 38

[Jared Wein [:jaws] (please needinfo? me)]

The failure is because the string was only added to browser/locales/en-US/chrome/browser/preferences/advanced.dtd but should also be added to browser/locales/en-US/chrome/browser/preferences-old/advanced.dtd too. We forked the preference strings along with the fork of the preferences.

Comment 39

[Alessio Placitelli [:Dexter]]

Comment on attachment 8872555 [details]
Bug 1312182 - Consolidate the uses of MOZ_TELEMETRY_REPORTING in the code.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/144078/diff/5-6/

Comment 40

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c74bea253277
Consolidate the uses of MOZ_TELEMETRY_REPORTING in the code. r=flod,gfritzsche,jaws

Comment 41

[Alessio Placitelli [:Dexter]]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #38)
> The failure is because the string was only added to
> browser/locales/en-US/chrome/browser/preferences/advanced.dtd but should
> also be added to
> browser/locales/en-US/chrome/browser/preferences-old/advanced.dtd too. We
> forked the preference strings along with the fork of the preferences.

Thanks Jared (& flod too)!

Comment 42

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/c74bea253277

Comment 43

[Madalin Cotetiu]

I have created a set of tests to validate this. They can be found in here: https://docs.google.com/document/d/17vRzqXk2ZdlKL6wsMaekQGCakEfppIAAeSPE_ND58Tg/edit
Also I have done some exploratory checking around those scenario.
The testing was don in latest nightly and a developer build provided by Alessio.
Closing this bug as verified fixed.

Comment 44

[Alessio Placitelli [:Dexter]]

(In reply to Madalin Cotetiu from comment #43)
> I have created a set of tests to validate this. They can be found in here:
> https://docs.google.com/document/d/
> 17vRzqXk2ZdlKL6wsMaekQGCakEfppIAAeSPE_ND58Tg/edit
> Also I have done some exploratory checking around those scenario.
> The testing was don in latest nightly and a developer build provided by
> Alessio.
> Closing this bug as verified fixed.

Great job, thanks Madalin!