Add support for CSS.paintWorklet.addModule(), which should run with its own global object on the main thread.
Created attachment 353217 [details] Patch
Comment on attachment 353217 [details] Patch Attachment 353217 [details] did not pass bindings-ews (mac): Output: https://webkit-queues.webkit.org/results/9745508 New failing tests: Please see test output for results
Created attachment 353305 [details] Patch
Comment on attachment 353305 [details] Patch Attachment 353305 [details] did not pass bindings-ews (mac): Output: https://webkit-queues.webkit.org/results/9770503 New failing tests: Please see test output for results
Created attachment 353311 [details] Patch
Comment on attachment 353311 [details] Patch Attachment 353311 [details] did not pass bindings-ews (mac): Output: https://webkit-queues.webkit.org/results/9770987 New failing tests: Please see test output for results
Created attachment 353315 [details] Patch
Comment on attachment 353315 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=353315&action=review This is great! Someone with more JSC binding experience should look at the code more but looks ok from where I stand. Just saw a few formatting issues. > Source/WebCore/DerivedSources.make:1029 > + $(WebCore)/worklets/PaintWorkletGlobalScope.idl \ > + $(WebCore)/worklets/Worklet.idl \ > + $(WebCore)/worklets/WorkletGlobalScope.idl \ You got some weird spacing here. > Source/WebCore/DerivedSources.make:1162 > + $(WORKLETGLOBALSCOPE_CONSTRUCTORS_FILE) \ Same weird spacing. > Source/WebCore/worklets/PaintWorkletGlobalScope.h:35 > + Remove blank line here?
Comment on attachment 353315 [details] Patch Attachment 353315 [details] did not pass bindings-ews (mac): Output: https://webkit-queues.webkit.org/results/9771232 New failing tests: Please see test output for results
Created attachment 353316 [details] Patch
Comment on attachment 353316 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=353316&action=review > Source/WebCore/DerivedSources.make:1162 > + $(WORKLETGLOBALSCOPE_CONSTRUCTORS_FILE) \ Still has the weird spacing
Comment on attachment 353316 [details] Patch Attachment 353316 [details] did not pass bindings-ews (mac): Output: https://webkit-queues.webkit.org/results/9771356 New failing tests: Please see test output for results
(In reply to Don Olmstead from comment #11) > Comment on attachment 353316 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=353316&action=review > > > Source/WebCore/DerivedSources.make:1162 > > + $(WORKLETGLOBALSCOPE_CONSTRUCTORS_FILE) \ > > Still has the weird spacing Heh, Xcode isn't cooperating. I'll fix it with vim.
Created attachment 353318 [details] Patch
Comment on attachment 353318 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=353318&action=review > Source/WebCore/bindings/js/JSWorkletGlobalScopeBase.cpp:42 > +namespace WebCore { > +using namespace JSC; Nit: We usually put a blank line between these. > Source/WebCore/css/CSSPaintImageValue.cpp:53 > + auto* selectedGlobalScope = renderElement.document().getCSSPaintWorkletGlobalScope(); Shouldn't this be getPaintWorkletGlobalScope() now? > Source/WebCore/css/DOMCSSPaintWorklet.idl:30 > + [ImplementedAs=ensurePaintWorklet,CallWith=Document] static readonly attribute Worklet paintWorklet; Should DOMCSSPaintWorklet.* be DOMPaintWorklet.* now? > Source/WebCore/dom/Document.cpp:8381 > +Worklet& Document::ensureCSSPaintWorklet() ensurePaintWorklet? > Source/WebCore/worklets/PaintWorkletGlobalScope.cpp:45 > + return 1.0; Why not fetch this from the document? > Source/WebCore/worklets/WorkletGlobalScope.h:55 > + String origin() const final { ASSERT_NOT_REACHED(); return ""; } emptyString() > Source/WebCore/worklets/WorkletScriptController.cpp:170 > + // This FIXME is from in WorkerScriptController. Typo: in
Comment on attachment 353318 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=353318&action=review > Source/WebCore/worklets/Worklet.cpp:49 > + auto context = PaintWorkletGlobalScope::create(makeWeakPtr(document), ScriptSourceCode(moduleURL)); We usually prefer to pass the object and let the callee decide to take a WeakPtr. So: auto context = PaintWorkletGlobalScope::create(document, ScriptSourceCode(moduleURL)); here and then makeWeakPtr() inside the PaintWorkletGlobalScope constructor when assigning it to the member.
Created attachment 353423 [details] Patch
Comment on attachment 353423 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=353423&action=review > Source/WebCore/Sources.txt:388 > +bindings/js/JSPaintWorkletGlobalScopeCustom.cpp Bad sorting now. > Source/WebCore/bindings/js/JSPaintWorkletGlobalScopeCustom.cpp:47 > +void JSPaintWorkletGlobalScope::visitAdditionalChildren(JSC::SlotVisitor& visitor) I hear these visit methods can get called on a background thread. As a result, you'll likely need to protect all accesses / modifications to paintDefinitionMap with a Lock. > Source/WebCore/bindings/js/JSPaintWorkletGlobalScopeCustom.cpp:54 > +JSValue JSPaintWorkletGlobalScope::registerPaint(ExecState& state) Please add a comment explaining why this requires custom code. What is the bindings generator missing to generate the code you need? We don't like to add new custom code unless there is a very good reason to. > Source/WebCore/bindings/js/JSPaintWorkletGlobalScopeCustom.cpp:58 > + PaintWorkletGlobalScope& workletGlobalScope = wrapped(); auto& > Source/WebCore/bindings/js/JSWorkletGlobalScopeBase.cpp:87 > + JSWorkletGlobalScopeBase* thisObject = jsCast<JSWorkletGlobalScopeBase*>(cell); auto* > Source/WebCore/bindings/js/JSWorkletGlobalScopeBase.cpp:120 > + const JSWorkletGlobalScopeBase* thisObject = jsCast<const JSWorkletGlobalScopeBase*>(object); auto* > Source/WebCore/bindings/js/JSWorkletGlobalScopeBase.cpp:134 > + JSWorkletGlobalScope* contextWrapper = script->workletGlobalScopeWrapper(); auto* > Source/WebCore/bindings/js/JSWorkletGlobalScopeBase.h:40 > + typedef JSDOMGlobalObject Base; using Base = JSDOMGlobalObject; > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:509 > + return 0 if $interface->type->name eq "WorkletGlobalScope"; Please add a comment before this line like so: # For worklets, the global object is a PaintWorkletGlobalScope. > Source/WebCore/dom/Document.cpp:8390 > + m_paintWorkletGlobalScope = WTFMove(scope); Should we ASSERT(!m_paintWorkletGlobalScope); before this? or is it OK to overwrite a previous WorkletGlobalScope? > Source/WebCore/dom/Document.h:1526 > + PaintWorkletGlobalScope* getPaintWorkletGlobalScope() { return m_paintWorkletGlobalScope.get(); } Getters in WebKit do not use the 'get' prefix. Should be: PaintWorkletGlobalScope* paintWorkletGlobalScope(); > Source/WebCore/worklets/PaintWorkletGlobalScope.cpp:38 > + return adoptRef(*new PaintWorkletGlobalScope(makeWeakPtr(document), WTFMove(code))); return adoptRef(*new PaintWorkletGlobalScope(document, WTFMove(code))); > Source/WebCore/worklets/PaintWorkletGlobalScope.cpp:42 > + : WorkletGlobalScope(WTFMove(document), WTFMove(code)) makeWeakPtr(document) > Source/WebCore/worklets/PaintWorkletGlobalScope.h:43 > + bool isPaintWorkletGlobalScope() const final { return true; } Should probably be private. > Source/WebCore/worklets/PaintWorkletGlobalScope.h:58 > + PaintWorkletGlobalScope(WeakPtr<Document>&&, ScriptSourceCode&&); Document& > Source/WebCore/worklets/Worklet.cpp:42 > + Extra blank line here. > Source/WebCore/worklets/Worklet.cpp:49 > + auto context = PaintWorkletGlobalScope::create(document, ScriptSourceCode(moduleURL)); Does this need a FIXME comment indicating that moduleURL should actually be a URL and not source code? > Source/WebCore/worklets/Worklet.cpp:51 > + document.setPaintWorkletGlobalScope(WTFMove(context)); What if the document already had a PaintWorkletGlobalScope? > Source/WebCore/worklets/Worklet.h:33 > +class Worklet : public RefCounted<Worklet> { Should probably subclass ScriptWrappable since it can have a JS wrapper. > Source/WebCore/worklets/Worklet.h:37 > + void addModule(Document&, String moduleURL); const String& > Source/WebCore/worklets/Worklet.h:41 > + No need for this blank line. > Source/WebCore/worklets/Worklet.h:45 > +#endif Missing blank line before this. > Source/WebCore/worklets/WorkletGlobalScope.cpp:49 > +WorkletGlobalScope::WorkletGlobalScope(WeakPtr<Document>&& document, ScriptSourceCode&& code) Document& > Source/WebCore/worklets/WorkletGlobalScope.cpp:50 > + : m_document(WTFMove(document)) makeWeakPtr(document) > Source/WebCore/worklets/WorkletGlobalScope.cpp:52 > + , m_script(std::make_unique<WorkletScriptController>(this)) if m_script cannot be null then we should use UniqueRef instead of unique_ptr. > Source/WebCore/worklets/WorkletGlobalScope.cpp:59 > + Frame* frame = m_document->frame(); auto& > Source/WebCore/worklets/WorkletGlobalScope.cpp:60 > + m_jsRuntimeFlags = (frame ? frame->settings().javaScriptRuntimeFlags() : JSC::RuntimeFlags()); No need for the surrounding brackets. > Source/WebCore/worklets/WorkletGlobalScope.cpp:75 > + return m_topOrigin->toString(); Isn't the origin of the WorkletGlobalScope the origin of its document? The call to `setSecurityOriginPolicy(SecurityOriginPolicy::create(m_document->securityOrigin()));` in the constructor made me believe so. > Source/WebCore/worklets/WorkletGlobalScope.cpp:110 > + // Always use UTF-8 in Workers. Drop this comment, this is not a worker. > Source/WebCore/worklets/WorkletGlobalScope.h:50 > + virtual ~WorkletGlobalScope(); no need for the 'virtual'. > Source/WebCore/worklets/WorkletGlobalScope.h:52 > + virtual bool isPaintWorkletGlobalScope() const { return false; } Should probably be private. > Source/WebCore/worklets/WorkletGlobalScope.h:59 > + IDBClient::IDBConnectionProxy* idbConnectionProxy() final { ASSERT_NOT_REACHED(); return nullptr; } Should probably be private. > Source/WebCore/worklets/WorkletGlobalScope.h:62 > + void postTask(Task&&) final { ASSERT_NOT_REACHED(); } Should probably be private. > Source/WebCore/worklets/WorkletScriptController.cpp:188 > + JSC::ExecState* exec = m_workletGlobalScopeWrapper->globalExec(); auto*
Comment on attachment 353423 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=353423&action=review >> Source/WebCore/bindings/js/JSPaintWorkletGlobalScopeCustom.cpp:47 >> +void JSPaintWorkletGlobalScope::visitAdditionalChildren(JSC::SlotVisitor& visitor) > > I hear these visit methods can get called on a background thread. As a result, you'll likely need to protect all accesses / modifications to paintDefinitionMap with a Lock. You should probably confirm this with JSC engineer (like Mark Lam) before doing the extra work since I am only 90% certain.
Comment on attachment 353423 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=353423&action=review >>> Source/WebCore/bindings/js/JSPaintWorkletGlobalScopeCustom.cpp:47 >>> +void JSPaintWorkletGlobalScope::visitAdditionalChildren(JSC::SlotVisitor& visitor) >> >> I hear these visit methods can get called on a background thread. As a result, you'll likely need to protect all accesses / modifications to paintDefinitionMap with a Lock. > > You should probably confirm this with JSC engineer (like Mark Lam) before doing the extra work since I am only 90% certain. This can. >> Source/WebCore/bindings/js/JSPaintWorkletGlobalScopeCustom.cpp:54 >> +JSValue JSPaintWorkletGlobalScope::registerPaint(ExecState& state) > > Please add a comment explaining why this requires custom code. What is the bindings generator missing to generate the code you need? We don't like to add new custom code unless there is a very good reason to. Isn't this just copied from the previous implementation? > Source/WebCore/bindings/js/JSWorkletGlobalScopeBase.h:84 > +} // namespace WebCore nit: You should add a newline above this > Source/WebCore/bindings/js/JSWorkletGlobalScopeBase.h:85 > +#endif style nit: You should comment what this endif is for. You should also add a newline above this. > Source/WebCore/worklets/WorkletGlobalScope.h:109 > + bool wrapCryptoKey(const Vector<uint8_t>&, Vector<uint8_t>&) final { RELEASE_ASSERT_NOT_REACHED(); return false; } > + bool unwrapCryptoKey(const Vector<uint8_t>&, Vector<uint8_t>&) final { RELEASE_ASSERT_NOT_REACHED(); return false; } Do you have a bug for this? > Source/WebCore/worklets/WorkletGlobalScope.h:123 > + // FIXME: This is not implemented properly, it just satisfies the compiler. Should have a bug # > Source/WebCore/worklets/WorkletScriptController.cpp:105 > + JSLockHolder lock(m_vm.get()); might as well be consistent with above and do: JSLockHolder lock { vm() }; > Source/WebCore/worklets/WorkletScriptController.cpp:109 > + // Explicitly protect the global object's prototype so it isn't collected > + // when we allocate the global object. (Once the global object is fully > + // constructed, it can mark its own prototype.) This Strong<> is not needed. It'll either be alive in the stack or in a register since it's used below. Also, you pass in the "structure" below, which will mark its prototype. That "structure" is live until the global object sets its value. So this can just be a regular JSGlobalScopePrototype* > Source/WebCore/worklets/WorkletScriptController.cpp:170 > + // This FIXME is from WorkerScriptController. This is ambiguous. You should link to a bug
Created attachment 353551 [details] Patch
Comment on attachment 353551 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=353551&action=review > Source/WebCore/worklets/PaintWorkletGlobalScope.h:42 > + bool isPaintWorkletGlobalScope() const final { return true; } Why is this public? > Source/WebCore/worklets/PaintWorkletGlobalScope.h:54 > + HashMap<String, std::unique_ptr<PaintDefinition>>& paintDefinitionMap() { return m_paintDefinitionMap; } You should assert that m_paintDefinitionMapLock is locked here. > Source/WebCore/worklets/PaintWorkletGlobalScope.h:55 > + Lock& lock() { return m_lock; } name is too generic. > Source/WebCore/worklets/PaintWorkletGlobalScope.h:61 > + Lock m_lock; m_lock seems to generic a name. I'd call this m_paintDefinitionMapLock;
Comment on attachment 353423 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=353423&action=review >>> Source/WebCore/bindings/js/JSPaintWorkletGlobalScopeCustom.cpp:54 >>> +JSValue JSPaintWorkletGlobalScope::registerPaint(ExecState& state) >> >> Please add a comment explaining why this requires custom code. What is the bindings generator missing to generate the code you need? We don't like to add new custom code unless there is a very good reason to. > > Isn't this just copied from the previous implementation? No, this is new. >> Source/WebCore/dom/Document.cpp:8390 >> + m_paintWorkletGlobalScope = WTFMove(scope); > > Should we ASSERT(!m_paintWorkletGlobalScope); before this? or is it OK to overwrite a previous WorkletGlobalScope? I added a FIXME >> Source/WebCore/worklets/PaintWorkletGlobalScope.h:43 >> + bool isPaintWorkletGlobalScope() const final { return true; } > > Should probably be private. This can't be, it is used in WorkletScriptController >> Source/WebCore/worklets/WorkletGlobalScope.h:109 >> + bool unwrapCryptoKey(const Vector<uint8_t>&, Vector<uint8_t>&) final { RELEASE_ASSERT_NOT_REACHED(); return false; } > > Do you have a bug for this? No. It looks like it isn't actually a bug, since Worklets don't allow access to indexeddb.
Created attachment 353557 [details] Patch
(In reply to Chris Dumez from comment #22) > Comment on attachment 353551 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=353551&action=review > > > Source/WebCore/worklets/PaintWorkletGlobalScope.h:42 > > + bool isPaintWorkletGlobalScope() const final { return true; } > > Why is this public? It is needed by WorkletScriptController. I tried to use is<>() instead, but the type traits would also require it to be public. > > Source/WebCore/worklets/PaintWorkletGlobalScope.h:54 > > + HashMap<String, std::unique_ptr<PaintDefinition>>& paintDefinitionMap() { return m_paintDefinitionMap; } > > You should assert that m_paintDefinitionMapLock is locked here. Great idea! I put in a RELEASE_ASSERT, although I am not sure if that should be a regular ASSERT instead. I am not sure if a race here could cause a security issue.
Comment on attachment 353557 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=353557&action=review > Source/WebCore/worklets/PaintWorkletGlobalScope.h:54 > + HashMap<String, std::unique_ptr<PaintDefinition>>& paintDefinitionMap() { RELEASE_ASSERT(m_paintDefinitionLock.isLocked()); return m_paintDefinitionMap; } I would use a debug assertion for performance reasons here.
(In reply to Justin Michaud from comment #25) > (In reply to Chris Dumez from comment #22) > > Comment on attachment 353551 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=353551&action=review > > > > > Source/WebCore/worklets/PaintWorkletGlobalScope.h:42 > > > + bool isPaintWorkletGlobalScope() const final { return true; } > > > > Why is this public? > > It is needed by WorkletScriptController. I tried to use is<>() instead, but > the type traits would also require it to be public. Does not make much sense, it should be public in the base class, and private in the subclass because it makes no sense to call this method on the subclass (if you are, you are doing a useless check). And Yes, you should be using is<>() / downcast<>() whenever possible but is<>() does usually rely on having such a virtual function defined.
Created attachment 353560 [details] Patch
Comment on attachment 353560 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=353560&action=review > Source/WebCore/bindings/js/JSPaintWorkletGlobalScopeCustom.cpp:55 > +// This custom binding is required to handle the paint callback conversion according to the spec. Still not clear for me why we need custom bindings. We have plenty of API that takes callbacks without custom bindings (e.g. MutationObserver constructor, FileSystemEntry.getParent, Geolocation.getCurrentPosition). You still have not convinced me you NEED custom bindings. > Source/WebCore/bindings/js/JSPaintWorkletGlobalScopeCustom.cpp:127 > + auto paintDefinition = std::unique_ptr<PaintDefinition>(new PaintDefinition { name, paint.releaseNonNull(), inputProperties, inputArguments }); heh.. std::make_unique<PaintDefinition>(name, paint.releaseNonNull(), inputProperties, inputArguments) > Source/WebCore/bindings/js/JSWorkletGlobalScopeBase.cpp:120 > + const auto* thisObject = jsCast<const JSWorkletGlobalScopeBase*>(object); auto*, we like to omit 'const' whenever possible. > Source/WebCore/bindings/js/JSWorkletGlobalScopeBase.cpp:143 > + const ClassInfo* classInfo = asObject(value)->classInfo(vm); auto* > Source/WebCore/bindings/js/JSWorkletGlobalScopeBase.h:84 > +} // namespace WebCore Missing blank line before this. > Source/WebCore/dom/ScriptExecutionContext.cpp:488 > + RELEASE_ASSERT_NOT_REACHED(); Does this really need to be a RELEASE_ASSERT? > Source/WebCore/dom/ScriptExecutionContext.cpp:532 > + WorkerGlobalScope& workerGlobalScope = downcast<WorkerGlobalScope>(*this); This local variable is not needed, then you don't need the curly brackets either. > Source/WebCore/dom/ScriptExecutionContext.cpp:537 > + WorkletGlobalScope& workletGlobalScope = downcast<WorkletGlobalScope>(*this); This local variable is not needed. > Source/WebCore/worklets/PaintWorkletGlobalScope.cpp:49 > + ASSERT(m_document->domWindow()); These assertions look super risky to me and I'd suggest null checks instead. You can probably return 1.0 when document or window are null. > Source/WebCore/worklets/PaintWorkletGlobalScope.h:44 > + double devicePixelRatio(); const ? > Source/WebCore/worklets/PaintWorkletGlobalScope.idl:37 > + [Custom, MayThrowException] void registerPaint(DOMString name, Function paintCtor); paintCtor -> paintConstructor > Source/WebCore/worklets/WorkletConsoleClient.h:46 > +protected: It makes no sense for a class marked as final to have protected members. If the class is really final, then these members should be private. > Source/WebCore/worklets/WorkletConsoleClient.h:47 > + void messageWithTypeAndLevel(MessageType, MessageLevel, JSC::ExecState*, Ref<Inspector::ScriptArguments>&&) override; override -> final, here and below. > Source/WebCore/worklets/WorkletGlobalScope.cpp:54 > + , m_identifier("WorkletGlobalScope:" + Inspector::IdentifiersFactory::createIdentifier()) makeString("WorkletGlobalScope:"_s, Inspector::IdentifiersFactory::createIdentifier()) > Source/WebCore/worklets/WorkletGlobalScope.cpp:58 > + auto* frame = m_document->frame(); I'd rather you use document.frame() here as it looks safer than m_document-> > Source/WebCore/worklets/WorkletGlobalScope.cpp:60 > + ASSERT(m_document->page()); ditto. > Source/WebCore/worklets/WorkletGlobalScope.cpp:79 > + ASSERT(m_document); This assertion looks risky to me, I'd suggest a null check. Same comment in other places. Or tell me what guarantees that you always have a document. > Source/WebCore/worklets/WorkletGlobalScope.h:49 > +class WorkletGlobalScope : public RefCounted<WorkletGlobalScope>, public Supplementable<WorkletGlobalScope>, public ScriptExecutionContext, public EventTargetWithInlineData { I don't think you need this to be Supplementable, do you? > Source/WebCore/worklets/WorkletGlobalScope.h:64 > + WorkletScriptController* script() { return &m_script.get(); } Why doesn't this return a WorkletScriptController& instead of a WorkletScriptController* ? I can never return null. > Source/WebCore/worklets/WorkletGlobalScope.h:81 > + WeakPtr<Document> m_document; Having protected data members is bad practice. Data members should be private. You can add a protected getter for that data member if needed. > Source/WebCore/worklets/WorkletGlobalScope.h:126 > + mutable WorkerEventQueue m_eventQueue; Why does this need to be mutable? > Source/WebCore/worklets/WorkletScriptController.cpp:187 > +#endif Missing blank line before this.
Comment on attachment 353560 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=353560&action=review >> Source/WebCore/bindings/js/JSPaintWorkletGlobalScopeCustom.cpp:127 >> + auto paintDefinition = std::unique_ptr<PaintDefinition>(new PaintDefinition { name, paint.releaseNonNull(), inputProperties, inputArguments }); > > heh.. std::make_unique<PaintDefinition>(name, paint.releaseNonNull(), inputProperties, inputArguments) It's a struct, so that doesn't work. Should I leave it as-is, or add a constructor? >> Source/WebCore/dom/ScriptExecutionContext.cpp:488 >> + RELEASE_ASSERT_NOT_REACHED(); > > Does this really need to be a RELEASE_ASSERT? I wasn't sure if commonVM() was safe to be returned, but I needed to return a reference to something to satisfy the compiler. >> Source/WebCore/worklets/WorkletGlobalScope.h:126 >> + mutable WorkerEventQueue m_eventQueue; > > Why does this need to be mutable? The signature of the getter is: EventQueue& eventQueue() const For some reason. I have no clue why, but I can't change it without changing ScriptExecutionContext
Created attachment 353643 [details] Patch
Comment on attachment 353643 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=353643&action=review > Source/WebCore/worklets/WorkletScriptController.cpp:157 > +void WorkletScriptController::evaluate(const ScriptSourceCode& sourceCode, NakedPtr<JSC::Exception>& returnedException, String* returnedExceptionMessage) Can you confirm this is correct? Worklets have a unique opaque origin, so normally this would always sanitize exception messages. At the same time, it is important that exceptions show up in the inspector. As far as I know, there is nowhere where a worklet can import a cross-origin script (do ES6 modules allow cross-origin imports?), so this is safe.
Comment on attachment 353643 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=353643&action=review r=me >> Source/WebCore/worklets/WorkletScriptController.cpp:157 >> +void WorkletScriptController::evaluate(const ScriptSourceCode& sourceCode, NakedPtr<JSC::Exception>& returnedException, String* returnedExceptionMessage) > > Can you confirm this is correct? Worklets have a unique opaque origin, so normally this would always sanitize exception messages. At the same time, it is important that exceptions show up in the inspector. As far as I know, there is nowhere where a worklet can import a cross-origin script (do ES6 modules allow cross-origin imports?), so this is safe. You are not exposing this exception message to JS, are you? Logging it in WebInspector does not count as passing it to JS.
Created attachment 353751 [details] Patch
Comment on attachment 353751 [details] Patch Clearing flags on attachment: 353751 Committed r237766: <https://trac.webkit.org/changeset/237766>
All reviewed patches have been landed. Closing bug.
<rdar://problem/45780764>