WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
Details
Formatted Diff
Diff
Patch
(8.48 KB, patch)
2018-09-15 11:39 PDT
,
Simon Fraser (smfr)
darin
: review+
Details
Formatted Diff
Diff
View All
Add attachment
proposed patch, testcase, etc.
Radar WebKit Bug Importer
Comment 1
2018-09-07 15:29:31 PDT
<
rdar://problem/44241903
>
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
Created
attachment 349860
[details]
Patch
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
https://trac.webkit.org/changeset/236100/webkit
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug