733582 - Test add-ons are not recognized as installed by their edit pages, and do not uninstall when all Builder tabs are closed

Status: RESOLVED WONTFIX

(addons.mozilla.org Graveyard :: Add-on Builder, defect, P1)

Description

[Daniel Buchner [:dbuc]]

If you install an add-on then navigate away from the edit page of the installed add-on, the test button logic loses awareness of the add-on's installed state.

In the previous version of the ABH, all test add-ons were uninstalled when all Builder tabs were closed. This has regressed and is no longer functioning.

Comment 1

[Alexandre Poirot [:ochameau]]

This issue is due to the new ABH feature. It now supports multiple addons.
So that you can run one addon per tab.
Unfortunatly, it ends up breaking current usage of `isInstalled` command.
We will need to improve code on both sides.
`isInstalled` should be send with an addon id, and ABH should read this addon id and return the expected answer.
I think we will have a similar issue with `uninstall` command. We should sent an precise addon id there too.

`isInstalled`:
https://github.com/mozilla/FlightDeck/blob/master/media/base/js/flightdeck/FlightDeck.js#L127
`uninstall`:
https://github.com/mozilla/FlightDeck/blob/master/media/base/js/flightdeck/FlightDeck.js#L301

Would it be ok on your side?
I can handle ABH fix if it is ok for FlightDeck.

Comment 2

[Piotr Zalewa [:zalun]]

I think it's a good idea

There are some issues thought:

1. Will ABH uninstall onbeforeunload?
   We can register the loaded add-on ID in some special variable so the ABH would know which to uninstall

2. It would be good if ABH would run the callback with the Add-on ID as a response value.

Comment 3

[Daniel Buchner [:dbuc]]

Within the scope of this fix, it would be awesome to modify the install experience to that the object in the page that does the postMessage passing to the add-on is injected into the already open Builder tabs. Currently you have to reload the page because clicking on the test button again causes the "Download the Add-on Builder Helper" message to be thrown again and again.

Comment 4

[Sean McArthur [:seanmonstar]]

Specifically, the old ABH would emit an event "addonbuilderhelperstart": https://github.com/mozilla/FlightDeck/blob/master/media/base/js/flightdeck/FlightDeck.js#L102

It would also attach `mozFlightDeck` to the open window.

Comment 5

[Alexandre Poirot [:ochameau]]

Attachment: Pull request 4

Here is the pull request for this bug.
I started addressing issues mentioned during PR#3 that haven't being addressed as the PR was merged before the review.

Comment 3 depends on bug 708190. I'll try to fix this in SDK so that we can easily get this behavior fixed in ABH. (Note that I let the code that fires "addonbuilderhelperstart", it should still be sent! But mozFlightDeck isn't injected anymore.)

I still need a confirmation from you about how to change the mozFlightDeck API in order to fix these issues.
The first main question to answer is when should we uninstall addons: on firefox quit? on tab close? on `onunload` event? something else?
(currently they are only uninstalled if we re-click on test button.)

Piotr tend to suggest to automatically uninstall addons on unload event. It basically means that when a new AB tab is open, no addon can be already installed and only one can be installed at a time. So that we can keep the existing mozFlightDeck API as-is. But Daniel, you tend to suggest something different, where the addon can be kept installed when you browser the documentation.

Then, when we will have decided when addons are uninstalled, we will be able to define if we should modify the API and how.

Comment 6

[Sean McArthur [:seanmonstar]]

I'm not sure what was best, but the previous ABH seemed to uninstall tested addons at Firefox quit.

Comment 7

[Piotr Zalewa [:zalun]]

What I was suggesting is to uninstall the add-on onbeforeunload, which means on close of the tab. One certainly can browse other add-ons without uninstalling it.

Comment 8

[Piotr Zalewa [:zalun]]

@seanmonstar in old ABH only one add-on at a time could be installed.

Comment 9

[Daniel Buchner [:dbuc]]

I'd like to allow multiple add-ons to be installed at a time, and the uninstallation of an add-on happen either when the user clicks the test button to deactivate the add-on directly, or the tab that installed the add-on is closed (unload). If there is any other issue that requires feedback, just let me know.

Comment 10

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

(In reply to Daniel Buchner [:dbuc] from comment #9)
> I'd like to allow multiple add-ons to be installed at a time, and the
> uninstallation of an add-on happen either when the user clicks the test
> button to deactivate the add-on directly, or the tab that installed the
> add-on is closed (unload). If there is any other issue that requires
> feedback, just let me know.

What about when you navigate away from the addon's edit page? Should ABH add an onUnload to the document? Stop the user from navigating away while the editor's open?

Comment 11

[Daniel Buchner [:dbuc]]

Yes, I would view any unload event occurrence on a page that has installed an add-on as a trigger to uninstall it, we could certainly prompt the user to ask whether they really want to leave the page and uninstall the test add-on associated. 

I'm open to other ideas, but this seems like it would satisfy the majority of use-cases.

Comment 12

[Alexandre Poirot [:ochameau]]

Comment on attachment 608350 [details]
Pull request 4

I've addressed all review comments reported lately, after the addon being published on AMO.

Comment 13

[Erik Vold [:erikvold] (please needinfo? me)]

The pull request looks fine Alex, tho I don't think it closes the bug.

Comment 14

[Alexandre Poirot [:ochameau]]

Landed:
https://github.com/mozilla/addon-builder-helper/commit/0c040e6f081472be35a5bbe7885fdc8d2fab4636

Yes, some more work will be needed to address this bug.
This revision allows to build ABH with SDK master.

Comment 15

[Piotr Zalewa [:zalun]]

It is still the case - I'll check what's wrong on Builder side

Comment 16

[Alexandre Poirot [:ochameau]]

It can be fixed on both side. Either listen to page unload in the page itself or from the addon. Currently the addon doesn't keep a registry to addon currently installed, so it may be easier to do that from the website.

Comment 17

[Wil Clouser [:clouserw]]

the add-on builder project has been retired.  More info at https://blog.mozilla.org/addons/2013/12/18/add-on-builder/