Bug 198964 - Safari crashes after ~2028 OfflineAudioContext objects are created (they never get garbage collected, consuming a thread each)
Summary: Safari crashes after ~2028 OfflineAudioContext objects are created (they neve...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Audio (show other bugs)
Version: Safari 12
Hardware: All All
: P2 Normal
Assignee: youenn fablet
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-06-18 08:14 PDT by schngrg
Modified: 2019-06-21 11:01 PDT (History)
8 users (show)

See Also:


Attachments
Patch (11.47 KB, patch)
2019-06-19 12:49 PDT, youenn fablet
ews-watchlist: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews103 for mac-highsierra (3.35 MB, application/zip)
2019-06-19 14:02 PDT, EWS Watchlist
no flags Details
Patch (12.19 KB, patch)
2019-06-19 14:31 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (12.57 KB, patch)
2019-06-19 15:07 PDT, youenn fablet
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description schngrg 2019-06-18 08:14:31 PDT
Trying to create more than 2028 OfflineAudioContext (or AudioContext) crashes the Safari/WebKit page. In WebKit C++ code (AudioContext.cpp), it looks like every OfflineAudioContext takes a thread and the crash is forced when the total pool of 2048 threads is exhausted.

Possible solution: Fix garbage collection of OfflineAudioContexts. OfflineAudioContexts are never garbage collected. In WebKit C++ code, any code that could mark OfflineAudioContext as ready for garbage collection is not reached because of isOfflineContext() checks in AudioContext.cpp.

Background: WebPage gets small buffers of audio data, and wants to load it in audiobuffers to process with OfflineAudioContext. OfflineAudioContext can't be reused (https://github.com/WebAudio/web-audio-api/issues/346). For each small buffer of audio data received a new OfflineAudioContext needs to be created, but the total limit is soon exhausted because OfflineAudioContext never gets garbage collected.

Tried calling both stop() or close() etc but that didn't help because of isOfflineContext() checks in AudioContext.cpp.


Sample code:
    for (let i = 0; i < 3000; i++) { // Safari will crash at i 2026 or 2028, on both iOS and Mac
      let ctx = new OfflineAudioContext(1, 16384, 44100);
      console.log(i);
    }
Comment 1 Radar WebKit Bug Importer 2019-06-19 05:07:34 PDT
<rdar://problem/51891520>
Comment 2 schngrg 2019-06-19 05:46:48 PDT
Above for loop is only test code to reproduce the problem quickly. The actual application was receiving new audio data every 100ms, loading it in a AudioBuffer to use with AudioBufferSourceNode.start() and calling OfflineAudioContext.startRendering(). The oncomplete callback is successfully called. 

But nothing gets garbage collected (all the buffers etc stay), making it look like a memory leak. But as a hack, setting oncomplete = null after oncomplete is called causes garbage collection of everything except OfflineAudioContext objects. Very little memory is consumed by the OfflineAudioContext objects, < 40KB total. But OfflineAudioContext objects seem to be holding on to the threads causing a crash as soon as around 2020 to 2035 objects are created.

Trying to call AudioBufferSourceNode.stop(), AudioBufferSourceNode.disconnect(), setting all web-audio objects to null in or after oncomplete, etc doesn't help.
Comment 3 youenn fablet 2019-06-19 11:54:11 PDT
I cannot reproduce the crash on latest MacOS.

Thread increase is probably due to AsyncAudioDecoder.
It does not seem great to create a decoding thread per AudioContext, since it might never be used in practice. At least this should be lazily initialized.

Agreed OfflineAudioContext should be easily GCable, at least once it has finished its task.
Comment 4 schngrg 2019-06-19 12:00:37 PDT
I can reproduce the crash using Safari Technology Preview 85 on macOS. Using the for loop in my first post that just creates 3000 OfflineAudioContexts.

Note that same crash occurs even with the normal AudioContext, but it might be less of a problem with AudioContext as that is very unlikely to get created so many times.

GC is probably blocked by the setPendingActivity call in AudioContext::constructCommon in C++ code? (I am very new to webkit code, just built it for first time few days ago to try and figure out this crash).
Comment 5 youenn fablet 2019-06-19 12:38:03 PDT
> GC is probably blocked by the setPendingActivity call in
> AudioContext::constructCommon in C++ code? (I am very new to webkit code,
> just built it for first time few days ago to try and figure out this crash).

Welcome to WebKit!
And yep, that is probably the issue.
Comment 6 youenn fablet 2019-06-19 12:49:38 PDT
Created attachment 372487 [details]
Patch
Comment 7 youenn fablet 2019-06-19 12:54:12 PDT
(In reply to youenn fablet from comment #6)
> Created attachment 372487 [details]
> Patch

This patch delays setting pending activity of an OfflineAudioContext until startsRendering is called. I think this is ok but might need some additional study.

In case finishedRendering exits in case of renderedBuffer is null, there is a chance the OfflineAudioContext gets GCed while a state change event might be enqueued. This seems ok to me as renderedBuffer is not supposed to be null for offline audio contexts.
Comment 8 schngrg 2019-06-19 13:11:09 PDT
Thanks :-)
Comment 9 EWS Watchlist 2019-06-19 14:02:39 PDT
Comment on attachment 372487 [details]
Patch

Attachment 372487 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/12523687

New failing tests:
webaudio/mediaelementaudiosourcenode-gc.html
webaudio/offlineaudiocontext-gc.html
Comment 10 EWS Watchlist 2019-06-19 14:02:40 PDT
Created attachment 372492 [details]
Archive of layout-test-results from ews103 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-highsierra  Platform: Mac OS X 10.13.6
Comment 11 youenn fablet 2019-06-19 14:31:47 PDT
Created attachment 372493 [details]
Patch
Comment 12 youenn fablet 2019-06-19 15:07:27 PDT
Created attachment 372494 [details]
Patch
Comment 13 WebKit Commit Bot 2019-06-21 11:01:19 PDT
Comment on attachment 372494 [details]
Patch

Clearing flags on attachment: 372494

Committed r246685: <https://trac.webkit.org/changeset/246685>
Comment 14 WebKit Commit Bot 2019-06-21 11:01:21 PDT
All reviewed patches have been landed.  Closing bug.