931188 - [mozprofile] AddonManager fails in iterating over add-ons (.xpi) inside a folder

Status: RESOLVED FIXED

(Testing :: Mozbase, defect)

Description

[Henrik Skupin [:whimboo][⌚️UTC+1]]

As of now the AddonManager fails to collect the XPI files which are located inside a directory because of checking for a directory but not an XPI file:

            addons = [os.path.join(path, x) for x in os.listdir(path) if
                      os.path.isdir(os.path.join(path, x))]

Not sure when this came in but it's clearly not the right code. I will come up with a patch for it.

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+1]]

So the problem is actually when add-ons are available as XPI or JAR files. It works fine for directories. So we have to extend our checks a bit, given that nowadays nearly no add-on will be specified in unzipped format.

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: Patch v1

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Actually we do no longer offer complete themes as .jar files, but also pack them as XPI. So no need to specifically check for them.

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 822719 [details] [diff] [review]
Patch v1

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

Given that I can't reproduce bug 931333 anytime longer I will push an update of this patch.

Comment 5

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: Patch v1.1

Sorry, that this got larger as expected now, but the tests for addons were kinda wonky. I fixed most of them except some tests we cannot do at the moment due to other known bugs.

Comment 6

[Jeff Hammel]

Comment on attachment 823551 [details] [diff] [review]
Patch v1.1

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

Nice catch + fix!

::: mozprofile/mozprofile/addons.py
@@ +48,5 @@
> +        if os.path.isdir(path):
> +            return os.path.isfile(os.path.join(path, 'install.rdf'))
> +        elif os.path.isfile(path):
> +            name, ext = os.path.splitext(path)
> +            return ext.lower() == '.xpi'

This works for now.  I in general hate behaviour to be file extension dependent, since these can be wrong and in this case we could (at least in theory) do the right thing and try to see if it is a zip file of the proper structure, but YAGNI perhaps

@@ +50,5 @@
> +        elif os.path.isfile(path):
> +            name, ext = os.path.splitext(path)
> +            return ext.lower() == '.xpi'
> +        else:
> +            return False

No real need for the else clause but fine.

Comment 7

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to Jeff Hammel [:jhammel] from comment #6)
> ::: mozprofile/mozprofile/addons.py
> @@ +48,5 @@
> > +        if os.path.isdir(path):
> > +            return os.path.isfile(os.path.join(path, 'install.rdf'))
> > +        elif os.path.isfile(path):
> > +            name, ext = os.path.splitext(path)
> > +            return ext.lower() == '.xpi'
> 
> This works for now.  I in general hate behaviour to be file extension
> dependent, since these can be wrong and in this case we could (at least in
> theory) do the right thing and try to see if it is a zip file of the proper
> structure, but YAGNI perhaps

Oh indeed! So lets not re-invent the wheel and simply create a wrapper around addon_details which doesn't throw an exception but returns a Boolean value then. Updated patch upcoming.

Comment 8

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: Patch v2

Dave accepted to have a quick glance over that all is fine.

Comment 9

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Comment on attachment 823926 [details] [diff] [review]
Patch v2

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

::: mozprofile/mozprofile/addons.py
@@ +47,5 @@
> +
> +        :param addon_path: path to the add-on directory or XPI
> +        """
> +        try:
> +            return 'id' in self.addon_details(addon_path)

Wouldn't this always return true unless an exception is thrown? The 'id' key exists but with a value of None.

Comment 10

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: Patch v3

That was indeed a glitch, which also wasn't tested yet. Thanks Dave! So I updated is_addon() and also added another test for an addon with a missing id.

Also I will again ask Jeff for review given that he has the oversight of mozprofile.

Comment 11

[Jeff Hammel]

Comment on attachment 823969 [details] [diff] [review]
Patch v3

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

::: mozprofile/mozprofile/addons.py
@@ +48,5 @@
> +        :param addon_path: path to the add-on directory or XPI
> +        """
> +        try:
> +            details = self.addon_details(addon_path)
> +            return details.get('id') is not None

I would have done return 'id' in details, but fine

@@ +49,5 @@
> +        """
> +        try:
> +            details = self.addon_details(addon_path)
> +            return details.get('id') is not None
> +        except:

Bare except is probably fine here, but always makes me nervous of covering up unintended errors.

::: mozprofile/tests/addon_stubs.py
@@ +4,5 @@
>  import os
>  import zipfile
>  
> +import mozfile
> +import mozhttpd

If you're changing import order, might as well alphabetize them.

Comment 12

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to Jeff Hammel [:jhammel] from comment #11)
> > +        try:
> > +            details = self.addon_details(addon_path)
> > +            return details.get('id') is not None
> 
> I would have done return 'id' in details, but fine

That can't be done as pointed out by Dave above. We have a default dict for details which includes also id as None by default. So we would always return true.

> > +        try:
> > +            details = self.addon_details(addon_path)
> > +            return details.get('id') is not None
> > +        except:
> 
> Bare except is probably fine here, but always makes me nervous of covering
> up unintended errors.

As discussed on IRC I filed a follow-up bug 932426.

> ::: mozprofile/tests/addon_stubs.py
> @@ +4,5 @@
> >  import os
> >  import zipfile
> >  
> > +import mozfile
> > +import mozhttpd
> 
> If you're changing import order, might as well alphabetize them.

It is except for the tempfile module, which I have now moved down between os and zipfile. mozfile and mozhttpd are separate because those are local packages.

Looks like all is fine here so I will go ahead and land it. Thanks Jeff!

Comment 13

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Landed as:
https://github.com/mozilla/mozbase/commit/12303c6464ff0370d52db06e297472b37e01a0f3

Comment 14

[Jeff Hammel]

> That can't be done as pointed out by Dave above. We have a default
> dict for details which includes also id as None by default. So we
> would always return true.

Ah, indeed.