1551183 - Default theme is not present in the "Add-ons" section

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Add-on Manager, defect, P2)

Description

[Diana Rus]

Attachment: Missing Default Theme.png

Environment

Devices:
Samsung Galaxy Tab S3 (Android 8.0);
Samsung Galaxy S8 (Android 9);
Xiaomi MiPad 2 (Android 5.0.1) - x86-64 architecture;
Lenovo Yoga Tablet (Android 4.4.2) - x86 architecture.
Sony XPeria Z5 (Android 7.0.0)

Fennec-ESR builds:

Nightly 68.0a1;
Beta 68.0b1;

Steps to reproduce:

1. From Fennec custom menu go to "Add-ons" section.

Expected Result: The "Default theme" add-on is present in "Add-on Manager"

Actual Result: The "Default theme" add-on is missing from "Add-on Manager"

Comment 1

[Julien Cristau [:jcristau]]

Possibly related to bug 1525762?

Comment 2

[Ryan VanderMeulen [:RyanVM]]

I'm wondering if this is actually a regression from the Fennec ESR patches which have landed so far or just a general regression with Fx68-as-Beta. Can you please check an earlier 68-as-Beta build? Aryx should be able to help you find one from one of the sheriffs' simulations.

Comment 3

[Diana Rus]

Hi,

Tested on device Sony Xperia Z5 on environment Firefox Nightly: 68.0a1(2019-05-13) - Official Version, Fennec ESR - Nightly 68.0a1 - Trial Build, Fennec ESR - Beta 68.0b1 - Trial build

Did not reproduce on Firefox Beta, version 67.0b18 - Official Version.

At the moment I am doing a regression window, to see when the issue was introduced in the application.

Regards,
Diana

Comment 4

[Diana Rus]

Hi,

The moz-regression results on Firefox Nightly, 68.0a1(2019-03-30)

Last known good build: 2019-03-31 09:46:24.126000, changeset: 9b129ff965d75b364c9bdb4d737542cb4d1c40d1, pushlog

First known bad build: 2019-03-31, changeset: c06dfc552c647a6ce96f35cd84c32a589dc85608, pushlog

Regards,
Diana

Comment 5

[Petru-Mugurel Lingurar [:petru]]

Chris, can you look into this issue?
At fault for this is the 2a commit of bug 1525762. Basically LightweightThemeManager doesn't have a builtInTheme to show anymore.
I initially went with the idea to basically revert this change https://hg.mozilla.org/integration/mozilla-inbound/diff/b62c3bde4bc3f7cba452ad57f8d728b175c15a62/toolkit/mozapps/extensions/AddonManager.jsm
but I think you had other things in mind for the themes going forward.

Comment 6

[Kris Maglione [:kmag]]

(In reply to Petru-Mugurel Lingurar[:petru] from comment #5)

I initially went with the idea to basically revert this change https://hg.mozilla.org/integration/mozilla-inbound/diff/b62c3bde4bc3f7cba452ad57f8d728b175c15a62/toolkit/mozapps/extensions/AddonManager.jsm
but I think you had other things in mind for the themes going forward.

That won't work. LightweightThemeManager doesn't actually have any theme management functionality anymore. The default theme is now an ordinary static theme installed and managed by XPIProvider:

https://searchfox.org/mozilla-central/rev/94c6b5f06d2464f6780a52f32e917d25ddc30d6b/toolkit/mozapps/extensions/internal/XPIProvider.jsm#2269-2271

I'm not sure why it doesn't show up in Fennec, though. I'll look into it.

Comment 7

[Kris Maglione [:kmag]]

sigh The default theme is still winding up as an XPI on Fennec rather than being in omni.ja.

Glandium, any idea what's going on here?

Comment 8

[Mike Hommey [:glandium]]

(In reply to Kris Maglione [:kmag] from comment #7)

sigh The default theme is still winding up as an XPI on Fennec rather than being in omni.ja.

Glandium, any idea what's going on here?

Can you be more specific? I downloaded target.apk from the "bad" nightly from comment 4, and all I can see is what looks like a theme addon in modules/themes/default in assets/omni.ja that is not in a xpi.

Comment 9

[David Durst [:ddurst]]

Do you see it in the Add-ons section, though?

Comment 10

[Kris Maglione [:kmag]]

Huh, yeah, I must have been looking at a bad build. With a fresh local build or a fresh build from inbound, I see the resources being packaged correctly but the theme still not showing up in the add-ons list.

Comment 11

[Kris Maglione [:kmag]]

Attachment: Bug 1551183: Part 1 - Add SCOPE_APPLICATION to enabledScopes for Fennec. r=aswan

This scope now controls add-ons bundled in omni.ja as well as those in the app
directory, so we need to enable it on Fennec in order for the default theme to
work.

Comment 12

[Kris Maglione [:kmag]]

Attachment: Bug 1551183: Part 2 - Support enumerating directories in extensions in nested JARs. r=aswan

On Android, omni.ja is bundled inside an API, and therefore loaded as a nested
JAR. That means that its resource URIs resolve to something resembling
"jar:jar:...!/omni.ja!/...". Our current enumeration code assumes no nesting
of jar: URIs, and therefore can't handle this.

This patch changes our enumeration helpers to accept URIs rather than nsIFile
instances, and to correctly handle resolving the ZipReaders for those nested
JARs. It also moves the path filter generation into the native helper to make
things easier for other callers which may need this behavior.

Comment 13

[Kris Maglione [:kmag]]

Attachment: Bug 1551183: Part 3 - Add a test to sanity check the default theme on all platforms. r=aswan

Comment 14

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ebe8130889fb810feaa5013b1947b663887a4bf6
Bug 1551183: Part 1 - Add SCOPE_APPLICATION to enabledScopes for Fennec. r=aswan

https://hg.mozilla.org/integration/mozilla-inbound/rev/15532d30afff8e284823d850ea943e3cb909cf36
Bug 1551183: Part 2 - Support enumerating directories in extensions in nested JARs. r=aswan

https://hg.mozilla.org/integration/mozilla-inbound/rev/261cec1f88853d2707f1b6277b91add0f37690c4
Bug 1551183: Part 3 - Add a test to sanity check the default theme on all platforms. r=aswan

Comment 15

[Chris Peterson [:cpeterson]]

Kris, we'll want to uplift your fix to Fennec 68 Beta. Fennec will move to the ESR 68 channel, so 68 will be the last major version and we'll need to uplift or backport any Fennec fixes to 68.

Comment 16

[Kris Maglione [:kmag]]

I was planning to file an uplift request once the fix has been verified.

Comment 17

[Oana Pop-Rus]

https://hg.mozilla.org/mozilla-central/rev/ebe8130889fb
https://hg.mozilla.org/mozilla-central/rev/15532d30afff
https://hg.mozilla.org/mozilla-central/rev/261cec1f8885

Comment 18

[Diana Rus]

Hi,

I tested with device Samsung Galaxy S9, Android(8.0.0) and device: Samsung Galaxy S7 Edge, Android(8.0.0) on build Firefox Nightly 68.0a1(2019-05-30) - the default theme add-on is still missing. Video attached.

Comment 19

[Julien Cristau [:jcristau]]

This isn't in 68 yet. You'd need to try with an apk built from mozilla-central, e.g. https://queue.taskcluster.net/v1/task/RmpZHBzKQHyZVObYijEm1Q/runs/0/artifacts/public/build/target.apk for arm.

More at https://treeherder.mozilla.org/#/jobs?repo=mozilla-central&revision=63568b2a8178eca237b71cd661e2aaab3b6a360c&searchStr=build-android

Comment 20

[Laurentiu Apahidean]

Hello,

I have tested the issue on an Pixel 3 XL (Android 9) using the build from Comment 19 and can confirm that the Default theme is present in the add-ons section. When the fix will be available on the official Nightly build the issue will be retested.

Comment 21

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

(In reply to Laurentiu Apahidean from comment #20)

When the fix will be available on the official Nightly build the issue will be retested.

Beta is meant here?

Comment 22

[Laurentiu Apahidean]

(In reply to Sebastian Hengst [:aryx] (needinfo on intermittent or backout) from comment #21)

Beta is meant here?

Yes, that is correct.

Comment 23

[Kris Maglione [:kmag]]

Comment on attachment 9066868 [details]
Bug 1551183: Part 2 - Support enumerating directories in extensions in nested JARs. r=aswan

Beta/Release Uplift Approval Request

• User impact if declined: This issue prevents the default theme from being installed on Fennec. Users won't see it in about:addons, or be able to directly select it. If they have third-party themes installed, they will still be able to re-enable the default theme styling by deactivating the active one, but the platform expects to always have one theme enabled, and a fallback default theme to enable when a third-party theme is disabled, so there may be unexpected bugs associated with that setup.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Medium
• Why is the change risky/not risky? (and alternatives if risky): This fix requires a fairly significant change to the core logic of the zip file directory enumeration code used when installing extensions. That code is well tested, and the change should make it somewhat more robust, but it is nonetheless fairly key code.
• String changes made/needed: None

Comment 24

[Julien Cristau [:jcristau]]

Comment on attachment 9066868 [details]
Bug 1551183: Part 2 - Support enumerating directories in extensions in nested JARs. r=aswan

fix fennec default theme, approved for 68.0b7

Comment 25

[Alexandru Michis [:malexandru]]

https://hg.mozilla.org/releases/mozilla-beta/rev/868a040f89c3
https://hg.mozilla.org/releases/mozilla-beta/rev/1878395be29a
https://hg.mozilla.org/releases/mozilla-beta/rev/d31cfde1fa7b

Comment 26

[Stefan Hindli [:stefan_hindli]]

Backed out 3 changesets (Bug 1551183) for causing xpcshell permafailure in security/manager/ssl/tests/unit/test_cert_storage.js CLOSED TREE a=backout

https://treeherder.mozilla.org/#/jobs?repo=mozilla-beta&resultStatus=testfailed%2Cbusted%2Cexception&classifiedState=unclassified&selectedJob=249727779

https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=249727779&repo=mozilla-beta&lineNumber=1546

[task 2019-06-03T15:56:55.588Z] 15:56:55 INFO - TEST-SKIP | netwerk/test/unit_ipc/test_trackingProtection_annotateChannels_wrap1.js | took 0ms
[task 2019-06-03T15:56:55.588Z] 15:56:55 INFO - TEST-START | netwerk/test/unit_ipc/test_trackingProtection_annotateChannels_wrap2.js
[task 2019-06-03T15:56:55.588Z] 15:56:55 INFO - TEST-SKIP | netwerk/test/unit_ipc/test_trackingProtection_annotateChannels_wrap2.js | took 0ms
[task 2019-06-03T15:56:55.588Z] 15:56:55 INFO - TEST-START | netwerk/test/unit_ipc/test_channel_priority_wrap.js
[task 2019-06-03T15:56:55.588Z] 15:56:55 INFO - TEST-SKIP | netwerk/test/unit_ipc/test_channel_priority_wrap.js | took 0ms
[task 2019-06-03T15:56:55.588Z] 15:56:55 INFO - TEST-START | netwerk/test/unit_ipc/test_multipart_streamconv_wrap.js
[task 2019-06-03T15:56:55.588Z] 15:56:55 INFO - TEST-SKIP | netwerk/test/unit_ipc/test_multipart_streamconv_wrap.js | took 0ms
[task 2019-06-03T15:56:55.588Z] 15:56:55 INFO - TEST-START | netwerk/test/unit_ipc/test_alt-data_cross_process_wrap.js
[task 2019-06-03T15:56:55.589Z] 15:56:55 INFO - TEST-SKIP | netwerk/test/unit_ipc/test_alt-data_cross_process_wrap.js | took 1ms
[task 2019-06-03T15:56:56.316Z] 15:56:56 INFO - TEST-START | parser/xml/test/unit/test_sanitizer.js
[task 2019-06-03T15:56:56.942Z] 15:56:56 INFO - TEST-PASS | parser/xml/test/unit/test_sanitizer.js | took 628ms
[task 2019-06-03T15:56:58.290Z] 15:56:58 INFO - TEST-START | security/manager/ssl/tests/unit/test_add_preexisting_cert.js
[task 2019-06-03T15:56:58.898Z] 15:56:58 INFO - TEST-PASS | security/manager/ssl/tests/unit/test_add_preexisting_cert.js | took 607ms
[task 2019-06-03T15:57:00.233Z] 15:57:00 INFO - TEST-START | security/manager/ssl/tests/unit/test_baseline_requirements_subject_common_name.js
[task 2019-06-03T15:57:00.742Z] 15:57:00 INFO - TEST-PASS | security/manager/ssl/tests/unit/test_baseline_requirements_subject_common_name.js | took 510ms
[task 2019-06-03T15:57:01.361Z] 15:57:01 INFO - TEST-START | security/manager/ssl/tests/unit/test_broken_fips.js
[task 2019-06-03T15:57:01.361Z] 15:57:01 INFO - TEST-SKIP | security/manager/ssl/tests/unit/test_broken_fips.js | took 0ms
[task 2019-06-03T15:57:02.083Z] 15:57:02 INFO - TEST-START | security/manager/ssl/tests/unit/test_cert_storage.js
[task 2019-06-03T15:57:02.989Z] 15:57:02 WARNING - TEST-UNEXPECTED-FAIL | security/manager/ssl/tests/unit/test_cert_storage.js | xpcshell return code: 0
[task 2019-06-03T15:57:02.990Z] 15:57:02 INFO - TEST-INFO took 907ms
[task 2019-06-03T15:57:02.990Z] 15:57:02 INFO - >>>>>>>
[task 2019-06-03T15:57:02.990Z] 15:57:02 INFO - security/manager/ssl/tests/unit/test_cert_storage.js | xpcw: cd /sdcard/tests/xpc/security/manager/ssl/tests/unit
[task 2019-06-03T15:57:02.991Z] 15:57:02 INFO - security/manager/ssl/tests/unit/test_cert_storage.js | xpcw: xpcshell -r /sdcard/tests/xpc/c/httpd.manifest --greomni /data/local/xpcb/target.apk -m -s -e const _HEAD_JS_PATH = "/sdcard/tests/xpc/head.js"; -e const _MOZINFO_JS_PATH = "/sdcard/tests/xpc/p/mozinfo.json"; -e const _PREFS_FILE = "/sdcard/tests/xpc/user.js"; -e const _TESTING_MODULES_DIR = "/sdcard/tests/xpc/m"; -f /sdcard/tests/xpc/head.js -e const _HEAD_FILES = ["/sdcard/tests/xpc/security/manager/ssl/tests/unit/head_psm.js"]; -e const _JSDEBUGGER_PORT = 0; -e const _TEST_FILE = ["test_cert_storage.js"]; -e const _TEST_NAME = "security/manager/ssl/tests/unit/test_cert_storage.js"; -e _execute_test(); quit(0);
[task 2019-06-03T15:57:02.991Z] 15:57:02 INFO - security/manager/ssl/tests/unit/test_cert_storage.js | JavaScript error: resource://gre/modules/ExtensionProcessScript.jsm, line 159: NS_ERROR_FAILURE:
[task 2019-06-03T15:57:02.992Z] 15:57:02 INFO - security/manager/ssl/tests/unit/test_cert_storage.js | 1559577421690 addons.xpi WARN List of valid built-in add-ons could not be parsed.: [Exception... "Component returned failure code: 0xc1f30001 (NS_ERROR_NOT_INITIALIZED) [nsIXPCComponents_Utils.readUTF8URI]" nsresult: "0xc1f30001 (NS_ERROR_NOT_INITIALIZED)" location: "JS frame :: resource://gre/modules/addons/XPIProvider.jsm :: startup :: line 2243" data: no] Stack trace: startup()@resource://gre/modules/addons/XPIProvider.jsm:2243
[task 2019-06-03T15:57:02.992Z] 15:57:02 INFO - security/manager/ssl/tests/unit/test_cert_storage.js | callProvider()@resource://gre/modules/AddonManager.jsm:193
[task 2019-06-03T15:57:02.992Z] 15:57:02 INFO - security/manager/ssl/tests/unit/test_cert_storage.js | _startProvider()@resource://gre/modules/AddonManager.jsm:568
[task 2019-06-03T15:57:02.992Z] 15:57:02 INFO - security/manager/ssl/tests/unit/test_cert_storage.js | startup()@resource://gre/modules/AddonManager.jsm:723
[task 2019-06-03T15:57:02.992Z] 15:57:02 INFO - security/manager/ssl/tests/unit/test_cert_storage.js | startup()@resource://gre/modules/AddonManager.jsm:2787
[task 2019-06-03T15:57:02.996Z] 15:57:02 INFO - security/manager/ssl/tests/unit/test_cert_storage.js | observe()@resource://gre/modules/addonManager.js:65
[task 2019-06-03T15:57:02.996Z] 15:57:02 INFO - security/manager/ssl/tests/unit/test_cert_storage.js | test_cert_storage.js:163
[task 2019-06-03T15:57:02.996Z] 15:57:02 INFO - security/manager/ssl/tests/unit/test_cert_storage.js | load_file()@/sdcard/tests/xpc/head.js:637
[task 2019-06-03T15:57:02.996Z] 15:57:02 INFO - security/manager/ssl/tests/unit/test_cert_storage.js | _load_files()@/sdcard/tests/xpc/head.js:649
[task 2019-06-03T15:57:02.996Z] 15:57:02 INFO - security/manager/ssl/tests/unit/test_cert_storage.js | _execute_test()@/sdcard/tests/xpc/head.js:503
[task 2019-06-03T15:57:02.996Z] 15:57:02 INFO - security/manager/ssl/tests/unit/test_cert_storage.js | -e:1
[task 2019-06-03T15:57:02.996Z] 15:57:02 INFO - (xpcshell/head.js) | test MAIN run_test pending (1)
[task 2019-06-03T15:57:02.996Z] 15:57:02 INFO - (xpcshell/head.js) | test run_next_test 0 pending (2)
[task 2019-06-03T15:57:02.996Z] 15:57:02 INFO - (xpcshell/head.js) | test MAIN run_test finished (2)
[task 2019-06-03T15:57:02.996Z] 15:57:02 INFO - running event loop

Comment 27

[Kris Maglione [:kmag]]

https://hg.mozilla.org/releases/mozilla-beta/rev/ffa52c63ff472d43c6b81170a32dc98b05cc7dde
https://hg.mozilla.org/releases/mozilla-beta/rev/76077274869a12283e79ab53491fff41264efda8
https://hg.mozilla.org/releases/mozilla-beta/rev/259f5bcec259ed020c3612b4278c2ffa184ec77f
https://hg.mozilla.org/releases/mozilla-beta/rev/092bef8c0d6e514b3ad85c6d17a7c66531f63749

Comment 28

[Natalia Csoregi [:nataliaCs]]

Backed out for failures on test_default_theme.html

backout: https://hg.mozilla.org/releases/mozilla-beta/rev/3b4af55e6edc19cd481737216fd5616d28bb61b0

push: https://treeherder.mozilla.org/#/jobs?repo=mozilla-beta&revision=092bef8c0d6e514b3ad85c6d17a7c66531f63749&searchStr=m-1proc%28c3%29&selectedJob=250503130

failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=250503130&repo=mozilla-beta&lineNumber=4886

23:40:26 INFO - TEST-PASS | toolkit/mozapps/extensions/test/mochitest/test_default_theme.html | Add-on type is correct
23:40:26 INFO - Buffered messages finished
23:40:26 INFO - TEST-UNEXPECTED-FAIL | toolkit/mozapps/extensions/test/mochitest/test_default_theme.html | Add-on is active - got false, expected true
23:40:26 INFO - SimpleTest.is@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:320:16
23:40:26 INFO - @chrome://mochitests/content/chrome/toolkit/mozapps/extensions/test/mochitest/test_default_theme.html:22:3
23:40:26 INFO - TEST-PASS | toolkit/mozapps/extensions/test/mochitest/test_default_theme.html | Add-on is not hidden

Comment 29

[Kris Maglione [:kmag]]

https://hg.mozilla.org/releases/mozilla-beta/rev/9b79e08b7ac02dd5c6dd703e64942d09829efc69
https://hg.mozilla.org/releases/mozilla-beta/rev/1118e4a2eb8b0e64e0dd11ca223e411e684540f8
https://hg.mozilla.org/releases/mozilla-beta/rev/09624608fc97fc4b52a317f965a75aebf29073c9
https://hg.mozilla.org/releases/mozilla-beta/rev/49a329216f65a28acdcaa77729030d7b22fadfb5

Comment 30

[Eliza Balazs]

Verified as fixed on Beta 68.0b9 with Nexus 6P (Android 8.1.0) and Samsung Galaxy S8 (Android 9).
Due to my findings, I'll mark this issue as verified on Firefox 68. Thanks.