Bug 211154 - REGRESSION (r260243): [ Mac WK1 ] fast/media/mq-inverted-colors-live-update-for-listener.html is a flaky failure
Summary: REGRESSION (r260243): [ Mac WK1 ] fast/media/mq-inverted-colors-live-update-f...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-04-28 16:20 PDT by Truitt Savell
Modified: 2020-05-01 12:42 PDT (History)
20 users (show)

See Also:


Attachments
Patch (7.86 KB, patch)
2020-04-30 16:14 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Truitt Savell 2020-04-28 16:20:44 PDT
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
Comment 1 Radar WebKit Bug Importer 2020-04-28 16:21:56 PDT
<rdar://problem/62555128>
Comment 2 Alexey Shvayka 2020-04-29 11:41:54 PDT
(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.
Comment 3 Chris Dumez 2020-04-30 15:55:09 PDT
Will fix this.
Comment 4 Alexey Shvayka 2020-04-30 16:11:45 PDT
(In reply to Chris Dumez from comment #3)
> Will fix this.

Chris, I am on it: we should make MediaQueryList an ActiveDOMObject.
Comment 5 Chris Dumez 2020-04-30 16:12:20 PDT
(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.
Comment 6 Chris Dumez 2020-04-30 16:14:01 PDT
Created attachment 398114 [details]
Patch
Comment 7 Alexey Shvayka 2020-04-30 16:19:37 PDT
(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 8 Alexey Shvayka 2020-04-30 16:26:12 PDT
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 9 Chris Dumez 2020-04-30 16:28:33 PDT
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.
Comment 10 Alexey Shvayka 2020-04-30 16:44:20 PDT
(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!
Comment 11 EWS 2020-05-01 12:42:49 PDT
Committed r261012: <https://trac.webkit.org/changeset/261012>

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