Closed Bug 996669 Opened 6 years ago Closed 6 years ago

onuninstall listener can be called after panel is unregistered

Categories

(Firefox for Android :: General, defect)

ARM
Android
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 31

People

(Reporter: Margaret, Assigned: Margaret)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

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.
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 :)
Attachment #8407840 - Flags: review?(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);
Attachment #8407840 - Flags: review?(jdover) → feedback+
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 on attachment 8407840 [details] [diff] [review]
Assert panel exists in oninstall/onuninstall listeners

Re-requesting review based on my pushback in my last comment :)
Attachment #8407840 - Flags: review?(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
Attachment #8407840 - Flags: review?(jdover) → review+
https://hg.mozilla.org/mozilla-central/rev/52cb8e937d12
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 31
You need to log in before you can comment on or make changes to this bug.