1416480 - "List of valid built-in add-ons could not be parsed. (NS_ERROR_FILE_NOT_FOUND) [nsIXPCComponents_Utils.readURI]" at Thunderbird startup

Status: RESOLVED FIXED

(MailNews Core :: Backend, defect)

Description

[:aceman]

I get this warning in Error console at each TB startup:
1510413615502   addons.xpi      WARN    List of valid built-in add-ons could not be parsed.: [Exception... "Component returned failure code: 0x80520012 (NS_ERROR_FILE_NOT_FOUND) [nsIXPCComponents_Utils.readURI]"  nsresult: "0x80520012 (N
S_ERROR_FILE_NOT_FOUND)"  location: "JS frame :: resource://gre/modules/addons/XPIProvider.jsm :: _readAddons :: line 6386"  data: no] Stack trace: _readAddons()@resource://gre/modules/addons/XPIProvider.jsm:6386 < getAddonLocations()@re
source://gre/modules/addons/XPIProvider.jsm:6047 < getInstallState()@resource://gre/modules/addons/XPIProvider.jsm:1582 < checkForChanges()@resource://gre/modules/addons/XPIProvider.jsm:3195 < startup()@resource://gre/modules/addons/XPIP
rovider.jsm:2176 < callProvider()@resource://gre/modules/AddonManager.jsm:263 < _startProvider()@resource://gre/modules/AddonManager.jsm:730 < startup()@resource://gre/modules/AddonManager.jsm:897 < startup()@resource://gre/modules/Addon
Manager.jsm:3082 < observe()@addonManager.js:65

Does this affect TB in any way? What built-in addons may this affect? Lightning? Or the hidden internal addons that Firefox is carrying (like Pocket)? We may not have such addons, just may need to declare that in some file.

Comment 1

[Jorg K (CEST = GMT+2)]

Maybe ask Kris (:kmag).

Comment 2

[:aceman]

The _loadAddons function read from URI chrome://browser/content/built_in_addons.json but the function is in toolkit/mozapps/extensions/internal/XPIProvider.jsm (toolkit).

Kris, is this appropriate for toolkit to load a file that looks Firefox specific? Our Thunderbird URIs start with chrome://messenger/ so we probably can't define/override a chrome://browser/ URI if we wanted to provide the built_in_addons.json file. Even though our copy of mozmill does that internally.
What do you think?

The code is:
    let manifest;
    try {
      let url = Services.io.newURI(BUILT_IN_ADDONS_URI);
      let data = Cu.readURI(url);
      manifest = JSON.parse(data);
    } catch (e) {
      logger.warn("List of valid built-in add-ons could not be parsed.", e);
      return;
    }

Yes, the exception is caught and function cleanly exited, but the warning is there as if is a problem.
Maybe it could be first checked if the file at 'url' exists before reading it in (readURI is the line that throws) ?

Comment 3

[Jorg K (CEST = GMT+2)]

Clearing NI and setting it again to create some gentle noise ;-)

Comment 4

[Jorg K (CEST = GMT+2)]

That large debug is already bugging us for 2.5 months, so perhaps time to address it.

Or maybe, Aceman, you could create a patch along the lines of what you described in comment #2 and present it for review.

Comment 5

[:aceman]

Attachment: 1416480.patch

I didn't find out how to check if the file exists (actually the URL target) before trying to read it, so I use the exception result.

Comment 6

[Kris Maglione [:kmag]]

Comment on attachment 8948121 [details] [diff] [review]
1416480.patch

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

I'd rather we not make exceptions like this. If the file is missing, we should definitely show a warning. If you want to get rid of the warning in Thunderbird, you should be able to ship a file with an empty list.

Comment 7

[Jorg K (CEST = GMT+2)]

OK, we could try shipping an empty file, but as Aceman remarked in comment #2, here toolkit references a hard-coded path in browser. That doesn't appear to be right. Also see:
https://searchfox.org/mozilla-central/search?q=built_in_addons.json&case=false&regexp=false&path=

Comment 8

[Kris Maglione [:kmag]]

There's nothing that prevents you from overriding that URL. Alternatively, we could switch to using a resource:/// URL, I suppose. I'd accept patches for that.

Comment 9

[:aceman]

Yeah, I'm playing with placing a dummy file in the correct location.
I have a jar.mn that gets it into dist/bin/chrome/browser/content/built_in_addons.json but the toolkit code still says it does not exist. There must be some tiny problem in the jar.mn:
+browser.jar:
+% content browser %content/
+  content/built_in_addons.json    (built_in_addons.json)

This is the only file that would be in chrome/browser so far in TB.

Comment 10

[:aceman]

Attachment: 1416480.patch v2

This seems to work now, albeit I had to do clean build to get all the files right. Jorg, can you please try it?

Comment 11

[Jorg K (CEST = GMT+2)]

Comment on attachment 8948177 [details] [diff] [review]
1416480.patch v2

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

I'm pretty ignorant if to comes to packaging and jar files. How would comment #8 look like?
> ... overriding that URL. Alternatively, we could switch to using a resource:/// URL ...

Maybe Richard has a better opinion. Richard, note comment #7.

::: mail/extensions/jar.mn
@@ +3,5 @@
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +browser.jar:
> +% content browser %content/
> +  content/built_in_addons.json    (built_in_addons.json)

Isn't that a bit hacky? Why do we suddenly have 'browser' when usually our stuff is in 'messenger'?

Comment 12

[:aceman]

(In reply to Jorg K (GMT+1) from comment #11)
> Isn't that a bit hacky? Why do we suddenly have 'browser' when usually our
> stuff is in 'messenger'?

Yes, it's a hack that toolkit code reads a file in browser/ . But that is what I talk about in all the comments. So once it looks for chrome://browser/content/built_in_addons.json, we provide a file at that location. Yes, it is the only file we will have in browser/content. I don't know if this can be worked around differently.

Comment 13

[Richard Marti (:Paenglab)]

Attachment: 1416480.patch

I don't like that we have to create a browser directory for this.

Unfortunately this error appears very seldom here and because of this I can't test it. Aceman, could you try this patch? It should override the path to browser and use the file in messenger. Be sure the file in browser directory is deleted.

Comment 14

[:aceman]

(In reply to Richard Marti (:Paenglab) from comment #13)
> Created attachment 8948182 [details] [diff] [review]
> 1416480.patch
> 
> I don't like that we have to create a browser directory for this.

Me neither.
 
> Unfortunately this error appears very seldom here and because of this I
> can't test it.

How is that possible. The file is never there so the warning should happen at every start.

> Aceman, could you try this patch? It should override the path
> to browser and use the file in messenger.

Interesting, I'll try that.

Comment 15

[:aceman]

Comment on attachment 8948182 [details] [diff] [review]
1416480.patch

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

The patch does something but now I get NS_ERROR_ILLEGAL_VALUE from nsIXPCComponents_Utils.readUTF8URI(). So the file is found, but something is wrong with it.
I did a clean build.

Comment 16

[Philipp Kewisch [:Fallen][📱📆]]

(In reply to Jorg K (GMT+1) from comment #11)
> Comment on attachment 8948177 [details] [diff] [review]
> 1416480.patch v2
> 
> Review of attachment 8948177 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'm pretty ignorant if to comes to packaging and jar files. How would
> comment #8 look like?
> > ... overriding that URL. Alternatively, we could switch to using a resource:/// URL ...

I think that would look like this:

* change the m-c url to resource:///modules/addons/built_in_addons.json
* change https://dxr.mozilla.org/mozilla-central/source/browser/app/Makefile.in#110 to put the file into a place accessible by the url, e.g. $(FINAL_TARGET)/modules/addons/built_in_addons.json and making sure that exists

It is not technically a module, so URL and final target might be subject to input from Kris.

Comment 17

[Richard Marti (:Paenglab)]

Comment on attachment 8948177 [details] [diff] [review]
1416480.patch v2

Cancelling the f? as the other patch with the override seems to work. The other error we have now looks like we don't give the expected data. What should be in it when no system extension is installed?

Comment 18

[:aceman]

The contents of the file should be OK, it worked in my patch, we just try to change where the file is stored.
There may be some problem with the override.

Comment 19

[Philipp Kewisch [:Fallen][📱📆]]

Attachment: m-c patch to move to a resource url - v1

This is the patch for comment 8 and comment 16, which I believe will make the c-c patch more reliable. One possible downside to this approach is that I believe changing the .json file of a such resource:// url is more filesystem accessible than doing so via the chrome:// url, which may make it easier for a more casual user to work around possible signing restrictions.

aceman, for c-c all you'd need to do is add the json file to FINAL_TARGET_FILES.modules.addons

Comment 20

[Philipp Kewisch [:Fallen][📱📆]]

Attachment: c-c part - v1

As requested by aceman I'm also attaching the c-c part.

Comment 21

[:aceman]

Comment on attachment 8948835 [details] [diff] [review]
c-c part - v1

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

Thanks, this seems to work too. Can the file and moz.build change be placed in mail/extensions (similar to /firefox/features in m-c) ?

Comment 22

[Philipp Kewisch [:Fallen][📱📆]]

(In reply to :aceman from comment #21)
> Thanks, this seems to work too. Can the file and moz.build change be placed
> in mail/extensions (similar to /firefox/features in m-c) ?

I chose mail/app/moz.build because in Firefox the json file is being created in browser/app/Makefile.in. I considered mail/extensions, but since its content is rather the actual extensions, I decided against. I'm not too attached to the location though.

Comment 23

[Jorg K (CEST = GMT+2)]

Kris, could you please look at this soon so we can get this landed before the branch date next week.

Comment 24

[Kris Maglione [:kmag]]

Comment on attachment 8948714 [details] [diff] [review]
m-c patch to move to a resource url - v1

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

::: browser/app/Makefile.in
@@ +106,5 @@
>  endif
>  
>  .PHONY: features
>  tools features::
> +	$(PYTHON) -c 'import os, json; listing = {"system": sorted(os.listdir("$(DIST)/bin/browser/features"))}; print json.dumps(listing)' > $(DIST)/bin/modules/addons/built_in_addons.json

This needs to be something like $(DIST)/bin/browser/built_in_addons.json

Comment 25

[Philipp Kewisch [:Fallen][📱📆]]

I don't see any simple files in resource:///, so how does resource:///defaults/built_in_addons.json sound? This would be $(DIST)/bin/browser/defaults/built_in_addons.json then.

Comment 26

[Philipp Kewisch [:Fallen][📱📆]]

Attachment: m-c patch to move to a resource url - v2

Added a $(MKDIR) for safety. Note this is untested, I'm happy to do a try run if you like.

Comment 27

[Philipp Kewisch [:Fallen][📱📆]]

Comment on attachment 8948835 [details] [diff] [review]
c-c part - v1

I pinged Kris a while ago and he mentioned we need to keep it a chrome:// uri so tests can override the url. Therefore let's just go with either of the two approaches I will unmark as obsolete in a sec.

Comment 28

[Philipp Kewisch [:Fallen][📱📆]]

Comment on attachment 8948182 [details] [diff] [review]
1416480.patch

If this works, I'd prefer it. I can imagine it may not since we don't actually create the chrome://browser/ substitution, so maybe the other one.

Comment 29

[Philipp Kewisch [:Fallen][📱📆]]

Comment on attachment 8948177 [details] [diff] [review]
1416480.patch v2

This is the second option, which most likely will work best.

Comment 30

[Philipp Kewisch [:Fallen][📱📆]]

Comment on attachment 8948177 [details] [diff] [review]
1416480.patch v2

I see v2 is actually aceman's patch, so this needs a different reviewer.

Comment 31

[Jorg K (CEST = GMT+2)]

Thanks for getting back to this, Philipp! I was just thinking about it. I prefer attachment 8948182 [details] [diff] [review], so let's see whether that works first.

Comment 32

[Jorg K (CEST = GMT+2)]

"List of valid built-in add-ons could not be parsed." was printed with a lot of surrounding noise at every start-up in the debug console and also visible in the error console. Now I don't see it any more, not even in the beta. So a little hard to test :-(

Comment 33

[Jorg K (CEST = GMT+2)]

Still present in an old Daily 61.0a1 (2018-03-26) (64-bit). Looks like running that once now also triggers the error in a local build. Strange.

Comment 34

[Jorg K (CEST = GMT+2)]

Attachment: 1416480.patch - rebased

I rebased this and with the trick of running different versions of TB on the same profile, I can still see the error. So this solution doesn't fly.

Comment 35

[Jorg K (CEST = GMT+2)]

Comment on attachment 8971857 [details] [diff] [review]
1416480.patch - rebased

Well, Aceman can confirm my findings.

Comment 36

[:aceman]

I also see the message intermittenly. Will try the patch later.

Comment 37

[Jorg K (CEST = GMT+2)]

Comment on attachment 8948177 [details] [diff] [review]
1416480.patch v2

This doesn't fix the problem. I can reliably reproduce the problem by running an old TB 61 Daily on the profile, then the local build with Aceman's or Richard's patch.

So in conclusion: We don't have any working solution, ugly or not.

Comment 38

[Jorg K (CEST = GMT+2)]

(In reply to :aceman from comment #36)
> I also see the message intermittenly. Will try the patch later.
You have to find a way to reproduce this reliably, otherwise you can't validate the fix. As I said, running 61.0a1 (2018-03-26) (64-bit) on the profile first will cause the message when running a local build afterwards.

Comment 39

[:aceman]

So the current patch from Paenglab changes the message from FILE_NOT_FOUND to:
List of valid built-in add-ons could not be parsed.: [Exception... "Component returned failure code: 0x80070057 (NS_ERROR_ILLEGAL_VALUE) [nsIXPCComponents_Utils.readUTF8URI]"  nsresult: "0x80070057
 (NS_ERROR_ILLEGAL_VALUE)"  location: "JS frame :: resource://gre/modules/addons/XPIProvider.jsm :: _readAddons :: line 3063"  data: no] Stack trace: _readAddons()@resource://gre/modules/addons/XPIProvider.jsm:3063

We still do not know why the override does not work as expected, but it does affect the problem, it changes the error.

My patch "1416480.patch v2" (creating the file in browser/) still works (no error message).
I can reliably reproduce the error when creating a new profile.

Comment 40

[Jorg K (CEST = GMT+2)]

With Aceman's "v2" patch I get:

1525039159272	addons.xpi	WARN	List of valid built-in add-ons could not be parsed.: [Exception... "Component returned failure code: 0x80070057 (NS_ERROR_ILLEGAL_VALUE) [nsIXPCComponents_Utils.readUTF8URI]"  nsresult: "0x80070057 (NS_ERROR_ILLEGAL_VALUE)"  location: "JS frame :: resource://gre/modules/addons/XPIProvider.jsm :: _readAddons :: line 3063"  data: no] Stack trace: _readAddons()@resource://gre/modules/addons/XPIProvider.jsm:3063
getAddonLocations()@resource://gre/modules/addons/XPIProvider.jsm:2984
getInstallState()@resource://gre/modules/addons/XPIProvider.jsm:1056
checkForChanges()@resource://gre/modules/addons/XPIProvider.jsm:2075
startup()@resource://gre/modules/addons/XPIProvider.jsm:1588
callProvider()@resource://gre/modules/AddonManager.jsm:207
_startProvider()@resource://gre/modules/AddonManager.jsm:657
startup()@resource://gre/modules/AddonManager.jsm:821
startup()@resource://gre/modules/AddonManager.jsm:2887
observe()@addonManager.js:66

Which is the same result as with Richard's patch. Sorry, I didn't notice the subtle shift in the error message.

Comment 41

[Jorg K (CEST = GMT+2)]

Comment on attachment 8948177 [details] [diff] [review]
1416480.patch v2

[Triage Comment]

OK, I needed to remove messenger.manifest from the obj directory first.

Now it works. Let's go with this.

Comment 42

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/3bc91d7df1e9
provide a dummy built_in_addons.json file that toolkit is looking for. r=jorgk

Comment 43

[Jorg K (CEST = GMT+2)]

Beta (TB 60 beta 6):
https://hg.mozilla.org/releases/comm-beta/rev/099a969df7a3833ec89eff5434d18ff7403d92e4

Comment 44

[Jorg K (CEST = GMT+2)]

I see this in the beta build:

No chrome package registered for chrome://browser/content/built_in_addons.json
1525276219414	addons.xpi	WARN	List of valid built-in add-ons could not be parsed.: [Exception... "Component returned failure code: 0x80520012 (NS_ERROR_FILE_NOT_FOUND) [nsIXPCComponents_Utils.readUTF8URI]"  nsresult: "0x80520012 (NS_ERROR_FILE_NOT_FOUND)"  location: "JS frame :: resource://gre/modules/addons/XPIProvider.jsm :: _readAddons :: line 6405"  data: no] Stack trace: _readAddons()@resource://gre/modules/addons/XPIProvider.jsm:6405
getAddonLocations()@resource://gre/modules/addons/XPIProvider.jsm:6066
getInstallState()@resource://gre/modules/addons/XPIProvider.jsm:1588
getNewSideloads()@resource://gre/modules/addons/XPIProvider.jsm:3356
getNewSideloads()@resource://gre/modules/AddonManager.jsm:2991
_detectNewSideloadedAddons()@jar:file:///C:/Program%20Files/Mozilla%20Thunderbird%2060/omni.ja!/components/mailGlue.js:154
MailGlue__onMailStartupDone()@jar:file:///C:/Program%20Files/Mozilla%20Thunderbird%2060/omni.ja!/components/mailGlue.js:172
MailGlue_observe()@jar:file:///C:/Program%20Files/Mozilla%20Thunderbird%2060/omni.ja!/components/mailGlue.js:76
completeStartup()@msgMail3PaneWindow.js:544

Comment 45

[Jorg K (CEST = GMT+2)]

Firefox packages built_in_addons.json into omni.ja into chrome/browser/content/browser. We don't appear to package it.

Aceman, Richard, how do we need to package that?

Comment 46

[:aceman]

Maybe like https://hg.mozilla.org/try-comm-central/rev/3f603d78d9541a980f3aa7d27a39acc67a17819e .
Maybe for Firefox they already package browser/ folder in some way (as we do messenger/), but for us it is a new folder and manifest so we must add it to the installer-manifest.in.

Comment 47

[Jorg K (CEST = GMT+2)]

Attachment: 1416480-follow-up.patch

Thanks.

Comment 48

[Jorg K (CEST = GMT+2)]

Attachment: 1416480-follow-up.patch

Actually: Better with a comment.

Comment 49

[:aceman]

Whoah, it seems to work at first try:) The resulting package (e.g. https://queue.taskcluster.net/v1/task/Mz9Ez0SgR86e8x8cvJnTug/runs/0/artifacts/public/build/target.tar.bz2) does contain our built_in_addons.json file inside omni.ja .

So let's try to ship it and check in nightly.

Comment 50

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/049ad6ff01b8
Follow-up: package built_in_addons.json in installer. r=jorgk

Comment 51

[Jorg K (CEST = GMT+2)]

Beta (TB 60 beta 6):
https://hg.mozilla.org/releases/comm-beta/rev/6148303b6b767118e104d434cf5d279a30931d8e