Address Book API should not remove notification listeners
Categories
(Thunderbird :: Add-Ons: Extensions API, enhancement)
Tracking
(thunderbird66 fixed, thunderbird67 fixed)
People
(Reporter: darktrojan, Assigned: darktrojan)
References
Details
Attachments
(1 file, 3 obsolete files)
(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.
Assignee | ||
Comment 1•6 years ago
|
||
This adds the listeners the first time the API is used and never removes them.
Comment 2•6 years ago
|
||
That makes sense, given that the API is part of the application.
Be sure not to trigger leak detectors.
Assignee | ||
Comment 3•6 years ago
|
||
I've no fear of our disabled leak detectors, but I should do this right anyway.
Comment 4•6 years ago
|
||
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 5•6 years ago
|
||
Assignee | ||
Comment 6•6 years ago
|
||
I do, cut-and-paste error.
Assignee | ||
Updated•6 years ago
|
Comment 8•6 years ago
|
||
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
Assignee | ||
Updated•6 years ago
|
Comment 9•6 years ago
|
||
Assignee | ||
Comment 10•6 years ago
|
||
ESR doesn't have this code.
Comment 11•6 years ago
|
||
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]
Comment 12•6 years ago
|
||
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.
Comment 13•6 years ago
|
||
Comment 14•6 years ago
|
||
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.
Comment 15•6 years ago
|
||
Sorry, wrong bug number. I meant bug 1523904.
Updated•6 years ago
|
Assignee | ||
Comment 16•6 years ago
|
||
What's weird is that I sent this to Try without the shutdown cleanup parts, and it still crashes.
Assignee | ||
Comment 17•6 years ago
|
||
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.
Updated•6 years ago
|
Updated•6 years ago
|
Comment 18•6 years ago
|
||
Comment 19•6 years ago
|
||
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
Comment 20•6 years ago
|
||
TB 66 beta 2:
https://hg.mozilla.org/releases/comm-beta/rev/85ae1ae9eaa022dd07414a4749e7eaa47f3101bd
Description
•