Bug 224387

Summary: [ macOS ] 3 webaudio/OfflineAudioContext/ layout-tests are flakey text failures
Product: WebKit Reporter: Robert Jenner <jenner>
Component: New BugsAssignee: Chris Dumez <cdumez>
Status: RESOLVED FIXED    
Severity: Normal CC: cdumez, darin, eric.carlson, ews-watchlist, ggaren, glenn, jer.noble, peng.liu6, philipj, sergio, webkit-bot-watchers-bugzilla, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=224399
Attachments:
Description Flags
224387-Test list
none
Patch
ews-feeder: commit-queue-
Patch
ews-feeder: commit-queue-
Patch
none
Patch
none
Patch none

Description Robert Jenner 2021-04-09 12:57:30 PDT
The following three tests are flakey text failures on macOS Catalina and up. 

webaudio/OfflineAudioContext/offlineaudiocontext-leak-after-rendering.html
webaudio/OfflineAudioContext/offlineaudiocontext-leak-while-suspended.html
webaudio/OfflineAudioContext/offlineaudiocontext-leak.html

HISTORY URL:
https://results.webkit.org/?suite=layout-tests&suite=layout-tests&suite=layout-tests&test=webaudio%2FOfflineAudioContext%2Fofflineaudiocontext-leak-after-rendering.html&test=webaudio%2FOfflineAudioContext%2Fofflineaudiocontext-leak-while-suspended.html&test=webaudio%2FOfflineAudioContext%2Fofflineaudiocontext-leak.html

It looks like these tests were started fairly recently, and it appears they've had flakey failures since they started. It also looks like all three tests fail at the same time, and have the same text diff. 

TEXT DIFF:
-PASS internals.numberOfBaseAudioContexts() < instancesToCreate is true
+FAIL internals.numberOfBaseAudioContexts() < instancesToCreate should be true. Was false.
 PASS successfullyParsed is true
+Some tests failed.
 
 TEST COMPLETE
Comment 1 Radar WebKit Bug Importer 2021-04-09 13:43:13 PDT
<rdar://problem/76468058>
Comment 2 Robert Jenner 2021-04-09 14:09:53 PDT
When running these tests standalone, they do not fail anywhere. However, I was finally able to reproduce their failures at BigSur Release ToT using a test list, and the following test:

run-webkit-tests <path to revision being tested> --test-list <path to test list location> --child-process=1

What this indicates is that these tests failures are dependent upon tests that run before them. I have attached the test list I used for reproduction, and will begin narrowing down what causes these tests to fail.
Comment 3 Robert Jenner 2021-04-09 14:11:49 PDT
Created attachment 425651 [details]
224387-Test list

Test list used to reproduce the test failures.
Comment 4 Robert Jenner 2021-04-09 14:17:05 PDT
The following two tests were introduced at r275668(https://trac.webkit.org/changeset/275668/webkit):

webaudio/OfflineAudioContext/offlineaudiocontext-leak.html
webaudio/OfflineAudioContext/offlineaudiocontext-leak-after-rendering.html

The third test was introduced at r275679 (https://trac.webkit.org/changeset/275679/webkit):
webaudio/OfflineAudioContext/offlineaudiocontext-leak-while-suspended.html
Comment 5 Robert Jenner 2021-04-09 15:17:15 PDT
After a process of elimination, I have verified that the following test causes the three tests to fail when ran before them:

webaudio/AudioListener/audiolistener-set-position.html

When I ran the test directly before the three tests in a four test list on a loop, it caused the three tests to fail. When I removed the AudioListener test from the entire 6000+ test list, the three tests passed.
Comment 6 Chris Dumez 2021-04-09 15:19:04 PDT
Investigating now, thanks.
Comment 7 Chris Dumez 2021-04-09 15:26:29 PDT
Can reproduce like so:
run-webkit-tests webaudio/AudioListener/audiolistener-set-position.html webaudio/OfflineAudioContext/offlineaudiocontext-leak.html --child-processes=1 --iterations=30

It does not fail on first iteration but it starts failing after a while. webaudio/AudioListener/audiolistener-set-position.html is probably leaking contexts.
Comment 8 Chris Dumez 2021-04-09 15:48:50 PDT
Looks like webaudio/AudioListener/audiolistener-set-position.html is leaking PannerNodes and the PannerNodes (like all AudioNodes) are ref'ing the AudioContext.
Comment 9 Chris Dumez 2021-04-09 16:40:27 PDT
Created attachment 425663 [details]
Patch
Comment 10 Chris Dumez 2021-04-09 16:43:25 PDT
Created attachment 425665 [details]
Patch
Comment 11 Peng Liu 2021-04-09 16:58:41 PDT
Comment on attachment 425665 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=425665&action=review

> Source/WebCore/testing/Internals.cpp:2659
> +uint64_t Internals::baseAudioContextIdentifier(BaseAudioContext& context)

Nit. const?
Comment 12 Chris Dumez 2021-04-09 17:01:32 PDT
Created attachment 425669 [details]
Patch
Comment 13 Chris Dumez 2021-04-09 17:01:52 PDT
(In reply to Peng Liu from comment #11)
> Comment on attachment 425665 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=425665&action=review
> 
> > Source/WebCore/testing/Internals.cpp:2659
> > +uint64_t Internals::baseAudioContextIdentifier(BaseAudioContext& context)
> 
> Nit. const?

Fixed, thanks.
Comment 14 Chris Dumez 2021-04-09 17:05:39 PDT
Created attachment 425670 [details]
Patch
Comment 15 Eric Carlson 2021-04-09 18:41:06 PDT
Comment on attachment 425670 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=425670&action=review

> Source/WebCore/Modules/webaudio/BaseAudioContext.cpp:123
> +    static uint64_t contextIDSeed = 0;

Not: won’t this be initialized to zero automatically?
Comment 16 Chris Dumez 2021-04-09 19:44:24 PDT
(In reply to Eric Carlson from comment #15)
> Comment on attachment 425670 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=425670&action=review
> 
> > Source/WebCore/Modules/webaudio/BaseAudioContext.cpp:123
> > +    static uint64_t contextIDSeed = 0;
> 
> Not: won’t this be initialized to zero automatically?

Yes, will fix.
Comment 17 Chris Dumez 2021-04-09 19:46:36 PDT
Created attachment 425675 [details]
Patch
Comment 18 Chris Dumez 2021-04-09 19:47:30 PDT
Comment on attachment 425675 [details]
Patch

Looks like the stress bot wasn't happy. Will check.
Comment 19 EWS 2021-04-10 10:11:12 PDT
Committed r275798 (236369@main): <https://commits.webkit.org/236369@main>

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