Bug 216555 - Improve thread-safety in ScriptProcessorNode implementation
Summary: Improve thread-safety in ScriptProcessorNode implementation
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Audio (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords: InRadar
Depends on:
Blocks: 212611
  Show dependency tree / graph
 
Reported: 2020-09-15 09:07 PDT by Chris Dumez
Modified: 2020-09-15 11:58 PDT (History)
9 users (show)

See Also:

Attachments
Patch (7.86 KB, patch)
2020-09-15 09:10 PDT, Chris Dumez 		no flags Details | Formatted Diff | Diff
Patch (7.75 KB, patch)
2020-09-15 10:34 PDT, Chris Dumez 		no flags Details | Formatted Diff | Diff
Show Obsolete (1) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2020-09-15 09:07:42 PDT 
Improve thread-safety in ScriptProcessorNode implementation.
Comment 1 Chris Dumez 2020-09-15 09:10:01 PDT 
Created attachment 408822 [details]
Patch
Comment 2 Geoffrey Garen 2020-09-15 10:01:41 PDT 
Comment on attachment 408822 [details]
Patch

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

> Source/WebCore/Modules/webaudio/ScriptProcessorNode.cpp:189
> +            // Reference ourself so we don't accidentally get deleted before fireProcessEvent() gets called.
> +            ref();

Can this just be a RefPtr?

Also: How is it OK that the main thread can ref/deref at any time? I don't see anything that would make that reference count correct.
Comment 3 Chris Dumez 2020-09-15 10:04:26 PDT 
Comment on attachment 408822 [details]
Patch

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

>> Source/WebCore/Modules/webaudio/ScriptProcessorNode.cpp:189
>> +            ref();
> 
> Can this just be a RefPtr?
> 
> Also: How is it OK that the main thread can ref/deref at any time? I don't see anything that would make that reference count correct.

This is not new code. I will check, we may be able to use RefPtr.
Comment 4 Chris Dumez 2020-09-15 10:34:00 PDT 
Created attachment 408830 [details]
Patch
Comment 5 Chris Dumez 2020-09-15 10:34:44 PDT 
I switched the code to using a Ref<> instead of explicit ref/deref, as suggested.
Comment 6 Geoffrey Garen 2020-09-15 11:31:08 PDT 
Comment on attachment 408830 [details]
Patch

r=me

This seems like an improvement; but I'm pretty sure this code is still not thread-safe.
Comment 7 EWS 2020-09-15 11:57:18 PDT 
Committed r267094: <https://trac.webkit.org/changeset/267094>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 408830 [details].
Comment 8 Radar WebKit Bug Importer 2020-09-15 11:58:16 PDT 
<rdar://problem/68934215>