Closed Bug 1450271 Opened 2 years ago Closed 2 years ago

MediaQueryList leaks if it has an onchange handler referencing the window

Categories

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

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: bkelly, Assigned: bkelly)

Details

(Whiteboard: [MemShrink])

Attachments

(1 file, 3 obsolete files)

While investigating bug 1447871 and 1450266 I wrote a leak test for MediaQueryList.  It suggests that if the MQL has an onchange handler referencing the window it will leak.  We need to investigate and fix this.  Any fix will likely depend on bug 1450266.
Comment on attachment 8964010 [details] [diff] [review]
P1 Make MediaQueryList bind to its document's inner window. r=baku

This patch makes MediaQueryList bind to it's document's inner window.  This is necessary to get DisconnectFromOwner() called properly.  We need that called in order for the ref created by KeepAliveIfEventListenersFor() to be cleared when the window is closed.
Attachment #8964010 - Flags: review?(amarchesini)
Comment on attachment 8964023 [details] [diff] [review]
P2 Add a MediaQueryList event listener leak test. r=baku

Andrea, this adds a runtime leak test for MediaQueryList.  Its based on the one I wrote for service workers in bug 1447871.

Currently trunk fails all three leak tests.

The P1 patch allows us to pass the "default" case where we just use MediaQueryList in an iframe and then close the iframe.

In order to pass the bfcache and document.open() cases we need bug 1450266 to land.  With that bug fixed the P1 patch here allows us to pass all cases in this test.
Attachment #8964023 - Flags: review?(amarchesini)
Attachment #8964010 - Flags: review?(amarchesini) → review+
Comment on attachment 8964023 [details] [diff] [review]
P2 Add a MediaQueryList event listener leak test. r=baku

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

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

we don't need these p/div/pre elements

@@ +93,5 @@
> +</script>
> +</pre>
> +</body>
> +</html>
> +

no extra line here.
Attachment #8964023 - Flags: review?(amarchesini) → review+
(In reply to Andrea Marchesini [:baku] from comment #7)
> Comment on attachment 8964023 [details] [diff] [review]
> P2 Add a MediaQueryList event listener leak test. r=baku

I'm going to move the test over to bug 1450358 so I can land the P1 fix now.  It will help fix a some memory leaks immediately even before bug 1450266 lands.

Also, I want to factor out the runtime checking test stuff into a re-usable library that all the tests can use.
Comment on attachment 8964023 [details] [diff] [review]
P2 Add a MediaQueryList event listener leak test. r=baku

Moved to bug 1450358.
Attachment #8964023 - Attachment is obsolete: true
No longer depends on: 1450266
Pushed by bkelly@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/59464153053d
Make MediaQueryList bind to its document's inner window. r=baku
https://hg.mozilla.org/mozilla-central/rev/59464153053d
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.