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

RESOLVED FIXED in Thunderbird 61.0

Status

defect
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: aceman, Assigned: Fallen)

Tracking

Trunk
Thunderbird 61.0

Thunderbird Tracking Flags

(thunderbird60+ fixed, thunderbird61 fixed)

Details

Attachments

(2 attachments, 7 obsolete attachments)

Reporter

Description

2 years ago
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.
Reporter

Updated

2 years ago
Summary: "List of valid built-in add-ons could not be parsed." at Thunderbird startup → "List of valid built-in add-ons could not be parsed. (NS_ERROR_FILE_NOT_FOUND) [nsIXPCComponents_Utils.readURI]" at Thunderbird startup

Comment 1

2 years ago
Maybe ask Kris (:kmag).
Reporter

Comment 2

2 years ago
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) ?
Flags: needinfo?(kmaglione+bmo)

Comment 3

a year ago
Clearing NI and setting it again to create some gentle noise ;-)
Flags: needinfo?(kmaglione+bmo)

Comment 4

a year ago
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.
Flags: needinfo?(kmaglione+bmo)
Reporter

Comment 5

a year ago
Posted patch 1416480.patch (obsolete) β€” β€” Splinter Review
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.
Attachment #8948121 - Flags: review?(kmaglione+bmo)
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.
Attachment #8948121 - Flags: review?(kmaglione+bmo) → review-

Comment 7

a year ago
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=
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.
Flags: needinfo?(kmaglione+bmo)
Reporter

Comment 9

a year ago
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.
Reporter

Comment 10

a year ago
Posted patch 1416480.patch v2 β€” β€” Splinter Review
This seems to work now, albeit I had to do clean build to get all the files right. Jorg, can you please try it?
Attachment #8948121 - Attachment is obsolete: true
Attachment #8948177 - Flags: feedback?(jorgk)

Comment 11

a year ago
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'?
Attachment #8948177 - Flags: feedback?(jorgk) → feedback?(richard.marti)
Reporter

Comment 12

a year ago
(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.
Posted patch 1416480.patch (obsolete) β€” β€” Splinter Review
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.
Attachment #8948182 - Flags: feedback?(acelists)
Reporter

Comment 14

a year ago
(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.
Reporter

Comment 15

a year ago
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.
(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 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?
Attachment #8948177 - Flags: feedback?(richard.marti)
Reporter

Comment 18

a year ago
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.
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
Attachment #8948714 - Flags: review?(kmaglione+bmo)
Posted patch c-c part - v1 (obsolete) β€” β€” Splinter Review
As requested by aceman I'm also attaching the c-c part.
Assignee: nobody → philipp
Attachment #8948177 - Attachment is obsolete: true
Attachment #8948182 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8948182 - Flags: feedback?(acelists)
Attachment #8948835 - Flags: review?(acelists)
Reporter

Comment 21

a year ago
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) ?
Attachment #8948835 - Flags: review?(acelists) → review+
(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

a year ago
Kris, could you please look at this soon so we can get this landed before the branch date next week.
Flags: needinfo?(kmaglione+bmo)
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
Attachment #8948714 - Flags: review?(kmaglione+bmo)
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.
Added a $(MKDIR) for safety. Note this is untested, I'm happy to do a try run if you like.
Attachment #8948714 - Attachment is obsolete: true
Flags: needinfo?(kmaglione+bmo)
Attachment #8958775 - Flags: review?(kmaglione+bmo)
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.
Attachment #8948835 - Attachment is obsolete: true
Attachment #8958775 - Attachment is obsolete: true
Attachment #8958775 - Flags: review?(kmaglione+bmo)
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.
Attachment #8948182 - Flags: review?(acelists)
Attachment #8948182 - Attachment is obsolete: false
Comment on attachment 8948177 [details] [diff] [review]
1416480.patch v2

This is the second option, which most likely will work best.
Attachment #8948177 - Attachment is obsolete: false
Attachment #8948177 - Flags: review?(acelists)
Comment on attachment 8948177 [details] [diff] [review]
1416480.patch v2

I see v2 is actually aceman's patch, so this needs a different reviewer.
Attachment #8948177 - Flags: review?(acelists) → review?(jorgk)

Comment 31

a year ago
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

a year ago
"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

a year ago
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

a year ago
Posted patch 1416480.patch - rebased (obsolete) β€” β€” Splinter Review
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.
Attachment #8948182 - Attachment is obsolete: true
Attachment #8948182 - Flags: review?(acelists)
Attachment #8971857 - Flags: feedback-

Comment 35

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

Well, Aceman can confirm my findings.
Attachment #8971857 - Flags: review?(acelists)
Reporter

Comment 36

a year ago
I also see the message intermittenly. Will try the patch later.

Comment 37

a year ago
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.
Attachment #8948177 - Flags: review?(jorgk) → feedback-

Comment 38

a year ago
(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.
Reporter

Comment 39

a year ago
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

a year ago
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.

Updated

a year ago
Attachment #8971857 - Attachment is obsolete: true
Attachment #8971857 - Flags: review?(acelists)

Comment 41

a year ago
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.
Attachment #8948177 - Flags: review+
Attachment #8948177 - Flags: feedback-
Attachment #8948177 - Flags: approval-comm-beta+

Updated

a year ago
Keywords: checkin-needed

Comment 42

a year ago
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/3bc91d7df1e9
provide a dummy built_in_addons.json file that toolkit is looking for. r=jorgk
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
Keywords: checkin-needed
Resolution: --- → FIXED

Updated

a year ago
Target Milestone: --- → Thunderbird 61.0

Comment 44

a year ago
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Comment 45

a year ago
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?
Flags: needinfo?(richard.marti)
Flags: needinfo?(acelists)
Reporter

Comment 46

a year ago
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.
Flags: needinfo?(acelists)

Comment 47

a year ago
Posted patch 1416480-follow-up.patch (obsolete) β€” β€” Splinter Review
Thanks.
Flags: needinfo?(richard.marti)
Attachment #8972721 - Flags: review+
Attachment #8972721 - Flags: approval-comm-beta+

Comment 48

a year ago
Actually: Better with a comment.
Attachment #8972721 - Attachment is obsolete: true
Attachment #8972722 - Flags: review+
Attachment #8972722 - Flags: approval-comm-beta+
Reporter

Comment 49

a year ago
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.
Keywords: checkin-needed

Comment 50

a year ago
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/049ad6ff01b8
Follow-up: package built_in_addons.json in installer. r=jorgk
Status: REOPENED → RESOLVED
Last Resolved: a year agoa year ago
Keywords: checkin-needed
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.