983642 - AddonRepository.getAddonsByIDs fails in mutiple tps tests

Status: RESOLVED FIXED

(Testing Graveyard :: TPS, defect)

Description

[Andrei Eftimie]

Log:
> CROSSWEAVE TEST PASS: executing action VERIFY-NOT on addons
> CROSSWEAVE INFO: starting action: Addons__install
> CROSSWEAVE INFO: executing action ADD on addon "[e-mail]"
> System JS : ERROR resource://gre/modules/services-sync/addonutils.js:360 - Error: AddonRepository search failed
> CROSSWEAVE ERROR: [phase01] Exception caught: AddonRepository search failed JS Stack trace: [e-mail]:360:9 < [e-mail]:955:5 < this.AddonRepository._beginSearch/<@AddonRepository.jsm:1426:9 < [e-mail]:99:7 < makeSpinningCallback/[e-mail]:141:32 < [e-mail]:99:1 < [e-mail]:386:11 < [e-mail]:917:5 < [e-mail]:521:7 < [e-mail]:533:5 < [e-mail]:533:5
> 
> TEST-UNEXPECTED-FAIL | test_addon_nonrestartless_xpi.js | [phase01] Exception caught: AddonRepository search failed JS Stack trace: [e-mail]:360:9 < [e-mail]:955:5 < this.AddonRepository._beginSearch/<@AddonRepository.jsm:1426:9 < [e-mail]:99:7 < makeSpinningCallback/[e-mail]:141:32 < [e-mail]:99:1 < [e-mail]:386:11 < [e-mail]:917:5 < [e-mail]:521:7 < [e-mail]:533:5 < [e-mail]:533:5

Afftected tests:
> test_addon_sanity.js
> test_addon_restartless_xpi.js
> test_addon_nonrestartless_xpi.js
> test_addon_reconciling.js
> test_addon_wipe.js

Comment 1

[Andrei Eftimie]

Lets try getting to the bottom of this since it's affecting 5 tests.

Comment 2

[Andrei Eftimie]

Installing an addon fails when we search for it here:
http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/internal/AddonRepository.jsm#1408

We get a 404 response on the URI:
http://127.0.0.1:4567/en-US/firefox/api/1.5/search/guid:unsigned-xpi%40tests.mozilla.org

Comment 3

[Andrei Eftimie]

While the test is running I have on http://127.0.0.1:4567/ the directory listing for 
> services/sync/tests/tps
which doesn't include any addon search api

Comment 4

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

Which pref is controlling the AMO search? Does a newer Mozmill release fix that? I could imagine that given that we disabled 'extensions.getAddons.cache.enabled'.

Comment 5

[Andrei Eftimie]

Attachment: WIP1.patch

Here is a hack that fixes the issue.

Basically we want to fetch the required addon xml which is located directly in the delivered folder. All addon install problems are solved by it:
> TEST-PASS | test_addon_sanity.js
> 
> 	phase1: PASS
> 	phase2: PASS
> 
> TEST-PASS | test_addon_restartless_xpi.js
> 
> 	phase01: PASS
> 	phase02: PASS
> 	phase03: PASS
> 	phase04: PASS
> 	phase05: PASS
> 	phase06: PASS
> 	phase07: PASS
> 	phase08: PASS
> 
> TEST-UNEXPECTED-FAIL | test_addon_nonrestartless_xpi.js | [phase09] Exception caught: ASSERTION FAILED! Weave status OK; expected "success.status_ok", got "error.login.failed" No traceback available
> 
> 	phase01: PASS
> 	phase02: PASS
> 	phase03: PASS
> 	phase04: PASS
> 	phase05: PASS
> 	phase06: PASS
> 	phase07: PASS
> 	phase08: PASS
> 	phase09: FAIL
> 
> TEST-UNEXPECTED-FAIL | test_addon_reconciling.js | [phase06] Exception caught: ASSERTION FAILED! addon [e-mail] is present, but it shouldn't be No traceback available
> 
> 	phase01: PASS
> 	phase02: PASS
> 	phase03: PASS
> 	phase04: PASS
> 	phase05: PASS
> 	phase06: FAIL
> 
> TEST-PASS | test_addon_wipe.js
> 
>   phase01: PASS
>   phase02: PASS
>   phase03: PASS

We'll need however a better solution.

1) The "search" query was supposed to work for multiple addons, hence we could sent an array. If we take directly an addon xml, this is no longer the case. Right now this would fail if we'd try to have multiple addons since we directly fetch the xml.

2) I think it would be better to handle the ID > NAME transformation in the testrunner python side. But I'm not sure how we should approach this.

Comment 6

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

Comment on attachment 8393440 [details] [diff] [review]
WIP1.patch

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

::: testing/tps/tps/testrunner.py
@@ -59,5 @@
>                              'browser.tabs.warnOnClose' : False,
>                              'browser.warnOnQuit': False,
>                              # Allow installing extensions dropped into the profile folder
>                              'extensions.autoDisableScopes': 10,
> -                            'extensions.getAddons.get.url': 'http://127.0.0.1:4567/en-US/firefox/api/%API_VERSION%/search/guid:%IDS%',

Have you checked with extensions logging enabled what happens? I don't want to see proposals until we do not know what is the underlying issue.

::: toolkit/mozapps/extensions/internal/AddonRepository.jsm
@@ +781,5 @@
>  
>      let params = {
>        API_VERSION : API_VERSION,
> +      IDS : ids.map(encodeURIComponent).join(','),
> +      NAMES : ids.map((el)=>el.split("@")[0]).join(',')

We will not make modifications to Firefox core code. That's totally out of question.

Comment 7

[Andrei Eftimie]

(In reply to Henrik Skupin (:whimboo) from comment #6)
> Comment on attachment 8393440 [details] [diff] [review]
> WIP1.patch
> 
> Review of attachment 8393440 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: testing/tps/tps/testrunner.py
> @@ -59,5 @@
> >                              'browser.tabs.warnOnClose' : False,
> >                              'browser.warnOnQuit': False,
> >                              # Allow installing extensions dropped into the profile folder
> >                              'extensions.autoDisableScopes': 10,
> > -                            'extensions.getAddons.get.url': 'http://127.0.0.1:4567/en-US/firefox/api/%API_VERSION%/search/guid:%IDS%',
> 
> Have you checked with extensions logging enabled what happens? I don't want
> to see proposals until we do not know what is the underlying issue.

I've now enabled `extensions.logging.enabled` but I'm not sure what you want me to look for.

As for what's wrong, I've explained what's happening in previous comments.

Lets have another go at it:
a) We expect a working AMO-like api to be available at http://127.0.0.1:4567/, which would answer to requests like: http://127.0.0.1:4567/en-US/firefox/api/%API_VERSION%/search/guid:%IDS%
This is not the case right now.

Is this the case?
Who should deliver such an API locally?
(You've responded to me on IRC that mozhttpd yestrday. Well sure, but that is the webserver. Who delivers the API implementation? I haven't been able to find anything that should do this, no documentation.

b) As a hack I tried to deliver the wanted extension xml file (in the previous WIP patch) and it worked. So the AMO-like API should return the extension xml file.

=====

To reiterate. I don't know what the underlying issue is.
Because I don't know how this system is _supposed_ to work.

Comment 8

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

(In reply to Andrei Eftimie from comment #7)
> (You've responded to me on IRC that mozhttpd yestrday. Well sure, but that
> is the webserver. Who delivers the API implementation? I haven't been able
> to find anything that should do this, no documentation.

Why do you expect complicated code? mozhttpd simply will return static content, that's all. But as you said right now it doesn't really work because the URL fragment cannot be resolved by mozhttpd, so a 404 gets returned. So modify the search API URL to something which is resolvable, and check that mozhttpd really returns valid content.

> b) As a hack I tried to deliver the wanted extension xml file (in the
> previous WIP patch) and it worked. So the AMO-like API should return the
> extension xml file.

Here we are. Please check the above so that this file gets returned via the HTTP request call.

> To reiterate. I don't know what the underlying issue is.
> Because I don't know how this system is _supposed_ to work.

That's why investigation is necessary, which also means to figure out how the system is supposed to work.

Comment 9

[Andrei Eftimie]

Attachment: 2.patch

Ok then, here's another approach.

I think we're on the right track.
I've left the initial url scheme intact and added a custom urlhandler in mozhttpd that handles these particular requests.

This handler constructs the the name of the wanted xml file from the addon id, fetches its content and transparently delivers it as the content in the original request.

Comment 10

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

Comment on attachment 8394102 [details] [diff] [review]
2.patch

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

::: testing/tps/tps/testrunner.py
@@ +402,5 @@
> +        self.mozhttpd = mozhttpd.MozHttpd(port=4567,
> +                                          docroot=testdir,
> +                                          urlhandlers=[
> +                                            {'method': 'GET',
> +                                             'path': '/en-US/firefox/api/',

You might want to modify the pref so we can directly point to the root folder, which already contains the XPI file. I don't think that any additional handler would be necessary here.

Comment 11

[Andrei Eftimie]

(In reply to Henrik Skupin (:whimboo) from comment #10)
> Comment on attachment 8394102 [details] [diff] [review]
> 2.patch
> 
> Review of attachment 8394102 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: testing/tps/tps/testrunner.py
> @@ +402,5 @@
> > +        self.mozhttpd = mozhttpd.MozHttpd(port=4567,
> > +                                          docroot=testdir,
> > +                                          urlhandlers=[
> > +                                            {'method': 'GET',
> > +                                             'path': '/en-US/firefox/api/',
> 
> You might want to modify the pref so we can directly point to the root
> folder, which already contains the XPI file. I don't think that any
> additional handler would be necessary here.

We need a handler to transform the requested url.

What we get now is
> http://127.0.0.1:4567/en-US/firefox/api/%API_VERSION%/search/guid:%IDS%

We can change this, but %IDS% has the following form:
> [e-mail],[e-mail],[e-mail]

In our particular tests we have only 1 entry:
> [e-mail]

The file we need is located in:
> /addon1.xml

We need the handler to make this transformation.

If we go to simplify the url, I'd propose something simple, yet still clear, along these lines:
> http://127.0.0.1:4567/api/getAddonXML/%IDS%

Comment 12

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

Hm, so lets make it '/addons/api/%IDS%'. But then we should also be able to handle multiple addons. How are those getting returned by the official API?

Comment 13

[Andrei Eftimie]

If we map original link we use on the real API we have this:
https://services.addons.mozilla.org/en-US/firefox/api/1.5/search/guid:[e-mail]

With multiple entries:
https://services.addons.mozilla.org/en-US/firefox/api/1.5/search/guid:[e-mail],%20%7B8620c15f-30dc-4dba-a131-7c5d20cf4a29%7D
(Firebug and Nightly Test Tools)

Comment 14

[Andrei Eftimie]

Attachment: 3.patch

We now use the following url `/addons/api/%IDS%`.
There's no easy way of simultaneously installing multiple addons, we would have to  dynamically craft the XML responses. Instead we now raise an exception if this is attempted. We can still easily install multiple addons.

For example (in a test file), this works:
>  [Addons.install, ["[e-mail]"]],
>  [Addons.install, ["[e-mail]"]],

This also works:
>  [Addons.install, ["[e-mail]", "[e-mail]"]],

We throw if the following is attempted:
>  [Addons.install, ["[e-mail],[e-mail]"]],

Comment 15

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

Comment on attachment 8394640 [details] [diff] [review]
3.patch

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

This patch is not ready yet, so I don't see a reason for review. Please ask once it is ready. In the meantime you can attach WIP with the current work.

::: testing/tps/tps/testrunner.py
@@ +370,5 @@
> +    def handle_addons(self, request):
> +        ids = urllib.unquote(request.path).split('/')[-1].split(',')
> +        if len(ids) > 1:
> +            raise Exception("Multiple simultaneous addon installations are not"\
> +                            " supported. Install them sequentially.")

This is not an exception you want to raise on the Python side. Here we simply return the result for the first add-on to look for.

tps's addons.jsm needs an update to sequentially install add-ons and not on one go.

Comment 16

[Andrei Eftimie]

Attachment: 0004-Bug-983642-Add-a-custom-urlhandler-that-returns-the-.patch

We're no longer throwing anything.

Also changed tps' addons.jsm to be able to handle multiple addons. It will install them sequentially. I would have added a test for it, but this shouldn't be in a general Sync tests since it would test the framework, not the sync mechanism.

All addon tests are being correctly run now.
Most are passing. 2 of them are failing in the last Phase, but that is a different issue.

Comment 17

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

Comment on attachment 8395613 [details] [diff] [review]
0004-Bug-983642-Add-a-custom-urlhandler-that-returns-the-.patch

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

::: services/sync/tps/extensions/tps/resource/modules/addons.jsm
@@ +92,5 @@
>  
>    install: function install() {
>      // For Install, the id parameter initially passed is really the filename
>      // for the addon's install .xml; we'll read the actual id from the .xml.
> +    let ids = this.id.split(',');

Where is `this.id` set? If that really holds a list of ids, it's named wrongly and should be updated. Maybe its setter should already split the string into an array.

@@ +97,4 @@
>  
> +    ids.forEach(function (id) {
> +      let cb = Async.makeSpinningCallback();
> +      AddonUtils.installAddons([{id: id, requireSecureURI: false}], cb);

Please add a comment for the loop, so everyone knows why we have to do that.

::: testing/tps/tps/testrunner.py
@@ +366,5 @@
>          print 'Test Summary\n'
>          for test in self.postdata.get('tests', {}):
>              print '%s | %s | %s' % (test['state'], test['name'], test['message'])
>  
> +    def handle_addons(self, request):

Please flag this with '@mozhttpd.handlers.json_response'. Also I would suggest a name like httpd_search_addon().

@@ +369,5 @@
>  
> +    def handle_addons(self, request):
> +        ids = urllib.unquote(request.path).split('/')[-1].split(',')
> +        id = ids[0]
> +        name = id.split('@')[0] + '.xml'

Instead of doing that please rename the appropriate XML file so it has the ID as name.

@@ +370,5 @@
> +    def handle_addons(self, request):
> +        ids = urllib.unquote(request.path).split('/')[-1].split(',')
> +        id = ids[0]
> +        name = id.split('@')[0] + '.xml'
> +        response = urllib.urlopen('http://127.0.0.1:4567/' + name)

I don't see why another HTTP request has to be made here. Read it directly. I would also propose to stick the XML file into the addons/api folder.

Comment 18

[Andrei Eftimie]

Attachment: 0005-Bug-983642-Match-addon-xmls-and-addon-install-url-by.patch

(In reply to Henrik Skupin (:whimboo) from comment #17)
> Comment on attachment 8395613 [details] [diff] [review]
> 0004-Bug-983642-Add-a-custom-urlhandler-that-returns-the-.patch
> 
> Review of attachment 8395613 [details] [diff] [review]:
> -----------------------------------------------------------------
> @@ +369,5 @@
> >  
> > +    def handle_addons(self, request):
> > +        ids = urllib.unquote(request.path).split('/')[-1].split(',')
> > +        id = ids[0]
> > +        name = id.split('@')[0] + '.xml'
> 
> Instead of doing that please rename the appropriate XML file so it has the
> ID as name.

This is actually a good idea.
Attached is the most simple fix we can have. Just deliver the xml directly.

I propose to go to a route like this one. Let's not delve deeper into the fact that in theory if we request multiple addons simultaneously we'll get an array (http://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/internal/AddonRepository.jsm?from=AddonRepository.jsm#784).

This would only happen if we were to call multiple ids as a comma separated string in our tests. I'm not worrying about this right now.

We can always refactor code later if we feel we'll need that. To worry about that would be out of scope of our current problems.

Comment 19

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

Comment on attachment 8396249 [details] [diff] [review]
0005-Bug-983642-Match-addon-xmls-and-addon-install-url-by.patch

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

Well, if we want to keep it minimalistic lets at least add the addons/api subfolder and move the xml files in there, so we have a clear distinction between different types of data. Having all in one folder is a mess. With that change you will get my r+.

I would still like to see that we internally limit the call to a single add-on, but feel free to file a new bug for that.

Comment 20

[Andrei Eftimie]

Attachment: 0006-Bug-983642-Match-addon-xmls-and-addon-install-url-by.patch

As discussed on IRC, I've moved the addons under /addons/ and the xml descriptors under /addons/api

Comment 21

[Andrei Eftimie]

Comment on attachment 8396355 [details] [diff] [review]
0006-Bug-983642-Match-addon-xmls-and-addon-install-url-by.patch

I missed to add the review flag.

Comment 22

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

Comment on attachment 8396355 [details] [diff] [review]
0006-Bug-983642-Match-addon-xmls-and-addon-install-url-by.patch

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

The patch looks great but I have a problem to apply it to the mercurial repository with the patch command. The binary files are not getting updated, and also when I run the test it fails in the search code: "addonutils.js:360 - Error: AddonRepository search failed".

Comment 23

[Andrei Eftimie]

Comment on attachment 8396355 [details] [diff] [review]
0006-Bug-983642-Match-addon-xmls-and-addon-install-url-by.patch

(In reply to Henrik Skupin (:whimboo) from comment #22)
> Comment on attachment 8396355 [details] [diff] [review]
> 0006-Bug-983642-Match-addon-xmls-and-addon-install-url-by.patch
> 
> Review of attachment 8396355 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The patch looks great but I have a problem to apply it to the mercurial
> repository with the patch command. The binary files are not getting updated,
> and also when I run the test it fails in the search code: "addonutils.js:360
> - Error: AddonRepository search failed".

Why are you using patch and not hg import?

I do my dev work in git against http://github.com/mozilla/gecko-dev and run all attached patches through `git-patch-to-hg-patch` from https://github.com/mozilla/moz-git-tools

I've just applied this patch using hg import and it works great.

The error you see is because of the missing (or bad location of the) binary files.

Could you please try applying the patch again with `hg import`?

Comment 24

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

Well, I cannot use hg qimport for the git repository. The patch command was usually always working, but not I converted the patch to a git patch, and git am worked fine. But I still cannot get mozhttpd to work. For whatever URL I use a 404 is returned. I'm not sure why this is happening. I will have to investigate that first before I can test this patch.

Comment 25

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

Comment on attachment 8396355 [details] [diff] [review]
0006-Bug-983642-Match-addon-xmls-and-addon-install-url-by.patch

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

My venv was broken. After re-creating it all works fine. Thanks Andrei!

Comment 26

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

Attachment: Patch for landing

The patch didn't have the right commit message ([PATCH] left in, and no DONTBUILD suffix). I also updated the message itself to match the bug summary.

Comment 27

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/036eab21ad2a

Andrei, once merged to m-c please request approval for aurora on the patch.

Comment 28

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/036eab21ad2a

Comment 29

[Andrei Eftimie]

Comment on attachment 8398032 [details] [diff] [review]
Patch for landing

[Approval Request Comment]
Bug caused by (feature/regressing bug #): None
User impact if declined: None, automated tests will fail
Testing completed (on m-c, etc.): Yes
Risk to taking this patch (and alternatives if risky): None
String or IDL/UUID changes made by this patch: None

Comment 30

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8398032 [details] [diff] [review]
Patch for landing

If 29 is also affected, why are you not requesting an uplift for beta?

Comment 31

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

(In reply to Sylvestre Ledru [:sylvestre] from comment #30)
> If 29 is also affected, why are you not requesting an uplift for beta?

None of the tests are running on beta yet. For now we are targeting to get CI runs for Nightly and Aurora. If we have to test beta, we will request a further backport.

Comment 32

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/b20905dda53d