782118 - Dictionary add-ons cannot be updated from addons.mozilla.org

Status: RESOLVED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Jesper Kristensen]

Attachment: patch v1

Bug 591780 introduced a new <em:type>64</em:type> for dictionaries. However these new dictionaries cannot be automatically updated from addons.mozilla.org.

Steps to reproduce:
Download https://addons.mozilla.org/da/firefox/addon/dansk-ordbog/
Edit install.rdf to a lower version number
Install the modified xpi
Check for updates

Expected result:
The latest version from addons.mozilla.org should be downloaded and installed

Actual result:
The add-ons manager claims there are no updates.

This happens because addons.mozilla.org sends an update manifest containing "urn:mozilla:extension:" whereas Firefox expects one containing "urn:mozilla:item:".

I don't know if this should be fixed on the server side or the application side, but I have attached a patch, which fixes it on the application side, and filed this bug under Add-ons Manager.

Comment 1

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 651190 [details] [diff] [review]
patch v1

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

This will break dictionaries that are not hosted on AMO and are currently successfully updating. So I'd think it should be fixed on AMO's side.

(Arguably the most correct prefix is urn:mozilla:dictionary:, but that has the same problem as above.)

Comment 2

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Bouncing this to AMO (Hi Rob! I think you worked on the update script last).

TL;DR:
update.rdf needs to have "urn:mozilla:item:" instead of "urn:mozilla:extension:" for dictionaries (type=64).

Comment 3

[Jesper Kristensen]

(In reply to Blair McBride (:Unfocused) from comment #1)
> This will break dictionaries that are not hosted on AMO and are currently
> successfully updating.

I don't think such add-ons exist, given that this add-on type is relatively new (introduced in Firefox 10) and its existence has not been communicated broadly.

> So I'd think it should be fixed on AMO's side.

That would seem like the more pure fix. I mostly just made the patch in Firefox to confirm that this really was the cause.

(In reply to Blair McBride (:Unfocused) from comment #2)
> TL;DR:
> update.rdf needs to have "urn:mozilla:item:" instead of
> "urn:mozilla:extension:" for dictionaries (type=64).

No, I don't think this would be enough.

We need to support updates from type=2 to type=64 as well as from type=64 to type=64. Therefore update.rdf needs to know the type of the old, already installed version of the add-on (as far as I understand). A version, which might not even be hosted on AMO. I don't think AMO has the needed information from the update ping URL: https://versioncheck.addons.mozilla.org/update/VersionCheck.php?reqVersion=2&id=[e-mail]&version=2.2.0.1&maxAppVersion=16.0a1&status=userEnabled&appID={ec8030f7-c20a-464f-9b0e-13a3a9e97384}&appVersion=17.0a1&appOS=WINNT&appABI=x86-msvc&locale=en-US&currentAppVersion=17.0a1&updateType=97&compatMode=normal

Comment 4

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to Jesper Kristensen from comment #3)
> I don't think such add-ons exist, given that this add-on type is relatively
> new (introduced in Firefox 10) and its existence has not been communicated
> broadly.

Sadly, this isn't something we're able to collect any data on :\


> I don't think AMO has the needed
> information from the update ping URL:

I really want to avoid changing the update URL just for transitioning dictionaries.

If it ends up being that complicated, I think we'd be better off just doing what the patch here does.

Comment 5

[Jesper Kristensen]

I just made some tests of updating between the different types with a custom updateURL. As I described in comment 3 the server does need to know the correct prefix for the old installed version of the add-on, not the new version.

AMO could find this information, if it has the old version available, but not if the old version is one AMO does not know about.

I think this should be fixed on the application side.

A different fix on the application side could be to have it ignore the prefix, by trying all three. The different types of add-ons are installed side by side in a common directory structure, so you cannot have a theme and an extension installed with the same ID anyway. So why do we need to tell them apart in the update manifest? Ignoring the prefix makes sure no users are stranded on an old version because of a wrong prefix.

What do you say, Blair?

Comment 6

[Jesper Kristensen]

Attachment: Make addons manager updage from one type to another

This is a change to the application side, which makes it ignore the type of the addon, allowing you to update an addon of any type to an addon of any other type. This should leave no one stranded due to a wrong prefix in the update rdf.

Comment 7

[Jesper Kristensen]

Attachment: Make AMO deliver all three updates

I just found a way to fix this on the server side anyway: Just serve many updates for all types in one update rdf.

Comment 8

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Ugh, sorry, I was sure I had replied to comment 5.

(In reply to Jesper Kristensen from comment #6)
> This is a change to the application side, which makes it ignore the type of
> the addon, allowing you to update an addon of any type to an addon of any
> other type.

Dave: Do you know the history behind this?

Presumably this was originally done for a reason, and presumably it was disallow updating between types. And its certainly something we have absolutely *no* test coverage for. So I'm rather nervous about allowing updating from any type to any type.

Comment 9

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 657662 [details] [diff] [review]
Make addons manager updage from one type to another

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

This could be adapted to that when aType == "dictionary", it first looks for anything under PREFIX_ITEM via ds.ArcLabelsOut(itemRes).hasMoreElements(). If that fails, it can fall back to PREFIX_EXTENSION. Its late and by brain is fuzzy now, but I think that would solve both problems, and avoid allowing arbitrary updating between types.

And as a correctness followup, AMO's update response can be changed to use the PREFIX_ITEM prefix.

Comment 10

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to Blair McBride (:Unfocused) from comment #8)
> So I'm rather nervous about allowing
> updating from any type to any type.

Another reason to be nervous about this is that themes and dictionaries do not allow execution of code. Disallowing this sort of updating between types doesn't prevent updating to an extension, but it does make it harder to accidentally do it.

Comment 11

[Dave Townsend [:mossop]]

(In reply to Blair McBride (:Unfocused) from comment #8)
> Ugh, sorry, I was sure I had replied to comment 5.
> 
> (In reply to Jesper Kristensen from comment #6)
> > This is a change to the application side, which makes it ignore the type of
> > the addon, allowing you to update an addon of any type to an addon of any
> > other type.
> 
> Dave: Do you know the history behind this?
> 
> Presumably this was originally done for a reason, and presumably it was
> disallow updating between types. And its certainly something we have
> absolutely *no* test coverage for. So I'm rather nervous about allowing
> updating from any type to any type.

I could see an argument for that, since we used to (way back when) install themes with a much simpler confirmation, almost no warning as we thought they couldn't execute script. But this doesn't actually stop that since you can still manipulate the update.rdf to make it work.

This was all before even my time though so maybe Rob has an idea, but I don't see a reason to keep the restriction around.

Comment 12

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 657662 [details] [diff] [review]
Make addons manager updage from one type to another

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

Ok, based on comment 11, lets just go with this. Sorry for the back and forward in this bug - I'm generally a bit paranoid when it comes to changing how updating works.

It needs a test though, by adding to:
* toolkit/mozapps/extensions/test/xpcshell/test_update.js - for the general update case, using an update manifest with a different prefix
* toolkit/mozapps/extensions/test/xpcshell/test_dictionary.js - for the specific case of updating a dictionary from type=extension to type=dictionary

::: toolkit/mozapps/extensions/AddonUpdateChecker.jsm
@@ +267,5 @@
>    rdfParser.parseString(ds, aRequest.channel.URI, aRequest.responseText);
>  
> +  // A update manifest can use one of three URL prefixes.
> +  // We don't care which one it uses, because we should be able to update
> +  // from one type to another, e.g. from extension to item.

Lets just make this comment say the old way of differentiated between update types is deprecated.

Comment 13

[Robert Strong (they/them - no direct email)]

(In reply to Dave Townsend (:Mossop) from comment #11)
> (In reply to Blair McBride (:Unfocused) from comment #8)
> > Ugh, sorry, I was sure I had replied to comment 5.
> > 
> > (In reply to Jesper Kristensen from comment #6)
> > > This is a change to the application side, which makes it ignore the type of
> > > the addon, allowing you to update an addon of any type to an addon of any
> > > other type.
> > 
> > Dave: Do you know the history behind this?
> > 
> > Presumably this was originally done for a reason, and presumably it was
> > disallow updating between types. And its certainly something we have
> > absolutely *no* test coverage for. So I'm rather nervous about allowing
> > updating from any type to any type.
> 
> I could see an argument for that, since we used to (way back when) install
> themes with a much simpler confirmation, almost no warning as we thought
> they couldn't execute script. But this doesn't actually stop that since you
> can still manipulate the update.rdf to make it work.
> 
> This was all before even my time though so maybe Rob has an idea, but I
> don't see a reason to keep the restriction around.
I don't recall if there ever was a reason for this restriction beyond possibly one type of add-on having restrictions and another not which a user might not want to accept the change on update.

Comment 14

[Jesper Kristensen]

Attachment: patch v3

Comment 15

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 661602 [details] [diff] [review]
patch v3

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

Looks good :)

::: toolkit/mozapps/extensions/test/xpcshell/test_dictionary.js
@@ +671,5 @@
> +  ], check_test_27);
> +
> +  // Fake a timer event to cause a background update and wait for the magic to
> +  // happen
> +  gInternalManager.notify(null);

This is an artifact of olden times, these days you can (and should) just call AddonManagerPrivate.backgroundUpdateCheck(); directly.

Comment 16

[Jesper Kristensen]

Attachment: patch v4

Comment 17

[Ryan VanderMeulen [:RyanVM]]

This is causing test failures on Try.
https://tbpl.mozilla.org/?tree=Try&rev=e8bb654fd567

Comment 18

[Jesper Kristensen]

Attachment: patch v5

My previous patch would fail in some cases when there were no updates. This should fix it. I pushed this to try https://tbpl.mozilla.org/?tree=Try&rev=d9368c5f0505 where it failed, but the same test works for me locally.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

The Try push shows an intermittent hang, doesn't it?

Comment 20

[Jesper Kristensen]

(In reply to Ryan VanderMeulen from comment #19)
> The Try push shows an intermittent hang, doesn't it?

Maybe. I am not sure how to interpret it. When I run the test locally, a lot more output is given. Is that output available somewhere to see?

Comment 21

[Ryan VanderMeulen [:RyanVM]]

That I do not know. I think it would be a bad idea to land this without addressing the hang, though. If we're seeing in on Try, it's very likely we'll see it on inbound/m-c as well, and the last thing the tree needs is more intermittent orange (in fact, it would probably be backed out by the sheriffs anyway).

Comment 22

[Jesper Kristensen]

I have now pushed https://tbpl.mozilla.org/?tree=Try&rev=24bd0676d1be and hope it will make the test fail instead of timing out. I hope that will give some output.

Comment 23

[Jesper Kristensen]

Attachment: patch v6

I changed the tests to fail instead of time out on Try, which gave the results in https://tbpl.mozilla.org/?tree=Try&rev=24bd0676d1be . It seems that some install events fail when I uninstall a restartless add-on and then install an add-on with the same guid. I have added a restartManager call between the uninstall and the install in run_test_26. That is green on Try https://tbpl.mozilla.org/?tree=Try&rev=5949a2818543 . However I don't know why the test was random orange in the first place, so I cannot say with certainty if this fixes the randomness or if it just changes the timing to not be orange on the Try infrastructure.

Comment 24

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 666198 [details] [diff] [review]
patch v6

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

Remove all the additional changes relating to do_catch and gTimeout, etc - that's a whole other can of worms that needs to be sorted out in two other separate bugs.

I can't get that test failing locally without the restartManager() call, but it worries me that something can fail there. If we can't figure out the cause for that, file a separate bug so we can get this landed.

Comment 25

[Jesper Kristensen]

Attachment: patch v7

Comment 26

[Jesper Kristensen]

patch v7: https://tbpl.mozilla.org/?tree=Try&rev=4d2af73d9faf

I will file another bug to figure out why the restartManager is needed in the test.

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/58c0eda55272

Comment 28

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/58c0eda55272

Comment 30

[bjoern]

type 64 addons installed in Thunderbird 17 are still not being updated. They stay at the same version. Would be nice if this could be fixed in Thunderbird 17 with one of the next bugfix releases.