Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

messageerror event doesn't fire on ServiceWorkerGlobalScope #27974

Conversation

youennf
Copy link
Contributor

@youennf youennf commented May 1, 2024

fddda89

messageerror event doesn't fire on ServiceWorkerGlobalScope
https://bugs.webkit.org/show_bug.cgi?id=272967
rdar://127104436

Reviewed by Chris Dumez.

Like for messages on worker, we now deserialize the data just before firing the event.
If deserialization fails, we use messageerror event, other wise we keep using message event.

We update ExtendableMessageEvent to use a JSValueInWrappedObject instead of a SerializedScriptValue.
This allows to store a JSValue, which also simplifies the custom binding.

We reuse the same principle for the JS constructor.
We continue using the custom constructor as we create the JS wrapper in ExtendableMessageEvent::create.

* LayoutTests/imported/w3c/web-platform-tests/html/infrastructure/safe-passing-of-structured-data/shared-array-buffers/window-serviceworker-failure.https-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/service-workers/service-worker/ServiceWorkerGlobalScope/error-message-event-worker.js: Added.
* LayoutTests/imported/w3c/web-platform-tests/service-workers/service-worker/ServiceWorkerGlobalScope/error-message-event.https-expected.txt: Added.
* LayoutTests/imported/w3c/web-platform-tests/service-workers/service-worker/ServiceWorkerGlobalScope/error-message-event.https.html: Added.
* LayoutTests/imported/w3c/web-platform-tests/wasm/serialization/module/window-serviceworker-failure.https-expected.txt:
* Source/WebCore/bindings/js/JSExtendableMessageEventCustom.cpp:
(WebCore::constructJSExtendableMessageEvent):
(WebCore::JSExtendableMessageEvent::visitAdditionalChildren):
(WebCore::JSExtendableMessageEvent::data const): Deleted.
* Source/WebCore/workers/service/ExtendableMessageEvent.cpp:
(WebCore::createWrapperAndSetData):
(WebCore::ExtendableMessageEvent::create):
(WebCore::ExtendableMessageEvent::ExtendableMessageEvent):
* Source/WebCore/workers/service/ExtendableMessageEvent.h:
* Source/WebCore/workers/service/ExtendableMessageEvent.idl:
* Source/WebCore/workers/service/context/ServiceWorkerThread.cpp:
(WebCore::fireMessageEvent):

Canonical link: https://commits.webkit.org/278408@main

63f08ca

Misc iOS, tvOS & watchOS macOS Linux Windows
βœ… πŸ§ͺ style βœ… πŸ›  ios βœ… πŸ›  mac βœ… πŸ›  wpe βœ… πŸ›  wincairo
βœ… πŸ§ͺ bindings βœ… πŸ›  ios-sim βœ… πŸ›  mac-AS-debug βœ… πŸ§ͺ wpe-wk2
βœ… πŸ§ͺ webkitperl βœ… πŸ§ͺ ios-wk2 βœ… πŸ§ͺ api-mac βœ… πŸ§ͺ api-wpe
βœ… πŸ§ͺ ios-wk2-wpt βœ… πŸ§ͺ mac-wk1 βœ… πŸ›  wpe-skia
❌ πŸ§ͺ api-ios βœ… πŸ§ͺ mac-wk2 βœ… πŸ›  gtk
βœ… πŸ›  tv βœ… πŸ§ͺ mac-AS-debug-wk2 βœ… πŸ§ͺ gtk-wk2
βœ… πŸ›  tv-sim βœ… πŸ§ͺ mac-wk2-stress βœ… πŸ§ͺ api-gtk
βœ… πŸ›  πŸ§ͺ merge βœ… πŸ›  watch
βœ… πŸ›  watch-sim

@youennf youennf self-assigned this May 1, 2024
@youennf youennf added the Service Workers Component for Service Workers bugs. label May 1, 2024
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label May 1, 2024
@youennf youennf force-pushed the eng/messageerror-event-doesnt-fire-on-ServiceWorkerGlobalScope branch from 2b8ce58 to 7f3055f Compare May 1, 2024 10:32
@youennf youennf force-pushed the eng/messageerror-event-doesnt-fire-on-ServiceWorkerGlobalScope branch from 7f3055f to 818c866 Compare May 2, 2024 08:41
@youennf youennf removed the merging-blocked Applied to prevent a change from being merged label May 2, 2024
@youennf youennf marked this pull request as ready for review May 2, 2024 10:06
@youennf youennf requested a review from cdumez as a code owner May 2, 2024 10:06
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label May 2, 2024
@youennf youennf force-pushed the eng/messageerror-event-doesnt-fire-on-ServiceWorkerGlobalScope branch 2 times, most recently from 88a727b to 63f08ca Compare May 5, 2024 14:16
@youennf youennf removed the merging-blocked Applied to prevent a change from being merged label May 5, 2024
Copy link
Contributor

@cdumez cdumez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@youennf youennf added the merge-queue Applied to send a pull request to merge-queue label May 6, 2024
https://bugs.webkit.org/show_bug.cgi?id=272967
rdar://127104436

Reviewed by Chris Dumez.

Like for messages on worker, we now deserialize the data just before firing the event.
If deserialization fails, we use messageerror event, other wise we keep using message event.

We update ExtendableMessageEvent to use a JSValueInWrappedObject instead of a SerializedScriptValue.
This allows to store a JSValue, which also simplifies the custom binding.

We reuse the same principle for the JS constructor.
We continue using the custom constructor as we create the JS wrapper in ExtendableMessageEvent::create.

* LayoutTests/imported/w3c/web-platform-tests/html/infrastructure/safe-passing-of-structured-data/shared-array-buffers/window-serviceworker-failure.https-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/service-workers/service-worker/ServiceWorkerGlobalScope/error-message-event-worker.js: Added.
* LayoutTests/imported/w3c/web-platform-tests/service-workers/service-worker/ServiceWorkerGlobalScope/error-message-event.https-expected.txt: Added.
* LayoutTests/imported/w3c/web-platform-tests/service-workers/service-worker/ServiceWorkerGlobalScope/error-message-event.https.html: Added.
* LayoutTests/imported/w3c/web-platform-tests/wasm/serialization/module/window-serviceworker-failure.https-expected.txt:
* Source/WebCore/bindings/js/JSExtendableMessageEventCustom.cpp:
(WebCore::constructJSExtendableMessageEvent):
(WebCore::JSExtendableMessageEvent::visitAdditionalChildren):
(WebCore::JSExtendableMessageEvent::data const): Deleted.
* Source/WebCore/workers/service/ExtendableMessageEvent.cpp:
(WebCore::createWrapperAndSetData):
(WebCore::ExtendableMessageEvent::create):
(WebCore::ExtendableMessageEvent::ExtendableMessageEvent):
* Source/WebCore/workers/service/ExtendableMessageEvent.h:
* Source/WebCore/workers/service/ExtendableMessageEvent.idl:
* Source/WebCore/workers/service/context/ServiceWorkerThread.cpp:
(WebCore::fireMessageEvent):

Canonical link: https://commits.webkit.org/278408@main
@webkit-commit-queue webkit-commit-queue force-pushed the eng/messageerror-event-doesnt-fire-on-ServiceWorkerGlobalScope branch from 63f08ca to fddda89 Compare May 6, 2024 17:02
@webkit-commit-queue
Copy link
Collaborator

Committed 278408@main (fddda89): https://commits.webkit.org/278408@main

Reviewed commits have been landed. Closing PR #27974 and removing active labels.

@webkit-commit-queue webkit-commit-queue merged commit fddda89 into WebKit:main May 6, 2024
@webkit-commit-queue webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label May 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Service Workers Component for Service Workers bugs.
Projects
None yet
5 participants