1079451 - Log spew "No valid manifest directive." when uninstalling an add-on

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[:Margaret Leibovic]

I'm seeing this warning fill the log when uninstalling an add-on. Seems to happen with any add-on.

[JavaScript Warning: "No valid manifest directive." {file: "jar:jar:file:///data/app/org.mozilla.fennec_leibovic-1.apk!/assets/omni.ja!/components/components.manifest" line: 271}]

mfinkle, do you know what this is about? Or where we could look to debug it?

Comment 1

[:Margaret Leibovic]

Before all the warnings start, I also see this:

I/GeckoConsole(25069): Could not read chrome manifest 'file:///data/data/org.mozilla.fennec_leibovic/chrome.manifest'.

Comment 2

[Mark Finkle (:mfinkle) (use needinfo?)]

I don't really know, but...

Logged from ParseManifest:
http://mxr.mozilla.org/mozilla-central/source/xpcom/components/ManifestParser.cpp#477

Eventually shows up in XPIProvider.jsm as nsJar.getSigningCert in two places:
http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/internal/XPIProvider.jsm#1219
http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/internal/XPIProvider.jsm#5274

Comment 3

[:Margaret Leibovic]

(In reply to Mark Finkle (:mfinkle) from comment #2)

> Eventually shows up in XPIProvider.jsm as nsJar.getSigningCert in two places:
> http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/
> internal/XPIProvider.jsm#1219
> http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/
> internal/XPIProvider.jsm#5274

Both of those are from bug 1050518. I wonder if this is a regression.

Comment 4

[:Margaret Leibovic]

Hm, this happens on Nightly/Aurora, but not Beta/Release, so I don't think it's a regression from that bug.

Mossop, do you know what this could be about?

Comment 5

[Dave Townsend [:mossop]]

Looks like bug caused this. Affects desktop too.

Comment 6

[Dave Townsend [:mossop]]

That should have said bug 977026

Comment 7

[Dave Townsend [:mossop]]

I'm going to test a patch that backs out the logging from that patch, it looks unnecessary to me.

Comment 8

[Dave Townsend [:mossop]]

Attachment: patch rev 1

The test for directive->mgrfunc is unnecessary, every directive has either directive->mgrfunc or directive->regfunc. The logging only happens when the directive is for component registration and we are doing chrome-only registration so is not an error.

Comment 9

[Benjamin Smedberg]

Comment on attachment 8504142 [details] [diff] [review]
patch rev 1

Since not all directives have a mgrfunc, but we are using ->mgrfunc in this case, what makes this safe instead of potentially introducing a null-deref?

Comment 10

[Dave Townsend [:mossop]]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #9)
> Comment on attachment 8504142 [details] [diff] [review]
> patch rev 1
> 
> Since not all directives have a mgrfunc, but we are using ->mgrfunc in this
> case, what makes this safe instead of potentially introducing a null-deref?

It's safe because this is the else case from the previous "if (directive->regfunc)": http://mxr.mozilla.org/mozilla-central/source/xpcom/components/ManifestParser.cpp#752

All directives have either a mgrfunc or regfunc so if regfunc is null mgrfunc must not be null.

Comment 11

[Thinker Li [:sinker]]

(In reply to Dave Townsend [:mossop] from comment #8)
> Created attachment 8504142 [details] [diff] [review]
> patch rev 1

This patch seems to fix the issue.  My original idea is to print log for every directive not handled.  Since it annoys people, we should remove it, and I am sorry for that.
But, I suggest to have an assertion to make sure kParsingTable is always correct for debug build.

Comment 12

[Dave Townsend [:mossop]]

 https://hg.mozilla.org/integration/fx-team/rev/b008a281364d

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b008a281364d