fast/media/mq-inverted-colors-live-update-for-listener.html This test began flaky failing with https://trac.webkit.org/changeset/260243/webkit I am able to reproduce this using command: run-webkit-tests --iterations 2000 --exit-after-n-failures 1 --exit-after-n-crashes-or-timeouts 1 --debug-rwt-logging --no-retry --force --no-build -f -1 fast/media/mq-inverted-colors-live-update-for-listener.html I can reproduce this on 260243 but not on 260241 History: https://results.webkit.org/?suite=layout-tests&test=fast%2Fmedia%2Fmq-inverted-colors-live-update-for-listener.html Diff: --- /Volumes/Data/slave/catalina-release-tests-wk1/build/layout-test-results/fast/media/mq-inverted-colors-live-update-for-listener-expected.txt +++ /Volumes/Data/slave/catalina-release-tests-wk1/build/layout-test-results/fast/media/mq-inverted-colors-live-update-for-listener-actual.txt @@ -3,9 +3,10 @@ On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE". -PASS didCallListener is true +FAIL didCallListener should be true. Was false. PASS matchMedia("(inverted-colors)").matches is true PASS successfullyParsed is true +Some tests failed. TEST COMPLETE
<rdar://problem/62555128>
(In reply to Truitt Savell from comment #0) > History: > https://results.webkit.org/?suite=layout-tests&test=fast%2Fmedia%2Fmq-inverted-colors-live-update-for-listener.html Thank you tracking this down, Truitt! https://results.webkit.org/?suite=layout-tests&test=fast%2Fmedia%2Fmq-prefers-reduced-motion-live-update-for-listener.html is also flaky failing, yet https://results.webkit.org/?suite=layout-tests&test=fast%2Fmedia%2Fmq-prefers-reduced-motion-matchMedia.html is all green. The crucial difference between them is that latter references MQL instance inside addListener() callback, while former doesn't. All points to GC issue Darin mentioned in https://bugs.webkit.org/show_bug.cgi?id=203288#c19.
Will fix this.
(In reply to Chris Dumez from comment #3) > Will fix this. Chris, I am on it: we should make MediaQueryList an ActiveDOMObject.
(In reply to Alexey Shvayka from comment #4) > (In reply to Chris Dumez from comment #3) > > Will fix this. > > Chris, I am on it: we should make MediaQueryList an ActiveDOMObject. I have the patch ready. The radar was assigned to me as P1, sorry.
Created attachment 398114 [details] Patch
(In reply to Chris Dumez from comment #5) > I have the patch ready. The radar was assigned to me as P1, sorry. No problem. http://webkit.org/b/209862 seems to be failing the same ASSERT.
Comment on attachment 398114 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=398114&action=review > Source/WebCore/css/MediaQueryList.cpp:44 > + list->suspendIfNeeded(); I wonder if we could invoke this in constructor?
Comment on attachment 398114 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=398114&action=review >> Source/WebCore/css/MediaQueryList.cpp:44 >> + list->suspendIfNeeded(); > > I wonder if we could invoke this in constructor? It is not safe to call this in the constructor in general (although it would work for this particular class). The reason is that suspendIfNeeded() may call suspend() which may ref |this|. RefCounted has an assertion that forbids increasing the refcount before adoption. This is why this is the usual pattern for calling suspendIfNeeded() on ActiveDOMObjects.
(In reply to Chris Dumez from comment #9) > It is not safe to call this in the constructor in general (although it would > work for this particular class). The reason is that suspendIfNeeded() may > call suspend() which may ref |this|. RefCounted has an assertion that > forbids increasing the refcount before adoption. > This is why this is the usual pattern for calling suspendIfNeeded() on > ActiveDOMObjects. Thanks for your insight!
Committed r261012: <https://trac.webkit.org/changeset/261012> All reviewed patches have been landed. Closing bug and clearing flags on attachment 398114 [details].