Bug 189437 – Many modern media control tests leak documents in testing

• Component: Media
• Status: RESOLVED FIXED

[Simon Fraser (smfr)]

These and other media tests leak documents:

media/modern-media-controls/controls-visibility-support/controls-visibility-support-controls-off-audio.html [ Leak Pass ]
media/modern-media-controls/controls-visibility-support/controls-visibility-support-controls-on-audio.html [ Leak Pass ]
media/modern-media-controls/controls-visibility-support/controls-visibility-support-fullscreen-on-parent-element.html [ Leak Pass ]
media/modern-media-controls/css/pointer-events-none.html [ Leak ]
media/modern-media-controls/css/webkit-cursor-visibility-auto-hide.html [ Leak ]
media/modern-media-controls/fullscreen-support/fullscreen-support-disabled-video-with-audio-tracks-only.html [ Leak ]
media/modern-media-controls/fullscreen-support/fullscreen-support-press.html [ Leak ]
media/modern-media-controls/macos-fullscreen-media-controls/macos-fullscreen-media-controls-drag-is-prevented-over-button.html [ Leak ]
media/modern-media-controls/macos-fullscreen-media-controls/macos-fullscreen-media-controls-drag.html [ Leak ]
media/modern-media-controls/media-controller/media-controller-click-on-video-background-should-pause-fullscreen.html [ Leak Pass ]
media/modern-media-controls/media-controller/media-controller-click-on-video-background-to-dismiss-tracks-panel-should-not-toggle-playback.html [ Leak Pass ]
media/modern-media-controls/media-controller/media-controller-fullscreen-ltr.html [ Leak Pass ]
media/modern-media-controls/media-controller/media-controller-space-bar-toggle-playback.html [ Leak ]
media/modern-media-controls/placard-support/placard-support-airplay-fullscreen.html [ Leak ]
media/modern-media-controls/placard-support/placard-support-pip.html [ Leak ]
media/modern-media-controls/seek-forward-support/seek-forward-support.html [ Leak ]
media/modern-media-controls/start-support/start-support-audio.html [ Leak Pass ]
media/modern-media-controls/start-support/start-support-click-to-start.html [ Leak ]
media/modern-media-controls/tracks-panel/tracks-panel-controls-bar-remains-visible-after-clicking-over-it.html [ Leak ]
media/modern-media-controls/tracks-support/tracks-support-audio-tracks.html [ Leak Pass ]
media/modern-media-controls/tracks-support/tracks-support-auto-text-track.html [ Leak Pass ]
media/modern-media-controls/tracks-support/tracks-support-show-panel-fullscreen.html [ Leak ]
media/modern-media-controls/volume-down-support/volume-down-support.html [ Leak ]
media/modern-media-controls/volume-up-support/volume-up-support.html [ Leak ]

media/W3C/video/events/event_canplay.html [ Leak Pass ]
media/W3C/video/events/event_canplaythrough.html [ Leak Pass ]
media/W3C/video/events/event_progress.html [ Leak Pass ]

[Radar WebKit Bug Importer]

<rdar://problem/44241903>

[Simon Fraser (smfr)]

This is about SVGImages in the memory cache, which aren't cleared because they still have clients when we try to clear the cache (possibly because resources are still loading).

[Simon Fraser (smfr)]

These seem to be about the lifetime of the HTMLImageElements, which sometimes extends to after we've cleared caches and fetched live documents. Those HTMLImageElements keep the ImageLoader alive, which is a client of the CachedImage.

[Simon Fraser (smfr)]

There are a couple of variants of this.

Sometimes, the main document has live references, which seem to come from a Strong Handled pointing to the Window object.

Sometimes there are two HTMLImageElements which outlive the GC/clear caches, and they keep the document alive. They are usually one of the SVG data URI images, and modern-media-controls/images/macOS/Pause.svg

[Simon Fraser (smfr)]

We're racing with ImageLoader::m_derefElementTimer.

[Simon Fraser (smfr)]

Fixed one issue by waiting on a zero-delay timer before the GC. But tests still leak; some lifetime extension of CachedImages that I haven't figure out yet.

[Simon Fraser (smfr)]

Created attachment 349851 [details]
Patch to use RefTracker for tracking CachedResourceHandles on CachedResource

[Simon Fraser (smfr)]

To fix all the SVGImage leaks, we have to:

* fire a zero-delay timer
* call document->cachedResourceLoader().garbageCollectDocumentResources()

before the final GC and live document count.

[Simon Fraser (smfr)]

Some fullscreen tests seems to leak media elements via WebFullScreenManager::enterFullScreenForElement(WebCore::Element*)

[Simon Fraser (smfr)]

Here's some logging for running media/modern-media-controls/css/webkit-cursor-visibility-auto-hide.html twice in a row:

WebFullScreenManager 0x6d22a3540 ctor
WebFullScreenManager 0x6d22a3540 enterFullScreenForElement(0x6cb000990)
WebFullScreenManager 0x6d22a3540 willEnterFullScreen() - element 0x6cb000990
Testing that control are shown in fullscreen when the controls attribute is not present.
PASS media.webkitDisplayingFullscreen is false
PASS media.webkitDisplayingFullscreen is true

WebFullScreenManager 0x6d22a3540 enterFullScreenForElement(0x6cf000990)
WebFullScreenManager 0x6d22a3540 willEnterFullScreen() - element 0x6cf000990
Testing that control are shown in fullscreen when the controls attribute is not present.
PASS media.webkitDisplayingFullscreen is false
PASS media.webkitDisplayingFullscreen is true

Jeremy, how is it that WTR can navigate away from webkit-cursor-visibility-auto-hide.html to about:blank but not close the fullscreen?

[Simon Fraser (smfr)]

Filed bug 189648 on the WebFullscreenManager leaks.

[Simon Fraser (smfr)]

Created attachment 349860 [details]
Patch

[EWS Watchlist]

Attachment 349860 [details] did not pass style-queue:


ERROR: Source/WebKit/WebProcess/InjectedBundle/API/c/WKBundlePage.cpp:639:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit/WebProcess/InjectedBundle/API/c/WKBundlePage.cpp:654:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 3 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alex Christensen]

Comment on attachment 349860 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=349860&action=review

> Source/WebKit/WebProcess/InjectedBundle/API/c/WKBundlePage.cpp:637
> +    class TimerOwner {

Would RunLoop::main().dispatch work for this?

[Simon Fraser (smfr)]

(In reply to Alex Christensen from comment #14)
> Comment on attachment 349860 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=349860&action=review
> 
> > Source/WebKit/WebProcess/InjectedBundle/API/c/WKBundlePage.cpp:637
> > +    class TimerOwner {
> 
> Would RunLoop::main().dispatch work for this?

RunLoop::main().dispatch() uses a CFRunLoopSource, and I don't think that is guaranteed to run after timesrs. I wanted a WebCore timer to ensure that it would hop the Timer used by ImageLoader.

[Simon Fraser (smfr)]

https://trac.webkit.org/changeset/236100/webkit