onuninstall listener can be called after panel is unregistered

RESOLVED FIXED in Firefox 31

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Margaret, Assigned: Margaret)

Tracking

(Blocks 1 bug)

Trunk
Firefox 31
ARM
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

Assignee

Description

5 years ago
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.
Assignee

Comment 1

5 years ago
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+
Assignee

Comment 3

5 years ago
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.
Assignee

Comment 4

5 years ago
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: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 31
You need to log in before you can comment on or make changes to this bug.