Closed Bug 1450358 Opened 6 years ago Closed 6 years ago

Add more runtime leak checks for DETH child classes

Tracking

()

Status:

RESOLVED FIXED

Milestone:

mozilla61

Tracking Flags:

Tracking

Status

firefox61

---

fixed

People

(Reporter: bkelly, Assigned: bkelly)

References

(Blocks 2 open bugs)

Details

Attachments

(9 files, 8 obsolete files)

P1 Add an event listener runtime leak check for XHR. r=baku 6 years ago Ben Kelly [:bkelly, not reviewing] 1.43 KB, patch		Details \| Diff \| Splinter Review
P1 Add an event listener runtime leak check for XHR. r=baku 6 years ago Ben Kelly [:bkelly, not reviewing] 5.16 KB, patch		Details \| Diff \| Splinter Review
P2 Test AbortSignal for event listener related runtime leaks. r=baku 6 years ago Ben Kelly [:bkelly, not reviewing] 3.84 KB, patch		Details \| Diff \| Splinter Review
P3 Verify that Animation does not leak via event listeners. r=birtles 6 years ago Ben Kelly [:bkelly, not reviewing] 4.61 KB, patch		Details \| Diff \| Splinter Review
P4 Add a MediaQueryList event listener leak test. r=baku 6 years ago Ben Kelly [:bkelly, not reviewing] 4.09 KB, patch		Details \| Diff \| Splinter Review
P0 Factor the event listener runtime leak checking test into a reusable test framework. r=baku 6 years ago Ben Kelly [:bkelly, not reviewing] 8.69 KB, patch	baku : review+	Details \| Diff \| Splinter Review
P1 Add an event listener runtime leak check for XHR. r=baku 6 years ago Ben Kelly [:bkelly, not reviewing] 3.44 KB, patch	baku : review+	Details \| Diff \| Splinter Review
P2 Test AbortSignal for event listener related runtime leaks. r=baku 6 years ago Ben Kelly [:bkelly, not reviewing] 2.03 KB, patch	baku : review+	Details \| Diff \| Splinter Review
P3 Verify that Animation does not leak via event listeners. r=baku 6 years ago Ben Kelly [:bkelly, not reviewing] 3.53 KB, patch	baku : review+	Details \| Diff \| Splinter Review
P4 Add a MediaQueryList event listener leak test. r=baku 6 years ago Ben Kelly [:bkelly, not reviewing] 2.76 KB, patch	baku : review+	Details \| Diff \| Splinter Review
P3 Verify that Animation does not leak via event listeners. r=baku 6 years ago Ben Kelly [:bkelly, not reviewing] 3.44 KB, patch	bkelly : review+	Details \| Diff \| Splinter Review
P4 Add a MediaQueryList event listener leak test. r=baku 6 years ago Ben Kelly [:bkelly, not reviewing] 2.74 KB, patch	bkelly : review+	Details \| Diff \| Splinter Review
P5 Test EventSource for event listener leaks. r=baku 6 years ago Ben Kelly [:bkelly, not reviewing] 2.95 KB, patch	baku : review+	Details \| Diff \| Splinter Review
P6 Add BroadcastChannel event listener leak test. r=baku 6 years ago Ben Kelly [:bkelly, not reviewing] 3.02 KB, patch	baku : review+	Details \| Diff \| Splinter Review
P7 Test MessageChannel for event listener leaks. r=baku 6 years ago Ben Kelly [:bkelly, not reviewing] 2.71 KB, patch	baku : review+	Details \| Diff \| Splinter Review
P8 Test that SharedWorker does not leak windows through its event listener. r=baku 6 years ago Ben Kelly [:bkelly, not reviewing] 3.10 KB, patch	baku : review+	Details \| Diff \| Splinter Review
P2 Test AbortSignal for event listener related runtime leaks. r=baku 6 years ago Ben Kelly [:bkelly, not reviewing] 2.03 KB, patch	bkelly : review+	Details \| Diff \| Splinter Review

Ben Kelly [:bkelly, not reviewing]

Assignee

Description

•

6 years ago

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.

Ben Kelly [:bkelly, not reviewing]

Assignee

Comment 1

•

6 years ago

Attached patch P1 Add an event listener runtime leak check for XHR. r=baku (obsolete) — Details — Splinter Review

Ben Kelly [:bkelly, not reviewing]

Assignee

Comment 2

•

6 years ago

Attached patch P1 Add an event listener runtime leak check for XHR. r=baku (obsolete) — Details — Splinter Review

Attachment #8964045 - Attachment is obsolete: true

Ben Kelly [:bkelly, not reviewing]

Assignee

Comment 3

•

6 years ago

Attached patch P2 Test AbortSignal for event listener related runtime leaks. r=baku (obsolete) — Details — Splinter Review

Ben Kelly [:bkelly, not reviewing]

Assignee

Comment 4

•

6 years ago

Attached patch P3 Verify that Animation does not leak via event listeners. r=birtles (obsolete) — Details — Splinter Review

Ben Kelly [:bkelly, not reviewing]

Assignee

Comment 5

•

6 years ago

I feel like I should factor out the common bits from these tests before going much further.

Ben Kelly [:bkelly, not reviewing]

Assignee

Comment 6

•

6 years ago

Attached patch P4 Add a MediaQueryList event listener leak test. r=baku (obsolete) — Details — Splinter Review

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.

Ben Kelly [:bkelly, not reviewing]

Assignee

Updated

•

6 years ago

Depends on: 1450266

Ben Kelly [:bkelly, not reviewing]

Assignee

Comment 7

•

6 years ago

Attached patch P0 Factor the event listener runtime leak checking test into a reusable test framework. r=baku — Details — Splinter Review

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)

Ben Kelly [:bkelly, not reviewing]

Assignee

Comment 8

•

6 years ago

Attached patch P1 Add an event listener runtime leak check for XHR. r=baku — Details — Splinter Review

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)

Ben Kelly [:bkelly, not reviewing]

Assignee

Updated

•

6 years ago

Attachment #8964047 - Attachment is obsolete: true

Ben Kelly [:bkelly, not reviewing]

Assignee

Comment 9

•

6 years ago

Attached patch P2 Test AbortSignal for event listener related runtime leaks. r=baku (obsolete) — Details — Splinter Review

This adds a simple event listener leak test for AbortSignal.  It passes.  This is just a regression check.

Attachment #8964409 - Flags: review?(amarchesini)

Ben Kelly [:bkelly, not reviewing]

Assignee

Updated

•

6 years ago

Attachment #8964048 - Attachment is obsolete: true

Andrea Marchesini [:baku]

Updated

•

6 years ago

Attachment #8964409 - Flags: review?(amarchesini) → review+

Andrea Marchesini [:baku]

Updated

•

6 years ago

Attachment #8964406 - Flags: review?(amarchesini) → review+

Andrea Marchesini [:baku]

Updated

•

6 years ago

Attachment #8964400 - Flags: review?(amarchesini) → review+

Ben Kelly [:bkelly, not reviewing]

Assignee

Comment 10

•

6 years ago

Attached patch P3 Verify that Animation does not leak via event listeners. r=baku (obsolete) — Details — Splinter 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)

Ben Kelly [:bkelly, not reviewing]

Assignee

Comment 11

•

6 years ago

Attached patch P4 Add a MediaQueryList event listener leak test. r=baku (obsolete) — Details — Splinter Review

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)

Andrea Marchesini [:baku]

Comment 12

•

6 years ago

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+

Andrea Marchesini [:baku]

Comment 13

•

6 years ago

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+

Ben Kelly [:bkelly, not reviewing]

Assignee

Comment 14

•

6 years ago

Attached patch P3 Verify that Animation does not leak via event listeners. r=baku — Details — Splinter Review

Attachment #8964709 - Attachment is obsolete: true

Attachment #8964956 - Flags: review+

Ben Kelly [:bkelly, not reviewing]

Assignee

Comment 15

•

6 years ago

Attached patch P4 Add a MediaQueryList event listener leak test. r=baku — Details — Splinter Review

Attachment #8964710 - Attachment is obsolete: true

Attachment #8964957 - Flags: review+

Ben Kelly [:bkelly, not reviewing]

Assignee

Comment 16

•

6 years ago

Attached patch P5 Test EventSource for event listener leaks. r=baku — Details — Splinter 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)

Ben Kelly [:bkelly, not reviewing]

Assignee

Comment 17

•

6 years ago

Attached patch P6 Add BroadcastChannel event listener leak test. r=baku — Details — Splinter Review

This tests BroadcastChannel for event listener leaks.

Attachment #8964967 - Flags: review?(amarchesini)

Andrea Marchesini [:baku]

Updated

•

6 years ago

Attachment #8964964 - Flags: review?(amarchesini) → review+

Ben Kelly [:bkelly, not reviewing]

Assignee

Comment 18

•

6 years ago

Attached patch P7 Test MessageChannel for event listener leaks. r=baku — Details — Splinter Review

This tests MessageChannel for event listener leaks.

Attachment #8964971 - Flags: review?(amarchesini)

Ben Kelly [:bkelly, not reviewing]

Assignee

Updated

•

6 years ago

Blocks: 1451381

Ben Kelly [:bkelly, not reviewing]

Assignee

Comment 19

•

6 years ago

Attached patch P8 Test that SharedWorker does not leak windows through its event listener. r=baku — Details — Splinter Review

This tests SharedWorker for event listener leaks.

Attachment #8964974 - Flags: review?(amarchesini)

Andrea Marchesini [:baku]

Updated

•

6 years ago

Attachment #8964967 - Flags: review?(amarchesini) → review+

Andrea Marchesini [:baku]

Updated

•

6 years ago

Attachment #8964971 - Flags: review?(amarchesini) → review+

Andrea Marchesini [:baku]

Updated

•

6 years ago

Attachment #8964974 - Flags: review?(amarchesini) → review+

Ben Kelly [:bkelly, not reviewing]

Assignee

Comment 20

•

6 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=851c4cf8e8177b1a8625109401a02d999b2819f3

Ben Kelly [:bkelly, not reviewing]

Assignee

Comment 21

•

6 years ago

Attached patch P2 Test AbortSignal for event listener related runtime leaks. r=baku — Details — Splinter Review

Correct bug number in comment message.

Attachment #8964409 - Attachment is obsolete: true

Attachment #8964977 - Flags: review+

Ben Kelly [:bkelly, not reviewing]

Assignee

Updated

•

6 years ago

Depends on: 1451411

Ben Kelly [:bkelly, not reviewing]

Assignee

Updated

•

6 years ago

Blocks: 1451426

Pulsebot

Comment 22

•

6 years ago

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

Noemi Erli[:noemi_erli]

Comment 23

•

6 years ago

bugherder

https://hg.mozilla.org/mozilla-central/rev/a0c94ff9fedc
https://hg.mozilla.org/mozilla-central/rev/fff908e8cfaf
https://hg.mozilla.org/mozilla-central/rev/9663215cdcc6
https://hg.mozilla.org/mozilla-central/rev/9e939fc48dda
https://hg.mozilla.org/mozilla-central/rev/d1f938a8592a
https://hg.mozilla.org/mozilla-central/rev/a83a1ca1dffa
https://hg.mozilla.org/mozilla-central/rev/02c9cfcec711
https://hg.mozilla.org/mozilla-central/rev/56a86828050d
https://hg.mozilla.org/mozilla-central/rev/b5043e3fd658

Status: ASSIGNED → RESOLVED

Closed: 6 years ago

status-firefox61: --- → fixed

Resolution: --- → FIXED

Target Milestone: --- → mozilla61

Ben Kelly [:bkelly, not reviewing]

Assignee

Updated

•

6 years ago

Blocks: event-target-leaks

Brian Birtles (:birtles, Feb 26~ parental leave)

Updated

•

6 years ago

Updated

•

5 years ago

Component: DOM → DOM: Core & HTML

You need to log in before you can comment on or make changes to this bug.