RESOLVED FIXED 189437
Many modern media control tests leak documents in testing
https://bugs.webkit.org/show_bug.cgi?id=189437
Summary Many modern media control tests leak documents in testing
Simon Fraser (smfr)
Reported 2018-09-07 15:29:09 PDT
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 ]
Attachments
Patch to use RefTracker for tracking CachedResourceHandles on CachedResource (6.35 KB, patch)
2018-09-14 21:50 PDT, Simon Fraser (smfr)
no flags
Patch (8.48 KB, patch)
2018-09-15 11:39 PDT, Simon Fraser (smfr)
darin: review+
Radar WebKit Bug Importer
Comment 1 2018-09-07 15:29:31 PDT
Simon Fraser (smfr)
Comment 2 2018-09-08 22:42:03 PDT
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)
Comment 3 2018-09-10 10:34:06 PDT
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)
Comment 4 2018-09-10 11:03:06 PDT
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)
Comment 5 2018-09-10 21:03:12 PDT
We're racing with ImageLoader::m_derefElementTimer.
Simon Fraser (smfr)
Comment 6 2018-09-13 23:13:52 PDT
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)
Comment 7 2018-09-14 21:50:47 PDT
Created attachment 349851 [details] Patch to use RefTracker for tracking CachedResourceHandles on CachedResource
Simon Fraser (smfr)
Comment 8 2018-09-14 22:02:40 PDT
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)
Comment 9 2018-09-14 22:23:25 PDT
Some fullscreen tests seems to leak media elements via WebFullScreenManager::enterFullScreenForElement(WebCore::Element*)
Simon Fraser (smfr)
Comment 10 2018-09-14 22:40:53 PDT
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)
Comment 11 2018-09-15 11:30:41 PDT
Filed bug 189648 on the WebFullscreenManager leaks.
Simon Fraser (smfr)
Comment 12 2018-09-15 11:39:10 PDT
EWS Watchlist
Comment 13 2018-09-15 11:41:56 PDT
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 14 2018-09-15 20:45:18 PDT
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)
Comment 15 2018-09-16 08:09:09 PDT
(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)
Comment 16 2018-09-17 21:15:04 PDT
Note You need to log in before you can comment on or make changes to this bug.