1457209 - Discovery Pane in Add-ons Manager doesn't show installed add-on state after refresh

Status: VERIFIED FIXED

(Toolkit :: Add-ons Manager, defect, P1)

Description

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

STR:

1) Open about:addons in Firefox Nightly.
2) Select the Get Add-ons pane.
3) Install Facebook Container (or another extension listed in it).
4) After the install is finished, reload about:addons.

Expected:

The extension appears as installed.

Observed:

The extension doesn't appear as installed.

This is a recent regression, and appears to be related to bug 1454202. Also, it only occurs with extensions, not themes.

Comment 1

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

There's a warning in the console, after reloading:

Sending message that cannot be cloned. Are you trying to send an XPCOM object? addonManager.js:199:10

Comment 2

[Andrew Swan [:aswan]]

Also in the console:
NS_NOINTERFACE: Component returned failure code: 0x80004002 (NS_NOINTERFACE) [nsIInterfaceRequestor.getInterface]
amWebAPI.js:207

Comment 3

[Andrew Swan [:aswan]]

Whoops disregard comment 2, that was my mistake.
However, if I open the console for about:addons and run this:
navigator.mozAddonManager.getAddonByID("@contain-facebook").then(a => console.log(a.isEnabled))
it prints true.
The front end code is all minified so I'm not sure how to debug this further.  Bob, can you help?

Comment 4

[tofumatt [:tofumatt]]

If you follow the guide in https://github.com/mozilla/addons-frontend/blob/master/docs/moz-addon-manager.md up to "Developing on Android" you can find out how to load add-ons from staging sites. You may need to set the Discovery Pane about:config setting as well to point to Disco Pane stage.

If you do that, you'll at least get unminified code. Or maybe you'll need to use -dev instead of -stage, but that will mean you can see the code addons-frontend was using.

That said, is this reproducible only from Disco Pane or from AMO as well?

Also also: if you restart the browser, does it work? I know there are a bunch of edge cases around how about:addons loads/handles add-on changes that happen in the Disco Pane.

Comment 5

[Bob Silverberg [:bsilverberg]]

Thanks tofumatt. I've not worked with the disco pane yet, but I can try to get it running locally to see if I can debug the code. 

How new is this regression? The last commit to disco [1] was 24 days ago, and it shouldn't have impacted anything. How sure are we that this is a front-end issue, and not an API issue or a Firefox issue?

[1] https://github.com/mozilla/addons-frontend/commit/2525f06b1c83d112a75bd1e8f466f554fa42dff0

Comment 6

[Andrew Swan [:aswan]]

Looks like this broke in bug 1363925 (patch 8e (!!) to be specific)
The conversion to a class with getters broke the property iteration here: https://searchfox.org/mozilla-central/rev/c0d81882c7941c4ff13a50603e37095cdab0d1ea/toolkit/mozapps/extensions/AddonManager.jsm#252-260

And of course no test caught this :(

Comment 7

[Andrew Swan [:aswan]]

Attachment: Bug 1457209 Manually copy properties for mozAddonManager.getAddonByID results

Review commit: https://reviewboard.mozilla.org/r/241600/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/241600/

Comment 8

[Tomislav Jovanovic :zombie]

Comment on attachment 8973066 [details]
Bug 1457209 Manually copy properties for mozAddonManager.getAddonByID results

https://reviewboard.mozilla.org/r/241600/#review247548

From your mention of `MockAddon` I'm guessing we can't actually test this?

::: toolkit/mozapps/extensions/AddonManager.jsm:245
(Diff revision 1)
>   */
>  function getLocale() {
>    return Services.locale.getRequestedLocale() || "en-US";
>  }
>  
> +const WEB_ADDON_PROPERTIES = [ "id", "version", "type", "name",

`WEB_EXPOSED_` perhaps?

::: toolkit/mozapps/extensions/AddonManager.jsm:253
(Diff revision 1)
>  function webAPIForAddon(addon) {
>    if (!addon) {
>      return null;
>    }
>  
> +  // Copy some properties directly

Ah, the tradition of useful AoM comments.. ;)

But seriously, keeping a reference to `AddonManager.webidl` around is probably worth it.

Comment 9

[Andrew Swan [:aswan]]

Comment on attachment 8973066 [details]
Bug 1457209 Manually copy properties for mozAddonManager.getAddonByID results

https://reviewboard.mozilla.org/r/241600/#review247548

We could test this with a rewrite of the test case.  I think that would be worthwhile but since this is a regression I'd like to get this in now and I'll file a follow-up for rewriting the test.

> Ah, the tradition of useful AoM comments.. ;)
> 
> But seriously, keeping a reference to `AddonManager.webidl` around is probably worth it.

Yeah that's pretty bad I'll fix it up.

Comment 10

[Andrew Swan [:aswan]]

Comment on attachment 8973066 [details]
Bug 1457209 Manually copy properties for mozAddonManager.getAddonByID results

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/241600/diff/1-2/

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/01c0c18cd355
Manually copy properties for mozAddonManager.getAddonByID results r=zombie

Comment 12

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/01c0c18cd355

Comment 13

[Emil Ghitta, QA [:emilghitta]]

I have managed to reproduce the issue described in comment 0 using Firefox 61.0a1 (BuildId:20180426220144).

This issue is verified fixed using Firefox 62.0a1 (BuildId:20180509220105) and Firefox 61.0b3 (BuildId:20180507191226) on Windows 10 64bit, macOS 10.12.6 and Ubuntu 16.04 64bit.