Bug 178534

Created attachment 324289 [details] WIP Patch

Created attachment 324303 [details] WIP Patch

Created attachment 324560 [details] WIP Patch

Created attachment 324573 [details] WIP Patch

Created attachment 324594 [details] WIP Patch

Attachment 324594 [details] did not pass style-queue: ERROR: Source/WebCore/workers/service/ServiceWorker.h:32: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 1 in 22 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 324614 [details] WIP Patch

Created attachment 324630 [details] Patch

Created attachment 324631 [details] Patch

Created attachment 324634 [details] Patch

Created attachment 324636 [details] Patch

Created attachment 324639 [details] Patch

Created attachment 324702 [details] Patch

Comment on attachment 324639 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=324639&action=review > Source/WebCore/ChangeLog:18 > + * bindings/js/JSExtendableMessageEvent.cpp: Copied from Source/WebCore/workers/service/ServiceWorker.cpp. Probably added, not copied. > Source/WebCore/bindings/js/JSExtendableMessageEventCustom.cpp:54 > + return result; Is there a way to share some code with JSMessageEvent::data? > Source/WebCore/dom/ScriptExecutionContext.h:114 > + virtual String origin() const = 0; Why not using a SecurityOrigin&/SecurityOrigin*? > Source/WebCore/workers/service/ExtendableMessageEvent.h:57 > + Vector<RefPtr<MessagePort>> ports; Would it be difficult to have a Vector<Ref<MessagePort>> instead? > Source/WebCore/workers/service/ServiceWorker.cpp:68 > + if (!scriptExecutionContext()) I am not sure we can get both a valid ExecState and a null scriptExecutionContext. If so, I would add it as a first check. Can we get a ScriptExecutionContext& as first parameter and get an ExecState from it? Maybe add an ASSERT. > Source/WebCore/workers/service/ServiceWorkerContainer.cpp:84 > + m_controller = ServiceWorker::create(*context, context->selectedServiceWorkerIdentifier()); Can we get a controller whose identifier is not context selectedServiceWorkerIdentifier? > Source/WebCore/workers/service/context/ServiceWorkerThread.cpp:109 > + serviceWorkerGlobalScope.dispatchEvent(ExtendableMessageEvent::create(WTFMove(ports), WTFMove(message), sourceOrigin)); I guess sourceOrigin might be different from the origin of the service worker, like in the case of data URL iframes. If I am wrong, we might not even need to pass the origin. If the origin cannot be any random origin, maybe we should assert somehow that this is in a restricted set. I probably need to read the spec. > Source/WebCore/workers/service/context/ServiceWorkerThread.h:43 > +using MessagePortChannelArray = Vector<std::unique_ptr<MessagePortChannel>, 1>; Should probably be UniqueRef if possible. > Source/WebCore/workers/service/context/ServiceWorkerThread.h:56 > + WEBCORE_EXPORT void postMessageToServiceWorkerGlobalScope(Ref<SerializedScriptValue>&&, std::unique_ptr<MessagePortChannelArray>&&, const String& sourceOrigin); Here as well. > Source/WebKit/WebProcess/Storage/ServiceWorkerContextManager.cpp:76 > + auto* workerThread = m_workerThreadMap.get(serviceWorkerIdentifier); Should we protect for a serviceWorkerIdentifier being zero? Even more valid for startFetch above.

Comment on attachment 324639 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=324639&action=review >> Source/WebCore/bindings/js/JSExtendableMessageEventCustom.cpp:54 >> + return result; > > Is there a way to share some code with JSMessageEvent::data? I'll see what I can do. >> Source/WebCore/dom/ScriptExecutionContext.h:114 >> + virtual String origin() const = 0; > > Why not using a SecurityOrigin&/SecurityOrigin*? This is a pre-existing method on Document / WorkerGlobalScope is is used for the self.origin Web API. This is why it currently returns a String. Also note that I need the origin as a String currently since it is used to populate the messageEvent.source attribute. >> Source/WebCore/workers/service/ExtendableMessageEvent.h:57 >> + Vector<RefPtr<MessagePort>> ports; > > Would it be difficult to have a Vector<Ref<MessagePort>> instead? Yes. This is a long-standing issue with our bindings converters. >> Source/WebCore/workers/service/ServiceWorker.cpp:68 >> + if (!scriptExecutionContext()) > > I am not sure we can get both a valid ExecState and a null scriptExecutionContext. > If so, I would add it as a first check. > > Can we get a ScriptExecutionContext& as first parameter and get an ExecState from it? > Maybe add an ASSERT. Ok. >> Source/WebCore/workers/service/ServiceWorkerContainer.cpp:84 >> + m_controller = ServiceWorker::create(*context, context->selectedServiceWorkerIdentifier()); > > Can we get a controller whose identifier is not context selectedServiceWorkerIdentifier? I think this ways: 1. Register service worker A 2. Access container.controller 3. Register service worker B 4. Access container.controller >> Source/WebCore/workers/service/context/ServiceWorkerThread.cpp:109 >> + serviceWorkerGlobalScope.dispatchEvent(ExtendableMessageEvent::create(WTFMove(ports), WTFMove(message), sourceOrigin)); > > I guess sourceOrigin might be different from the origin of the service worker, like in the case of data URL iframes. > If I am wrong, we might not even need to pass the origin. > If the origin cannot be any random origin, maybe we should assert somehow that this is in a restricted set. > I probably need to read the spec. No, the source origin is not necessarily the same as the worker's origin. It is the origin of the calling script. >> Source/WebCore/workers/service/context/ServiceWorkerThread.h:43 >> +using MessagePortChannelArray = Vector<std::unique_ptr<MessagePortChannel>, 1>; > > Should probably be UniqueRef if possible. This is a pre-existing WebCore type. I am merely forward declaring. >> Source/WebCore/workers/service/context/ServiceWorkerThread.h:56 >> + WEBCORE_EXPORT void postMessageToServiceWorkerGlobalScope(Ref<SerializedScriptValue>&&, std::unique_ptr<MessagePortChannelArray>&&, const String& sourceOrigin); > > Here as well. No, as mentioned in an earlier patch, channels can be null. >> Source/WebKit/WebProcess/Storage/ServiceWorkerContextManager.cpp:76 >> + auto* workerThread = m_workerThreadMap.get(serviceWorkerIdentifier); > > Should we protect for a serviceWorkerIdentifier being zero? > Even more valid for startFetch above. It would hit an assertion in HashMap.

Created attachment 324713 [details] Patch

Comment on attachment 324713 [details] Patch Clearing flags on attachment: 324713 Committed r223922: <https://trac.webkit.org/changeset/223922>

All reviewed patches have been landed. Closing bug.

<rdar://problem/35568649>