Changeset 282680 in webkit

Timestamp:

Sep 17, 2021, 11:57:26 AM (5 years ago)

Author:

commit-queue@webkit.org

Message:

REGRESSION(r282220): [GCC] Several flaky crashes on media/track/cue tests
​https://bugs.webkit.org/show_bug.cgi?id=230318

Patch by Philippe Normand <​pnormand@igalia.com> on 2021-09-17
Reviewed by Xabier Rodriguez-Calvar.

Source/WebCore:

The crashes were happening because the text track(s) managed by the media element still had
their client set to the media element being destroyed, so the TextTrack destructor was
looping back to its client, the media element being destroyed. This is particularly an issue
in GCC/Release builds, most likely undefined behavior. The proposed solution is to
explicitely break the link between the media element and the tracks it manages when
destroying the media element. This matches existing code as well in the same destructor
(CDMClient, audio session).

• html/HTMLMediaElement.cpp:

(WebCore::HTMLMediaElement::~HTMLMediaElement):

LayoutTests:

• platform/glib/TestExpectations: Unflag tests no longer flaky.

Location:

trunk

Files:

• LayoutTests/ChangeLog (modified) (1 diff)
• LayoutTests/platform/glib/TestExpectations (modified) (1 diff)
• Source/WebCore/ChangeLog (modified) (1 diff)
• Source/WebCore/html/HTMLMediaElement.cpp (modified) (1 diff)

trunk/LayoutTests/ChangeLog

@@ +1 @@
+2021-09-17  Philippe Normand  <pnormand@igalia.com>
+
+        REGRESSION(r282220): [GCC] Several flaky crashes on media/track/cue tests
+        https://bugs.webkit.org/show_bug.cgi?id=230318
+
+        Reviewed by Xabier Rodriguez-Calvar.
+
+        * platform/glib/TestExpectations: Unflag tests no longer flaky.
+
 2021-09-17  Arcady Goldmints-Orlov  <agoldmints@igalia.com>
 

trunk/LayoutTests/platform/glib/TestExpectations

@@ -2469 +2469 @@
 webkit.org/b/230317 http/tests/privateClickMeasurement/send-attribution-conversion-request.html [ Failure ]
 
-webkit.org/b/230318 imported/w3c/web-platform-tests/html/semantics/embedded-content/media-elements/interfaces/TextTrack/cues.html [ Pass Crash ]
-webkit.org/b/230318 imported/w3c/web-platform-tests/html/semantics/embedded-content/media-elements/interfaces/TextTrack/kind.html [ Pass Crash ]
-webkit.org/b/230318 imported/w3c/web-platform-tests/html/semantics/embedded-content/media-elements/interfaces/TextTrack/label.html [ Pass Crash ]
-webkit.org/b/230318 imported/w3c/web-platform-tests/html/semantics/embedded-content/media-elements/interfaces/TextTrack/language.html [ Pass Crash ]
-webkit.org/b/230318 imported/w3c/web-platform-tests/html/semantics/embedded-content/media-elements/interfaces/TextTrack/oncuechange.html [ Pass Crash ]
-webkit.org/b/230318 imported/w3c/web-platform-tests/html/semantics/embedded-content/media-elements/interfaces/TextTrack/removeCue.html [ Pass Crash ]
-webkit.org/b/230318 imported/w3c/web-platform-tests/html/semantics/embedded-content/media-elements/track/track-element/track-cues-cuechange-dynamically-created-track-element.html [ Pass Crash ]
-webkit.org/b/230318 imported/w3c/web-platform-tests/html/semantics/embedded-content/media-elements/track/track-element/track-cues-enter-seeking.html [ Pass Crash ]
-webkit.org/b/230318 media/track/track-cue-inline-assertion-crash.html [ Pass Crash ]
-webkit.org/b/230318 media/track/track-cue-left-align.html [ Pass Crash ]
-webkit.org/b/230318 media/track/track-cue-line-position.html [ Pass Crash ]
-webkit.org/b/230318 media/track/track-cues-cuechange.html [ Pass Crash ]
-
 # End: Common failures between GTK and WPE.
 

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2021-09-17  Philippe Normand  <pnormand@igalia.com>
+
+        REGRESSION(r282220): [GCC] Several flaky crashes on media/track/cue tests
+        https://bugs.webkit.org/show_bug.cgi?id=230318
+
+        Reviewed by Xabier Rodriguez-Calvar.
+
+        The crashes were happening because the text track(s) managed by the media element still had
+        their client set to the media element being destroyed, so the TextTrack destructor was
+        looping back to its client, the media element being destroyed. This is particularly an issue
+        in GCC/Release builds, most likely undefined behavior. The proposed solution is to
+        explicitely break the link between the media element and the tracks it manages when
+        destroying the media element. This matches existing code as well in the same destructor
+        (CDMClient, audio session).
+
+        * html/HTMLMediaElement.cpp:
+        (WebCore::HTMLMediaElement::~HTMLMediaElement):
+
 2021-09-17  Sam Weinig  <weinig@apple.com>
 

trunk/Source/WebCore/html/HTMLMediaElement.cpp

@@ -537 +537 @@
 
     beginIgnoringTrackDisplayUpdateRequests();
+
+    if (m_textTracks) {
+        for (unsigned i = 0; i < m_textTracks->length(); ++i) {
+            auto track = m_textTracks->item(i);
+            track->clearClient(*this);
+        }
+    }
+
+    if (m_audioTracks) {
+        for (unsigned i = 0; i < m_audioTracks->length(); ++i) {
+            auto track = m_audioTracks->item(i);
+            track->clearClient(*this);
+        }
+    }
+
+    if (m_videoTracks) {
+        for (unsigned i = 0; i < m_videoTracks->length(); ++i) {
+            auto track = m_videoTracks->item(i);
+            track->clearClient(*this);
+        }
+    }
+
     allMediaElements().remove(this);