1503236 - Move WorkletImpl reference from WorkletGlobalScope to classes inheriting WorkletGlobalScope

Status: RESOLVED FIXED

(Core :: Web Audio, enhancement, P2)

Comment 1

[Arnaud Bienner]

This would need bug 1503228 to be landed first.

Comment 2

[Arnaud Bienner]

Summarizing a discussion I had with Karl:
I believe having WorkletImpl being a member of the inherited classes instead of the WorkletGlobalScope class is better, because otherwise accessing any method/member specific to a given worklet (e.g. audio related things in AudioWorkletGlobalScope, for which it doesn't make sense to have methods in WorkletGlobalScope) would require a static_cast. This has undefined behavior if we would cast to the wrong type.
Thus this has little chance to happen because we have few places where we could mis-initialized the WorkletImpl member, with this patch we will be sure this can never happen at all.

To make the WorkletImpl object accessible from WorkletGlobalScope, Impl() is now pure virtual.
Karl noted this could be an issue if we need to call Impl() at constructor/destruction, but this is not needed currently.
If someone accidentally do so, we believe this will probably lead to a crash so be noticeable right away. And clang has warning to spot this case. I wrote a patch for clang to control it with a -W flag, to have the possibility to turn it into an error, something we should probably do on our codebase once it is landed and we have updated our clang version. https://reviews.llvm.org/D53807

Comment 3

[Arnaud Bienner]

Attachment: Move WorkletImpl reference from WorkletGlobalScope to classes inheriting WorkletGlobalScope

Comment 4

[Karl Tomlinson (:karlt)]

Just recording that I didn't see a problem with the static_cast because the code would provide that the cast is to the correct type.  If that approach turns out to be more practical, then we can switch back to that.

I don't expect the virtual calls to be particularly frequent and so I'm not aware of any significant problems with this change in approach (unless the method is required from the destructor).

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/10d4bab0bccb
Move WorkletImpl reference from WorkletGlobalScope to classes inheriting WorkletGlobalScope r=karlt

Comment 6

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/10d4bab0bccb