Deal with the fallout of bug 1413413 [Remove support for extensions having their own prefs file]

RESOLVED FIXED in Thunderbird 59.0

Status

enhancement
--
blocker
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: jorgk, Assigned: Fallen)

Tracking

58 Branch
Thunderbird 59.0
Dependency tree / graph

Thunderbird Tracking Flags

(thunderbird_esr52 unaffected, thunderbird58 fixed, thunderbird59 fixed)

Details

Attachments

(3 attachments, 20 obsolete attachments)

1.34 KB, application/octet-stream
Details
10.10 KB, patch
jorgk
: review+
Details | Diff | Splinter Review
6.16 KB, patch
jorgk
: review+
Details | Diff | Splinter Review
(Reporter)

Description

2 years ago
Bug 1413413 removed the ability for extensions to declare their own preferences in a file defaults/preferences/xxx.js.

Our bundled extension Lightning uses such a file:
https://dxr.mozilla.org/comm-central/source/calendar/lightning/content/lightning.js

When
https://hg.mozilla.org/integration/mozilla-inbound/rev/43c726ab7f71353f4b8d0c14bca27d65edc6ad99
gets merged to M-C, all Thunderbird Xpcshell and Mozmill tests will be obliterated since all calendar tests will fail.

In this bug we'll be discussing temporary and permanent measures to work around the problem.

First possible measure: Append the aforementioned file to all-thunderbird.js. Ugly, but will avoid bustage.

Better ideas?

It won't be easy to fork the removed M-C code into C-C for two reasons:
1) The removed code is internal to modules/libpref/Preferences.cpp
   and uses various (private) 'static' functions.
2) It's not obvious how to trigger that code once forked.
Flags: needinfo?(philipp)
Flags: needinfo?(mkmelin+mozilla)
Flags: needinfo?(acelists)
(Reporter)

Comment 1

2 years ago
Posted patch 1414398-lightning-pref-all-tb.patch (obsolete) β€” β€” Splinter Review
Ugly!
Attachment #8925136 - Flags: feedback?(philipp)

Comment 2

2 years ago
can't you just change JS_PREFERENCE_PP_FILES to JS_PREFERENCE_FILES, here:

https://dxr.mozilla.org/comm-central/rev/952762964f408e33d060dd1110cac0626ae5fa51/calendar/lightning/moz.build#39
(Reporter)

Comment 3

2 years ago
Maybe I can ;-) Please enlighten me what these two are.

Meanwhile I've been looking at porting the C++ stuff remove here
https://hg.mozilla.org/integration/mozilla-inbound/rev/43c726ab7f71353f4b8d0c14bca27d65edc6ad99
and right now, I don't see a way to do it due to the issues 1) and 2) mentioned in comment #0.
(Reporter)

Comment 4

2 years ago
Posted patch 1414398-JS_PREFERENCE_FILES.patch (obsolete) β€” β€” Splinter Review
OK, here comes the second idea.
Attachment #8925140 - Flags: feedback?(philipp)
(Reporter)

Comment 5

2 years ago
What about the other JS_PREFERENCE_PP_FILES?
https://dxr.mozilla.org/comm-central/search?q=JS_PREFERENCE_PP_FILES&redirect=false

Comment 6

2 years ago
https://dxr.mozilla.org/comm-central/rev/371e44e0034771ec8a5ac3c5a6518ef608227b99/mozilla/python/mozbuild/mozbuild/frontend/context.py#2251

so PP is preprocessed.  but lightning can put its prefs file wherever it wants, rather than in its extensions dir, or else the build system should be changed to put it where all-thunderbird.js lives or such.  it seems the thing removed is from [installdir]/extensions/myextension/defaults/preferences only.
(Reporter)

Comment 7

2 years ago
Comment on attachment 8925140 [details] [diff] [review]
1414398-JS_PREFERENCE_FILES.patch

Hmm, so dropping the "PP_" won't buy us anything, right? Plus, that file needs preprocessing.

I'm happy for someone to change the build config, but sadly I don't have skills to do that.
Attachment #8925140 - Attachment is obsolete: true
Attachment #8925140 - Flags: feedback?(philipp)
(Reporter)

Comment 8

2 years ago
Hmm, looks like it was all a false alarm for Lightning. Debugging in M-C's Preferences.cpp showed that the obsolete function ReadExtensionPrefs() wasn't even called for Lightning, only for a few other add-ons I have installed.

Building with the changeset from bug 1413413 to see whether Lightning is broken or not. Still building ... sigh.
(Reporter)

Comment 9

2 years ago
Hmm, Lightning still seems to be working.
Comment on attachment 8925136 [details] [diff] [review]
1414398-lightning-pref-all-tb.patch

This is a brutal hack we should avoid.
Flags: needinfo?(philipp)
Attachment #8925136 - Flags: feedback?(philipp) → feedback-
Posted file WiP - v1 (obsolete) β€”
This mostly untested wad of code will let Thunderbird read extension prefs again. I tested the actual reading and setting prefs in scratchpad, but I did not test the addon manager listener, nor the branch clearing mecanics, nor the walkExtensionPrefs function.

What it needs is the testing above (not with Lightning, that will just work), plus a unit test, and any potential issues fixed from that. I'd really appreciate if someone else could do that.

For testing you need an addon that has one of those files, usind pref() and user_pref(). Install and check if prefs are set via about:config, uninstall and make sure they are gone. Also test with pref() overriding a pref already default set by tb, that one should get a user value that is merely cleared on uninstall.
Attachment #8925254 - Attachment mime type: application/x-javascript → text/plain
(Reporter)

Comment 12

2 years ago
(In reply to Philipp Kewisch [:Fallen] from comment #10)
> This is a brutal hack we should avoid.
Sure. This was the last resort had Lightning been affected. I was going to clear the f? but you beat me to it.

Comment 13

2 years ago
Comment on attachment 8925254 [details]
WiP - v1

I imagined some code that would run soon after startup and read the defaults files simulating what the old m-c code did.

Do I understand it correctly that this still ignores the defaults file, but overrides some pref manipulating functions so that they pretend there is no problem with the default prefs (as if they existed) ? How do you propose to hook this into TB, where would it be placed and loaded? E.g. I see no setPref() call in /mail yet. Do the extensions call it?

+ for (let file of fixIterator(prefFilePath.directoryEntries, Components.interfaces.nsISimpleEnumerator)) {
 
Should this be nsIFile, not enumerator? The argument specifies the type of the members.
Flags: needinfo?(acelists)
(Reporter)

Comment 14

2 years ago
Philipp, you need to take care of
### Error loading backend: [Exception... "Component returned failure code: 0x8000ffff (NS_ERROR_UNEXPECTED) [nsIPrefBranch.getBoolPref]"  nsresult: "0x8000ffff (NS_ERROR_UNEXPECTED)"  location: "JS frame :: file:///C:/mozilla-source/comm-central/obj-i686-pc-mingw32/dist/bin/extensions/%7Be2fda1a4-762b-4020-b5ad-a41df1933103%7D/components/calBackendLoader.js :: loadBackend :: line 41"  data: no]

Seen when running
  mozmake -C calendar/test/mozmill SOLO_TEST=testTodayPane.js mozmill-one
which has been reported as failing in bug 1414533.
(In reply to :aceman from comment #13)
> Comment on attachment 8925254 [details]
> WiP - v1
> 
> I imagined some code that would run soon after startup and read the defaults
> files simulating what the old m-c code did.
This does something similar, it reads and executes the defaults/preferences/*.js files from add-ons as soon as they are enabled. I believe all add-ons get an onEnabled message on startup, which would need to be tested. If that doesn't work, then this would need to run for all installed add-ons on startup. The code would have to be embedded into Thunderbird early enough that it runs before the add-ons are loaded. 


> 
> Do I understand it correctly that this still ignores the defaults file, but
> overrides some pref manipulating functions so that they pretend there is no
> problem with the default prefs (as if they existed) ?
The defaults file is read! What it does is create a sandbox that provides an implementation for pref() and user_pref() (I ignored sticky_pref() for now), loads the text content of each defaults/preferences/*.js file, and executes it as javascript in the created sandbox.



> How do you propose to
> hook this into TB, where would it be placed and loaded? E.g. I see no
> setPref() call in /mail yet. Do the extensions call it?

This needs to be done as per above. It needs to run once, the listener should take care of the rest.


> + for (let file of fixIterator(prefFilePath.directoryEntries,
> Components.interfaces.nsISimpleEnumerator)) {
>  
> Should this be nsIFile, not enumerator? The argument specifies the type of
> the members.
Ah yes, of course.


(In reply to Jorg K (GMT+2) from comment #14)
> Philipp, you need to take care of
> ### Error loading backend: [Exception... "Component returned failure code:
> 0x8000ffff (NS_ERROR_UNEXPECTED) [nsIPrefBranch.getBoolPref]"  nsresult:
> "0x8000ffff (NS_ERROR_UNEXPECTED)"  location: "JS frame ::
> file:///C:/mozilla-source/comm-central/obj-i686-pc-mingw32/dist/bin/
> extensions/%7Be2fda1a4-762b-4020-b5ad-a41df1933103%7D/components/
> calBackendLoader.js :: loadBackend :: line 41"  data: no]
> 
> Seen when running
>   mozmake -C calendar/test/mozmill SOLO_TEST=testTodayPane.js mozmill-one
> which has been reported as failing in bug 1414533.

This can certainly be fixed, if my wad of code is turned into a patch it will no longer be necessary though (aside from the fact that there will be a deprecated warning for Lightning and we need to move all prefs anyway)
Maybe you can reuse something from bug 1240798
(Reporter)

Comment 17

2 years ago
Maybe interesting reading: Bug 1413413 comment #13 (quote):
(Kris Maglione [:kmag] (long backlog; ping on IRC if you're blocked) from comment Bug 1413413 #13)
> Honestly, if you're still using non-bootstrapped extensions, you're going to
> have much bigger problems soon.
 
> The traditional route bootstrapped extensions have taken in the past is to
> just use the subscript loader to load the prefs.js files into a scope with
> pref and user_pref functions defined.

Updated

2 years ago
Blocks: 1414621
(Reporter)

Updated

2 years ago
Duplicate of this bug: 1414956
(Reporter)

Comment 19

2 years ago
Any chance to get this done quickly? Daily is basically unusable since add-ons don't work and add-on authors are starting to complain, see bug 1414956.

This is *bustage* which we should fix asap.
Flags: needinfo?(philipp)

Comment 20

2 years ago
Just to make sure I'm understanding correctly what's going on here...

* M-C no longer automatically loads prefs.js files in non-bootstrapped extensions.

* The Thunderbird developers are working on figuring out how to restore that functionality (that's this bug).

* As an add-on maintainer, I should consider this a heads-up that non-bootstrapped extensions are going to continue to encounter issues in Thunderbird as M-C code changes, and I'm eventually going to have to convert my add-ons to bootstrapped.

Do I have all that right?
(Reporter)

Comment 21

2 years ago
Yes, to all the above.

M-C no longer load preferences from default/preferences/xxx.js. Lightning and other add-ons are affected. BTW, for Lightning it's default/preferences/lightning .js

This bug is about restoring this functionality in the short term. The other option is to declare those preference defaults on the fly.

In the long term, non-bootstrapped extensions won't be supported any more, please follow bug 1413432. We don't know the exact timing yet.
I probably don't have time to do this quickly, hence I was hoping someone could pick it up and iron out the corners.
Flags: needinfo?(philipp)
(Reporter)

Comment 23

2 years ago
Comment on attachment 8925254 [details]
WiP - v1

OK, I'll see what I can do. What are the knows missing corners? How do you intend to wire it in so it runs early enough? I see that you have an addon listener, so does that need registering? And then it should work?
Attachment #8925254 - Attachment is patch: true
(Reporter)

Comment 24

2 years ago
Comment on attachment 8925254 [details]
WiP - v1

Oops, not a patch, just a piece of new code that needs to go somewhere. Where?
Attachment #8925254 - Attachment is patch: false
Maybe we should create a /toolkitcc or so dir for additional shared code? Will probably not the only code end up duplicated with no or minimal changes otherwise for SeaMonkey and Thunderbird.
Otherwise maybe in mail/components/ and loaded from mailGlue.js?
(Reporter)

Comment 27

2 years ago
FRG, are you volunteering? Please do ;-)
To my eye, anything shared goes to mailnews. Where it's called is another issue.
I can see mailnews/base/prefs. Is this not about prefs?
(In reply to Jorg K (:jorgk, GMT+1) from comment #23)
> OK, I'll see what I can do. What are the knows missing corners? How do you
> intend to wire it in so it runs early enough? I see that you have an addon
> listener, so does that need registering? And then it should work?

The code I provided along with comment 15 should have enough information to turn this into a patch, if you have specific questions that you cannot find an answer for yourself please ping me on IRC.

Regarding shared location, I agree mailnews has been the place to put common things. I agree the name is not great and it would be nice to have a new place for common comm things, but lets do that in a separate bug, with some discussion on maildev/m.d.a.thunderbird and relevant other channels.
Comment hidden (offtopic)
Comment hidden (offtopic)
Comment hidden (offtopic)
Comment hidden (offtopic)
For the record I have this further in patch form now, but need to do some magic to read directory entries from jar: URLs.
(Reporter)

Comment 34

2 years ago
Thanks for the update. Goodness gracious, jar: URL, so that's for those crazy packed add-ons. How about getting a partial solution, oh boy, a temporary fix, landed so some people can return back to the Daily channel. Otherwise we're all stuck on the 3rd of November.
Posted patch WiP - v2 (obsolete) β€” β€” Splinter Review
Very well :) Here is a patch that actually reads from jar uris and starts just when the addon manager has started.

I've only done some brief testing on this. It does correctly set the prefs and clear them on uninstall, but there is an edge case we might not be able to easily fix: if you use pref("it.works", true); then it will set this in the default branch. If you then uninstall, we have no way to differentiate between "it.works" and e.g. "mailnews.reply_header_authorwrotesingle". The code opts to just reset the pref to it's default value, but since it is already a default pref it stays set.

The code also needs to move to somewhere sensible (and still be called as early as possible with the correct category manager entries). If it were only tb I'd go for a new dir mail/components/extensions that we can later use for WX as well.

To finish this off it needs more testing and the code move, which I'd appreciate if someone could take over.
Assignee: nobody → philipp
Status: NEW → ASSIGNED
Attachment #8925254 - Attachment is obsolete: true
Attachment #8925136 - Attachment is obsolete: true
Posted file Add-on for testing β€”
This is the add-on I used for testing. Might not cover all cases.

Oh, piling on to the previous comment, some tests would be nice.
(Reporter)

Comment 37

a year ago
(In reply to Philipp Kewisch [:Fallen] from comment #35)
> To finish this off it needs more testing and the code move, which I'd
> appreciate if someone could take over.
Who do you have in mind? I still don't have a contract, and even if I had, this wouldn't be my first priority. I unsuccessfully tried to get Aceman interested. Perhaps you can recruit Magnus. Both have better JS skills than I do.

Comment 38

a year ago
I can play with this once it is in usable patch form and does run at TB startup. Seems like the current patch could do that.
I could also make a mozmill test, if the test addon is restartless and get enabled immediately upon install and gets its default prefs read (I don't know if that can happen in a restartless addon). Then I can make a test that installs it (something like mozmill/content-tabs/test-install-xpi.js) and then checks if the default prefs got read.
(Reporter)

Comment 39

a year ago
In comment #11 Philipp talked of a "unit test" which sounds more like Xpcshell to me (of which we have very few experts). What did you have in mind? I believe that the preferences mechanism we want to re-establish here is for non-restartless add-ons, right?
I think a simple unit test would be sufficient. We probably don't need to go overboard with the tests, so I'd suggest a simple (bootstrapped) add-on, that is installed at startup (using both user_pref() and pref(), and the xpcshell test just checks for the preference's existence on the default branch and user branch respectively. Then disable the add-on using the addon manager, and check if: 1) the user_pref() is cleared, 2) a pref() on the extensions.* branch is deleted 3) a pref() on the mail.* branch is just cleared, not deleted. For (3) be careful about side-effects.
Severity: normal → blocker
Version: Trunk → 58 Branch
(Reporter)

Updated

a year ago
Duplicate of this bug: 1417694
(Reporter)

Comment 42

a year ago
Is this working 100%? I've just tried using a developer version of SmartTemplate4 as per bug 1417209 comment #31 and I see heaps of

SmartTemplate4 [logTime init]
getBoolPref(extensions.smartTemplate4.debug) failed:
Err:[Exception... "Component returned failure code: 0x8000ffff (NS_ERROR_UNEXPECTED) [nsIPrefBranch.getBoolPref]"  nsresult: "0x8000ffff (NS_ERROR_UNEXPECTED)"  location: "JS frame :: chrome://smarttemplate4/content/smartTemplate-prefs.js :: getBoolPref :: line 32"  data: no]

So that looks as if that add-on doesn't get its preferences.

When I back out bug 1413413 (which is becoming increasingly more difficult due to later conflicting changes), the add-on runs considerably better and I don't see those errors.
Flags: needinfo?(philipp)

Comment 43

a year ago
Comment on attachment 8928033 [details] [diff] [review]
WiP - v2

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

I did some testing with this patch and with the below mentioned ; issue fixed it seems in general to work with a new profile, although it took some attempts to get there (maybe this was a caching issue). For my existing local build profile, it still doesn't work, but that seems to be mixed up anyway. Maybe other can do further tests for existing profils.

Currently also default prefs for disabled addons are loaded, which should be avoided. Also I get a |no chrome package registered| message for the test addon and the Google provider (with the first enabled and the second disabled), not sure wether this is related.

There is an issue with the Lightning pref calendar.integration.notify, which seems to get preloaded with the wrong type (string instead of boolean; you can reproduce this when resetting the pref in the pref editor), which makes the bar to come back after restart, even if you have choosen |keep| before.

::: mail/components/mailGlue.js
@@ +15,5 @@
>  
> +function blergh() {
> +  /* This Source Code Form is subject to the terms of the Mozilla Public
> +   * License, v. 2.0. If a copy of the MPL was not distributed with this
> +   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */

The license block is not needed here, as the file already has one.

@@ +22,5 @@
> +  Components.utils.import("resource://gre/modules/Deprecated.jsm")
> +  Components.utils.import("resource://gre/modules/NetUtil.jsm");
> +  Components.utils.import("resource://gre/modules/osfile.jsm")
> +
> +  Components.utils.import("resource:///modules/iteratorUtils.jsm");

You can use Cu here as above and also move the imports out of the scope of the function.

@@ +66,5 @@
> +  function walkExtensionPrefs(addon, callback) {
> +    let prefPath = addon.getResourceURI("defaults/preferences");
> +    let foundFiles = false;
> +    if (prefPath.schemeIs("file")) {
> +        let prefFilePath = prefPath.QueryInterface(Components.interfaces.nsIFileURL).file;

indentation too big.

@@ +84,5 @@
> +    } else if (prefPath.schemeIs("jar")) {
> +      let handler = Services.io.getProtocolHandler("jar").QueryInterface(Components.interfaces.nsIJARProtocolHandler);
> +      let jarUri = prefPath.QueryInterface(Components.interfaces.nsIJARURI);
> +      let jarFile = jarUri.JARFile.QueryInterface(Components.interfaces.nsIFileURL).file;
> +      let zipReader = handler.JARCache.getZip(jarFile)

Here a trailing ; is missing - with that fixed, the patch should work.
(Reporter)

Comment 44

a year ago
Can you please try a "real world" add-on. Without the patch "Lightning Calendar Tabs" gives
NS_ERROR_UNEXPECTED: Component returned failure code: 0x8000ffff (NS_ERROR_UNEXPECTED) [nsIPrefBranch.getBoolPref]
and with the patch, it's the same, even with the missing semicolon added :-(

Also I see this during startup:

### Error loading backend: [Exception... "Component returned failure code: 0x800
0ffff (NS_ERROR_UNEXPECTED) [nsIPrefBranch.getBoolPref]"  nsresult: "0x8000ffff
(NS_ERROR_UNEXPECTED)"  location: "JS frame :: file:///C:/mozilla-source/comm-ce
ntral/obj-i686-pc-mingw32/dist/bin/extensions/%7Be2fda1a4-762b-4020-b5ad-a41df19
33103%7D/components/calBackendLoader.js :: loadBackend :: line 41"  data: no]

1511122570097   addons.manager  WARN    Exception calling callback: [Exception..
. "Component returned failure code: 0x80520012 (NS_ERROR_FILE_NOT_FOUND) [nsIFil
e.isDirectory]"  nsresult: "0x80520012 (NS_ERROR_FILE_NOT_FOUND)"  location: "JS
 frame :: file:///C:/mozilla-source/comm-central/obj-i686-pc-mingw32/dist/bin/co
mponents/mailGlue.js :: walkExtensionPrefs :: line 71"  data: no] Stack trace: w
alkExtensionPrefs()@mailGlue.js:71 < enableAddon()@mailGlue.js:108 < blergh/init
AddonListener/<()@mailGlue.js:136 < safeCall()@resource://gre/modules/AddonManag
er.jsm:187 < makeSafe/<()@resource://gre/modules/AddonManager.jsm:202

So clearly something has gone wrong in the new code.

On a new profile I still see the first error, but not the second and "Lightning Calendar Tabs" actually works. However, in the add-ons manager it's missing the "Options" button.
(Reporter)

Comment 45

a year ago
Posted patch WIP - v2b (obsolete) β€” β€” Splinter Review
This works a little better. With the patch, "Lightning Calendar Tabs" works after a restart. However, still no "Options" button. Looks like after another restart it doesn't work again.

I also see an awful lot of these:
JavaScript error: resource://calendar/modules/calUtils.jsm -> file:///C:/mozilla
-source/comm-central/obj-i686-pc-mingw32/dist/bin/extensions/%7Be2fda1a4-762b-40
20-b5ad-a41df1933103%7D/calendar-js/calTimezone.js, line 34: NS_ERROR_NOT_IMPLEM
ENTED: Component returned failure code: 0x80004001 (NS_ERROR_NOT_IMPLEMENTED) [c
alIIcalComponent.icalComponent]
Attachment #8928033 - Attachment is obsolete: true
(In reply to [:MakeMyDay] from comment #43)
> Comment on attachment 8928033 [details] [diff] [review]
> WiP - v2
> 
> Review of attachment 8928033 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I did some testing with this patch and with the below mentioned ; issue
> fixed it seems in general to work with a new profile, although it took some
> attempts to get there (maybe this was a caching issue). For my existing
> local build profile, it still doesn't work, but that seems to be mixed up
> anyway. Maybe other can do further tests for existing profils.
Having the feeling it does not work yet. There are two possible races here, one being the code not running early enough in startup, the other being the code not running early enough after add-ons are enabled.

Two things to be tested:
1a) instead of using AddonManager.isReady, try to reach the scope of AddonManager.jsm and check for gStarted instead.
1b) alternatively, try using addAddonListener, if that thows an exception that AOM is not initialized, add the manager listener and wait for startup

2) Try monkeypatching AOM methods to run our code as soon as possible after add-on startup.


> 
> Currently also default prefs for disabled addons are loaded, which should be
> avoided. Also I get a |no chrome package registered| message for the test
> addon and the Google provider (with the first enabled and the second
> disabled), not sure wether this is related.
Indeed, we need to add in a check in the initial init code with getAddonsByType to see if the add-on is disabled.

> 
> There is an issue with the Lightning pref calendar.integration.notify, which
> seems to get preloaded with the wrong type (string instead of boolean; you
> can reproduce this when resetting the pref in the pref editor), which makes
> the bar to come back after restart, even if you have choosen |keep| before.
This might explain a few other issues we've had w.r.t. Lighting being re-enabled after each upgrade?
> 
> ::: mail/components/mailGlue.js
> @@ +15,5 @@
> >  
> > +function blergh() {
> > +  /* This Source Code Form is subject to the terms of the Mozilla Public
> > +   * License, v. 2.0. If a copy of the MPL was not distributed with this
> > +   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> 
> The license block is not needed here, as the file already has one.
The file should also not have a function named blergh ;-) The contents of this file need to be moved somewhere, e.g. mail/components/extensions/ and then loaded early via category entries.

> 
> @@ +22,5 @@
> > +  Components.utils.import("resource://gre/modules/Deprecated.jsm")
> > +  Components.utils.import("resource://gre/modules/NetUtil.jsm");
> > +  Components.utils.import("resource://gre/modules/osfile.jsm")
> > +
> > +  Components.utils.import("resource:///modules/iteratorUtils.jsm");
> 
> You can use Cu here as above and also move the imports out of the scope of
> the function.
Not a fan of Cu, but if that is prevalent style I'm happy to adapt. On the issue of imports, see above.



(In reply to Jorg K [Almost not working on Thunderbird (some bustage-fix only) due to non-renewal of contract] from comment #45)
> Created attachment 8929888 [details] [diff] [review]
> WIP - v2b
> 
> This works a little better. With the patch, "Lightning Calendar Tabs" works
> after a restart. However, still no "Options" button. Looks like after
> another restart it doesn't work again.
Can you explain what this fixed? I'm not sure I understand the changes.

> 
> I also see an awful lot of these:
> JavaScript error: resource://calendar/modules/calUtils.jsm ->
> file:///C:/mozilla
> -source/comm-central/obj-i686-pc-mingw32/dist/bin/extensions/%7Be2fda1a4-
> 762b-40
> 20-b5ad-a41df1933103%7D/calendar-js/calTimezone.js, line 34:
> NS_ERROR_NOT_IMPLEM
> ENTED: Component returned failure code: 0x80004001
> (NS_ERROR_NOT_IMPLEMENTED) [c
> alIIcalComponent.icalComponent]
This is a sign that the approach is not running early enough, and/or there is a race condition. the backend loader runs very early in Lightning init, this prefs code needs to run before that.

One thing that could improve the situation is loading the prefs file synchronous instead of using OS.File.

Another things we can do if the main goal is to fix possible bustages is to fix Lightning to not rely on the prefs file. This will not help other add-ons though.
Flags: needinfo?(philipp)
(Reporter)

Comment 47

a year ago
(In reply to Philipp Kewisch [:Fallen]  from comment #46)
> Can you explain what this fixed? I'm not sure I understand the changes.
https://bugzilla.mozilla.org/attachment.cgi?oldid=8928033&action=interdiff&newid=8929888&headers=1
old: if (!prefFilePath.isDirectory()) {
new: if (!prefFilePath.exists() || !prefFilePath.isDirectory()) {
You just test defaults/preferences even for add-ons that don't have it. Hence the NS_ERROR_FILE_NOT_FOUND, comment #44.

old: if (file.isFile() && file.exists() && file.leafName.toLowerCase().endsWith(".js")) {
new: if (file.exists() && file.isFile() && file.leafName.toLowerCase().endsWith(".js")) {
Maybe pure paranoia: Test that the file exists before checking that it's a file and not a directory.
Maybe unnecessary but aligns the code with the hunk above.
(In reply to Jorg K [Almost not working on Thunderbird (some bustage-fix only) due to non-renewal of contract] from comment #47)
> (In reply to Philipp Kewisch [:Fallen]  from comment #46)
> > Can you explain what this fixed? I'm not sure I understand the changes.
> https://bugzilla.mozilla.org/attachment.
> cgi?oldid=8928033&action=interdiff&newid=8929888&headers=1
> old: if (!prefFilePath.isDirectory()) {
> new: if (!prefFilePath.exists() || !prefFilePath.isDirectory()) {
> You just test defaults/preferences even for add-ons that don't have it.
> Hence the NS_ERROR_FILE_NOT_FOUND, comment #44.
Ah yes, of course. Good catch!

> 
> old: if (file.isFile() && file.exists() &&
> file.leafName.toLowerCase().endsWith(".js")) {
> new: if (file.exists() && file.isFile() &&
> file.leafName.toLowerCase().endsWith(".js")) {
> Maybe pure paranoia: Test that the file exists before checking that it's a
> file and not a directory.
> Maybe unnecessary but aligns the code with the hunk above.
I'd skip the check here as we should be able to assume that just evaluated directory entries still exist.
(Reporter)

Updated

a year ago
Duplicate of this bug: 1423243

Comment 50

a year ago
Shouldn't string type of preference be treated as unicode? Looking at the patch, it uses branch.setCharPref which will loose any non-ASCII characters.

However, not sure how much this related, but it seems nsIPrefBranch.getComplexValue (and setComplexValue) https://developer.mozilla.org/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsIPrefBranch#getComplexValue()
is changed somehow.
In my test extension when using NsIPrefLocalizedString it fails with:

"Component returned failure code: 0x804b000a (NS_ERROR_MALFORMED_URI) [nsIPrefBranch.getComplexValue]"  nsresult: "0x804b000a (NS_ERROR_MALFORMED_URI)"


and with NsISupportsString it fails with:

"Component returned failure code: 0x80004002 (NS_NOINTERFACE) [nsIPrefBranch.getComplexValue]"  nsresult: "0x80004002 (NS_NOINTERFACE)"

Comment 51

a year ago
Can you try setStringPref()? See bug 1415567, nsIPrefLocalizedString should still work but the usage with nsISupportsString was changed to *StringPref().
(Reporter)

Comment 52

a year ago
(In reply to V@no from comment #50)
> Shouldn't string type of preference be treated as unicode? Looking at the
> patch, it uses branch.setCharPref which will loose any non-ASCII characters.
I think here Mr. V is referring to the patch:
+    if (typeof value == "boolean") {
+      branch.setBoolPref(name, value);
+    } else if (typeof value == "string") {
+      branch.setCharPref(name, value);
Should we use setStringPref()?

Comment 53

a year ago
Maybe. From looking at the prefs file you do not know whether the pref value needs CharPref() or StringPref(). Maybe using StringPref() could work for safety. I'm playing with it.

Comment 54

a year ago
Posted patch WIP v3 (obsolete) β€” β€” Splinter Review
I improved the v2b to account for some more oddities in pref files (e.g. zero-length pref files) and that evaluating some pref files can throw (due to illegal chars). I tested this on the collection of about 10 addons I have in my test profiles.

This version at least really reads the prefs in. Lightning's pref UI has populated fields now.

But this code still runs too late, I still get error from Lightning:
### Error loading backend: [Exception... "Component returned failure code: 0x8000ffff (NS_ERROR_UNEXPECTED) [nsIPrefBranch.getBoolPref]"  nsresult: "0x8000ffff (NS_ERROR_UNEXPECTED)"  location: "JS frame :: file:///TB-hg/tbird-bi
n/dist/bin/extensions/%7Be2fda1a4-762b-4020-b5ad-a41df1933103%7D/components/calBackendLoader.js :: loadBackend :: line 41"  data: no]

Glodaquilla still shows a page after upgrade to TB3, which should mean some pref isn't set or stored that the page shouldn't be shown anymore.

I'm also not sure if I got the LocalizedPrefs right.
Some of them work (e.g. in Lightning), but some fail with an error, e.g.
"Couldn't convert chrome URL: chrome://gdata-provider/locale/gdata.properties"
Attachment #8929888 - Attachment is obsolete: true
(Reporter)

Comment 55

a year ago
Never mind Glodaquilla, it hasn't been updated since 2011 and is compatible up to TB 24. I know that some parts are not working. BTW, I don't understand your sentence at all: "Shows a page"?? "Upgrade to TB3 (three??)". Or do you mean Gdata?

While you're changing code, can you remove a change I made, see comment #48, last paragraph, Philipp wanted to remove the file.exists() in |if (file.exists() && file.isFile() &&|.

Overall: Thanks for looking into it.

Comment 56

a year ago
(In reply to Jorg K (GMT+1) from comment #55)
> Never mind Glodaquilla, it hasn't been updated since 2011 and is compatible
> up to TB 24. I know that some parts are not working.

I have updated Glodaquilla half a year ago so it works in my setup. If the author hasn't released the fixes I have sent him, that is just sad.

> BTW, I don't understand
> your sentence at all: "Shows a page"?? "Upgrade to TB3 (three??)". Or do you
> mean Gdata?

Glodaquilla shows a page what to do when you have migrated to TB3.1 (or above). It detects this when you run a newer version and then stores this in its pref to not show the page again. Even though I already have that pref set to the right value (in my prefs.js) it shows the page nevertheless, so this is further indication that something is not yet fully working with the addon prefs. It gets the default value (from it's default/preferences/) instead of my already customized value.
 
> While you're changing code, can you remove a change I made, see comment #48,
> last paragraph, Philipp wanted to remove the file.exists() in |if
> (file.exists() && file.isFile() &&|.

Ok.

(In reply to :aceman from comment #54)
> I'm also not sure if I got the LocalizedPrefs right.
> Some of them work (e.g. in Lightning), but some fail with an error, e.g.
> "Couldn't convert chrome URL:
> chrome://gdata-provider/locale/gdata.properties"

OK, the failing ones seem to be with addons that are disabled. We still read their prefs (should we really?) but accessing their chrome files fails, as those may not be evaluated.
(Reporter)

Comment 57

a year ago
(In reply to :aceman from comment #56)
> OK, the failing ones seem to be with addons that are disabled.
That needs to be fixed, see comment #43: "Currently also default prefs for disabled addons are loaded, ..."

Comment 58

a year ago
Thanks, I will look into comment 43.

The Glodaquilla problem seems to boil down to this code in the patch:
    let branch = Services.prefs.getBranch("");
    if (preferDefault) {
      let defaultBranch = Services.prefs.getDefaultBranch("");
      if (defaultBranch.getPrefType(name) == Components.interfaces.nsIPrefBranch.PREF_INVALID) {
        // Only use the default branch if it doesn't already have the pref set.
        // If there is already a pref with this value on the default branch, the
        // extension wants to override a built-in value.

For the pref that I am talking about (already has user-modified value in prefs.js), this code does not choose the default branch. It overwrites the normal value from the file in the addon (as if resetting the pref). But this overwriting isn't propagated to the users prefs.js, where I can always see my modified value. This needs to be looked at.
I thought about the final resting place for the code. Used the latest version and put a js (similar to mailMigrator) into common/src. 

This works for SeaMonkey 2.56 and should also do it for TB. Not sure if this is the best way so feel free to rip it apart or change it as you like :)

I needed to add an import to Services. There are Ci. and Components.utils.import. calls in the body but I didn't touch them.
(Reporter)

Comment 60

a year ago
Hey, this is in the best hands we have, so I'm confident :-) Thanks!

Comment 61

a year ago
I had an improved version already locally but I can somehow merge it now :)
Should it go into /src or would /content be better? Is there any clear separation between them? Maybe /content is for "UI" stuff?
unsure. If it is kept as a module manybe also common/content/modules or common/modules or common/src/modules or...
I am not sure, but from looking at the code in the attached patch, it appears to me that it will look for and load files in the defaults/preferences folder for both overlay and bootstrapped add-ons. If I've misread the code, please ignore the rest of this comment.

Before the code for loading default preferences was removed from mozilla-central, that code only loaded preferences for overlay add-ons.

So this patch, if I'm understanding it correctly, is not just restoring removed functionality; it's also adding _new_ functionality.

Authors of bootstrapped add-ons who want / need default preferences already have put code into their add-ons to load them. The code added her could potentially conflict with the code already put into the add-ons by their maintainers, if the maintainers choose to store the preferences for their bootstrapped add-ons in defaults/preferences.

In short, if I'm correct that this code will load defaults/preferences in bootstrapped add-ons, then I think that's probably a bug and it should be fixed.

Comment 64

a year ago
Posted patch V4 (obsolete) β€” β€” Splinter Review
This version only loads active addons.
It fixes the problem with Glodaquilla (a value already modified by user/addon previously). The check 'defaultBranch.getPrefType(name) == Ci.nsIPrefBranch.PREF_INVALID' isn't true (type isn't invalid) when there is already a user-set value for the pref (regardless if there is a default value set).

This also improves the LocalizedPrefs to only use setComplexValue for 'chrome://*.properties' links. Previously also links to chrome://*.wav (e.g. calendar.alarm) were interpreted as localized strings and it blew up (binary data wasn't a valid UTF8 string).

I couldn't observe the problem with "calendar.integration.notify". Any better steps to reproduce it?

Can you guys play with this again on any addons?

Lightning still doesn't work completely, because this code runs too late.
I still don't know how calBackendLoader.loadBackend() runs so fast before this pref reading code. But it mostly complains about the timezone service (which may relate to bug 1424350).
Attachment #8936111 - Attachment is obsolete: true
Attachment #8936125 - Attachment is obsolete: true
Flags: needinfo?(makemyday)
Flags: needinfo?(jorgk)

Comment 65

a year ago
(In reply to Jonathan Kamens from comment #63)
> In short, if I'm correct that this code will load defaults/preferences in
> bootstrapped add-ons, then I think that's probably a bug and it should be
> fixed.

Good point. Do you have an idea how to distinguish such addons?
Tested with latest CompactHeaders <https://addons.mozilla.org/firefox/downloads/file/790528/compactheader-2.1.4beta3-tb.xpi?src=external-mozillazine-thread-top> and manually sort folders.

CompactHeaders works already better than without the patch but still shows no buttons in compact mode and shows this error:
Error reading default prefs of addon CompactHeader: [Exception... "Component returned failure code: 0x8000ffff (NS_ERROR_UNEXPECTED) [nsIPrefBranch.getStringPref]"  nsresult: "0x8000ffff (NS_ERROR_UNEXPECTED)"  location: "JS frame :: resource:///modules/extensionSupport.jsm :: setPref :: line 41"  data: no]  extensionSupport.jsm:126

and

DEPRECATION WARNING: CompactHeader uses defaults/preferences/*.js files to load prefs
You may find more details about this deprecation at: https://bugzilla.mozilla.org/show_bug.cgi?id=1414398
resource:///modules/extensionSupport.jsm 131 loadAddonPrefs
resource:///modules/extensionSupport.jsm 160 extensionDefaults/initAddonListener/<
resource://gre/modules/AddonManager.jsm 188 safeCall
resource://gre/modules/AddonManager.jsm 203 makeSafe/<
  Deprecated.jsm:79

TBsortFolders fails with not starting the defined start folder, the same deprecation warning and:

SyntaxError: missing { before function body  folderPane.js:56:51
(In reply to :aceman from comment #65)
> Good point. Do you have an idea how to distinguish such addons?

There's a "bootstrap" property in install.rdf which indicates where the add-on is bootstrapped, but I don't see any way to access this property directly in the Addon object returned by AddonManager. Is there some other mechanism for fetching information about add-ons besides AddonManager?
(Reporter)

Comment 68

a year ago
(In reply to :aceman from comment #64)
> Lightning still doesn't work completely, because this code runs too late.
> I still don't know how calBackendLoader.loadBackend() runs so fast before
> this pref reading code. But it mostly complains about the timezone service
> (which may relate to bug 1424350).
Are you referring the error mentioned in comment #45? Then I must disappoint you, that predates bug 1424350 and the shared global stuff. With your patch I still see an awful lot of those. Lightning also doesn't work, I can't add an event:
  JavaScript error: chrome://calendar/content/calendar-item-editing.js, line 266:
  TypeError: currentView(...).selectedDay is null
Oh, I see that chagrin is caused be "Lightning Calendar Tabs", which I use for testing since it has default prefs. Hold on, even without LCT I get those errors at shutdown.

I gave the fixed version of "Header Tools Light" a go (http://www.jorgk.com/misc/addons58/) and that works.

Richard, does Compact Headers work in TB 58 beta? If not, that needs to be fixed first.

As for the bootstrapped add-ons: They shouldn't have default preferences. I'd suggest to land "part 1" here once we sort out the calendar issues and fix other issued in later parts. The first solution doesn't need to be perfect but it would be good to get most regular add-ons working again in Daily 59.
Flags: needinfo?(jorgk)
(Reporter)

Comment 69

a year ago
Hmm, CompactHeader again: Yes, works in TB 58 beta and appears to be working with this patch. The deprecation warning is intended. I don't see the "Error reading default prefs of addon CompactHeader".
Correct, Compact headers was updated to be compatible with betas.

I have "Two lines in compact view" set and then no header buttons are shown.
(Reporter)

Comment 71

a year ago
(In reply to Philipp Kewisch [:Fallen]  from comment #46)
> ... the backend loader runs very early in Lightning init,
> this prefs code needs to run before that.
Can someone explain this sentence. Lightning is an add-on and so surely this "prefs code" needs to be run before add-on initialisation, otherwise if the add-ons come to be initialised and don't have their prefs, hell will break loose.

Comment 72

a year ago
(In reply to Jorg K (GMT+1) from comment #71)
> (In reply to Philipp Kewisch [:Fallen]  from comment #46)
> > ... the backend loader runs very early in Lightning init,
> > this prefs code needs to run before that.
> Can someone explain this sentence. Lightning is an add-on and so surely this
> "prefs code" needs to be run before add-on initialisation, otherwise if the
> add-ons come to be initialised and don't have their prefs, hell will break
> loose.

If I'm not mistaken, the earliest a addon's code can run is via "component" (https://developer.mozilla.org/en-US/docs/Mozilla/Chrome_Registration#component), this is before any bootstrap.js or overlay execution.

Comment 73

a year ago
(In reply to Jorg K (GMT+1) from comment #71)
> (In reply to Philipp Kewisch [:Fallen]  from comment #46)
> > ... the backend loader runs very early in Lightning init,
> > this prefs code needs to run before that.
> Can someone explain this sentence. Lightning is an add-on and so surely this
> "prefs code" needs to be run before add-on initialisation, otherwise if the
> add-ons come to be initialised and don't have their prefs, hell will break
> loose.

And this exactly is happening:

### Error loading backend: [Exception... "Component returned failure code: 0x8000ffff (NS_ERROR_UNEXPECTED) [nsIPrefBranch.getBoolPref]"  nsresult: "0x8000ffff (NS_ERROR_UNEXPECTED)"  location: "JS frame :: file:///var/SSD/TB-hg/tbird-bi
n/dist/bin/extensions/%7Be2fda1a4-762b-4020-b5ad-a41df1933103%7D/components/calBackendLoader.js :: loadBackend :: line 41"  data: no]
ACE: skipping disabled addon Test Pilot for Thunderbird
ACE: skipping disabled addon ConsoleΒ²
ACE: skipping disabled addon FiltaQuilla
ACE: skipping disabled addon QuickFolders
Reading in prefs file:glodaquilla.js
(Reporter)

Comment 74

a year ago
Just documenting what we discussed on IRC:
First the initAddonListener() runs, then Lightning is initialised trying to read the pref, then the async callback passed into AddonManager.getAddonsByTypes() runs and sets preferences. Too late :-(
Using the AddonManager, there doesn't appear to be a way to get the list of add-ons *before* they get initialised.

Comment 75

a year ago
(In reply to Richard Marti (:Paenglab) from comment #66)
> CompactHeaders works already better than without the patch but still shows
> no buttons in compact mode and shows this error:
> Error reading default prefs of addon CompactHeader: [Exception... "Component
> returned failure code: 0x8000ffff (NS_ERROR_UNEXPECTED)
> [nsIPrefBranch.getStringPref]"  nsresult: "0x8000ffff (NS_ERROR_UNEXPECTED)"
> location: "JS frame :: resource:///modules/extensionSupport.jsm :: setPref
> :: line 41"  data: no]  extensionSupport.jsm:126

OK, I solved this, was leftover debugging. It is triggered by duplicate line
pref("extensions.CompactHeader.headersize.flatButtons", false); in the prefs file of the addon.
This should be removed in the addon.

> DEPRECATION WARNING: CompactHeader uses defaults/preferences/*.js files to
> load prefs

Yes, deprecation warning intentional for now.

(In reply to Richard Marti (:Paenglab) from comment #70)
> Correct, Compact headers was updated to be compatible with betas.

It still uses PrefBranch2 which was removed in 57, so it throw at start.
 
> I have "Two lines in compact view" set and then no header buttons are shown.

Fixing the above 2 problems in the addon code I can see the toolbar buttons with the addon.

Comment 76

a year ago
(In reply to Richard Marti (:Paenglab) from comment #66)
> TBsortFolders fails with not starting the defined start folder, the same
> deprecation warning and:

Which addon do you mean here? Is it "Manually sort folders" ?
Any chance observing profile-do-change might help here? It is a bit earlier than profile-after-change.

https://dxr.mozilla.org/comm-central/source/mozilla/toolkit/xre/nsXREDirProvider.cpp#990
(Reporter)

Comment 78

a year ago
Maybe we should get some inspiration from the C++ code I restore in the beta versions:

It looks for .xpi files:
https://hg.mozilla.org/releases/mozilla-beta/rev/4854beb5dc02#l1.151

and then processes the ones it finds with the ZipReader:
https://hg.mozilla.org/releases/mozilla-beta/rev/4854beb5dc02#l1.82

Do we really need to read the extension.json file?

Comment 79

a year ago
(In reply to :aceman from comment #75)
> (In reply to Richard Marti (:Paenglab) from comment #66)
> > CompactHeaders works already better than without the patch but still shows
> > no buttons in compact mode and shows this error:
> > Error reading default prefs of addon CompactHeader: [Exception... "Component
> > returned failure code: 0x8000ffff (NS_ERROR_UNEXPECTED)
> > [nsIPrefBranch.getStringPref]"  nsresult: "0x8000ffff (NS_ERROR_UNEXPECTED)"
> > location: "JS frame :: resource:///modules/extensionSupport.jsm :: setPref
> > :: line 41"  data: no]  extensionSupport.jsm:126
> 
> OK, I solved this, was leftover debugging. It is triggered by duplicate line
> pref("extensions.CompactHeader.headersize.flatButtons", false); in the prefs
> file of the addon.
> This should be removed in the addon.
I uploaded a new beta version (2.1.4beta4) to AMO which fixes this problem (removing the second line from prefs.js)

> 
> (In reply to Richard Marti (:Paenglab) from comment #70)
> > Correct, Compact headers was updated to be compatible with betas.
> 
> It still uses PrefBranch2 which was removed in 57, so it throw at start.
This was already fixed in 2.1.4beta3 (available from the development channel at AMO.

Comment 80

a year ago
Posted patch V4.1 (obsolete) β€” β€” Splinter Review
Thanks for fixing the addon and this patch fixes my leftover debugging that was unnecessarily throwing.
This should work with CompactHeaders too now.
Attachment #8936145 - Attachment is obsolete: true
(In reply to :aceman from comment #76)
> (In reply to Richard Marti (:Paenglab) from comment #66)
> > TBsortFolders fails with not starting the defined start folder, the same
> > deprecation warning and:
> 
> Which addon do you mean here? Is it "Manually sort folders" ?

Sorry, yes "Manually sort folders". But it can have some other bugs in it it was long time not updated.
(Reporter)

Comment 82

a year ago
Comment on attachment 8936178 [details] [diff] [review]
V4.1

Documenting things for posterity a little:

Since AddonManager.getAddonsByTypes() called our callback too late, it was suggested to parse extensions.json (or analyse the profile directory in some other form, comment #78).

Naturally, we need to get the profile directory first, but sadly
  Services.dirsvc.get("ProfD", Ci.nsIFile);
gives
NS_ERROR_FAILURE: Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIProperties.get] on that line.

That's somewhat surprising since FF's "browser glue" uses the same call during startup:
https://dxr.mozilla.org/mozilla-central/rev/a461fe03fdb07218b7f50e92c59dde64b8f8a5b0/browser/components/nsBrowserGlue.js#2054
That code was run during the update to 42, so perhaps it would fail now, too. Hard to tell.

Comment 83

a year ago
Have you considered to use TB's extensions.xpistate preference directly (which is used by the xpiprovider to determine whether an addon is newly installed)? It could provide the neccessary information for existing addons at least (though it just a single enabled state information, but that should be sufficient here). Maybe registration for newly addons can be intercepted otherwise, then?
(Reporter)

Comment 84

a year ago
(In reply to [:MakeMyDay] from comment #83)
> Have you considered to use TB's extensions.xpistate preference directly
Not so far. Thanks!
I can see that it contains a whole heap of stuff, like:
"lookout@addons.mozilla.org":{"d":"D:\\MAIL-THUNDERBIRD\\jorgk.default\\extensions\\lookout@addons.mozilla.org.xpi","e":true,"v":"1.2.15","st":1446588188919}

Comment 85

a year ago
I see this pref in 52, but not in 59. Was it removed in m-c?
(Reporter)

Comment 86

a year ago
Looks like that pref is on the way out:
https://dxr.mozilla.org/mozilla-central/rev/a461fe03fdb07218b7f50e92c59dde64b8f8a5b0/toolkit/mozapps/extensions/internal/XPIProvider.jsm#110

I can see it in TB 58 and also in TB 59 Daily of a few days ago. Maybe there is other useful stuff in XPIProvider.jsm.
(Reporter)

Comment 87

a year ago
This fixes things for me. Lightning works and so does Lightning Calendar Tabs. None of the endless errors from comment #45. I admit, not 100% elegant, but working.

Maybe land this in the interim with the dumps removed?
Attachment #8936178 - Attachment is obsolete: true
Attachment #8936299 - Flags: feedback?(richard.marti)
Attachment #8936299 - Flags: feedback?(acelists)

Comment 88

a year ago
Posted patch V4.2 alternate (obsolete) β€” β€” Splinter Review
And this is the implementation reading extensions.json directly... Also runs before addons and Lightning is happy with having icaljs set.
Comment on attachment 8936299 [details] [diff] [review]
V4.2 (original approach, not for landing)

Great work. Lightning and Compact headers are working and get their prefs. Manually sort folders still doesn't work, but I don't blame this patch but the extension itself.
Attachment #8936299 - Flags: feedback?(richard.marti) → feedback+
(In reply to :aceman from comment #88)
> Created attachment 8936300 [details] [diff] [review]
> V4.2 alternate
> 
> And this is the implementation reading extensions.json directly... Also runs
> before addons and Lightning is happy with having icaljs set.

Tested this too and it works for me. :)

Comment 91

a year ago
Comment on attachment 8936299 [details] [diff] [review]
V4.2 (original approach, not for landing)

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

Yes, this works for me too.
I just wonder how it works. I would prefer this if we can be sure this safely keeps the addon code from running before our pref loading finishes.
Attachment #8936299 - Flags: feedback?(acelists) → feedback+

Comment 92

a year ago
Posted patch V5 (obsolete) β€” β€” Splinter Review
A simplified version as discussed on IRC. We can land this to make nightly usable and improve upon this in further patches. It does not include the removal of prefs upon addon disabling. We need to discuss if that is needed.
Attachment #8936300 - Attachment is obsolete: true
Attachment #8936312 - Flags: review?(jorgk)
(Reporter)

Comment 93

a year ago
Comment on attachment 8936312 [details] [diff] [review]
V5

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

I think this is good to go to get Daily back on track.

::: common/src/extensionSupport.jsm
@@ +9,5 @@
> +var Cu = Components.utils;
> +
> +Cu.import("resource://gre/modules/Services.jsm");
> +Cu.import("resource://gre/modules/AddonManager.jsm");
> +Cu.import("resource://gre/modules/Deprecated.jsm")

Comment that out as well.

@@ +72,5 @@
> +      if (!prefFile.exists() || !prefFile.isDirectory())
> +        return [];
> +
> +      for (let file of fixIterator(prefFile.directoryEntries, Components.interfaces.nsIFile)) {
> +        if (file.exists() && file.isFile() && file.leafName.toLowerCase().endsWith(".js")) {

We wanted to remove the |file.exists() && | here.

@@ +135,5 @@
> +    }
> +
> +    for (let addon of addonsData.addons) {
> +      if (addon.type != "extension" || !addon.active || addon.userDisabled || addon.appDisabled || addon.bootstrap)
> +        continue;

Any reason why we need the continue? Just negate the condition, no?
Attachment #8936312 - Flags: review?(jorgk) → review+

Comment 94

a year ago
@92 Removing prefs with disabled addon isn't a good idea, it's a nogo. Removing should only be done under addon control, NO automatism
(Reporter)

Updated

a year ago
Keywords: leave-open
Have we addresses not loading default preferences for bootstrapped add-ons?

Comment 96

a year ago
Thanks, fixed the nits.
Attachment #8936312 - Attachment is obsolete: true
(Reporter)

Comment 97

a year ago
(In reply to Jonathan Kamens from comment #95)
> Have we addresses not loading default preferences for bootstrapped add-ons?
Yes:
+    for (let addon of addonsData.addons) {
+      if (addon.type == "extension" && addon.active && !addon.userDisabled && !addon.appDisabled && !addon.bootstrap)
+        loadAddonPrefs(addon);

Comment 98

a year ago
(In reply to Jorg K (GMT+1) from comment #93)
> Any reason why we need the continue? Just negate the condition, no?

Yes, I just expected more code there, that didn't happen :)

(In reply to GΓΌnter from comment #94)
> @92 Removing prefs with disabled addon isn't a good idea, it's a nogo.
> Removing should only be done under addon control, NO automatism

Yes, we should keep user-set values. We were only removing the default values, but it needs more thought whether even that is needed. So we stripped that too for now.

(In reply to Jonathan Kamens from comment #95)
> Have we addresses not loading default preferences for bootstrapped add-ons?

Yes, we check for addon.bootstrap in the current patch. We read the attribute from the extensions.json file. If you have experience with such addons, can you please check if it works?
(Reporter)

Updated

a year ago
Flags: needinfo?(mkmelin+mozilla)
Flags: needinfo?(makemyday)
Attachment #8936315 - Flags: review+
(Reporter)

Updated

a year ago
Attachment #8936299 - Attachment description: V4.2 → V4.2 (original approach, not for landing)
(Reporter)

Updated

a year ago
Attachment #8936315 - Flags: review?(philipp)

Comment 99

a year ago
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/60d8f821b8b3
Read default preferences supplied by add-ons. r=jorgk

Comment 100

a year ago
I already observed a potential problem (missing feature):
When new addon is installed, it's prefs are not loaded with the current patch.
On first restart, probably our code runs before the addon is first enabled.
On next run it is OK, with prefs loaded.

So we may need to add listening to the 'onEnabled' or 'onEnabling' event from the addon manager (as it was in Fallen's patch).

Comment 101

a year ago
I can confirm this problem: There are mozmill tests for CompactHeader (https://github.com/jmozmoz/compactheader/tree/master/test) and they immediately fail if applied to current beta version of CompactHeader together with Thunderbird 59.0a1.

../mozmill-virtualenv/Scripts/python runtest.py --binary=../thunderbird/thunderbird.exe -a ../../ftp/CompactHeader-2.1.4beta4.xpi -t compactheader 2>&1
Test failures:
### Error loading backend: [Exception... "Component returned failure code: 0x8000ffff (NS_ERROR_UNEXPECTED) [nsIPrefBranch.getBoolPref]"  nsresult: "0x8000ffff (NS_ERROR_UNEXPECTED)"  location: "JS frame :: file:///C:/Users/joachim/AppData/Local/Temp/compactheader/test-59.0a1/thunderbird/extensions/%7Be2fda1a4-762b-4020-b5ad-a41df1933103%7D/components/calBackendLoader.js :: loadBackend :: line 41"  data: no]
  Europe/Berlin (UTC+0100/+0200).
JavaScript error: chrome://compactheader/content/compactHeaderOverlay.js, line 203: NS_ERROR_UNEXPECTED: Component returned failure code: 0x8000ffff (NS_ERROR_UNEXPECTED) [nsIPrefBranch.getBoolPref]
JavaScript error: chrome://compactheader/content/compactHeaderOverlay.js, line 900: NS_ERROR_UNEXPECTED: Component returned failure code: 0x8000ffff (NS_ERROR_UNEXPECTED) [nsIPrefBranch.getBoolPref]
  File "runtest.py", line 523, in <module>
    ThunderTestCLI().run()
  File "C:\Users\joachim\AppData\Local\Temp\compactheader\test-59.0a1\mozmill-virtualenv\lib\site-packages\mozmill\__init__.py", line 790, in run
    self.mozmill.start(runner=runner, profile=runner.profile)
  File "runtest.py", line 287, in start
    return super(ThunderTestMozmill, self).start(profile, runner)
  File "C:\Users\joachim\AppData\Local\Temp\compactheader\test-59.0a1\mozmill-virtualenv\lib\site-packages\mozmill\__init__.py", line 231, in start
    self.appinfo = self.get_appinfo(self.bridge)
  File "C:\Users\joachim\AppData\Local\Temp\compactheader\test-59.0a1\mozmill-virtualenv\lib\site-packages\mozmill\__init__.py", line 337, in get_appinfo
    mozmill = jsbridge.JSObject(bridge, mozmillModuleJs)
  File "C:\Users\joachim\AppData\Local\Temp\compactheader\test-59.0a1\mozmill-virtualenv\lib\site-packages\jsbridge\jsobjects.py", line 76, in __init__
    name = bridge.set(name)['data']
  File "C:\Users\joachim\AppData\Local\Temp\compactheader\test-59.0a1\mozmill-virtualenv\lib\site-packages\jsbridge\network.py", line 226, in set
    return self.run(_uuid, 'bridge.set('+', '.join([encoder.encode(_uuid), obj_name])+')')
  File "C:\Users\joachim\AppData\Local\Temp\compactheader\test-59.0a1\mozmill-virtualenv\lib\site-packages\jsbridge\network.py", line 194, in run
    raise JSBridgeDisconnectError("Connection timed out")



(In reply to :aceman from comment #100)
> I already observed a potential problem (missing feature):
> When new addon is installed, it's prefs are not loaded with the current
> patch.
> On first restart, probably our code runs before the addon is first enabled.
> On next run it is OK, with prefs loaded.
> 
> So we may need to add listening to the 'onEnabled' or 'onEnabling' event
> from the addon manager (as it was in Fallen's patch).
(Reporter)

Comment 102

a year ago
(In reply to Joachim Herb from comment #101)
> I can confirm this problem: There are mozmill tests for CompactHeader
> (https://github.com/jmozmoz/compactheader/tree/master/test) and they
> immediately fail if applied to current beta version of CompactHeader
> together with Thunderbird 59.0a1.
Joachim, thanks for testing. Which version of TB 59 were you using? The patch landed yesterday and since then, Dailies have been broken. Did you use a Tinderbox build? For example the one I'm using myself from:
https://archive.mozilla.org/pub/thunderbird/tinderbox-builds/comm-central-win32/1513036581/ 32bit
https://archive.mozilla.org/pub/thunderbird/tinderbox-builds/comm-central-win64/1513036581/ 64bit
Or did you compile your own version?

Please avoid long comments by pasting long (irrelevant) debug information, the first eight lines were sufficient. Or please attach logs as files.
(Reporter)

Comment 103

a year ago
Actually, 12 lines. But they show the calBackendLoader.js :: loadBackend failure which indicates that you used a version that didn't contain the fix.
(Reporter)

Comment 104

a year ago
(In reply to :aceman from comment #100)
> I already observed a potential problem (missing feature):
My test case is simply this: With Calendar/Lightning installed and enabled, I install "Lightning Calendar Tabs". I restart, the add-on doesn't work, I restart again, it works now.
(Reporter)

Comment 105

a year ago
Posted patch 1414398-2.patch - WIP, not working (obsolete) β€” β€” Splinter Review
Just for the record: Aceman sent me this patch to check Philipp's original code. Turns out that none of onInstalled, onEnabled and onEnabling are actually called.

As the patch shows, we'd like to load the add-on's default prefs when the add-on is first "activated" during the first restart after the installation.
(Reporter)

Updated

a year ago
Attachment #8936315 - Attachment description: V5.1 → V5.1 [landed comment #99]
(Reporter)

Comment 106

a year ago
I'd coded up these events: onInstallStarted, onInstalling, onInstallEnded, onInstalled, onEnabled, onEnabling and none of them fire.

Kris, how his this meant to work? I install a restart-required add-on. On restart, the add-on is not yet in extensions.json, hence we don't load its default preferences. We'd like to get some notification for those add-ons being prepared for the first time. Is there a trick?
Flags: needinfo?(kmaglione+bmo)
(Reporter)

Comment 107

a year ago
Here's the bad new and the good news. Going back to 4.2 in the patch series instead of the landed 5.1, the problem after first restart goes away. The add-on manager *does* pass back the add-ons which has just been enabled. So should we reconsider?
(Reporter)

Comment 108

a year ago
Posted patch Going back to Philipp's original idea (obsolete) β€” β€” Splinter Review
That brings up the issue of skipping restartless extensions which isn't solved here.
Attachment #8936684 - Flags: feedback?(acelists)
(Reporter)

Comment 109

a year ago
Comment on attachment 8936684 [details] [diff] [review]
Going back to Philipp's original idea

Kris, is this a better idea? Instead of reading extensions.json (as currently implemented), get the add-ons from the add-ons manager. The ugly bit is that it runs very late after the add-ons are initialised (causing errors since they don't have their preferences), so I'm pumping the event queue so get that stuff in sync.
Attachment #8936684 - Flags: feedback?(kmaglione+bmo)
Comment hidden (obsolete)
Comment hidden (obsolete)
(Reporter)

Comment 112

a year ago
I added a "safety net" so we don't end up in an endless loop here. I noticed that we still process restartless extensions here which is undesired, so this is not ready to go yet.

Philipp, you said on IRC:
  I think bootstrapped is also available via API, just not that apparent.
Can you please give more detail.
Attachment #8936684 - Attachment is obsolete: true
Attachment #8936684 - Flags: feedback?(kmaglione+bmo)
Attachment #8936684 - Flags: feedback?(acelists)
Flags: needinfo?(philipp)
Attachment #8937479 - Flags: feedback?(kmaglione+bmo)
Attachment #8937479 - Flags: feedback?(acelists)

Comment 113

a year ago
Try addon.bootstrap . It is also used in XPIProvider.jsm and I used it in bug 1419145.
(Reporter)

Comment 114

a year ago
Thanks, my debug shows:
          dump(addon.name+"=====================================\n");
          dump(addon.type+"=====================================\n");
          dump(addon.bootstrap+"=====================================\n");

Dictionary for recipient=====================================
extension=====================================
undefined=====================================
Lightning=====================================
extension=====================================
undefined=====================================

"Dictionary for recipient" is restartless :-(
(Reporter)

Comment 115

a year ago
addon.bootstrapped also returns 'undefined'.
I'm a little confused about status. It appears from the discussion above that although we're still discussing the best fix, something was pushed to comm-central a week ago which purports to at least partially address this issue, and https://wiki.mozilla.org/Thunderbird/Add-ons_Guide_57 says, "A fix has landed on Daily 59 as of 2017-12-12," but I'm using the 2017-12-14 daily build and default preferences aren't loading for at least some of my add-ons.
(Reporter)

Comment 117

a year ago
Right. We landed a fix so Daily became usable again. The fix loads preferences for all non-restartless add-ons. However, when you install or enable and add-on, it only loads the preferences on the second restart, since on the first restart the add-on isn't fully enabled and doesn't appear in the extensions.json file.

Therefore we're discussing a better fix, one that works on first restart. However, in the second approach we haven't worked out yet how to skip restartless add-ons. The 'bootstrap' or 'bootstrapped' properties don't appear to be set.

So which of your add-ons don't have their preferences loaded? Can you nominate one for testing? Where do we download a compatible version? Which preference doesn't get loaded?
Ah. So here's what I'm seeing...

1. Quit Daily.
2. Launch 52.5.
3. Quit 52.5.
4. Launch Daily.
5. Default preferences aren't being loaded.
6. Quit Daily.
7. Launch Daily again.
8. Default preferences still aren't being loaded.
9. Disable one of my add-ons and click the Restart link.
10. Default preferences are now loaded.
(Reporter)

Comment 119

a year ago
Strange. We haven't tested any mixing with TB 52. Does it also support that extensions.json file? Anyway, I'm keen to land a second patch to try our other solution, however, that's blocked by not being able to weed out bootstrapped add-ons.

Can you clarify your comment #63 further: You suggested not to load preferences for bootstrapped add-ons, however, they should not use the defaults/preferences mechanism to start with. So are we trying to cater for an invalid case here?
(Reporter)

Comment 120

a year ago
I'm going to land this now. I'm seeing to many complaints about preferences not loading at first restart. Let's see whether this works better.

We need to follow up on the bootstrapped add-ons issue.
Attachment #8937479 - Attachment is obsolete: true
Attachment #8937479 - Flags: feedback?(kmaglione+bmo)
Attachment #8937479 - Flags: feedback?(acelists)
Attachment #8937533 - Flags: feedback?(kmaglione+bmo)
(Reporter)

Comment 121

a year ago
Forgot to refresh, this is v2b now.
Attachment #8937534 - Flags: feedback?(kmaglione+bmo)
(Reporter)

Updated

a year ago
Attachment #8937533 - Attachment is obsolete: true
Attachment #8937533 - Flags: feedback?(kmaglione+bmo)
(In reply to Jorg K (GMT+1) from comment #119)
> Can you clarify your comment #63 further: You suggested not to load
> preferences for bootstrapped add-ons, however, they should not use the
> defaults/preferences mechanism to start with. So are we trying to cater for
> an invalid case here?

The guide for converting overlay add-ons to bootstrapped (which is now at https://developer.mozilla.org/en-US/docs/Archive/Add-ons/How_to_convert_an_overlay_extension_to_restartless) provides two mechanisms for loading default preferences in a bootstrapped add-on. One of them involves essentially adding code to the bootstrapped add-on to do for it what was done by Thunderbird automatically for the overlay add-on, i.e., look in defaults/preferences and load the files there as default preferences.

A similar mechanism may have been used by people writing bootstrapped extensions from scratch rather than converting from overlay to bootstrapped. Indeed, I did exactly that for several bootstrapped extensions I wrote from scratch as bootstrapped.

If there is code in the add-on to load default preferences from defaults/preferences, and there is _also_ code in Thunderbird which starts doing the same thing, that could cause a problem.

Also: what if someone has a file in defaults/preferences in their bootstrapped add-on which they're no longer using but have neglected to remove from their shipped add-on. If Thunderbird suddenly starts loading everything in defaults/preference, then it will suddenly be loading default preferences that weren't being loaded before.

Another scenario: A bootstrapped add-on has different default preferences for different platforms. It has logic in its code to decide which file to load from defaults/preferences. The new TB code overrides that code and loads _all_ of the files in defaults/preferences. Definitely a problem.

I can't say how many add-ons are going to be affected by scenarios like this. All I can say definitively is that if TB suddenly starts loading everything in defaults/preferences for bootstrapped add-ons, that will be a backward-incompatible change in behavior which has the potential to break at least some add-ons.

Comment 123

a year ago
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/c05fa00a2d5f
Read default preferences supplied by add-ons. Use add-ons manager instead of reading extensions.json. r=me
(Reporter)

Comment 124

a year ago
Jonathan, thanks for the explanation. However, I've landed a second patch now that should solve the problem of having to restart twice. Please test this in tomorrow's Daily build.

We're looking into the bootstrapped add-ons further, but right now there has been too much confusion with the current fix, so I decided to try our other version which potentially breaks bootstrapped add-ons as you described.
(Reporter)

Updated

a year ago
Attachment #8937534 - Attachment description: 1414398-use-add-on-manager.patch (v2b) - Going back to Philipp's original idea → 1414398-use-add-on-manager.patch (v2b) - Going back to Philipp's original idea [landed comment #123]
+1 on landing that patch while trying to figure out how to exclude bootstrapped add-ons. I think that's absolutely the right call. Thanks.

Comment 126

a year ago
Comment on attachment 8937534 [details] [diff] [review]
1414398-use-add-on-manager.patch (v2b) - Going back to Philipp's original idea [landed comment #123, backout comment #144]

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

Yes, I'm for trying this approach.

::: common/src/extensionSupport.jsm
@@ +127,5 @@
> +      done = true;
> +    });
> +    let thread = Components.classes["@mozilla.org/thread-manager;1"].
> +                 getService().currentThread;
> +    while (!done && count++ < 100) {

I see this 'count' safeguard is new. What is the interval between increasing the count? Is 100 enough? When would it happen that done is never true and we hit count==100? Should this be handled in a better way? E.g. may AddonManager throw without returning the addons?
Attachment #8937534 - Flags: review+
(Reporter)

Comment 127

a year ago
(In reply to :aceman from comment #126)
> I see this 'count' safeguard is new. What is the interval between increasing
> the count? Is 100 enough? When would it happen that done is never true and
> we hit count==100? Should this be handled in a better way? E.g. may
> AddonManager throw without returning the addons?
The 'interval', as you call it, is processing one event. It's not a timer. During my debugging I've seen the "pumping event queue" debug about 20 times. Until M-C restructure internally, 100 should be plenty. Feel free to suggest some better error handling. Since all this is WIP, I don't mind running with a "less than perfect" solution for a while. We could also make it 1000, that might cause a significant delay but would still avoid a complete hang.
(Reporter)

Comment 128

a year ago
Aceman pointed out that with the original approach we still saw
Error loading backend: [Exception... "Component returned failure code: 0x8000ffff (NS_ERROR_UNEXPECTED) [nsIPrefBranch.getBoolPref]"
in the error logs on the servers. With the latest patch those errors are gone :-)
Newest patch seems to be working OK for me, and switching back and forth between 52.5 and Daily doesn't cause any problems like it was before. Thanks.
(Reporter)

Comment 130

a year ago
I've talked to Kris on IRC. Two answers:
For the bootstrapped test he said: "That's easy enough. You just check operationsRequiringRestart".
On pumping the event loop he said: "you definitely don't want to do that at startup. It's super expensive. There's an AddonManagerInternal API that telemetry uses, that doesn't require loading the add-ons DB."

And further he said: "Although, honestly, you're fighting an uphill battle here. We're going to be removing install.rdf support within the next few weeks. ... Followed by RDF support."
Flags: needinfo?(philipp)
Flags: needinfo?(kmaglione+bmo)
(Reporter)

Updated

a year ago
Attachment #8937534 - Flags: feedback?(kmaglione+bmo)
(Reporter)

Comment 132

a year ago
For the record, I get this debug dumping out addon.operationsRequiringRestart:
Dictionary for recipient
0
Lightning
6

So "Dictionary for recipient" is restartless and no operation needs restart, Lightning is installed and enabled, so currently disabling (2) or uninstalling (4) would need a restart:
https://searchfox.org/mozilla-central/rev/ff462a7f6b4dd3c3fdc53c9bc0b328f42a5b3d2b/toolkit/mozapps/extensions/AddonManager.jsm#3401

So just checking for addon.operationsRequiringRestart != AddonManager.OP_NEEDS_RESTART_NONE should be right.

Comment 133

a year ago
Comment on attachment 8937861 [details] [diff] [review]
1414398-restartless.patch [landed comment #134, backout comment #144]

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

Thanks, seems to work.
Attachment #8937861 - Flags: review+

Comment 134

a year ago
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/8ccb4ed89302
Follow-up: Skip preferences for restartless add-ons. r=aceman
(Reporter)

Updated

a year ago
Attachment #8937861 - Attachment description: 1414398-restartless.patch → 1414398-restartless.patch [landed comment #134]

Comment 135

a year ago
I tried the mozmill tests of CompactHeader, but I still do not get them to run (with Thunderbird build ID 20171219030203). But neither did the mozmill tests of Thunderbird itself work. Sorry for the suppid question: Should they (the mozmill tests included in https://ftp.mozilla.org/pub/thunderbird/nightly/latest-comm-central/thunderbird-59.0a1.en-US.win64.common.tests.zip) still work or has mozmill be replaced by something else (also for Thunderbird)?

Comment 136

a year ago
Yes, mozmill tests should work and you can see that we run them at https://treeherder.mozilla.org/#/jobs?repo=comm-central .
(Reporter)

Comment 137

a year ago
I've talked to Kris on IRC again and he told me to use the directory service with XRE_EXTENSIONS_DIR_LIST "XREExtDL".
(Reporter)

Comment 138

a year ago
Kris told me that the earliest we can run this is in profile-after-change.

This works and gives:
headerToolsLite@kaosmos.nnp.xpi======================
lightningcalendartabs@jlx.84.xpi======================
{58D4392A-842E-11DE-B51A-C7B855D89593}.xpi======================
{e2fda1a4-762b-4020-b5ad-a41df1933103}======================

*before* Lightning is complaining about the missing pref.

Apparently only active add-ons are returned. We still need to deal with bootstrapped add-ons separately again.

To apply this patch, back out 8ccb4ed89302 and c05fa00a2d5f first.
Attachment #8938185 - Flags: review?(acelists)
(Reporter)

Comment 139

a year ago
Comment on attachment 8936315 [details] [diff] [review]
V5.1 [landed comment #99]

No point for a review here. We're still chopping and changing.
Attachment #8936315 - Flags: review?(philipp)
(Reporter)

Comment 140

a year ago
Comment on attachment 8938185 [details] [diff] [review]
1414398-XRE_EXTENSIONS_DIR_LIST.patch (v1)

With this approach, the add-on is returned on first restart, so that makes this solution viable.
(Reporter)

Comment 141

a year ago
Bootstrapped add-ons are not returned in that list, so we're done here. I've adjusted comments.
Attachment #8938185 - Attachment is obsolete: true
Attachment #8938185 - Flags: review?(acelists)
Attachment #8938188 - Flags: review?(acelists)
(Reporter)

Updated

a year ago
Attachment #8936299 - Attachment is obsolete: true
(Reporter)

Updated

a year ago
Attachment #8936664 - Attachment is obsolete: true
(Reporter)

Updated

a year ago
Keywords: leave-open

Comment 142

a year ago
Comment on attachment 8938188 [details] [diff] [review]
1414398-XRE_EXTENSIONS_DIR_LIST.patch (v1b)

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

Thanks, this seems to work too, without the strange event wait loop and it should be safer to assume this reads the addons before the addon manager has a chance tho run their code (and return the list of addons).

::: common/src/extensionSupport.jsm
@@ +117,5 @@
>    }
>  
> +  // Fetch enabled non-bootstrapped add-ons.
> +  let enabledAddons = Services.dirsvc.get("XREExtDL", Ci.nsISimpleEnumerator);
> +  while (enabledAddons.hasMoreElements()) {

I would have used fixIterator here.
Attachment #8938188 - Flags: review?(acelists) → review+
(Reporter)

Comment 143

a year ago
Final version, used fixIterator().
Attachment #8938188 - Attachment is obsolete: true
Attachment #8938193 - Flags: review+

Comment 144

a year ago
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/528c352530e7
Backed out changeset 8ccb4ed89302 . a=jorgk
https://hg.mozilla.org/comm-central/rev/27a869913c53
Backed out changeset c05fa00a2d5f . a=jorgk
https://hg.mozilla.org/comm-central/rev/a5821ed6d476
use XRE_EXTENSIONS_DIR_LIST to detect add-ons. r=aceman
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
(Reporter)

Updated

a year ago
Target Milestone: --- → Thunderbird 59.0
(Reporter)

Updated

a year ago
Attachment #8937534 - Attachment description: 1414398-use-add-on-manager.patch (v2b) - Going back to Philipp's original idea [landed comment #123] → 1414398-use-add-on-manager.patch (v2b) - Going back to Philipp's original idea [landed comment #123, backout comment #144]
Attachment #8937534 - Attachment is obsolete: true
(Reporter)

Updated

a year ago
Attachment #8937861 - Attachment description: 1414398-restartless.patch [landed comment #134] → 1414398-restartless.patch [landed comment #134, backout comment #144]
Attachment #8937861 - Attachment is obsolete: true
(Reporter)

Updated

a year ago
Attachment #8938193 - Attachment description: 1414398-XRE_EXTENSIONS_DIR_LIST.patch (v1c) → 1414398-XRE_EXTENSIONS_DIR_LIST.patch (v1c) [landed comment #144]
(Reporter)

Updated

a year ago
Attachment #8936315 - Flags: approval-comm-beta+
You need to log in before you can comment on or make changes to this bug.