Closed Bug 782118 Opened 12 years ago Closed 12 years ago

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

Categories

(Toolkit :: Add-ons Manager, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla18

People

(Reporter: bugzilla, Assigned: bugzilla)

References

Details

Attachments

(1 file, 7 obsolete files)

Attached patch patch v1 (obsolete) — Splinter Review
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.
Attachment #651190 - Flags: review?(dtownsend+bugmail)
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.)
Attachment #651190 - Flags: review?(dtownsend+bugmail) → review-
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).
Component: Add-ons Manager → API
Product: Toolkit → addons.mozilla.org
(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=danish@dictionaries.addons.mozilla.org&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
(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.
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?
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.
Attachment #657662 - Flags: feedback?(bmcbride)
Attached file Make AMO deliver all three updates (obsolete) —
I just found a way to fix this on the server side anyway: Just serve many updates for all types in one update rdf.
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 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.
Attachment #657662 - Flags: feedback?(bmcbride) → feedback-
(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.
(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 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.
Attachment #657662 - Flags: feedback- → feedback+
Status: NEW → ASSIGNED
Component: API → Add-ons Manager
Product: addons.mozilla.org → Toolkit
(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.
Attached patch patch v3 (obsolete) — Splinter Review
Assignee: nobody → bugzilla
Attachment #651190 - Attachment is obsolete: true
Attachment #657662 - Attachment is obsolete: true
Attachment #661602 - Flags: review?(bmcbride)
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.
Attachment #661602 - Flags: review?(bmcbride) → review+
Attachment #657664 - Attachment is obsolete: true
Attached patch patch v4 (obsolete) — Splinter Review
Attachment #663756 - Flags: checkin?
Keywords: checkin-needed
Attachment #661602 - Attachment is obsolete: true
Attachment #663756 - Flags: checkin?
Attached patch patch v5 (obsolete) — Splinter Review
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.
Attachment #663756 - Attachment is obsolete: true
Attachment #663802 - Flags: review?(bmcbride)
Attachment #663802 - Flags: review?(bmcbride) → review+
Keywords: checkin-needed
The Try push shows an intermittent hang, doesn't it?
(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?
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).
Keywords: checkin-needed
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.
Blocks: 795294
Attached patch patch v6 (obsolete) — Splinter Review
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.
Attachment #663802 - Attachment is obsolete: true
Attachment #666198 - Flags: review?(bmcbride)
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.
Attachment #666198 - Flags: review?(bmcbride) → review-
Attached patch patch v7Splinter Review
Attachment #666198 - Attachment is obsolete: true
Attachment #666579 - Flags: review?(bmcbride)
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.
Attachment #666579 - Flags: review?(bmcbride) → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/58c0eda55272
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
Blocks: 798416
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.
You need to log in before you can comment on or make changes to this bug.