996669 - onuninstall listener can be called after panel is unregistered

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[:Margaret Leibovic]

There's not much we can do if the add-on author decides to unregister and uninstall their panel at the same time, but we should avoid hitting an exception in Home.jsm in that case.

Comment 1

[:Margaret Leibovic]

Attachment: Assert panel exists in oninstall/onuninstall listeners

We can't really control whether an add-on decides to unregister a panel before it's onuninstall listener gets called, but we can at least offer a more helpful error message.

We should never run into this situation with the oninstall listener, but in case we do for some reason, it's also good to have a more helpful exception.

jdover, I think you have enough experience to review this patch :)

Comment 2

[Josh Dover [:jdover]]

Comment on attachment 8407840 [details] [diff] [review]
Assert panel exists in oninstall/onuninstall listeners

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

I think we can make this a bit safer by making a getter that does this check for you every time.

::: mobile/android/modules/Home.jsm
@@ +301,1 @@
>        let options = _registeredPanels[id]();

What if we repurposed _assertPanelExists to also call optionsCallback and return the resulting object? This way every time we call for the options we don't have to remember that it's a function and we get the safe checking of the assert every time. So something like:

  let options = _getExistingPanel(id);

Comment 3

[:Margaret Leibovic]

I started to implement this suggestion, but I decided I think it's an unnecessary level of complexity. In multiple places where we call _assertPanelExists we don't actually generate the panel, so if we're not going to get rid of _assertPanelExists, I don't want to introduce another small helper method. Also, but explicitly putting in an assert at the beginning of these methods, it's more obvious to developers that these methods might throw.

Comment 4

[:Margaret Leibovic]

Comment on attachment 8407840 [details] [diff] [review]
Assert panel exists in oninstall/onuninstall listeners

Re-requesting review based on my pushback in my last comment :)

Comment 5

[Josh Dover [:jdover]]

Comment on attachment 8407840 [details] [diff] [review]
Assert panel exists in oninstall/onuninstall listeners

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

(In reply to :Margaret Leibovic from comment #3)
> I started to implement this suggestion, but I decided I think it's an
> unnecessary level of complexity. In multiple places where we call
> _assertPanelExists we don't actually generate the panel, so if we're not
> going to get rid of _assertPanelExists, I don't want to introduce another
> small helper method. Also, but explicitly putting in an assert at the
> beginning of these methods, it's more obvious to developers that these
> methods might throw.

Ah yes I see. LGTM

Comment 6

[:Margaret Leibovic]

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

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/52cb8e937d12