REGRESSION (r260243): [ Mac WK1 ] fast/media/mq-inverted-colors-live-update-for-listener.html is a flaky failure

• Component: CSS
• Status: RESOLVED FIXED

[Truitt Savell]

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

[Radar WebKit Bug Importer]

<rdar://problem/62555128>

[Alexey Shvayka]

(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.

[Chris Dumez]

Will fix this.

[Alexey Shvayka]

(In reply to Chris Dumez from comment #3)
> Will fix this.

Chris, I am on it: we should make MediaQueryList an ActiveDOMObject.

[Chris Dumez]

(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.

[Chris Dumez]

Created attachment 398114 [details]
Patch

[Alexey Shvayka]

(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.

[Alexey Shvayka]

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?

[Chris Dumez]

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.

[Alexey Shvayka]

(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!

[EWS]

Committed r261012: <https://trac.webkit.org/changeset/261012>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 398114 [details].