Closed Bug 1521958 Opened 6 years ago Closed 6 years ago

Address Book API should not remove notification listeners

Categories

(Thunderbird :: Add-Ons: Extensions API, enhancement)

Product:
Thunderbird
Component:
Add-Ons: Extensions API
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

(thunderbird66 fixed, thunderbird67 fixed)

Status:
RESOLVED FIXED
Milestone:
Thunderbird 67.0
Tracking Flags:
Tracking Status
thunderbird66 --- fixed
thunderbird67 --- fixed

People

(Reporter: darktrojan, Assigned: darktrojan)

References

Details

Attachments

(1 file, 3 obsolete files)

1521958-ab-listeners-4.diff
(deleted), patch
mkmelin
: review+
jorgk-bmo
: approval-comm-beta+
Details | Diff | Splinter Review

+++ This bug was initially created as a clone of Bug #1512788 +++

From bug 1512788 comment 4:

Actually this makes things worse; if you don't have your listeners installed, your cache will never be invalidated.

Attached patch 1521958-ab-listeners-1.diff (obsolete) (deleted) — — Splinter Review

This adds the listeners the first time the API is used and never removes them.

Attachment #9038370 - Flags: review?(mkmelin+mozilla)

That makes sense, given that the API is part of the application.

Be sure not to trigger leak detectors.

Summary: Address Book API should never not be listening for notifications → Address Book API should always be listening for notifications
Attached patch 1521958-ab-listeners-2.diff (obsolete) (deleted) — — Splinter Review

I've no fear of our disabled leak detectors, but I should do this right anyway.

Attachment #9038370 - Attachment is obsolete: true
Attachment #9038370 - Flags: review?(mkmelin+mozilla)
Attachment #9039015 - Flags: review?(mkmelin+mozilla)

I've no fear of our disabled leak detectors

Our leak detectors are disabled? That explains a lot.

It would make sense to enable them, see whether they trigger on this, and somehow counter that. Otherwise the next guy might re-add the removeListeners. I'm not saying we should keep the removeListeners. I agree they make no sense. But just make sure the leak tests are happy with your patch.

Comment on attachment 9039015 [details] [diff] [review] 1521958-ab-listeners-2.diff Review of attachment 9039015 [details] [diff] [review]: ----------------------------------------------------------------- Great, simpler too. r=mkmelin ::: mail/components/extensions/parent/ext-addressBook.js @@ +265,5 @@ > + MailServices.ab.removeAddressBookListener(this); > + Services.obs.removeObserver(this, "addrbook-contact-created"); > + Services.obs.removeObserver(this, "addrbook-contact-updated"); > + Services.obs.removeObserver(this, "addrbook-list-updated"); > + Services.obs.removeObserver(this, "addrbook-list-member-added"); You don't want to remove the quit-application-granted observer too?
Attachment #9039015 - Flags: review?(mkmelin+mozilla) → review+
Attached patch 1521958-ab-listeners-3.diff (obsolete) (deleted) — — Splinter Review

I do, cut-and-paste error.

Attachment #9039015 - Attachment is obsolete: true
Attachment #9041326 - Flags: review+
Keywords: checkin-needed

Uplifts?

Target Milestone: --- → Thunderbird 67.0

https://hg.mozilla.org/comm-central/rev/c379fb59e99562659f51f3cf2c25ede112c66092
Start listeners/observers at first use of Address Book API and only stop them at shutdown; r=mkmelin

Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Attachment #9041326 - Flags: approval-comm-beta?
Comment on attachment 9041326 [details] [diff] [review] 1521958-ab-listeners-3.diff No ESR uplift required (sorry, I don't follow all the detail in all the bugs)?
Attachment #9041326 - Flags: approval-comm-beta? → approval-comm-beta+

ESR doesn't have this code.

This seems to crash in debug builds:

TEST-UNEXPECTED-FAIL | comm/mail/components/extensions/test/xpcshell/test_ext_addressBook.js | xpcshell return code: -11
PID 9796 | Assertion failure: pages_.empty(), at /builds/worker/workspace/build/src/js/src/jit/ProcessExecutableMemory.cpp:538
PROCESS-CRASH | comm/mail/components/extensions/test/xpcshell/test_ext_addressBook.js | application crashed [@ libxul.so + 0x569a857]

Status: RESOLVED → REOPENED
Flags: needinfo?(geoff)
Resolution: FIXED → ---

Somehow you seem to upset JS garbage collection at shutdown:

0:05.28 pid:10632 [10632, Main Thread] WARNING: Extra shutdown CC: 'i < NORMAL_SHUTDOWN_COLLECTIONS', file m:/mozilla-source/comm-central/xpcom/base/nsCycleCollector.cpp, line 3355
0:05.34 pid:10632 ERROR: GC found live Cell 000002311D76B040 of kind FUNCTION at shutdown
...
0:05.36 pid:10632 ERROR: GC found 1517 live Cells at shutdown
0:05.36 pid:10632 ERROR: GC found live Cell 000037903E541060 of kind FUNCTION_EXTENDED at shutdown
and many more.

This crash is not the same crash as bug 1523028.

If this patch did what the bug title here currently says ("Address Book API should always be listening for notifications"), and really add the listeners on TB startup, then this patch here would expose the crash from bug 1523028 to all TB users.
Luckily, the patch here does not actually do that, and still adds listeners only when an extension actually uses the API. This patch merely avoids removing the listeners. So, I change the bug title to reflect what the patch does.

Summary: Address Book API should always be listening for notifications → Address Book API should not remove notification listeners

Sorry, wrong bug number. I meant bug 1523904.

Attachment #9041326 - Flags: approval-comm-beta+

What's weird is that I sent this to Try without the shutdown cleanup parts, and it still crashes.

Flags: needinfo?(geoff)
Attached patch 1521958-ab-listeners-4.diff (deleted) — — Splinter Review

Okay, we'll do it this way, since it actually works and doesn't cause a shutdown problem.

My guess as to what's happening is that the WebExt mechanism is closing down the scope the APIs live in before quit-application-granted. So not removing the listeners failed because they weren't removed; and waiting on quit-application-granted failed because the code is gone before the notification happens.

Attachment #9042865 - Flags: review?(mkmelin+mozilla)
Attachment #9042865 - Flags: approval-comm-beta?
Attachment #9042865 - Flags: review?(mkmelin+mozilla) → review+
Attachment #9041326 - Attachment is obsolete: true
Comment on attachment 9042865 [details] [diff] [review] 1521958-ab-listeners-4.diff It would be TB 66 beta 2.
Attachment #9042865 - Flags: approval-comm-beta? → approval-comm-beta+

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/342e6868d0da
Start listeners/observers at first use of Address Book API and only stop them at shutdown. r=mkmelin

Status: REOPENED → RESOLVED
Closed: 6 years ago → 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: