1139958 - Start using AppConstants.jsm in Toolkit

Status: RESOLVED FIXED

(Toolkit :: General, defect)

Description

[Philip Chee]

+++ This bug was initially created as a clone of Bug #1124503 +++

Bug 1124503 moved AppConstants.jsm to toolkit. Now we can use it in Toolkit and other places.

Comment 1

[Philip Chee]

Attachment: V1 WIP patch

Need to investigate some failures on mozilla-central-try

Comment 2

[Philip Chee]

Attachment: Patch v2 passes try-server tests

Try server results https://treeherder.mozilla.org/#/jobs?repo=try&revision=cd9f27170a7d
> +++ b/testing/mochitest/browser-test.js

>  XPCOMUtils.defineLazyModuleGetter(this, "ContentSearch",
> 
>          // Uninitialize a few things explicitly so that they can clean up
>          // frames and browser intentionally kept alive until shutdown to
>          // eliminate false positives.
>          if (gConfig.testRoot == "browser") {
> -          // Replace the document currently loaded in the browser's sidebar.
> -          // This will prevent false positives for tests that were the last
> -          // to touch the sidebar. They will thus not be blamed for leaking
> -          // a document.
> -          let sidebar = document.getElementById("sidebar");
> -          sidebar.setAttribute("src", "data:text/html;charset=utf-8,");
> -          sidebar.docShell.createAboutBlankContentViewer(null);
> -          sidebar.setAttribute("src", "about:blank");
> +          // Skip if not Firefox e.g. SeaMonkey.
> +          if (AppConstants.MOZ_APP_NAME == "browser") {
I build SeaMonkey trunk locally and was using it to test this patch.
This change is needed to prevent browser-test.js causing errors due to Firefox specific code. Plus it uses AppConstants!
e.g. These three modules only exist in browser/

>   "resource:///modules/CustomizationTabPreloader.jsm");
>   "resource:///modules/ContentSearch.jsm");
>   "resource:///modules/SelfSupportBackend.jsm");

And several node.id's only exist in Firefox.

> +  MOZ_WIDGET_GTK:
> +#ifdef MOZ_WIDGET_GTK
Needed for some code that is GTK specific and not just Linux.

> --- a/toolkit/modules/GMPInstallManager.jsm
> +++ b/toolkit/modules/GMPInstallManager.jsm
This part of the patch is a P.I.T.A. will attach a diff -wb for review purposes.

> +++ b/toolkit/modules/Troubleshoot.jsm

> -#if defined(XP_LINUX) && defined (MOZ_SANDBOX)
> -  sandbox: function sandbox(done) {
> +if (AppConstants.MOZ_CRASHREPORTER) {
> +  dataProviders.crashes = function crashes(done) {
> +    let CrashReports = Cu.import("resource://gre/modules/CrashReports.jsm").CrashReports;
> +    let reports = CrashReports.getReports();
> +    let now = new Date();
> +    let reportsNew = reports.filter(report => (now - report.date < Troubleshoot.kMaxCrashAge));
> +    let reportsSubmitted = reportsNew.filter(report => (!report.pending));
> +    let reportsPendingCount = reportsNew.length - reportsSubmitted.length;
> +    let data = {submitted : reportsSubmitted, pending : reportsPendingCount};
> +    done(data);
> +  }
> +}
> +
> +if (AppConstants.platform == "linux" && AppConstants.MOZ_SANDBOX) {
> +  dataProviders.sandbox = function sandbox(done) {
>      const keys = ["hasSeccompBPF", "hasSeccompTSync",
>                    "hasPrivilegedUserNamespaces", "hasUserNamespaces",
>                    "canSandboxContent", "canSandboxMedia"];
>  
>      let sysInfo = Cc["@mozilla.org/system-info;1"].
>                    getService(Ci.nsIPropertyBag2);
>      let data = {};
>      for (let key of keys) {
>        if (sysInfo.hasKey(key)) {
>          data[key] = sysInfo.getPropertyAsBool(key);
>        }
>      }
>      done(data);
>    }
> -#endif
> -};
> +}

Had to do it this way to make browser_Troubleshoot.js pass on all platforms tested. This keeps the current semantics in case any platform specific code depends on the presence/absence of these dataProviders. (Plus it was less messy to fix this in Troubleshoot.jsm than in the test)

> +++ b/toolkit/modules/secondscreen/RokuApp.jsm

> -#ifdef RELEASE_BUILD
> -  this.app = "Firefox";
> -#else
> -  this.app = "Firefox Nightly";
> -#endif
> +  this.app = AppConstants.RELEASE_BUILD ? "Firefox" : "Firefox Nightly";
I could use MOZ_APP_DISPLAY_NAME but I didn't want to accidentally break anything that might be hard-coded to look for these strings.

Comment 3

[Philip Chee]

Attachment: qdiff -wb for review purposes. !DO NOT CHECK THIS IN!

hg qdiff -gpbU8 for review purposes only especially for GMPInstallManager.jsm

Comment 4

[Philip Chee]

I did not touch WindowsPrefSync.jsm because Metro specific code is going to be removed from mozilla-central and moved to a separate repository.

Comment 5

[Dave Townsend [:mossop]]

Comment on attachment 8582410 [details] [diff] [review]
qdiff -wb for review purposes. !DO NOT CHECK THIS IN!

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

Just one change I'd like to see here

::: toolkit/modules/AppConstants.jsm
@@ +113,5 @@
> +#ifdef MOZ_WIDGET_GTK
> +  true,
> +#else
> +  false,
> +#endif

To avoid escalation of constants I think I'd rather just expose MOZ_WIDGET_TOOLKIT and test against /^gtk/

Comment 6

[Philip Chee]

Attachment: Patch v3 using AppConstants.MOZ_WIDGET_TOOLKIT

try-server: https://treeherder.mozilla.org/#/jobs?repo=try&revision=cee1509fabe1
All green except two known intermittents.

> To avoid escalation of constants I think I'd rather just expose 
> MOZ_WIDGET_TOOLKIT and test against /^gtk/
Fixed, now using AppConstants.MOZ_WIDGET_TOOLKIT and test against /^gtk/

Comment 7

[Dave Townsend [:mossop]]

Comment on attachment 8584662 [details] [diff] [review]
Patch v3 using AppConstants.MOZ_WIDGET_TOOLKIT

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

Thanks!

Comment 8

[Philip Chee]

inbound: http://hg.mozilla.org/integration/mozilla-inbound/rev/764b057e1c3f

Comment 9

[Philip Chee]

Backed out due to leaking windows all over the place.
https://hg.mozilla.org/integration/mozilla-inbound/rev/0a7fd420b3eb

Comment 10

[Philip Chee]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ce60e625e6e6 
https://treeherder.mozilla.org/#/jobs?repo=try&revision=49a03ed3f724

Hi! Need some assistance. I'm getting consistent XPC shell failures on:
Android 2.3 API9 opt
Android 4.0 API11+ opt
Android 4.0 API11+ debug

https://treeherder.mozilla.org/logviewer.html#?job_id=6151460&repo=try
https://treeherder.mozilla.org/logviewer.html#?job_id=6151640&repo=try
https://treeherder.mozilla.org/logviewer.html#?job_id=6152416&repo=try
https://treeherder.mozilla.org/logviewer.html#?job_id=6151640&repo=try

Need help from an Android developer. Who can I CC?

Comment 11

[Philip Chee]

Errors just say:

05:20:48 WARNING - TEST-UNEXPECTED-FAIL | services/common/tests/unit/test_load_modules.js | xpcshell return code: 0
05:20:48 INFO - TEST-INFO took 2866ms
05:20:48 INFO - >>>>>>>
05:20:48 INFO - PROCESS | services/common/tests/unit/test_load_modules.js | xpcw: cd /mnt/sdcard/tests/xpcshell/services/common/tests/unit
05:20:48 INFO - PROCESS | services/common/tests/unit/test_load_modules.js | xpcw: xpcshell -r /mnt/sdcard/tests/xpcshell/c/httpd.manifest --greomni /data/local/xpcb/fennec-40.0a1.en-US.android-arm.apk -m -s -e const _HTTPD_JS_PATH = "/mnt/sdcard/tests/xpcshell/c/httpd.js"; -e const _HEAD_JS_PATH = "/mnt/sdcard/tests/xpcshell/head.js"; -e const _TESTING_MODULES_DIR = "/mnt/sdcard/tests/xpcshell/m"; -f /mnt/sdcard/tests/xpcshell/head.js -e const _SERVER_ADDR = "localhost" -e const _HEAD_FILES = ["/mnt/sdcard/tests/xpcshell/services/common/tests/unit/head_global.js", "/mnt/sdcard/tests/xpcshell/services/common/tests/unit/head_helpers.js", "/mnt/sdcard/tests/xpcshell/services/common/tests/unit/head_http.js"]; -e const _TAIL_FILES = []; -e const _JSDEBUGGER_PORT = 0; -e const _TEST_FILE = ["test_load_modules.js"]; -e const _TEST_NAME = "services/common/tests/unit/test_load_modules.js" -e _execute_test(); quit(0);
05:20:48 INFO - PROCESS | services/common/tests/unit/test_load_modules.js | [25249] WARNING: XPCOM objects created/destroyed from static ctor/dtor: file /builds/slave/try-and-api-11-d-0000000000000/build/xpcom/base/nsTraceRefcnt.cpp, line 132
05:20:48 INFO - PROCESS | services/common/tests/unit/test_load_modules.js | [25249] WARNING: Couldn't get the user appdata directory. Crash events may not be produced.: file /builds/slave/try-and-api-11-d-0000000000000/build/toolkit/crashreporter/nsExceptionHandler.cpp, line 2333
05:20:48 INFO - (xpcshell/head.js) | test MAIN run_test pending (1)
05:20:48 INFO - NS_ERROR_FILE_NOT_FOUND: Component returned failure code: 0x80520012 (NS_ERROR_FILE_NOT_FOUND) [nsIXPCComponents_Utils.import]
05:20:48 INFO - expectImportsToSucceed@test_load_modules.js:33:5
05:20:48 INFO - run_test@test_load_modules.js:57:5
05:20:48 INFO - _execute_test@/mnt/sdcard/tests/xpcshell/head.js:504:5
05:20:48 INFO - @-e:1:1
05:20:48 INFO - "CONSOLE_MESSAGE: (warn) [JavaScript Warning: "ReferenceError: assignment to undeclared variable base" {file: "test_load_modules.js" line: 54}]"
05:20:48 INFO - PROCESS | services/common/tests/unit/test_load_modules.js | JavaScript strict warning: test_load_modules.js, line 54: ReferenceError: assignment to undeclared variable base
05:20:48 INFO - PROCESS | services/common/tests/unit/test_load_modules.js | [25249] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80520012: file /builds/slave/try-and-api-11-d-0000000000000/build/js/xpconnect/loader/mozJSComponentLoader.cpp, line 827
05:20:48 INFO - PROCESS | services/common/tests/unit/test_load_modules.js | nsStringStats
05:20:48 INFO - PROCESS | services/common/tests/unit/test_load_modules.js | => mAllocCount: 5233
05:20:48 INFO - PROCESS | services/common/tests/unit/test_load_modules.js | => mReallocCount: 244
05:20:48 INFO - PROCESS | services/common/tests/unit/test_load_modules.js | => mFreeCount: 5233
05:20:48 INFO - PROCESS | services/common/tests/unit/test_load_modules.js | => mShareCount: 6175
05:20:48 INFO - PROCESS | services/common/tests/unit/test_load_modules.js | => mAdoptCount: 344
05:20:48 INFO - PROCESS | services/common/tests/unit/test_load_modules.js | => mAdoptFreeCount: 344
05:20:48 INFO - PROCESS | services/common/tests/unit/test_load_modules.js | => Process ID: 25249, Thread ID: 1074197668
05:20:48 INFO - PROCESS | services/common/tests/unit/test_load_modules.js | [25249] WARNING: OOPDeinit() without successful OOPInit(): file /builds/slave/try-and-api-11-d-0000000000000/build/toolkit/crashreporter/nsExceptionHandler.cpp, line 2795
05:20:48 INFO - PROCESS | services/common/tests/unit/test_load_modules.js | [25249] WARNING: XPCOM objects created/destroyed from static ctor/dtor: file /builds/slave/try-and-api-11-d-0000000000000/build/xpcom/base/nsTraceRefcnt.cpp, line 132
05:20:48 INFO - PROCESS | services/common/tests/unit/test_load_modules.js | [25249] WARNING: XPCOM objects created/destroyed from static ctor/dtor: file /builds/slave/try-and-api-11-d-0000000000000/build/xpcom/base/nsTraceRefcnt.cpp, line 132
05:20:48 INFO - PROCESS | services/common/tests/unit/test_load_modules.js | [25249] WARNING: XPCOM objects created/destroyed from static ctor/dtor: file /builds/slave/try-and-api-11-d-0000000000000/build/xpcom/base/nsTraceRefcnt.cpp, line 132
05:20:48 INFO - PROCESS | services/common/tests/unit/test_load_modules.js | \x00
05:20:48 INFO - <<<<<<<

Comment 12

[Philip Chee]

I *think* there's a bug or two in the test [test_load_modules.js]

Comment 13

[Philip Chee]

Attachment: Patch v3a fix test_load_modules.js

This patch fixes the error in /services/common/tests/unit/test_load_modules.js

>  #elif MOZ_WIDGET_ANDROID
>    "android",
>  #elif MOZ_WIDGET_GONK
>    "gonk",
> +#elif XP_LINUX
> +  "linux",

XP_LINUX has to come after MOZ_WIDGET_ANDROID otherwise Android builds will be identified as linux.

Now that test_load_modules.js is fixed try-server build is failing in test_compartments.js. Grrrr.

Comment 14

[Philip Chee]

Attachment: Patch v3b fix test_load_modules.js - patch for review

Try Server results:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=86029fe3ce2a
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4bee8552324e

Asking for another review because I've made certain changes to fix try-server failures.

> +++ b/services/common/tests/unit/test_load_modules.js
There was a bug in my code but tracking it down was difficult as test_load_modules.js itself was buggy and generating too much noise making it hard to figure what my problem was. So I fixed the test as well and made the error messages clearer.

> +++ b/testing/mochitest/browser-test.js

> +          //Skip if SeaMonkey
> +          if (AppConstants.MOZ_APP_NAME != "seamonkey") {
I was testing for "browser" when I should be testing for "firefox". I reversed the test so that != "seamonkey". This should make it more unlikely to break anyone else.

>  #elif XP_WIN
>    "win",
>  #elif XP_MACOSX
>    "macosx",
>  #elif MOZ_WIDGET_ANDROID
>    "android",
>  #elif MOZ_WIDGET_GONK
>    "gonk",
> +#elif XP_LINUX
> +  "linux",
>  #else
>    "other",
>  #endif
XP_LINUX has to go at the end after MOZ_WIDGET_ANDROID otherwise android builds will be misdetected as linux. I should put a comment here otherwise someone might come along and "optimize" the arrangement.

> +++ b/toolkit/modules/UpdateChannel.jsm
I have removed this part from the patch because I was I was spending too much time trying to track down why it was causing try server failures. I'll file another bug for this. Probably.

Comment 15

[Dave Townsend [:mossop]]

Comment on attachment 8588066 [details] [diff] [review]
Patch v3b fix test_load_modules.js - patch for review

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

Please do add that comment

::: services/common/tests/unit/test_load_modules.js
@@ +58,5 @@
>  }
>  
>  function run_test() {
>    expectImportsToSucceed(shared_modules);
> +  expectImportsToSucceed(shared_test_modules, TEST_BASE);

Huh, someone has clearly been doing too much python

Comment 16

[Philip Chee]

http://hg.mozilla.org/integration/mozilla-inbound/rev/0149e6ca4cb3

Comment 17

[Philip Chee]

(In reply to Philip Chee from comment #16)
> http://hg.mozilla.org/integration/mozilla-inbound/rev/0149e6ca4cb
Forgot to add a comment as promised.
http://hg.mozilla.org/integration/mozilla-inbound/rev/a97790f1aa9f

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0149e6ca4cb3
https://hg.mozilla.org/mozilla-central/rev/a97790f1aa9f