Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Updated Preferences re-org according to proposal from May 11th

RESOLVED FIXED in Firefox 56

Status

()

Firefox
Preferences
P1
normal
RESOLVED FIXED
2 months ago
2 days ago

People

(Reporter: timdream, Assigned: evanxd)

Tracking

(Depends on: 5 bugs, Blocks: 5 bugs)

Trunk
Firefox 56
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +
qe-verify +

Firefox Tracking Flags

(firefox56 fixed)

Details

(Whiteboard: [photon-preference])

MozReview Requests

()

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

Attachments

(3 attachments, 6 obsolete attachments)

There has been a new proposal to further improve the current Preferences re-design (refresh 2017; bug 1324168).

What's being proposed was new heading levels to improve clarity, and move about 8 sections across panels. UX feels strongly they want to ship an improved version instead of bug 1324168. It sounds doable for 55 if the spec can be finalized on time. The alternative would be bug 1363850 which moves the re-design to future versions.

This bug represents the engineering work involved. Any bug found for bug 1324168 that would need a reevaluation after implementing this design will be blocked by this bug.

Updated

2 months ago
Flags: qe-verify+
QA Contact: hani.yacoub

Updated

2 months ago
Depends on: 1352404

Updated

2 months ago
Depends on: 1352420
Target Milestone: --- → Firefox 55
> It sounds doable for 55 if the spec can be finalized on time.

Can someone clarify how much of this is moving existing strings around, and how much is brand new strings? 

That doesn't sound like a small chance, including the stabilization period on Nightly.
Confirmed. Pref re-org is now a Fx56 feature. Let's look at bug 1363850 for now.
Summary: Updated Preferences re-org according to proposal from May 11th → Updated Preferences re-org according to proposal from May 11st
Target Milestone: Firefox 55 → Firefox 56
Summary: Updated Preferences re-org according to proposal from May 11st → Updated Preferences re-org according to proposal from May 11th
(Assignee)

Updated

2 months ago
Assignee: nobody → evan
Status: NEW → ASSIGNED
Priority: -- → P1
No longer depends on: 1352404
Blocks: 1370190
No longer blocks: 1370190
Depends on: 1370190
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 6

2 months ago
mozreview-review
Comment on attachment 8875200 [details]
Bug 1365133 - Reorganize Preferences sections - Part 1.

https://reviewboard.mozilla.org/r/146604/#review150624

::: browser/locales/en-US/chrome/browser/preferences/advanced.dtd:44
(Diff revision 2)
>  <!ENTITY alwaysSubmitCrashReports.accesskey "c">
>  <!ENTITY crashReporterLearnMore.label     "Learn more">
>  
>  <!ENTITY networkTab.label                "Network">
>  
> -<!ENTITY connection.label                "Connection">
> +<!ENTITY connection2.label               "Network Proxy">

Since the new label has nothing to do with "Connection", please pick a new string ID instead of versioning the existing one.

There are a few cases in this patch where you should just use a brand new string ID.

::: browser/locales/en-US/chrome/browser/preferences/content.dtd:36
(Diff revision 2)
>  <!ENTITY  colors.label                "Colors…">
>  <!ENTITY  colors.accesskey            "C">
>  
>  
> -<!ENTITY languages.label              "Languages">
> -<!ENTITY chooseLanguage.label         "Choose your preferred language for displaying pages">
> +<!ENTITY language2.label              "Language">
> +<!ENTITY chooseLanguage2.label        "Choose how Firefox handles the files you download from the web or the applications you use while browsing.">

This string has nothing to do with languages.

::: browser/locales/en-US/chrome/browser/preferences/main.dtd:13
(Diff revision 2)
>  <!ENTITY startupPage2.accesskey    "s">
>  <!ENTITY startupUserHomePage.label "Show your home page">
>  <!ENTITY startupBlankPage.label    "Show a blank page">
>  <!ENTITY startupPrevSession.label  "Show your windows and tabs from last time">
>  
> -<!ENTITY homepage2.label           "Home Page">
> +<!ENTITY homepage3.label           "Home page">

No need to version the ID if you only change capitalization.
(Assignee)

Comment 7

2 months ago
Thank you, Francesco. I will update it.
(Assignee)

Comment 8

2 months ago
I will do this task with three steps.

1. Move <xul:groupbox> elements to correct categories and update strings.
2. Group the <xul:groupbox> elements with the new sub-categories.
3. Update tests to ensure the re-org work doesn't break anything.

And I will write three patches for the three steps.
Blocks: 1370801
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

a month ago
Attachment #8875200 - Flags: review?(jaws)
(Assignee)

Comment 19

a month ago
Hi Jared,

Could you help review the patch?

We will have three patches for this bug because we will change lots of code here. I think it will help review work.
1. Move <xul:groupbox> elements to correct categories and update strings.
2. Add the new sub-categories and group the <xul:groupbox> elements with them.
3. Update tests to ensure the re-org work doesn't break anything.

This is the reorg spec[1].

Thank you.

[1]: https://mozilla.invisionapp.com/share/P4ACQT1E3#/screens/217167559_1-0_Cover
(Reporter)

Comment 20

a month ago
mozreview-review
Comment on attachment 8875200 [details]
Bug 1365133 - Reorganize Preferences sections - Part 1.

https://reviewboard.mozilla.org/r/146604/#review153864

::: browser/base/content/aboutDialog-appUpdater.js
(Diff revision 13)
>          button.label = this.bundle.formatStringFromName("update.downloadAndInstallButton.label", [updateVersion], 1);
>          button.accessKey = this.bundle.GetStringFromName("update.downloadAndInstallButton.accesskey");
>        }
>        this.updateDeck.selectedPanel = panel;
> -      if (!document.commandDispatcher.focusedElement || // don't steal the focus
> -          document.commandDispatcher.focusedElement.localName == "button") // except from the other buttons

Removing this will break about dialog. We should address this in a seperate bug.

Comment 21

a month ago
mozreview-review
Comment on attachment 8875200 [details]
Bug 1365133 - Reorganize Preferences sections - Part 1.

https://reviewboard.mozilla.org/r/146604/#review153904

::: browser/locales/en-US/chrome/browser/preferences/content.dtd:36
(Diff revision 13)
>  <!ENTITY  colors.label                "Colors…">
>  <!ENTITY  colors.accesskey            "C">
>  
>  
> -<!ENTITY languages.label              "Languages">
> -<!ENTITY chooseLanguage.label         "Choose your preferred language for displaying pages">
> +<!ENTITY language2.label              "Language">
> +<!ENTITY chooseLanguage2.label        "Choose your preferred language for displaying pages">

No need to increment this ID, the string didn't change
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

a month ago
Blocks: 1373570
(Assignee)

Comment 25

a month ago
Created attachment 8878994 [details]
visual-refresh-for-reorg-v2.png

Discussed with Helen in person. At this (reorg v2) stage, we would like to make our sub-category title like the attachment image.

Hi Helen,

You could check the image to see the result.

Thank you.

The style
margin-top: 64px
font-size:  22px
Flags: needinfo?(hhuang)
Thanks, Evan. 
The current screen looks good to me. Agree with the style for staging implement. Soon We will provide specs for visual refreshment once the design is done.
Flags: needinfo?(hhuang)
Comment hidden (mozreview-request)
(Assignee)

Updated

a month ago
Attachment #8878385 - Attachment is obsolete: true
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

a month ago
Attachment #8879018 - Flags: review?(jaws)
(Assignee)

Comment 32

a month ago
Hi Jared,

I've added the subcategory titles.

Could you help review the patch?

Thank you.
(Assignee)

Comment 33

a month ago
mozreview-review
Comment on attachment 8879018 [details]
Bug 1365133 - ReGroup <xul:groupbox> elements by new categories - Part 2.

https://reviewboard.mozilla.org/r/150300/#review154966

::: browser/themes/shared/incontentprefs/preferences.inc.css:56
(Diff revision 4)
>  
>  .accessory-button {
>    min-width: 145px;
>  }
>  
> +/* Subcategory title */

Adding below styles because of the comment[1].

[1]: https://bugzilla.mozilla.org/show_bug.cgi?id=1365133#c25

Comment 34

a month ago
mozreview-review
Comment on attachment 8875200 [details]
Bug 1365133 - Reorganize Preferences sections - Part 1.

https://reviewboard.mozilla.org/r/146604/#review154080

The horizontal separator should be removed from each page.

A heading above the "Language" should be added that reads "Language and Appearance"

Fonts & Colors should appear above Language

::: browser/components/preferences/in-content-new/main.js:153
(Diff revision 13)
>      }
>  
> +    function appendSearchKeywords(aId, keywords) {
> +      let element = document.getElementById(aId);
> +      let searchKeywords = element.getAttribute("searchkeywords");
> +      searchKeywords && keywords.push(searchKeywords);

Use if() here

::: browser/components/preferences/in-content-new/main.js:439
(Diff revision 13)
>      // Notify observers that the UI is now ready
>      Components.classes["@mozilla.org/observer-service;1"]
>                .getService(Components.interfaces.nsIObserverService)
>                .notifyObservers(window, "main-pane-loaded");
> +
> +    // Append search keywords into the elements could open subdialogs.

This comment is a bit confusing and too generic since this block is only about browserContainersSettings. Please remove it, and then add the following comment to the appendSearchKeywords function definition:

// This function is used to append search keywords found in the related subdialog to the button that will activate the subdialog.
Attachment #8875200 - Flags: review?(jaws) → review-
(Assignee)

Comment 35

a month ago
mozreview-review-reply
Comment on attachment 8875200 [details]
Bug 1365133 - Reorganize Preferences sections - Part 1.

https://reviewboard.mozilla.org/r/146604/#review154080

> The horizontal separator should be removed from each page.
> A heading above the "Language" should be added that reads "Language and Appearance"

We fixed the above issues in the Part 2 patch[1].

> Fonts & Colors should appear above Language

Originally, "Fonts & Colors" already appears above "Language". Or I misunderstood somethings?

[1]: https://reviewboard.mozilla.org/r/150300/diff/4/

> Use if() here

Sure.

> This comment is a bit confusing and too generic since this block is only about browserContainersSettings. Please remove it, and then add the following comment to the appendSearchKeywords function definition:
> 
> // This function is used to append search keywords found in the related subdialog to the button that will activate the subdialog.

Sure.
Comment hidden (mozreview-request)
(Assignee)

Updated

a month ago
Attachment #8879018 - Attachment is obsolete: true
Attachment #8879018 - Flags: review?(jaws)
Comment hidden (mozreview-request)
(Assignee)

Updated

a month ago
Attachment #8875200 - Flags: review?(jaws)
Attachment #8879392 - Flags: review?(jaws)
(Assignee)

Comment 38

a month ago
Hi Jared,

I updated the patch[1] for review comments. Could you review it again? Thank you.

[1]: https://reviewboard.mozilla.org/r/146604/diff/17#index_header
(Assignee)

Updated

a month ago
Attachment #8879392 - Attachment is obsolete: true
Attachment #8879392 - Flags: review?(jaws)
Comment hidden (mozreview-request)
(Assignee)

Updated

a month ago
Attachment #8879397 - Flags: review?(jaws)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 44

a month ago
mozreview-review
Comment on attachment 8875200 [details]
Bug 1365133 - Reorganize Preferences sections - Part 1.

https://reviewboard.mozilla.org/r/146604/#review155718

::: browser/components/preferences/in-content-new/main.js:150
(Diff revisions 16 - 18)
> +    // This function is used to append search keywords found
> +    // in the related subdialog to the button that will activate the subdialog.
>      function appendSearchKeywords(aId, keywords) {

Since this function is duplicated between main.js and privacy.js, please move it to preferences.js so it can be accessed from both places without duplicating it.

::: browser/base/content/aboutDialog-appUpdater.js:188
(Diff revision 18)
>            let day = buildID.slice(6, 8);
>            updateVersion += ` (${year}-${month}-${day})`;
>          }
>          button.label = this.bundle.formatStringFromName("update.downloadAndInstallButton.label", [updateVersion], 1);
>          button.accessKey = this.bundle.GetStringFromName("update.downloadAndInstallButton.accesskey");
> +      } 

nit, trailing whitespace. this should cause an eslint error.

::: browser/components/preferences/applicationManager.js:15
(Diff revision 18)
>  var gAppManagerDialog = {
>    _removed: [],
>  
>    init: function appManager_init() {
>      this.handlerInfo = window.arguments[0];
> -
> +    // The aplicationManager will be used

spelling error, "applicationManager" instead of "aplicationManager"
Attachment #8875200 - Flags: review?(jaws) → review+

Comment 45

a month ago
mozreview-review
Comment on attachment 8879397 [details]
Bug 1365133 - Reorganize Preferences sections and regroup <xul:groupbox> elements by new categories - Part 1.

https://reviewboard.mozilla.org/r/150718/#review155862

r=me with the following changes.

::: browser/components/preferences/in-content-new/privacy.xul:172
(Diff revision 2)
>  
>  <stringbundle id="bundlePreferences" src="chrome://browser/locale/preferences/preferences.properties"/>
>  <stringbundle id="signonBundle" src="chrome://passwordmgr/locale/passwordmgr.properties"/>
>  
> -<hbox id="header-privacy"
> -      class="header"
> +<hbox id="browserPrivacyCategory"
> +      calss="subcategory"

spelling error, "class" instead of "calss"

::: browser/themes/shared/incontentprefs/preferences.inc.css:59
(Diff revision 2)
>  }
>  
> +/* Subcategory title */
> +
> +/**
> + * The first subcategory title for each category do not have margin-top.

s/do not have/should not have/

::: browser/themes/shared/incontentprefs/preferences.inc.css:61
(Diff revision 2)
> +/* Subcategory title */
> +
> +/**
> + * The first subcategory title for each category do not have margin-top.
> + */
> +#generalCategory, #searchCategory, #browserPrivacyCategory, #firefoxAccountCategory{

nit, please place each ID on its own line, and put a space before the curly bracket

::: browser/themes/shared/incontentprefs/preferences.inc.css:66
(Diff revision 2)
> +#generalCategory, #searchCategory, #browserPrivacyCategory, #firefoxAccountCategory{
> +  margin-top: 0;
> +}
> +
> +.header-name {
> +  font-size: 22px;

This should be defined in units of 'rem'

::: browser/themes/shared/incontentprefs/preferences.inc.css:70
(Diff revision 2)
> +.header-name {
> +  font-size: 22px;
> +}
> +
> +.subcategory {
> +  margin-top: 64px;

This looks too large. Please reduce this to 48px.
Attachment #8879397 - Flags: review?(jaws) → review+
(Assignee)

Comment 46

a month ago
mozreview-review-reply
Comment on attachment 8879397 [details]
Bug 1365133 - Reorganize Preferences sections and regroup <xul:groupbox> elements by new categories - Part 1.

https://reviewboard.mozilla.org/r/150718/#review155862

> This looks too large. Please reduce this to 48px.

Sure, let's do it. We might update this margin-top when we do visual refresh.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

29 days ago
Attachment #8879454 - Flags: review?(jaws)
(Assignee)

Comment 72

29 days ago
Hi Jared,

Could you help review the Part 3 patch for updating tests?

Thank you.

Comment 73

29 days ago
mozreview-review
Comment on attachment 8879454 [details]
Bug 1365133 - Update tests - Part 2.

https://reviewboard.mozilla.org/r/150760/#review157316
Attachment #8879454 - Flags: review?(jaws) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 77

25 days ago
Thank you for reviewing, Jared.

Did rebase and fixed conflicts. Let's land the patches once the try[1] is good.

[1]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6acb95e03934
(Assignee)

Comment 78

25 days ago
The try looks good. Let's land it.
Keywords: checkin-needed

Comment 79

25 days ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/74b19063d4e2
Reorganize Preferences sections - Part 1. r=jaws
https://hg.mozilla.org/integration/autoland/rev/137ad88e786c
ReGroup <xul:groupbox> elements by new categories - Part 2. r=jaws
https://hg.mozilla.org/integration/autoland/rev/3c6fba8e4015
Update tests - Part 3. r=jaws
Keywords: checkin-needed
Backed out for failing browser/components/preferences/in-content-new/tests/browser_security.js and leaking in browser_notification_open_settings.js on Linux debug:

https://hg.mozilla.org/integration/autoland/rev/6190181ff4093756d3f8df754109ed16d132d215
https://hg.mozilla.org/integration/autoland/rev/babc3c92735e963dd893320c1c9d8a2955c92d89
https://hg.mozilla.org/integration/autoland/rev/7e16fd9659df7b65917092f02ecf76a3b4d2974f

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

Failure log browser_security.js: https://treeherder.mozilla.org/logviewer.html#?job_id=110261061&repo=autoland
[task 2017-06-28T03:34:11.577183Z] 03:34:11     INFO - TEST-PASS | browser/components/preferences/in-content-new/tests/browser_security.js | malware table has been sorted - ["goog-malware-proto","goog-unwanted-proto","goog-unwanted-shavar","test-malware-simple","test-unwanted-simple"] deepEqual ["goog-malware-proto","goog-unwanted-proto","goog-unwanted-shavar","test-malware-simple","test-unwanted-simple"] - 
[task 2017-06-28T03:34:11.579039Z] 03:34:11     INFO - Leaving test bound 
[task 2017-06-28T03:34:11.581879Z] 03:34:11     INFO - Buffered messages finished
[task 2017-06-28T03:34:11.585825Z] 03:34:11     INFO - TEST-UNEXPECTED-FAIL | browser/components/preferences/in-content-new/tests/browser_security.js | This test exceeded the timeout threshold. It should be rewritten or split up. If that's not possible, use requestLongerTimeout(N), but only as a last resort. - 

Failure log browser/base/content/test/alerts/browser_notification_open_settings.js | leaked 2 window(s) until shutdown [url = about:preferences#privacy] : https://treeherder.mozilla.org/logviewer.html#?job_id=110263379&repo=autoland
Flags: needinfo?(evan)
This also intermittently failed this: https://treeherder.mozilla.org/logviewer.html#?job_id=110267583&repo=autoland
> TEST-UNEXPECTED-FAIL | browser/components/preferences/in-content-new/tests/browser_change_app_handler.js | Uncaught exception - Condition timed out: () => win.document.getAnonymousElementByAttribute(ourItem, "class", "actionsMenu")
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 86

24 days ago
Thank you for pointing out the failures, Sebastian.

I've tried to fix it. Let's wait and see the result[1].

[1]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=36f7c8dedc4c
Flags: needinfo?(evan)

Comment 87

24 days ago
hg error in cmd: hg push -r . -f try: pushing to ssh://hg.mozilla.org/try
searching for changes
remote: adding changesets
remote: adding manifests
remote: adding file changes
remote: added 1 changesets with 0 changes to 0 files (+1 heads)
remote: autoland push detected
remote: recorded push in pushlog
remote: 
remote: View your change here:
remote:   https://hg.mozilla.org/try/rev/ec0d90c7f551ef150add2ad4824a3a3cc132963b
remote: 
remote: Follow the progress of your build on Treeherder:
remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=ec0d90c7f551ef150add2ad4824a3a3cc132963b
remote: recorded changegroup in replication log in 0.122s
remote: ** unknown exception encountered, please report by visiting
remote: ** https://mercurial-scm.org/wiki/BugTracker
remote: ** Python 2.7.5 (default, Nov  6 2016, 00:28:07) [GCC 4.8.5 20150623 (Red Hat 4.8.5-11)]
remote: ** Mercurial Distributed SCM (version 4.1.2)
remote: ** Extensions loaded: blackbox, clonebundles, obsolescencehacks, pushlog, serverlog, readonly, vcsreplicator
remote: Traceback (most recent call last):
remote:   File "/var/hg/venv_pash/bin/hg", line 45, in <module>
remote:     mercurial.dispatch.run()
remote:   File "/var/hg/venv_pash/lib/python2.7/site-packages/mercurial/dispatch.py", line 63, in run
remote:     sys.exit((dispatch(request(pycompat.sysargv[1:])) or 0) & 255)
remote:   File "/var/hg/venv_pash/lib/python2.7/site-packages/mercurial/dispatch.py", line 129, in dispatch
remote:     ret = _runcatch(req)
remote:   File "/var/hg/venv_pash/lib/python2.7/site-packages/mercurial/dispatch.py", line 219, in _runcatch
remote:     return callcatch(ui, _runcatchfunc)
remote:   File "/var/hg/venv_pash/lib/python2.7/site-packages/mercurial/dispatch.py", line 227, in callcatch
remote:     return scmutil.callcatch(ui, func)
remote:   File "/var/hg/venv_pash/lib/python2.7/site-packages/mercurial/scmutil.py", line 152, in callcatch
remote:     return func()
remote:   File "/var/hg/venv_pash/lib/python2.7/site-packages/mercurial/dispatch.py", line 208, in _runcatchfunc
remote:     return _dispatch(req)
remote:   File "/var/hg/venv_pash/lib/python2.7/site-packages/mercurial/dispatch.py", line 811, in _dispatch
remote:     cmdpats, cmdoptions)
remote:   File "/var/hg/venv_pash/lib/python2.7/site-packages/mercurial/dispatch.py", line 563, in runcommand
remote:     ret = _runcommand(ui, options, cmd, d)
remote:   File "/var/hg/venv_pash/lib/python2.7/site-packages/mercurial/dispatch.py", line 819, in _runcommand
remote:     return cmdfunc()
remote:   File "/var/hg/venv_pash/lib/python2.7/site-packages/mercurial/dispatch.py", line 808, in <lambda>
remote:     d = lambda: util.checksignature(func)(ui, *args, **strcmdopt)
remote:   File "/var/hg/venv_pash/lib/python2.7/site-packages/mercurial/util.py", line 1051, in check
remote:     return func(*args, **kwargs)
remote:   File "/var/hg/venv_pash/lib/python2.7/site-packages/mercurial/commands.py", line 5824, in serve
remote:     s.serve_forever()
remote:   File "/var/hg/version-control-tools/hgext/serverlog/__init__.py", line 320, in serve_forever
remote:     return super(sshserverwrapped, self).serve_forever()
remote:   File "/var/hg/venv_pash/lib/python2.7/site-packages/mercurial/sshserver.py", line 104, in serve_forever
remote:     while self.serve_one():
remote:   File "/var/hg/version-control-tools/hgext/serverlog/__init__.py", line 351, in serve_one
remote:     return super(sshserverwrapped, self).serve_one()
remote:   File "/var/hg/venv_pash/lib/python2.7/site-packages/mercurial/sshserver.py", line 122, in serve_one
remote:     rsp = wireproto.dispatch(self.repo, self, cmd)
remote:   File "/var/hg/version-control-tools/hgext/serverlog/__init__.py", line 343, in dispatch
remote:     return origdispatch(repo, proto, cmd)
remote:   File "/var/hg/venv_pash/lib/python2.7/site-packages/mercurial/extensions.py", line 223, in closure
remote:     return func(*(args + a), **kw)
remote:   File "/var/hg/version-control-tools/pylib/vcsreplicator/vcsreplicator/hgext.py", line 359, in wireprotodispatch
remote:     return orig(repo, proto, command)
remote:   File "/var/hg/venv_pash/lib/python2.7/site-packages/mercurial/wireproto.py", line 569, in dispatch
remote:     return func(repo, proto, *args)
remote:   File "/var/hg/venv_pash/lib/python2.7/site-packages/mercurial/wireproto.py", line 982, in unbundle
remote:     proto._client())
remote:   File "/var/hg/venv_pash/lib/python2.7/site-packages/mercurial/exchange.py", line 1771, in unbundle
remote:     lockandtr[2].close()
remote:   File "/var/hg/venv_pash/lib/python2.7/site-packages/mercurial/transaction.py", line 43, in _active
remote:     return func(self, *args, **kwds)
remote:   File "/var/hg/venv_pash/lib/python2.7/site-packages/mercurial/transaction.py", line 490, in close
remote:     self._postclosecallback[cat](self)
remote:   File "/var/hg/version-control-tools/hgext/pushlog/__init__.py", line 239, in onpostclose
remote:     conn.commit()
remote: sqlite3.OperationalError: database is locked
abort: stream ended unexpectedly (got 0 bytes, expected 4)
(Assignee)

Comment 88

24 days ago
Push the try[1] and debug the `LOG ERROR | TEST-UNEXPECTED-FAIL | browser/base/content/test/alerts/browser_notification_open_settings.js | leaked 4 window(s) until shutdown [url = about:preferences#privacy]` failure.

[1]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=99dfba8d29ad9ab4939588f0feb8a20128cf060f
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

23 days ago
Attachment #8875200 - Attachment is obsolete: true
Comment hidden (mozreview-request)
(Assignee)

Updated

23 days ago
Attachment #8882093 - Attachment is obsolete: true
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

23 days ago
Keywords: checkin-needed
(Assignee)

Comment 101

23 days ago
Looks like the try[1] is OK. Let's land the code.

[1]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e40340fb567c

Comment 102

23 days ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/ec2c4e0621cc
Reorganize Preferences sections and regroup <xul:groupbox> elements by new categories - Part 1. r=jaws
https://hg.mozilla.org/integration/autoland/rev/da290b2edb66
Update tests - Part 2. r=jaws
Keywords: checkin-needed
backed out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=110704405&repo=autoland
Flags: needinfo?(evan)

Comment 104

23 days ago
Backout by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1f32cd73e582
Backed out changeset da290b2edb66 
https://hg.mozilla.org/integration/autoland/rev/baed83e0dace
Backed out changeset ec2c4e0621cc for test failures in uncaught exception - TypeError: this.tree is null at rowCountChanged@chrome://browser/content/preferences/in-content-new/search.js:480:5
Comment hidden (mozreview-request)
(Assignee)

Comment 106

23 days ago
Interesting, in previous failure report didn't show the `TypeError: this.tree is null at rowCountChanged@chrome://browser/content/preferences/in-content-new/search.js:480:5` failure.

Tried to fix it at new try[1].

[1]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=902dba9afc43
Flags: needinfo?(evan)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 109

23 days ago
Pushed a new try https://treeherder.mozilla.org/#/jobs?repo=try&revision=008c50394c7a
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 112

23 days ago
 Comment 101 • 10 hours ago

Looks like the try[1] is OK. Let's land the code "again".

[1]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=008c50394c7a&selectedJob=110753605
Keywords: checkin-needed

Comment 113

23 days ago
Pushed by rchien@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/38cdadf97cbc
Reorganize Preferences sections and regroup <xul:groupbox> elements by new categories - Part 1. r=jaws
https://hg.mozilla.org/integration/autoland/rev/4938ab2adfe9
Update tests - Part 2. r=jaws
Keywords: checkin-needed
(Assignee)

Updated

23 days ago
Blocks: 1377315
Backed out for still failing intermittently browser_change_app_handler.js and browser_notification_open_settings.js:

https://hg.mozilla.org/integration/autoland/rev/eed23a465d8d17939d41250690e371504bd58609
https://hg.mozilla.org/integration/autoland/rev/284a915629be647eb6ceba149fdb4b9b3a0019c4

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

Failure log browser_change_app_handler.js: https://treeherder.mozilla.org/logviewer.html#?job_id=110915885&repo=autoland
15:54:36     INFO - TEST-START | browser/components/preferences/in-content-new/tests/browser_bug795764_cachedisabled.js
15:54:37     INFO - GECKO(5864) | MEMORY STAT | vsize 753MB | vsizeMaxContiguous 768MB | residentFast 285MB | heapAllocated 120MB
15:54:37     INFO - TEST-OK | browser/components/preferences/in-content-new/tests/browser_bug795764_cachedisabled.js | took 1187ms
15:54:37     INFO - checking window state
15:54:37     INFO - TEST-START | browser/components/preferences/in-content-new/tests/browser_change_app_handler.js
15:54:37     INFO - Entering test bound 
15:54:37     INFO - TEST-PASS | browser/components/preferences/in-content-new/tests/browser_change_app_handler.js | Should have at least one known handler - 
15:54:39     INFO - Preferences page opened on the applications pane.
15:54:39     INFO - TEST-PASS | browser/components/preferences/in-content-new/tests/browser_change_app_handler.js | handlersView is present - 
15:54:39     INFO - TEST-PASS | browser/components/preferences/in-content-new/tests/browser_change_app_handler.js | Should be able to select our item. - 
15:54:39     INFO - Got list after item was selected
15:54:39     INFO - TEST-PASS | browser/components/preferences/in-content-new/tests/browser_change_app_handler.js | Check the proper URL is loaded - 
15:54:39     INFO - TEST-PASS | browser/components/preferences/in-content-new/tests/browser_change_app_handler.js | Element should not be null, when checking visibility - 
15:54:39     INFO - TEST-PASS | browser/components/preferences/in-content-new/tests/browser_change_app_handler.js | Overlay is visible - 
15:54:39     INFO - found chrome://browser/skin/preferences/preferences.css
15:54:39     INFO - found chrome://global/skin/in-content/common.css
15:54:39     INFO - found chrome://browser/skin/preferences/in-content-new/preferences.css
15:54:39     INFO - found chrome://browser/skin/preferences/in-content-new/dialog.css
15:54:39     INFO - TEST-PASS | browser/components/preferences/in-content-new/tests/browser_change_app_handler.js | All expectedStyleSheetURLs should have been found - 
15:54:39     INFO - Dialog loaded
15:54:39     INFO - TEST-PASS | browser/components/preferences/in-content-new/tests/browser_change_app_handler.js | App should be set as preferred. - 
15:54:44     INFO - TEST-INFO | started process screenshot
15:54:44     INFO - TEST-INFO | screenshot: exit 0
15:54:44     INFO - TEST-UNEXPECTED-FAIL | browser/components/preferences/in-content-new/tests/browser_change_app_handler.js | Uncaught exception - Condition timed out: () => win.document.getAnonymousElementByAttribute(ourItem, "class", "actionsMenu")

Failure log browser_notification_open_settings.js: https://treeherder.mozilla.org/logviewer.html#?job_id=110912843&repo=autoland
> TEST-UNEXPECTED-FAIL | browser/base/content/test/alerts/browser_notification_open_settings.js | leaked 2 window(s) until shutdown [url = about:preferences#privacy]
Flags: needinfo?(evan)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 138

18 days ago
hg error in cmd: hg push -r . -f try: pushing to ssh://hg.mozilla.org/try
searching for changes
remote: adding changesets
remote: adding manifests
remote: adding file changes
remote: added 2 changesets with 1 changes to 21 files (+1 heads)
remote: autoland push detected
remote: recorded push in pushlog
remote: 
remote: View your changes here:
remote:   https://hg.mozilla.org/try/rev/70a529c2f574f7e23043c7a0d60d2f3a5809884b
remote:   https://hg.mozilla.org/try/rev/55271d7589ade80a0203a349a95e0eec7a13e940
remote: 
remote: Follow the progress of your build on Treeherder:
remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=55271d7589ade80a0203a349a95e0eec7a13e940
remote: recorded changegroup in replication log in 0.074s
remote: ** unknown exception encountered, please report by visiting
remote: ** https://mercurial-scm.org/wiki/BugTracker
remote: ** Python 2.7.5 (default, Nov  6 2016, 00:28:07) [GCC 4.8.5 20150623 (Red Hat 4.8.5-11)]
remote: ** Mercurial Distributed SCM (version 4.1.2)
remote: ** Extensions loaded: blackbox, clonebundles, obsolescencehacks, pushlog, serverlog, readonly, vcsreplicator
remote: Traceback (most recent call last):
remote:   File "/var/hg/venv_pash/bin/hg", line 45, in <module>
remote:     mercurial.dispatch.run()
remote:   File "/var/hg/venv_pash/lib/python2.7/site-packages/mercurial/dispatch.py", line 63, in run
remote:     sys.exit((dispatch(request(pycompat.sysargv[1:])) or 0) & 255)
remote:   File "/var/hg/venv_pash/lib/python2.7/site-packages/mercurial/dispatch.py", line 129, in dispatch
remote:     ret = _runcatch(req)
remote:   File "/var/hg/venv_pash/lib/python2.7/site-packages/mercurial/dispatch.py", line 219, in _runcatch
remote:     return callcatch(ui, _runcatchfunc)
remote:   File "/var/hg/venv_pash/lib/python2.7/site-packages/mercurial/dispatch.py", line 227, in callcatch
remote:     return scmutil.callcatch(ui, func)
remote:   File "/var/hg/venv_pash/lib/python2.7/site-packages/mercurial/scmutil.py", line 152, in callcatch
remote:     return func()
remote:   File "/var/hg/venv_pash/lib/python2.7/site-packages/mercurial/dispatch.py", line 208, in _runcatchfunc
remote:     return _dispatch(req)
remote:   File "/var/hg/venv_pash/lib/python2.7/site-packages/mercurial/dispatch.py", line 811, in _dispatch
remote:     cmdpats, cmdoptions)
remote:   File "/var/hg/venv_pash/lib/python2.7/site-packages/mercurial/dispatch.py", line 563, in runcommand
remote:     ret = _runcommand(ui, options, cmd, d)
remote:   File "/var/hg/venv_pash/lib/python2.7/site-packages/mercurial/dispatch.py", line 819, in _runcommand
remote:     return cmdfunc()
remote:   File "/var/hg/venv_pash/lib/python2.7/site-packages/mercurial/dispatch.py", line 808, in <lambda>
remote:     d = lambda: util.checksignature(func)(ui, *args, **strcmdopt)
remote:   File "/var/hg/venv_pash/lib/python2.7/site-packages/mercurial/util.py", line 1051, in check
remote:     return func(*args, **kwargs)
remote:   File "/var/hg/venv_pash/lib/python2.7/site-packages/mercurial/commands.py", line 5824, in serve
remote:     s.serve_forever()
remote:   File "/var/hg/version-control-tools/hgext/serverlog/__init__.py", line 320, in serve_forever
remote:     return super(sshserverwrapped, self).serve_forever()
remote:   File "/var/hg/venv_pash/lib/python2.7/site-packages/mercurial/sshserver.py", line 104, in serve_forever
remote:     while self.serve_one():
remote:   File "/var/hg/version-control-tools/hgext/serverlog/__init__.py", line 351, in serve_one
remote:     return super(sshserverwrapped, self).serve_one()
remote:   File "/var/hg/venv_pash/lib/python2.7/site-packages/mercurial/sshserver.py", line 122, in serve_one
remote:     rsp = wireproto.dispatch(self.repo, self, cmd)
remote:   File "/var/hg/version-control-tools/hgext/serverlog/__init__.py", line 343, in dispatch
remote:     return origdispatch(repo, proto, cmd)
remote:   File "/var/hg/venv_pash/lib/python2.7/site-packages/mercurial/extensions.py", line 223, in closure
remote:     return func(*(args + a), **kw)
remote:   File "/var/hg/version-control-tools/pylib/vcsreplicator/vcsreplicator/hgext.py", line 359, in wireprotodispatch
remote:     return orig(repo, proto, command)
remote:   File "/var/hg/venv_pash/lib/python2.7/site-packages/mercurial/wireproto.py", line 569, in dispatch
remote:     return func(repo, proto, *args)
remote:   File "/var/hg/venv_pash/lib/python2.7/site-packages/mercurial/wireproto.py", line 982, in unbundle
remote:     proto._client())
remote:   File "/var/hg/venv_pash/lib/python2.7/site-packages/mercurial/exchange.py", line 1771, in unbundle
remote:     lockandtr[2].close()
remote:   File "/var/hg/venv_pash/lib/python2.7/site-packages/mercurial/transaction.py", line 43, in _active
remote:     return func(self, *args, **kwds)
remote:   File "/var/hg/venv_pash/lib/python2.7/site-packages/mercurial/transaction.py", line 490, in close
remote:     self._postclosecallback[cat](self)
remote:   File "/var/hg/version-control-tools/hgext/pushlog/__init__.py", line 239, in onpostclose
remote:     conn.commit()
remote: sqlite3.OperationalError: database is locked
abort: stream ended unexpectedly (got 0 bytes, expected 4)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Blocks: 1324172
No longer blocks: 1324172
No longer depends on: 1352420
Comment hidden (mozreview-request)
Blocks: 1363960
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Blocks: 1377192
(Assignee)

Comment 155

16 days ago
After investigating and updating the patch[1], we still have two test failures[3] in Linux x64 debug and Windows 7 VM debug builds. They are all about "leaked window(s)" issue.

1. browser/base/content/test/alerts/browser_notification_open_settings.js
> 08:33:54    ERROR -  520 ERROR TEST-UNEXPECTED-FAIL | browser/components/preferences/in-content-new/tests/browser_change_app_handler.js | leaked 2 window(s) until shutdown [url = about:preferences#general]
> 08:33:54    ERROR -  521 ERROR TEST-UNEXPECTED-FAIL | browser/components/preferences/in-content-new/tests/browser_change_app_handler.js | leaked 1 window(s) until shutdown [url = about:blank]
> 08:33:54    ERROR -  522 ERROR TEST-UNEXPECTED-FAIL | browser/components/preferences/in-content-new/tests/browser_change_app_handler.js | leaked 1 window(s) until shutdown [url = chrome://browser/content/preferences/applicationManager.xul]

For `browser_notification_open_settings.js` test, the "leaked window(s)" issue might already exist in current mozilla-central code base. The below STR is how to reproduce the "leaked window(s)" issue.
1. Build Linux x64 debug build with current mozilla-central (without my patch) on local.
2. Run the browser_notification_open_settings.js test with jsdebugger. (./mach mochitest browser_notification_open_settings.js --jsdebugger)
3. Your console(terminal) will show the "leaked window(s)" error message.

But it can be passed (on local) if run it without jsdebugger. It also can be passed (on local) with patching my patch if run it without jsdebugger.

I think the "leaked window(s)" error might be caused by really slow run-time environment. That might be why the "browser_notification_open_settings.js" test can be passed in all other builds except Linux x64 debug build[2].

Maybe we should file a new bug to address the existed "leaked window(s)" issue.

2. browser/components/preferences/in-content-new/tests/browser_change_app_handler.js
> 08:32:01    ERROR -  1002 ERROR TEST-UNEXPECTED-FAIL | browser/components/preferences/in-content-new/tests/browser_change_app_handler.js | leaked 2 window(s) until shutdown [url = about:preferences#general]
> 08:32:01    ERROR -  1003 ERROR TEST-UNEXPECTED-FAIL | browser/components/preferences/in-content-new/tests/browser_change_app_handler.js | leaked 1 window(s) until shutdown [url = chrome://browser/content/preferences/applicationManager.xul]
> 08:32:01    ERROR -  1004 ERROR TEST-UNEXPECTED-FAIL | browser/components/preferences/in-content-new/tests/browser_change_app_handler.js | leaked 1 window(s) until shutdown [url = about:blank]

Not investigated yet. Will continue to investigate to figure out the possible root cause.

[1]: https://reviewboard.mozilla.org/r/146602/
[2]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=61ef933101a3&selectedJob=112190626
[3]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=61ef933101a3
Flags: needinfo?(evan)
(Assignee)

Comment 156

16 days ago
Hi Jared,

What do you think of Comment 155?

I would say we could consider to disable the "browser_notification_open_settings.js" for Linux x64 debug build at this stage because
1. The "leaked window(s)" issue for "browser_notification_open_settings.js" test might already exist in current mozilla-central code base. (See how to reproduce in Comment 155)
2. The failure only occurs in Linux x64 debug build, other builds are good.

What do you think? If you think it is OK to do, I'll file a bug and send a patch to disable it.
Or do you have any idea to fix the "leaked window(s)" failure?

Thank you.
Flags: needinfo?(jaws)
(Assignee)

Comment 157

16 days ago
Add two more reasons, we could consider to disable the test with "leaked window(s)" error.

3. There are other bugs (like Bug 1363960, Bug 1373570, Bug 1377192, Bug 1377315) depend on this bug at this stage.
4. Actually all assertions in the test are passed but it will show the "leaked window(s)" message after it shutdown the window.
(Assignee)

Comment 158

16 days ago
For "browser_change_app_handler.js" test failure, there is a weird situation.

The test can be passed[1] if run it along with the command `try: -b do -p linux64,macosx64,win32,win64 -t none browser/base/content/test/alerts/ browser/components/preferences/in-content-new/tests/` but it will be failed[2] if run it with all other test files (try: -b do -p linux64,macosx64,win32,win64 -u mochitests -t none).

Maybe the "leaked window(s)" failure is caused by other test file?

Additionally, same situation as "browser_notification_open_settings.js" test, "browser_change_app_handler.js" test can be passed[1] but it will cause the "leaked window(s)" issue after it is finished and shutdown the testing window

[1]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0362a0fe9981c57de62ad6590fb01e4d074e816b&selectedJob=112179428
[2]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=61ef933101a3&selectedJob=112193620
(Assignee)

Comment 159

16 days ago
And I saw a special script to create a loop to wait for the app list render or something which is
> let list = await waitForCondition(() => win.document.getAnonymousElementByAttribute(ourItem, "class", "actionsMenu")); [1]

Jared, do you know why we do this there?

[1]: http://searchfox.org/mozilla-central/source/browser/components/preferences/in-content-new/tests/browser_change_app_handler.js#30
I am not sure, but bug 1084626 might be the reason why "--js-debugger" causes leaks.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 163

15 days ago
(In reply to Ting-Yu Chou [:ting] from comment #160)
> I am not sure, but bug 1084626 might be the reason why "--js-debugger"
> causes leaks.

Thank you, Ting. Let's try to debug this with treeherder gc/cc log.
(Assignee)

Comment 164

15 days ago
The gc/cc log[1] for the leaked window(s) issue[2] of browser_notification_open_settings.js test.

[1]: https://drive.google.com/drive/folders/0B3MxK4_nj3bpYUJrc3hxREpoWnM
[2]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=30b8a7466e2bb1342051905ae11f3a9026de60a3&selectedJob=112454023
In comment 164, all the reported leaked windows:

  - TEST-UNEXPECTED-FAIL | browser/base/content/test/alerts/browser_notification_open_settings.js | leaked 4 window(s) until shutdown [url = about:preferences#privacy]
  - TEST-INFO | browser/base/content/test/alerts/browser_notification_open_settings.js | windows(s) leaked: [pid = 1089] [serial = 44], [pid = 1089] [serial = 36], [pid = 1089] [serial = 46], [pid = 1089] [serial = 38]

  --DOMWINDOW == 9 (0x7f3a93a8a800) [pid = 1089] [serial = 44] [outer = (nil)] [url = about:preferences#privacy]
  --DOMWINDOW == 8 (0x7f3a93b4e800) [pid = 1089] [serial = 36] [outer = (nil)] [url = about:preferences#privacy]
  --DOMWINDOW == 7 (0x7f3a9c389000) [pid = 1089] [serial = 46] [outer = (nil)] [url = about:preferences#privacy]
  --DOMWINDOW == 6 (0x7f3a9405c800) [pid = 1089] [serial = 38] [outer = (nil)] [url = about:preferences#privacy]

are listed as garbage in the CC log and the leakcheck was passed:

  == BloatView: ALL (cumulative) LEAK AND BLOAT STATISTICS, default process 1089

       |<----------------Class--------------->|<-----Bytes------>|<----Objects---->|
       |                                      | Per-Inst   Leaked|   Total      Rem|
     0 |TOTAL                                 |       25        0| 2970743        0|

  nsTraceRefcnt::DumpStatistics: 1387 entries

  TEST-PASS | leakcheck | default process: no leaks detected!

I checked the code in ShutdownLeaks [1], "--DOMWINDOW" logs seems are reported as leaked if they are appeared after message "Completed ShutdownLeaks collections in process". But the message is output from browser-test.js [2] without running GC/CC at all.

:jgraham , is [1] still valid to determine whether the DOMWINDOW is leaked after bug 1139021?

[1] http://searchfox.org/mozilla-central/rev/f1472e5b57fea8d4a7bbdd81c44e46958fe3c1ce/testing/mochitest/leaks.py#96-97
[2] http://searchfox.org/mozilla-central/rev/f1472e5b57fea8d4a7bbdd81c44e46958fe3c1ce/testing/mochitest/browser-test.js#326
Flags: needinfo?(james)
:mccr8, what do you think about comment 165?
Flags: needinfo?(continuation)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(In reply to Evan Tseng [:evanxd] from comment #156)
> Hi Jared,
> 
> What do you think of Comment 155?
> 
> I would say we could consider to disable the
> "browser_notification_open_settings.js" for Linux x64 debug build at this
> stage because
> 1. The "leaked window(s)" issue for "browser_notification_open_settings.js"
> test might already exist in current mozilla-central code base. (See how to
> reproduce in Comment 155)

I think you have found now that this doesn't reproduce in mozilla-central code without your patch?

> 2. The failure only occurs in Linux x64 debug build, other builds are good.

We don't do leakchecking on optimized builds, and as I understand it, only some platforms do leakchecking on debug builds. Therefor it doesn't surprise me that the leak shows up on Linux x64 debug but not on other builds, this is somewhat expected due to our configurations.

> What do you think? If you think it is OK to do, I'll file a bug and send a
> patch to disable it.
> Or do you have any idea to fix the "leaked window(s)" failure?

I think this is probably pointing out a legitimate issue either in the tests or in the preferences code.

> 3. There are other bugs (like Bug 1363960, Bug 1373570, Bug 1377192, Bug
> 1377315) depend on this bug at this stage.

It is unfortunate, but I don't think having a list of bugs waiting should cause us to move forward without figuring this out.

> 4. Actually all assertions in the test are passed but it will show the
> "leaked window(s)" message after it shutdown the window.

I'm curious to hear what jgraham and mccr8 say about this. It is possible that these --DOMWINDOW show up after the leak has been announced because these pages are expected to be cleaned up before the leakcheck. In other words, we will clean up these windows during shutdown in all cases, but we shouldn't have to wait until shutdown to clean up windows.
Flags: needinfo?(jaws)
(In reply to Evan Tseng [:evanxd] from comment #159)
> And I saw a special script to create a loop to wait for the app list render
> or something which is
> > let list = await waitForCondition(() => win.document.getAnonymousElementByAttribute(ourItem, "class", "actionsMenu")); [1]
> 
> Jared, do you know why we do this there?
> 
> [1]:
> http://searchfox.org/mozilla-central/source/browser/components/preferences/
> in-content-new/tests/browser_change_app_handler.js#30

This convention is usually used to wait for a binding to attach.
(In reply to Evan Tseng [:evanxd] from comment #158)
> For "browser_change_app_handler.js" test failure, there is a weird situation.
> 
> The test can be passed[1] if run it along with the command `try: -b do -p
> linux64,macosx64,win32,win64 -t none browser/base/content/test/alerts/
> browser/components/preferences/in-content-new/tests/` but it will be
> failed[2] if run it with all other test files (try: -b do -p
> linux64,macosx64,win32,win64 -u mochitests -t none).
> 
> Maybe the "leaked window(s)" failure is caused by other test file?
> 
> Additionally, same situation as "browser_notification_open_settings.js"
> test, "browser_change_app_handler.js" test can be passed[1] but it will
> cause the "leaked window(s)" issue after it is finished and shutdown the
> testing window

Yes, this seems like it may be caused by the combination of this test along with another test in the suite. I would recommend that you set up a Linux x64 debug environment, and then reproduce the failure locally. You can then do a binary search on the test manifest (comment out half, see if it still fails, then comment half of the failing side, etc). You can also try to use a one-click loaner machine if you don't want to set up a local build environment. Here's more information about one-click loaners, https://developer.mozilla.org/en-US/docs/Running_automated_tests/TaskCluster_interactive_session

Comment 172

15 days ago
mozreview-review
Comment on attachment 8879454 [details]
Bug 1365133 - Update tests - Part 2.

https://reviewboard.mozilla.org/r/150760/#review160294

::: browser/components/preferences/in-content-new/tests/browser_change_app_handler.js:22
(Diff revision 61)
> +  Services.obs.addObserver(() => {
> +    isGeneralPaneLoaded = true;
> +  }, "main-pane-loaded");
> +
> +  let prefs = await openPreferencesViaOpenPreferencesAPI("paneGeneral", {leaveOpen: true});
> +  is(prefs.selectedPane, "paneGeneral", "General pane was selected");
> +  await BrowserTestUtils.waitForCondition(() => isGeneralPaneLoaded);

This observer is never removed. This might be the source of your leak.

You can switch to using TestUtils.topicObserved. See http://searchfox.org/mozilla-central/rev/f1472e5b57fea8d4a7bbdd81c44e46958fe3c1ce/browser/base/content/test/general/browser_storagePressure_notification.js#19 for an example of how it is used.
I agree with Jared. (Though I think we do run this leak checking on all platforms and opt as well as debug, so it is a little odd it only shows up on Linux64 debug.) This leak is potentially bad, so you should investigate what the problem is and not just disable the test. This leak checking mechanism is generally quite reliable. It also looks like you are leaking 3 separate about:preferences windows, so it doesn't look like some odd timing issue.

The observer issue Jared points out could be the problem. Other possible issues that can cause things like this are failing to remove a listener, or having a global variable that contains a strong reference to a window (though given that you are leaking 3 windows that feels less likely here). Please let me know if you'd like some help to debug this issue. Looking over the patch is usually the easiest way to figure these leaks out, but if that doesn't work, looking at the GC/CC logs can be a path forward. What you really want is to see why the windows that are held alive too long are alive in the GC/CCs before the shutdown logs.
Flags: needinfo?(continuation)
My try push has confirmed that this change, while useful, did not fix the leak. I'm still looking at it some more.
This JavaScript error in the logs might point at the problem,
https://treeherder.mozilla.org/logviewer.html#?job_id=112577201&repo=try&lineNumber=2289

[task 2017-07-07T15:48:00.803466Z] 15:48:00     INFO - GECKO(1091) | JavaScript error: chrome://browser/content/preferences/in-content-new/preferences.js, line 56: ReferenceError: gMainPane is not defined
With this try push I no longer see the leak,
https://treeherder.mozilla.org/#/jobs?repo=try&revision=24cba007f8e1d1115b1a18d79093fa0d3fe2af6d

The patch on the try push only makes two changes:
1. The change to browser_change_app_handler.js is to properly clean up the observer. This change is not necessary to fix this leak, as this test doesn't run on non-Windows platforms and the leak is showing up on Linux. However, it will still be good to fix this.

2. I removed the notifyObserver call at the end of gPrivacyPane.init. This observer, "privacy-pane-loaded", is unused elsewhere and serves no purpose. It was added in the patches in this bug.

I talked with :mattw about this and the JS error in comment #175, and we saw that the error in comment #175 is coming from "DOMContentLoaded" which potentially could fire before the other JS subresources have finished loading. It's possible that code added to main.js or privacy.js makes them take longer to load or execute, and thus gMainPane is not defined by the time "DOMContentLoaded" is received.

The try push has 6 retriggers on mochitest(bc5) which is the failing job from other pushes, but with this patch it no longer has any failures.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 179

13 days ago
mozreview-review
Comment on attachment 8884464 [details]
Bug 1365133 - Properly clean up an observer as well as remove an observer notification that was unreferenced.

https://reviewboard.mozilla.org/r/155368/#review160568

Hi Jared,

This change could not fix the intermittent leaked window failure. See the failed job at [1]. We still have the failure in the Linux x64 debug build. I think we should still need to figure out the leak check issue mentioned on the bugzilla comment[2].

Thank you for the help.

[1]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4ce08e23535f14628d19f2d6a8ac3494ace93df2&selectedJob=112899263
[2]: https://bugzilla.mozilla.org/show_bug.cgi?id=1365133#c165
Attachment #8884464 - Flags: review?(evan) → review-
(Assignee)

Updated

12 days ago
Depends on: 1379549
(Assignee)

Updated

12 days ago
Blocks: 1379549
No longer depends on: 1379549
Flags: needinfo?(james) → needinfo?(ahalberstadt)
(In reply to Ting-Yu Chou [:ting] from comment #165)
> I checked the code in ShutdownLeaks [1], "--DOMWINDOW" logs seems are
> reported as leaked if they are appeared after message "Completed
> ShutdownLeaks collections in process". But the message is output from
> browser-test.js [2] without running GC/CC at all.
> 
> :jgraham , is [1] still valid to determine whether the DOMWINDOW is leaked
> after bug 1139021?
> 
> [1]
> http://searchfox.org/mozilla-central/rev/
> f1472e5b57fea8d4a7bbdd81c44e46958fe3c1ce/testing/mochitest/leaks.py#96-97
> [2]
> http://searchfox.org/mozilla-central/rev/
> f1472e5b57fea8d4a7bbdd81c44e46958fe3c1ce/testing/mochitest/browser-test.
> js#326

I'm not sure either, I'm not very familiar with the leak check code. Afaict, it looks like bug 1139021 is preventing GC from running twice, but it should still be running once, in which case there's likely no problem here. But if GC isn't running at all, then you might be right. My intuition says that if leak collection were broken in this way, then we'd have perma failing jobs.
Flags: needinfo?(ahalberstadt)
(Assignee)

Comment 181

12 days ago
Hi Andrew,

Do you know who can confirm this since Bug 1280571 was reviewed by you? 
(In reply to Ting-Yu Chou [:ting] from comment #165)
> I checked the code in ShutdownLeaks [1], "--DOMWINDOW" logs seems are
> reported as leaked if they are appeared after message "Completed
> ShutdownLeaks collections in process". But the message is output from
> browser-test.js [2] without running GC/CC at all.
> 
> :jgraham , is [1] still valid to determine whether the DOMWINDOW is leaked
> after bug 1139021?
> 
> [1]
> http://searchfox.org/mozilla-central/rev/
> f1472e5b57fea8d4a7bbdd81c44e46958fe3c1ce/testing/mochitest/leaks.py#96-97
> [2]
> http://searchfox.org/mozilla-central/rev/
> f1472e5b57fea8d4a7bbdd81c44e46958fe3c1ce/testing/mochitest/browser-test.
> js#326

And do you know why the leakcheck[1] says
> TEST-PASS | leakcheck | default process: no leaks detected!
but the log[1] still says
> ERROR - TEST-UNEXPECTED-FAIL | browser/base/content/test/alerts/browser_notification_open_settings.js | leaked 2 window(s) until shutdown [url = about:preferences#privacy]

They seems to be conflicted.

[1]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4ce08e23535f14628d19f2d6a8ac3494ace93df2&selectedJob=112899263
Flags: needinfo?(ahalberstadt)
(In reply to Evan Tseng [:evanxd] from comment #181)
> They seems to be conflicted.

These check for two separate kinds of leaks. The first one checks if any objects are still alive very late after we shutdown XPCOM. The second one checks that all windows and docshells created during a test are destroyed during that test (after we end the tab and GC/CC a bunch of times). This situation can happen if some global data structure that we destroy during shutdown
 holds a reference to the window (like the observer stuff Jared mentioned).
Flags: needinfo?(ahalberstadt)
Evan, are you able to apply only parts of your patch and push that to tryserver?

You could push two halves of your patch (separately) to tryserver and see if the leak still reproduces in one of the halves. Then you could repeat this process with the failing half until you are able to see where the problem is. If the leak doesn't reproduce on either half at some point, then you know that the leak has something to do with the combination of the two halves.
(Assignee)

Comment 184

12 days ago
> I checked the code in ShutdownLeaks [1], "--DOMWINDOW" logs seems are
> reported as leaked if they are appeared after message "Completed
> ShutdownLeaks collections in process". But the message is output from
> browser-test.js [2] without running GC/CC at all.
> 
> :jgraham , is [1] still valid to determine whether the DOMWINDOW is leaked
> after bug 1139021?
> 
> [1]
> http://searchfox.org/mozilla-central/rev/
> f1472e5b57fea8d4a7bbdd81c44e46958fe3c1ce/testing/mochitest/leaks.py#96-97
> [2]
> http://searchfox.org/mozilla-central/rev/
> f1472e5b57fea8d4a7bbdd81c44e46958fe3c1ce/testing/mochitest/browser-test.
> js#326

The leaked window issue still occurs[1] even we remove Bug 1139021's changes.

[1]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b6e51b627fc0ebad13c685887e47a2e4b5c637ed
(In reply to Andrew Halberstadt [:ahal] from comment #180)
> I'm not sure either, I'm not very familiar with the leak check code. Afaict,
> it looks like bug 1139021 is preventing GC from running twice, but it should
> still be running once, in which case there's likely no problem here. But if
> GC isn't running at all, then you might be right. My intuition says that if
> leak collection were broken in this way, then we'd have perma failing jobs.

I just double checked, bug 1139021 does prevent running GC/CC from the handler of "browser-test:collect-request" in parent process, but there're still several GC/CC runs before we see the message "Completed ShutdownLeaks collections in process", which is done here: https://searchfox.org/mozilla-central/rev/5dadcbe55b4ddd1e448c06c77390ff6483aa009b/testing/mochitest/browser-test.js#655-663, so it should be still valid.

Evan is getting a GC/CC log at the timing when print "Completed ShutdownLeaks collections in process" instead of MOZ_CC_LOG_SHUTDOWN, I'll check the log then.
(Assignee)

Comment 186

11 days ago
The GC/CC log[1] at the timing when print "Completed ShutdownLeaks collections in process".

[1]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5ed2bf6ef478e9d630a0b7f2e6df1f6dd89b7f93&selectedJob=113226019
(Assignee)

Comment 187

11 days ago
The GC/CC log[1] with dump and finishing dump timing.

[1]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e15393e5ee23f5d25c4db9434f8c47ecbbf7552d&selectedJob=113246003
(Assignee)

Comment 188

11 days ago
The GC/CC log[1] without minimizing.

[1]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f962c6e07816d332813582137d7af9f686de8bb8
(Assignee)

Comment 189

11 days ago
Change the loading timing of dialogTemplate vbox[1] to see if the leak issue is gone.

[1]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ef8b0fce678afabdc8b1b879f6c28cf4792e7dc1
(Assignee)

Comment 190

11 days ago
After investigating the GC/CC log at Comment 187, I find out that a leaked window (0x7f7d2cb8f800) mentioned in the try[1] log[2] it is a garbage item in the CC log[3]. It is weird (not correct) why a garbage window (reference) is listed as a leaked window in the try log.

The timing to dump the GC/CC log is after "Completed ShutdownLeaks collections in process" and before check the window leak. Check the patch[4] to see how to do this.

> [task 2017-07-11T05:48:18.919858Z] 05:48:18     INFO - GECKO(1091) | --DOMWINDOW == 10 (0x7f7d2cb8f800) [pid = 1091] [serial = 38] [outer = (nil)] [url = about:preferences#privacy]

> 0x7f7d2cb8f800 [garbage]

[1]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e15393e5ee23f5d25c4db9434f8c47ecbbf7552d&selectedJob=113246003
[2]: https://public-artifacts.taskcluster.net/f4gcdZGcTRSkvDkc99IJXA/0/public/logs/live_backing.log
[3]: https://public-artifacts.taskcluster.net/f4gcdZGcTRSkvDkc99IJXA/0/public/test_info//cc-edges.1091.test.1499752082331.log
[4]: https://hg.mozilla.org/try/rev/1b37097ffe4d
(Assignee)

Comment 191

11 days ago
Hi Andrew,

What do you think of Comment 190?

Thank you.
Flags: needinfo?(continuation)
(In reply to Evan Tseng [:evanxd] from comment #190)
> After investigating the GC/CC log at Comment 187, I find out that a leaked
> window (0x7f7d2cb8f800) mentioned in the try[1] log[2] it is a garbage item
> in the CC log[3]. It is weird (not correct) why a garbage window (reference)
> is listed as a leaked window in the try log.

The cycle collection that you added to record the log is happening after the last GC/CC we do in shutdownCleanup. My guess would be that some of the code we are running in between the last GC/CC we do, and when your logging runs, is what is allowing the window to be freed, but only when we next run a GC/CC, which could explain why the failure is intermittent (sometimes we might happen to run a GC/CC there before we print out the message, some times we don't). 

Maybe one of these things?
      TabDestroyObserver.destroy();
      Services.console.unregisterListener(this);
      this.PromiseTestUtils.uninit();

The cycle collector listing something as [garbage] means that we run the Unlink method on that nsGlobalWindow (which means that we null out strong references that the window holds, whereas the --DOMWINDOW line is printed when we run the destructor for the nsGlobalWindow. The destructor call can happen slightly later, which I think is why it shows up after "dumper onFinish is called.".

> The timing to dump the GC/CC log is after "Completed ShutdownLeaks
> collections in process" and before check the window leak. Check the patch[4]
> to see how to do this.

Oh, sorry, I forgot that the more direct way to do this is to create an instance of @mozilla.org/cycle-collector-logger;1 in JS, and then pass it in to the actual CC calls you are trying to log. Although I haven't tried it myself recently so maybe it doesn't work. (You still have to set the upload directory and so forth like you have done.) But that will let you see a log for the exact time the collection runs.
Flags: needinfo?(continuation)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
In reply to Evan Tseng [:evanxd] from comment #188)
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=f962c6e07816d332813582137d7af9f686de8bb8

From cc-edges.1093.test.1499754765787.log, the root of leaked window is a XUL document with 1 unknown edge, for instance:

  --DOMWINDOW == 3 (0x7f26ffcfa800) [pid = 1093] [serial = 44] [outer = (nil)] [url = about:preferences#privacy]

  ting@ting-pc:~/w/fx/mc$ python ../tools/heapgraph/find_roots.py ~/Desktop/cc-edges.1093.test.1499754765787.log 0x7f26ffcfa800
  Parsing /home/ting/Desktop/cc-edges.1093.test.1499754765787.log. Done loading graph. 

  0x7f26f7af9000 [nsDocument normal (XUL) about:preferences#privacy]
      --[mChildren[i]]--> 0x7f27159ff160 [FragmentOrElement (XUL) page about:preferences#privacy]
      --[Preserved wrapper]--> 0x7f2705995040 [JS Object (XULElement)]
      --[group_global]--> 0x7f27051f91a0 [JS Object (Window)]
      --[UnwrapDOMObject(obj)]--> 0x7f26f68b5000 [nsGlobalWindow # 46 inner about:preferences#privacy]
      --[mOuterWindow]--> 0x7f26ffcfa800 [nsGlobalWindow # 44 outer ]

      Root 0x7f26f7af9000 is a ref counted object with 1 unknown edge(s).
      known edges:
         0x7f26f68b5000 [nsGlobalWindow # 46 inner about:preferences#privacy] --[mDoc]--> 0x7f26f7af9000
         0x7f26f710f9b0 [nsNodeInfoManager] --[mDocument]--> 0x7f26f7af9000
         0x7f26f60c4790 [nsXULCommandDispatcher] --[mDocument]--> 0x7f26f7af9000
         0x7f2705995190 [JS Object (XULDocument)] --[UnwrapDOMObject(obj)]--> 0x7f26f7af9000

And I noticed this special warning from nsFrameLoader::LoadURI after "TEST-START | Shutdown" for "about:blank", which happens when insert the <browser> [1] after the sync.js [2] gets compiled off main thread:

  WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80040111: file /home/worker/workspace/build/src/dom/base/nsFrameLoader.cpp, line 328

This leads me to NotifyOffThreadScriptCompletedRunnable and its sReceivers [3], which seems is where the unknown edge from. My theory is the test loads preferences.xul but finishes before the last NotifyOffThreadScriptCompletedRunnable::Run() to release [4] the reference to the XUL document. I did an experiment by adding a sleep in the beginning of OffThreadScriptReceiverCallback() so the NotifyOffThreadScriptCompletedRunnable::Run() is delayed, then I can reproduce leaked windows of "about:preferences#privacy" reliably by running the test:

  ./mach mochitest --disable-e10s ./browser/base/content/test/alerts/browser_notification_open_settings.js

:mccr8, do we really need to keep the XUL document alive just for passing it the compiled script? If we don't, shouldn't we use WeakPtr for storing the mReceiver instead and get rid of sReceiver? Please let me know if I should ask someone else for this.

Meanwhile, I asked Evan to fix the test code to start testing after the XUL is loaded, the Try is still running but so far we don't see leaked "about:preferences#privacy" yet [5][6]. Also removing the observer that :jaws did in comment 176 does fix the leak of alert.xul.

[1] http://searchfox.org/mozilla-central/rev/5dadcbe55b4ddd1e448c06c77390ff6483aa009b/browser/components/preferences/in-content-new/preferences.xul#232-234
[2] http://searchfox.org/mozilla-central/rev/5dadcbe55b4ddd1e448c06c77390ff6483aa009b/browser/components/preferences/in-content-new/sync.xul#29
[3] http://searchfox.org/mozilla-central/rev/31311070d9860b24fe4a7a36976c14b328c16208/dom/xul/nsXULElement.cpp#2636
[4] http://searchfox.org/mozilla-central/rev/31311070d9860b24fe4a7a36976c14b328c16208/dom/xul/nsXULElement.cpp#2695
[5] https://treeherder.mozilla.org/#/jobs?repo=try&revision=0b8226e54217afa339bc2c4366902de2f4b9f57d&group_state=expanded
[6] https://treeherder.mozilla.org/#/jobs?repo=try&revision=7cf2e5011bcaabc406e3c98bfda3b08de441ef7b&group_state=expanded
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
NI for comment 195.
Flags: needinfo?(continuation)
(Assignee)

Comment 208

10 days ago
Looks like the try is good. Let's try to land the patch again.
Keywords: checkin-needed

Comment 209

9 days ago
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 7aa643cba6dc -d 1bb454794eab: rebasing 406864:7aa643cba6dc "Bug 1365133 - Reorganize Preferences sections and regroup <xul:groupbox> elements by new categories - Part 1. r=jaws"
local [dest] changed browser/components/preferences/in-content-new/applications.js which other [source] deleted
use (c)hanged version, (d)elete, or leave (u)nresolved? u
merging browser/app/profile/firefox.js
merging browser/themes/shared/incontentprefs/preferences.inc.css
unresolved conflicts (see hg resolve, then hg rebase --continue)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Patch rebased on top of the lastest nightly but we need to make change for eslint rule turned on in bug 1380009 accordingly.
Comment hidden (mozreview-request)
(Reporter)

Comment 214

9 days ago
mozreview-review
Comment on attachment 8879397 [details]
Bug 1365133 - Reorganize Preferences sections and regroup <xul:groupbox> elements by new categories - Part 1.

https://reviewboard.mozilla.org/r/146602/#review161930

::: toolkit/mozapps/extensions/test/browser/browser_experiments.js:167
(Diff revision 101)
>  });
>  
>  add_task(async function testOpenPreferences() {
>    await gCategoryUtilities.openType("experiment");
>    let btn = gManagerWindow.document.getElementById("experiments-change-telemetry");
>    Services.prefs.setBoolPref("browser.preferences.useOldOrganization", true);

The below changes need to be reverted on this test case because this test case uses the old pref page...
Fixed browser_experiments.js and re-try

https://treeherder.mozilla.org/#/jobs?repo=try&revision=956dda2757df2665bc70bd0c01808587a5acd35d
Comment hidden (mozreview-request)
(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #215)
> Fixed browser_experiments.js and re-try
> 
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=956dda2757df2665bc70bd0c01808587a5acd35d

I am trying to classify all intermittent here so it should give us a clearer view on what still need fixing.

- browser/components/preferences/in-content-new/tests/browser_change_app_handler.js on Windows 7 VM opt still fails consistently, need fixing.
- bug 1336839 have since become permafail.
Flags: needinfo?(evan)
(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #217)
> I am trying to classify all intermittent here so it should give us a clearer
> view on what still need fixing.
> 
> -
> browser/components/preferences/in-content-new/tests/
> browser_change_app_handler.js on Windows 7 VM opt still fails consistently,
> need fixing.
> - bug 1336839 have since become permafail.

I might be wrong. Both seems to be intermittent.
Flags: needinfo?(evan)
has a -r comment/review, so need to be fixed first before we can use autoland
Flags: needinfo?(evan)
Keywords: checkin-needed
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment on attachment 8884464 [details]
Bug 1365133 - Properly clean up an observer as well as remove an observer notification that was unreferenced.

Marking my patch as obsolete so this can land.
Attachment #8884464 - Attachment is obsolete: true
Flags: needinfo?(evan)
Keywords: checkin-needed
Comment hidden (mozreview-request)
(In reply to Ting-Yu Chou [:ting] from comment #195)
> :mccr8, do we really need to keep the XUL document alive just for passing it
> the compiled script? If we don't, shouldn't we use WeakPtr for storing the
> mReceiver instead and get rid of sReceiver? Please let me know if I should
> ask someone else for this.
Nice investigation! I'm not entirely sure how this should work, but it does seem bad if the current setup is causing difficult to diagnose leaks, so you should file a bug on that. Thanks.
Flags: needinfo?(continuation)

Comment 229

9 days ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/19d1e9dccdbb
Reorganize Preferences sections and regroup <xul:groupbox> elements by new categories - Part 1. r=jaws
https://hg.mozilla.org/integration/autoland/rev/fb02b6e36ae3
Update tests - Part 2. r=jaws
Keywords: checkin-needed
(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #218)
> (In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment
> #217)
> > -
> > browser/components/preferences/in-content-new/tests/
> > browser_change_app_handler.js on Windows 7 VM opt still fails consistently,
> > need fixing.
> > - bug 1336839 have since become permafail.
> 
> I might be wrong. Both seems to be intermittent.

I am very sorry that either I or Evan comment on the bug. The finding right after I made this comment was that this test currently fails on Try at roughly 50%. I am pretty sure it will get back out from autoland. Evan should continue to debug that (AND comment on the bug on your finding and what you ruled out!) until proven otherwise.
Flags: needinfo?(evan)
(Assignee)

Comment 231

8 days ago
The intermittent test failure is about waiting for the "handler-selected" xulbinding[1] to get the element's content and ensure the element is ready to test but it doesn't happen intermittently. So the failure log is like
> | browser/components/preferences/in-content-new/tests/browser_change_app_handler.js | Uncaught exception
> Condition timed out: () => { let children = win.document.getAnonymousNodes(ourItem); if (children) { for (let i = 0; i < children.length; i++) { info("loop1: ourItem 

The current finding is that during running the test[2], doing the "handler-selected" xulbinding[3] for the ("#handlersView richlistitem") element[4] (in the bug patch) is NOT successful intermittently after selecting it[5]. In the failed case, the richlistitem doesn't have any anonymous nodes. If we try to print the content of richlistitem after the test selects it[5], we could see the log[6], it said "loop1: nothing" which means the content is empty and there is no anonymous node in the richlistitem. It might also mean the xulbinding is not successful.

> 20:57:34     INFO - loop1: nothing
> 20:57:34     INFO - loop1: nothing
> 20:57:34     INFO - loop1: nothing
> ...
> ...

So I think we should find out the root cause of why the xulbinding fail intermittently to fix the problem.

[1]: http://searchfox.org/mozilla-central/source/browser/components/preferences/in-content-new/tests/browser_change_app_handler.js#30
[2]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8fc961ac4ae0f3ddb0eb7a65e73192edca98501b&selectedJob=114224880
[3]: http://searchfox.org/mozilla-central/source/browser/components/preferences/handlers.xml#48-64
[4]: http://searchfox.org/mozilla-central/source/browser/components/preferences/in-content-new/applications.xul#118
[5]: http://searchfox.org/mozilla-central/source/browser/components/preferences/in-content-new/tests/browser_change_app_handler.js#27
[6]: https://archive.mozilla.org/pub/firefox/try-builds/itoyxd@gmail.com-8fc961ac4ae0f3ddb0eb7a65e73192edca98501b/try-win32/try_win7_vm_test-mochitest-browser-chrome-1-bm140-tests1-windows-build1113.txt.gz
Flags: needinfo?(evan)
(In reply to Andrew McCreight [:mccr8] from comment #228)
> Nice investigation! I'm not entirely sure how this should work, but it does
> seem bad if the current setup is causing difficult to diagnose leaks, so you
> should file a bug on that. Thanks.

Bug 1380923.
Backed out for frequently failing modified browser/components/preferences/in-content-new/tests/browser_change_app_handler.js:

https://hg.mozilla.org/integration/autoland/rev/bffc3314c82b432646509c53b7a751a11b2284aa
https://hg.mozilla.org/integration/autoland/rev/4158208f8394c73b0a02cffe3202b3e96beb5bec

Failure log example: https://treeherder.mozilla.org/logviewer.html#?job_id=114211183&repo=autoland

19:43:59     INFO - TEST-PASS | browser/components/preferences/in-content-new/tests/browser_change_app_handler.js | All expectedStyleSheetURLs should have been found - 
19:43:59     INFO - Dialog loaded
19:43:59     INFO - TEST-PASS | browser/components/preferences/in-content-new/tests/browser_change_app_handler.js | App should be set as preferred. - 
19:44:43     INFO - Longer timeout required, waiting longer...  Remaining timeouts: 9
19:45:28     INFO - Longer timeout required, waiting longer...  Remaining timeouts: 8
19:46:13     INFO - Longer timeout required, waiting longer...  Remaining timeouts: 7
19:46:58     INFO - Longer timeout required, waiting longer...  Remaining timeouts: 6
19:47:43     INFO - Longer timeout required, waiting longer...  Remaining timeouts: 5
19:48:28     INFO - Longer timeout required, waiting longer...  Remaining timeouts: 4
19:49:03     INFO - TEST-INFO | started process screenshot
19:49:03     INFO - TEST-INFO | screenshot: exit 0
19:49:03     INFO - TEST-UNEXPECTED-FAIL | browser/components/preferences/in-content-new/tests/browser_change_app_handler.js | Uncaught exception - Condition timed out: () => win.document.getAnonymousElementByAttribute(ourItem, "class", "actionsMenu")
19:49:03     INFO - Leaving test bound
Flags: needinfo?(evan)
(Assignee)

Comment 234

8 days ago
Let's try to fix the timeout on the general page by removing the added UI bit by bit. I'll try to move the applications section back to applications pane to see if we still have the intermittent failure or not.
Here is summarize of our current investigation. 

According to comment 231, our current outcome leads to the weird behavior from XUL binding. The handler-selected binding is unable to be applied as expected after selecting. This case only happens on slower try machine (e.g. Windows 7 VM). The only reason I can image is that there might be a JS code changes the interaction that removes handler binding [1] but doesn't apply handler-selected binding properly.

We should do more investigation on Monday and then decide whether we should continue to find out the root cause of test failure or move on.

[1] http://searchfox.org/mozilla-central/source/browser/components/preferences/handlers.xml#32
(Assignee)

Comment 236

6 days ago
I found out this intermittent failure might be cause by a database access issue in the "browser_change_app_handler.js" test. In the "browser_change_app_handler.js", we create a fake app handler by adding a new app record into the database[1] but we continue to test without ensuring the record already stored in the db. Somehow the failure might happen. At the same time, we have another test file (browser_applications_selection.js[2]) without adding a fake app handler to test applications section, but it never fails or have any intermittent failure. So I guess the root cause might be the database issue.

To prove the opinion, I add a `setTimeout` for opening the General pane after we add the fake app handler to wait for record stored. It helps the test have the app handler record before doing the test. The result[3] is that the browser_change_app_handler.js can be passed continually for 50 times on Windows VM.

[1]: http://searchfox.org/mozilla-central/source/browser/components/preferences/in-content-new/tests/browser_change_app_handler.js#14
[2]: http://searchfox.org/mozilla-central/source/browser/components/preferences/in-content-new/tests/browser_applications_selection.js
[3]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=402b4f5fe94d60fcadc809fed4b6e3190d9ac7b9
Flags: needinfo?(evan)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Blocks: 1381138

Comment 240

5 days ago
mozreview-review
Comment on attachment 8879397 [details]
Bug 1365133 - Reorganize Preferences sections and regroup <xul:groupbox> elements by new categories - Part 1.

https://reviewboard.mozilla.org/r/150718/#review160276

::: browser/components/preferences/in-content-new/main.js:405
(Diff revision 24)
> +    } else
> +      this._sortColumn = document.getElementById("typeColumn");

nit, please put braces around the else-block if the if-block has braces

::: browser/locales/en-US/chrome/browser/preferences/advanced.dtd:38
(Diff revision 31)
> -<!ENTITY enableHealthReport.accesskey    "R">
>  <!ENTITY healthReportLearnMore.label     "Learn more">
>  
> -<!ENTITY telemetryDesc.label             "Shares performance, usage, hardware and customization data about your browser with &vendorShortName; to help us make &brandShortName; better">
> -<!ENTITY enableTelemetryData.label       "Share Additional Data (i.e., Telemetry)">
> -<!ENTITY enableTelemetryData.accesskey   "T">
> +<!ENTITY dataCollection.label            "&brandShortName; Data Collection and Use">
> +<!ENTITY dataCollectionDesc.label        "We strive to provide you with choices and collect only what we need to provide and improve &brandShortName; for everyone. We always ask permission before receiving personal information.">
> +<!ENTITY dataCollectionPrivacyNotice.label    "PrivacyNotice">

typo, "Privacy Notice"

Comment 241

5 days ago
mozreview-review
Comment on attachment 8879397 [details]
Bug 1365133 - Reorganize Preferences sections and regroup <xul:groupbox> elements by new categories - Part 1.

https://reviewboard.mozilla.org/r/150718/#review162898

::: browser/locales/en-US/chrome/browser/preferences/advanced.dtd:33
(Diff revision 31)
>  disabled data sharing options in developer builds or builds with no Telemetry support
>  available. -->
>  <!ENTITY healthReportingDisabled.label   "Data reporting is disabled for this build configuration">
>  
> -<!ENTITY healthReportDesc.label          "Helps you understand your browser performance and shares data with &vendorShortName; about your browser health">
> -<!ENTITY enableHealthReport.label        "Enable &brandShortName; Health Report">
> +<!ENTITY enableHealthReport1.label       "Allow &brandShortName; to automatically send technical and interaction data to Mozilla">
> +<!ENTITY enableHealthReport1.accesskey   "R">

r, not R (no practial effect, but still confusing since R is not available)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
browser_applications_selection.js always runs before browser_change_app_handler.js (they run in alphabetical order and I confirmed via looking at the log[1].

browser_applications_selection.js doesn't insert anything in to the database, it uses pre-existing entries.  Since we haven't seen a similar failure with browser_applications_selection.js, can you do a try run that uses the same entries for browser_change_app_handler.js?

We can't check in a test that adds a setTimeout. It's possible that your try push, which inserts the x-test-handler during browser_applications_selection.js, managed to fix the issue just because the insertion was done much earlier. timdream points out that this is not likely a database issue since `ourItem` is not null. However this could be an issue with using the x-test-handler mimetype.

[1] https://archive.mozilla.org/pub/firefox/try-builds/itoyxd@gmail.com-402b4f5fe94d60fcadc809fed4b6e3190d9ac7b9/try-win32/try_win7_vm_test-mochitest-e10s-browser-chrome-1-bm140-tests1-windows-build1215.txt.gz
(Assignee)

Comment 245

5 days ago
Hi Jared,

Here is the thing about the intermittent "browser_change_app_handler.js" failure. Let me summarize it and propose solutions to fix it for this stage here.

After investigating, I found out the failure might be caused by platform issue. In previous experience, this kind of test failure might be possibly caused by a platform issue (just like the previous leaked window(s) issue). The main point is the try[1] log[2] is conflicted with the screenshot[3] captured when the test fails. The below is the test code I added to log[4].
```
let list = await waitForCondition(() => {
  let children = win.document.getAnonymousNodes(ourItem);
  if (children) {
    for (let i = 0; i < children.length; i++) {
      info("loop1: ourItem innerHTML: ");
      info(children[i].innerHTML);
    }
  } else {
    info("loop1: nothing");
  }
  return win.document.getAnonymousElementByAttribute(ourItem, "class", "actionsMenu");
});
```

The log[2] said there is no any content in the `richlistitem[type='text/x-test-handler']` richlistitem, it printed
> 20:57:34     INFO - loop1: nothing
> 20:57:34     INFO - loop1: nothing
> 20:57:34     INFO - loop1: nothing
> ...
> ...
It just likes I mentioned previously at Comment 231, which means the test cannot get any anonymous nodes by `getAnonymousNodes` method. But the screenshot[3] just showed the richlistitem after test failed. The richlistitem should not be shown in the screenshot if richlistitem is an empty element. It should just show a white(or black) block there. So somehow maybe the `document.getAnonymousNodes` call at the runtime of running the test just cannot work (or something wrong with it) to get the content of the richlistitem element after doing the xulbinding in the test[5]. So the problem here for platform (gecko) could be why `document.getAnonymousNodes` cannot get anything from the richlistitem even the UI is rendered (the screenshot shows that).

At this stage for the reorg v2 work, I think we could have at least two approaches to fix it in the front-end code.
1. (In Comment 244, Jared said we should not use a setTimeout in test. I understand, maybe we should find another way or workaround to fix it) 
Add a setTimout to wait few seconds and then start the test. I found we can add 5 seconds timeout to avoid the intermittent failure. See the try[6], it is passed continuously for 20+ times. I tried 10, 5, and 1 second(s) timeout, 10 and 5 secs could work but 1 sec cannot. I found out this workaround because we have another test (browser_applications_selection.js[7]) to test applications section but it never fails and doesn't add a fake app handler in the test. Then I guess maybe something wrong with the `setupFakeHandler` method or other init things in the browser_change_app_handler.js test[8]. I tried other timing to wait, like the `app-handler-pane-loaded` observers, but it can not work. Then we file a platform bug to address it.

2. Disable the `browser_change_app_handler.js` test only on Windows VM platform, since the root cause might be related to platform issues. We could disable it and file a platform bug to address it.

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #244)
> browser_applications_selection.js always runs before
> browser_change_app_handler.js (they run in alphabetical order and I
> confirmed via looking at the log[1].
> 
> browser_applications_selection.js doesn't insert anything in to the
> database, it uses pre-existing entries.  Since we haven't seen a similar
> failure with browser_applications_selection.js, can you do a try run that
> uses the same entries for browser_change_app_handler.js?
I will try. I think you're saying we could just use the pre-existing entries for browser_applications_selection.js without adding a fake app handler, right?

> We can't check in a test that adds a setTimeout. It's possible that your try
> push, which inserts the x-test-handler during
> browser_applications_selection.js, managed to fix the issue just because the
> insertion was done much earlier. timdream points out that this is not likely
> a database issue since `ourItem` is not null. However this could be an issue
> with using the x-test-handler mimetype.
Agree, it might not be a database issue. I think the root cause might be related to xulbinding or document.getAnonymousNodes API call issue, just like I mentioned previously.

At this moment, if we could not have a good workaround, we could consider to disable the test at some point to land the major patch for 56 release. At the same time, we should file a new bug to address the possible platform bug (log conflicts with screenshot issue). Do we possibly disable the test at some point? Maybe after we try "uses the same entries for browser_change_app_handler.js?" and don't get a good result. What do you think, Jared?

[1]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8fc961ac4ae0f3ddb0eb7a65e73192edca98501b&selectedJob=114224880
[2]: https://archive.mozilla.org/pub/firefox/try-builds/itoyxd@gmail.com-8fc961ac4ae0f3ddb0eb7a65e73192edca98501b/try-win32/try_win7_vm_test-mochitest-browser-chrome-1-bm140-tests1-windows-build1113.txt.gz
[3]: http://mozilla-releng-blobs.s3.amazonaws.com/blobs/try/sha512/d9800c72458ae5570606d18695a6062704bbaee55c4949478e3a50196bb8e177ea4a02e32ddef092427ef823f764b6a9a4ed79917da2e876b9578111d61ee137
[4]: https://hg.mozilla.org/try/rev/ceba9aa757b3
[5]: http://searchfox.org/mozilla-central/source/browser/components/preferences/in-content-new/tests/browser_change_app_handler.js#27
[6]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a6bff0950d14a25cfc3da4ce429049028e3869a3
[7]: http://searchfox.org/mozilla-central/source/browser/components/preferences/in-content-new/tests/browser_applications_selection.js
[8]: http://searchfox.org/mozilla-central/source/browser/components/preferences/in-content-new/tests/browser_change_app_handler.js#18
[9]: http://searchfox.org/mozilla-central/source/browser/components/preferences/in-content-new/applications.js#948
Flags: needinfo?(jaws)
(Assignee)

Comment 246

5 days ago
The try[1] uses the pre-existing app handler as same in browser_applications_selection.js test file. Let's wait the result. If it is good, we don't need to consider to disable the test.

[1]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=023fadb836df69969e0a8f118807487a5f8bc31c
The test is failing because there is no preferred application handler for the mimetype.

browser_change_app_handler.js was setting an application handler in the setupFakeHandler function. My only guess here is that flushing the change is slow on the VM. You could add the setupFakeHandler call back and then await gHandlerSvc.exists[1] and then proceed with the test.

If this doesn't work then I'm OK with disabling the test on Windows 7.

[1] http://searchfox.org/mozilla-central/rev/01d27fdd3946f7210da91b18fcccca01d7324fe2/uriloader/exthandler/nsHandlerService.js#392
Flags: needinfo?(jaws)
(Assignee)

Comment 248

5 days ago
Hi Jared,

I've pushed a try[1] to `await gHandlerSvc.exists`. But it still cannot work. Let's disable the test at this stage. I'll file a follow bug to address the intermittent test failure issue.

Thank you for the help.

[1]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=779ab1ccbc4311947eb04ae326810888e848d730
(Assignee)

Updated

5 days ago
See Also: → bug 1381706
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 251

5 days ago
Cleaned up the code, fixed the nits from comments, and disable the test on Window 7 (after searching the code looks like we don't have the capability to disable the test only on Window 7 VM). Let's land the code once the try is good[1].

[1]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ac273b04e9e4
Comment hidden (mozreview-request)
(Assignee)

Comment 253

4 days ago
Compare to the the try without Bug 1365133's changes[1], I think the current failures in the try[2] are not caused by the patch. They already exists in our code base. Let's land the patch.

[1]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=737f1c26213477209e68f25bef50b7044b353bca
[2]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c4e497383a09
Keywords: checkin-needed

Comment 254

4 days ago
Pushed by rchien@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/234095195036
Reorganize Preferences sections and regroup <xul:groupbox> elements by new categories - Part 1. r=jaws
https://hg.mozilla.org/integration/autoland/rev/0e007009a6be
Update tests - Part 2. r=jaws
Keywords: checkin-needed

Comment 255

4 days ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/234095195036
https://hg.mozilla.org/mozilla-central/rev/0e007009a6be
Status: ASSIGNED → RESOLVED
Last Resolved: 4 days ago
status-firefox56: --- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Blocks: 1381953
Depends on: 1382057
Depends on: 1382059
Depends on: 1382134
Depends on: 1382170
(Assignee)

Updated

2 days ago
Blocks: 1324172
Depends on: 1382660
You need to log in before you can comment on or make changes to this bug.