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

RESOLVED FIXED

Status

Testing
Mozbase
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: whimboo, Assigned: whimboo)

Tracking

unspecified
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

5 years ago
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.
(Assignee)

Comment 1

5 years ago
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.
Summary: [mozprofile] AddonManager fails in iterating over add-ons from a folder → [mozprofile] AddonManager fails in iterating over add-ons (.xpi, .jar) inside a folder
(Assignee)

Comment 2

5 years ago
Created attachment 822719 [details] [diff] [review]
Patch v1
Attachment #822719 - Flags: review?(jhammel)
(Assignee)

Updated

5 years ago
Blocks: 931828
(Assignee)

Comment 3

5 years ago
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.
Summary: [mozprofile] AddonManager fails in iterating over add-ons (.xpi, .jar) inside a folder → [mozprofile] AddonManager fails in iterating over add-ons (.xpi) inside a folder
(Assignee)

Comment 4

5 years ago
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.
Attachment #822719 - Flags: review?(jhammel)
(Assignee)

Comment 5

5 years ago
Created attachment 823551 [details] [diff] [review]
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.
Attachment #822719 - Attachment is obsolete: true
Attachment #823551 - Flags: review?(jhammel)

Comment 6

5 years ago
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.
Attachment #823551 - Flags: review?(jhammel) → review+
(Assignee)

Comment 7

5 years ago
(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.
(Assignee)

Comment 8

5 years ago
Created attachment 823926 [details] [diff] [review]
Patch v2

Dave accepted to have a quick glance over that all is fine.
Attachment #823551 - Attachment is obsolete: true
Attachment #823926 - Flags: review?(dave.hunt)
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.
Attachment #823926 - Flags: review?(dave.hunt) → review-
(Assignee)

Comment 10

5 years ago
Created attachment 823969 [details] [diff] [review]
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.
Attachment #823926 - Attachment is obsolete: true
Attachment #823969 - Flags: review?(jhammel)
(Assignee)

Updated

5 years ago
Blocks: 931333

Comment 11

5 years ago
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.
Attachment #823969 - Flags: review?(jhammel) → review+
(Assignee)

Comment 12

5 years ago
(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!
(Assignee)

Comment 13

5 years ago
Landed as:
https://github.com/mozilla/mozbase/commit/12303c6464ff0370d52db06e297472b37e01a0f3
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
(Assignee)

Updated

5 years ago
Flags: in-testsuite+

Comment 14

5 years ago
> 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.
You need to log in before you can comment on or make changes to this bug.