Closed Bug 1450358 Opened Last year Closed Last year

Add more runtime leak checks for DETH child classes

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: bkelly, Assigned: bkelly)

References

(Blocks 2 open bugs)

Details

Attachments

(9 files, 8 obsolete files)

8.69 KB, patch
baku
: review+
Details | Diff | Splinter Review
3.44 KB, patch
baku
: review+
Details | Diff | Splinter Review
3.44 KB, patch
bkelly
: review+
Details | Diff | Splinter Review
2.74 KB, patch
bkelly
: review+
Details | Diff | Splinter Review
2.95 KB, patch
baku
: review+
Details | Diff | Splinter Review
3.02 KB, patch
baku
: review+
Details | Diff | Splinter Review
2.71 KB, patch
baku
: review+
Details | Diff | Splinter Review
3.10 KB, patch
baku
: review+
Details | Diff | Splinter Review
2.03 KB, patch
bkelly
: review+
Details | Diff | Splinter Review
I've found some leaks in different classes using the test I wrote for bug 1447871.  Lets just add some more of these tests for our various DETH classes.  This bug will be for tests that pass and simple serve as regression checks.  If I find any leaks I'll file those as separate bugs.
I feel like I should factor out the common bits from these tests before going much further.
Moving this from bug 1450271.  It got r+ over there, but I want to factor out the common bits of the test here.  Also, the global binding fix in bug 1450271 can land without the test for now.
Depends on: 1450266
Andrea, this patch factors the event listener runtime test code out of the

  dom/serviceworkers/test/test_event_listener_leaks.html

And into a re-usable script here:

  dom/events/test/event_leak_utils.js

The goal is to make it easy to test a bunch of different DETH-implemented APIs using this same test infrastructure.
Attachment #8964400 - Flags: review?(amarchesini)
This patch adds an event listener runtime checking test for xhr.  Trunk passes the test now.  This is simply to prevent regressions.
Attachment #8964406 - Flags: review?(amarchesini)
Attachment #8964047 - Attachment is obsolete: true
This adds a simple event listener leak test for AbortSignal.  It passes.  This is just a regression check.
Attachment #8964409 - Flags: review?(amarchesini)
Attachment #8964048 - Attachment is obsolete: true
Attachment #8964409 - Flags: review?(amarchesini) → review+
Attachment #8964406 - Flags: review?(amarchesini) → review+
Attachment #8964400 - Flags: review?(amarchesini) → review+
Andrea, this performs a runtime event listener leak check for the Animation API.
Attachment #8964051 - Attachment is obsolete: true
Attachment #8964709 - Flags: review?(amarchesini)
This is a re-written version of the MediaQueryList test that uses the new test infrastructure.
Attachment #8964289 - Attachment is obsolete: true
Attachment #8964710 - Flags: review?(amarchesini)
Comment on attachment 8964709 [details] [diff] [review]
P3 Verify that Animation does not leak via event listeners. r=baku

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

::: dom/animation/test/mozilla/test_event_listener_leaks.html
@@ +12,5 @@
> +</head>
> +<body>
> +<p id="display"></p>
> +<div id="content" style="display: none"></div>
> +<pre id="test"></pre>

All of these are not needed.
Attachment #8964709 - Flags: review?(amarchesini) → review+
Comment on attachment 8964710 [details] [diff] [review]
P4 Add a MediaQueryList event listener leak test. r=baku

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

::: layout/style/test/test_mql_event_listener_leaks.html
@@ +10,5 @@
> +  <script type="text/javascript" src="/tests/dom/events/test/event_leak_utils.js"></script>
> +  <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css" />
> +</head>
> +<body>
> +<p id="display"></p>

same here.
Attachment #8964710 - Flags: review?(amarchesini) → review+
Test EventSource for event listener leaks.  My guess is this probably leaked in the bfcache/document.open() cases before bug 1450266 landed.
Attachment #8964964 - Flags: review?(amarchesini)
This tests BroadcastChannel for event listener leaks.
Attachment #8964967 - Flags: review?(amarchesini)
Attachment #8964964 - Flags: review?(amarchesini) → review+
This tests MessageChannel for event listener leaks.
Attachment #8964971 - Flags: review?(amarchesini)
Blocks: 1451381
This tests SharedWorker for event listener leaks.
Attachment #8964974 - Flags: review?(amarchesini)
Attachment #8964967 - Flags: review?(amarchesini) → review+
Attachment #8964971 - Flags: review?(amarchesini) → review+
Attachment #8964974 - Flags: review?(amarchesini) → review+
Correct bug number in comment message.
Attachment #8964409 - Attachment is obsolete: true
Attachment #8964977 - Flags: review+
Depends on: 1451411
Blocks: 1451426
Pushed by bkelly@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a0c94ff9fedc
P0 Factor the event listener runtime leak checking test into a reusable test framework. r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/fff908e8cfaf
P1 Add an event listener runtime leak check for XHR. r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/9663215cdcc6
P2 Test AbortSignal for event listener related runtime leaks. r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/9e939fc48dda
P3 Verify that Animation does not leak via event listeners. r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/d1f938a8592a
P4 Add a MediaQueryList event listener leak test. r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/a83a1ca1dffa
P5 Test EventSource for event listener leaks. r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/02c9cfcec711
P6 Add BroadcastChannel event listener leak test. r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/56a86828050d
P7 Test MessageChannel for event listener leaks. r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/b5043e3fd658
P8 Test that SharedWorker does not leak windows through its event listener. r=baku
See Also: → 1453544
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.