737446 - "null" is shown in installation popup instead of name obtained from AMO metadata

Status: RESOLVED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Jorge Villalobos [:jorgev] (he/him)]

Attachment: Test XPI

Steps to reproduce:
1) Install attached file locally.
2) Notice the first dialog claims this add-on is named Zorcher. Accept it.
3) The popup with the 'null' name appears (see screenshot).
4) Notice that in the Add-ons Manager it also says Zorcher will be installed after the restart.

Expected result:
On step 3 it should also say Zorcher. This add-on doesn't have en-US localized data, so the AOM uses the AMO metadata to figure it out. However, this doesn't happen on step 3.

Comment 1

[Jorge Villalobos [:jorgev] (he/him)]

Attachment: Screenshot showing 'null' name error

Comment 2

[Sachin Hosmani [:sachin]]

Attachment: Shows the addon name in the notification.

Comment 3

[Jorge Villalobos [:jorgev] (he/him)]

Comment on attachment 749196 [details] [diff] [review]
Shows the addon name in the notification.

Looks okay to me, but Blair would be a better reviewer.

Comment 4

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 749196 [details] [diff] [review]
Shows the addon name in the notification.

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

Hmm, this is a little more complicated than it first looks to be. The example add-on doesn't contain any default locale strings, and we don't fall back to using any of the available locale strings (that's a bug). But when we fetch info from AMO, we do get back en-US strings - those are the strings used in the Add-ons Manager (ie, where the string for the name "Zorcher" comes from), which accesses it a AddonWrapper object. But it isn't used by the AddonInstall object (that's another bug), which is what the notification uses.

The first bug described above will be awkward to fix. Bug making AddonInstall use the data from AMO is easy, and that fix should be good enough for now. So...

::: toolkit/mozapps/extensions/XPIProvider.jsm
@@ +4678,5 @@
>  
>      this.updateAddonURIs();
>  
>      this.addon._install = this;
> +    this.name = this.addon.selectedLocale.name || this.addon.name;

this.addon is an AddonInternal object, not an AddonWrapper object. So it won't have a .name property. Instead, you have to use addon._repositoryAddon.name - and that should be preferred over selectedLocale.name. Note that you'll need to check that addon._repositoryAddon isn't null, as it isn't guaranteed to exist.

Also, loadMultipackageManifests() should be fixed too, since it'll be having the same issue.

Also, I have no idea if that change will break any tests or not.

Comment 5

[Sachin Hosmani [:sachin]]

(In reply to Blair McBride [:Unfocused] (Limited availability.) from comment #4)

> this.addon is an AddonInternal object, not an AddonWrapper object. So it
> won't have a .name property.

Oh, okay. Strangely, that patch seemed to have fixed it when I tested it. :) Anyway...

> Instead, you have to use
> addon._repositoryAddon.name - and that should be preferred over
> selectedLocale.name.

Shouldn't selectedLocale.name get preferred?
I don't understand why it should read from the addon repository when the correct localized name is available.
(In this test extension, it would rightly choose "Test" as the name instead of "Zorcher" when Firefox is in Spanish, since that is the localized name in install.rdf)

Otherwise, what you suggested seems to be working fine.

Comment 6

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to Sachin Hosmani from comment #5)
> Oh, okay. Strangely, that patch seemed to have fixed it when I tested it. :)
> Anyway...

Er, huh. If you figure out why, I'd love to know :)


> > Instead, you have to use
> > addon._repositoryAddon.name - and that should be preferred over
> > selectedLocale.name.
> 
> Shouldn't selectedLocale.name get preferred?

*sigh* Yea, sorry. Brain no work.

Was mis-remembering how chooseValue worked in AddonWrapper:
https://hg.mozilla.org/mozilla-central/file/daf809df7936/toolkit/mozapps/extensions/XPIProvider.jsm#l5900

Comment 7

[Sachin Hosmani [:sachin]]

Attachment: Addresses the comments.

It updates the AddonInstall's name property when addRepositoryData is called.
That would cover loadMultipackageManifests also.

Comment 8

[Sachin Hosmani [:sachin]]

Would it really break any tests? All changes made are internal and nothing exposed is changed.

Anyway, if tests were to break, changing installInfo.installs[0].name to installInfo.installs[0].addon.name here: http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser-addons.js#165, would fix it.
installs[0].addon is an AddonWrapper object there.

Comment 9

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to Sachin Hosmani from comment #8)
> Would it really break any tests? 

If we had preferred the data from _repositoryAddon, it may have. This won't though.

Comment 10

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 752628 [details] [diff] [review]
Addresses the comments.

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

Normally I'd say we should test this... but this is such an edge case, not something we properly support anyway, and it'd take quite a lot of work to test it.

Comment 11

[Blair McBride [:Unfocused] (UNAVAILABLE)]

https://hg.mozilla.org/integration/fx-team/rev/e9f7fb8e2342

Comment 12

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/e9f7fb8e2342