1647931 - Switching to dark or light theme in the Add-On Manager is broken - Uncaught (in promise) TypeError: addon.userPermissions is null

Status: RESOLVED FIXED

(Toolkit :: Add-ons Manager, defect, P3)

Description

[Geoff Lankow (:darktrojan)]

Here's what happens if I go to the Add-On Manager and click enable on one of the built-in themes:

Uncaught (in promise) TypeError: addon.userPermissions is null
    shouldShowPermissionsPrompt chrome://mozapps/content/extensions/aboutaddonsCommon.js:156
    handleEvent chrome://mozapps/content/extensions/aboutaddons.js:2814

This doesn't seem to affect 78.

Comment 1

[Geoff Lankow (:darktrojan)]

It also happens if I install a theme from ATN then try to return to the default theme by clicking Enable. Switching between ATN themes works, as does disabling the current theme to return to the default theme.

Comment 2

[Richard Marti (:Paenglab)]

Theme switching works for me. But I saw other mentioning this issue earlier and after some time it was gone away. Have you tried another profile?

Comment 3

[Wayne Mery (:wsmwk)]

I'm seeing this on Mac (first time I've tried dark mode there)

Uncaught (in promise) TypeError: addon.userPermissions is null
shouldShowPermissionsPrompt chrome://mozapps/content/extensions/aboutaddonsCommon.js:156
handleEvent chrome://mozapps/content/extensions/aboutaddons.js:2814
aboutaddonsCommon.js:156:36

Comment 4

[Wayne Mery (:wsmwk)]

Tested in "empty" profile. No error - dark enables fine.

Comment 5

[Wayne Mery (:wsmwk)]

In the problem profile (with many accounts) I see the following items that I don't see in the empty profile:

• 1595313308408 addons.xpi DEBUG Loaded add-on state: {"app-profile":{"addons":{"[e-mail]":{"dependencies":[],"enabled":false,"lastModifiedTime":1500209962364,"path":"[e-mail]","rootURI":"jar:file:///Users/wsm0/Library/Thunderbird/Profiles/q1j95wuf.default/extensions/[e-mail]!/","runInSafeMode":false,"signedState":2,"telemetryKey":"gpo%40extensions.org:0.6.7.2","version":"0.6.7.2"},"labelhref@test":{"dependencies":[],"enabled":false,"lastModifiedTime":1356320846000,"path":"[e-mail]","rootURI":"jar:file:///Users/wsm0/Library/Thunderbird/Profiles/q1j95wuf.default/extensions/[e-mail]!/","runInSafeMode":false,"signedState":0,"telemetryKey":"labelhref%40test:1.0","version":"1.0"},"[e-mail]":{"dependencies":[],"enabled":false,"lastModifiedTime":1574200968554,"loader":null,"path":"[e-mail]","rootURI":"jar:file:///Users/wsm0/Library/Thunderbird/Profiles/q1j95wuf.default/extensions/ma…

• Error while loading 'jar:file:///Users/wsm0/Library/Thunderbird/Profiles/q1j95wuf.default/extensions/[e-mail]!/manifest.json' (NS_ERROR_FILE_NOT_FOUND)

• [Exception... "Component returned failure code: 0x80520001 (NS_ERROR_FILE_UNRECOGNIZED_PATH) [nsIXPCComponents_Utils.readUTF8URI]" nsresult: "0x80520001 (NS_ERROR_FILE_UNRECOGNIZED_PATH)" location: "JS frame :: resource://gre/modules/L10nRegistry.jsm :: L10nRegistry.loadSync :: line 684" data: no]

• [Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIStringBundle.GetStringFromName]" nsresult: "0x80004005 (NS_ERROR_FAILURE)" location: "JS frame :: chrome://messenger/content/about-support/aboutSupport.js :: application :: line 236" data: no]

• Uncaught (in promise) undefined

• InvisibleToDebugger: Error reps.js:8527:15

Comment 6

[Magnus Melin [:mkmelin]]

So incompatible mailsummaries and crashme add-ons is the cause?

Comment 7

[Geoff Lankow (:darktrojan)]

I found that the extensions.json file in my profile had:

"userPermissions": null,
"optionalPermissions": null,

for the themes. If I changed them to:

"userPermissions": {
  "permissions": [],
  "origins": []
},
"optionalPermissions": {
  "permissions": [],
  "origins": []
},

I could again load the themes in my test profile (Daily). However, the values are also null in my normal profile (79.0b2), and that works fine.

Comment 8

[Wayne Mery (:wsmwk)]

Is there a path forward?

Comment 9

[Wayne Mery (:wsmwk)]

Testers (including me) cannot test theme issues.
Who can be assigned to solve this?
Do we need a regression range?

Comment 10

[Magnus Melin [:mkmelin]]

John, please investigate.

Comment 11

[John Bieling (:TbSync)]

I was testing 82.0a1 (2020-09-03) (64-bit) on Windows 10. All themes (built-in or installed from ATN) have

"userPermissions": null,

BUT I can switch between them without any issue, in old and new profiles. So I cannot reproduce comment 7. It works for me even with "null".

What we can do is to search for things like this:
https://searchfox.org/mozilla-central/source/toolkit/mozapps/extensions/content/aboutaddonsCommon.js#167

and fix those, as that obviously throws if userPermissions is null. It seems to be the only such usage;

Should I file a bug in m-c?

For further tests:
@Geoff + @ Wayne :

• Are you both on Mac?

@Geoff:

• Can you still reproduce this with latest Daily in your profile, when you switch back to null?

@ Wayne:

• You said it does not happen on a fresh profile, is there anything you can do to break a fresh profile? Install a certain add-on? Where did you get a working version from crashme?

Comment 12

[Magnus Melin [:mkmelin]]

You can just move this bug to the proper component if it's a problem m-c code error.

Comment 13

[John Bieling (:TbSync)]

I would at least need confirmation that this is the cause.

@Geoff + Wayne:

• You can still reproduce the issue with one of your broken profiles with current tip? If so, I can create patch and you try if it resolves the issue?

Comment 14

[Geoff Lankow (:darktrojan)]

Yes I've still got this problem and no I'm not on Mac (Linux). Dunno if it helps, but I have other non-default themes with null user permissions, and they work fine.

Comment 15

[John Bieling (:TbSync)]

Attachment: userPermissionsNull.patch

Patch to check if userPermissions is null.

Comment 16

[John Bieling (:TbSync)]

@Geoff: Could you give it a spin?

Comment 17

[Geoff Lankow (:darktrojan)]

Works for me.

Comment 18

[John Bieling (:TbSync)]

As suggested by Magnus I moved this bug into the toolkit component. Who can I ask for review?

Comment 19

[Magnus Melin [:mkmelin]]

Looking at the blame, r?mstriemer
https://hg.mozilla.org/mozilla-central/log/tip/toolkit/mozapps/extensions/content/aboutaddonsCommon.js

Comment 20

[John Bieling (:TbSync)]

Attachment: Bug 1647931 - Fix addon.userPermissions is null

Comment 21

[John Bieling (:TbSync)]

Attachment: Bug 1647931 - Fix addon.userPermissions is null r?mixedpuppy

Comment 22

[John Bieling (:TbSync)]

@mixedpuppy: As you suggested, I changed the patch to the nicer syntax without using isArray, but still had to provide a dummy userPermissions obj as {} would still cause the expansion to fail. But this is not checking for all possible input conditions. I would therefore propose to remove the expension altogether and just use this return statement:

return !!addon.userPermissions?.origins?.length 
    || !!addon.userPermissions?.permissions?.length;

So for origins and permissions, it does not matter if none, one or both have been defined or are of the wrong type.

I wanted to ask about your opinion before replacing the patch.

Comment 24

[John Bieling (:TbSync)]

Is there anything I can do to move this forward? I changed the patch as requested by the original reviewer. Is it ok?

Comment 25

[Wayne Mery (:wsmwk)]

(In reply to John Bieling (:TbSync) from comment #24)

Is there anything I can do to move this forward? I changed the patch as requested by the original reviewer. Is it ok?

Comment 26

[Clara Guerrero ( Need Info Brindusa Tot Please)]

Hi, as per comment 9 do you need help getting a regression range? Can I get some steps on how to reproduce in order to give you a hand?
Best,
Clara

Comment 27

[Wayne Mery (:wsmwk)]

We only need mixedpuppy's assistance. But thanks for offering to help.

Comment 28

[Shane Caraveo (:mixedpuppy)]

This dropped through the cracks, sorry. I've responded on the patch.

Comment 29

[John Bieling (:TbSync)]

@mixedpuppy: Thanks for the review. Yes, the Null Propagation Operator / Optional Chaining Operator is much better. Working on the test now.

Comment 30

[John Bieling (:TbSync)]

Attachment: Bug 1647931 - Fix addon.userPermissions is null r?mixedpuppy

Comment 31

[Shane Caraveo (:mixedpuppy)]

Hi John, are you able to update patches rather than push a new patch? Also the last patch didn't have a test in it.

Comment 32

[John Bieling (:TbSync)]

The phabricator work flow is pretty new to me. I will try to learn how to update a patch.

I have send the updated patch to not stall this bug. But I struggle to write a test, as for me, the crucial part is never executed as addon.seen is always true. The function always exits early. You want a test that simulated theme enabled/disabled?

Comment 33

[Shane Caraveo (:mixedpuppy)]

(In reply to John Bieling (:TbSync) from comment #32)

The phabricator work flow is pretty new to me. I will try to learn how to update a patch.

If you have arc working, you can do this:

edit files
hg commit -m "Bug XXX description
arc diff .^   (add push description in editor)

The to update:

edit files
hg commit --amend (just save when editor comes up)
arc diff .^  (add description for new push the save)

If you want to pull an existing phabricator patch:

hg up central (or specific commit)
arc patch Dnnnn

I have send the updated patch to not stall this bug. But I struggle to write a test, as for me, the crucial part is never executed as addon.seen is always true. The function always exits early. You want a test that simulated theme enabled/disabled?

If you can update the patch per the above with the test, I can take a look at what you are doing and make some suggestions.

Thanks!

Comment 34

[Shane Caraveo (:mixedpuppy)]

Hi John, just touching base to see if you're still stuck.

Comment 35

[John Bieling (:TbSync)]

Still stuck, learning how to write tests.

Comment 36

[John Bieling (:TbSync)]

So I gave it a try to update the revision using the documentation I found here:
https://github.com/mozilla-conduit/review/blob/master/README.md

This now includes a test to check for reproduction steps given in comment 1.

Comment 37

[John Bieling (:TbSync)]

To get this checked in, I think a testing tag needs to be set?

Comment 38

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7638d4daff9c
Fix addon.userPermissions is null r=mixedpuppy

Comment 39

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/7638d4daff9c