window.postMessage with OffscreenCanvas is broken with isolated world message listener #16003
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
cdumez
added
the
WebKit API
|
EWS run on previous version of this PR (hash d4cb260)
|
webkit-ews-buildbot
added
the
merging-blocked
darinadler
approved these changes
Jul 31, 2023
| #endif | ||
| Vector<RefPtr<MessagePort>>& inMemoryMessagePorts, |
There was a problem hiding this comment.
Slightly inconsistent indenting here. When we end up with these large packages of parameters, I wonder if we might need some kind of structure instead of a parameter list, anyway. Should be fine for now, but #if and parameter lists don’t combined well.
Comment on lines
+3902
to
+3908
| uint32_t index; | ||
| bool indexSuccessfullyRead = read(index); | ||
| if (!indexSuccessfullyRead || index >= m_inMemoryOffscreenCanvases.size()) { | ||
| fail(); | ||
| return JSValue(); | ||
| } | ||
| return getJSValue(m_inMemoryOffscreenCanvases[index].get()); |
There was a problem hiding this comment.
Would like to explore things in future to make this code less repetitive.
webkit-ews-buildbot
added
the
merging-blocked
cdumez
force-pushed
the
OffscreenCanvas_isolatedWorld
branch
from
d4cb260
to
c12afab
Compare
|
EWS run on current version of this PR (hash c12afab)
|
… message listener https://bugs.webkit.org/show_bug.cgi?id=259362 rdar://112618195 Reviewed by Darin Adler. When constructing a MessageEvent, we would deserialize the `data` SerializedScriptValue and cache the resulting JSValue. When accessing MessageEvent.data from the main world, we would return the cached JSValue and everything would work fine. However, upon accessing MessageEvent.data from a non-main world, the cached JSValue would not be usable and we would deserialize the original SerializedScriptValue again. The issue is that a SerializedScriptValue is not meant to be deserialized several times. This is because the deserialization "consumes" certain internal objects. For examples, OffscreenCanvas are stored as DetachedOffscreenCanvas internally and consumed upon deserialization to construct OffscreenCanvas objects again. To address the issue, this patch makes several changes: 1. MessageEvent::create() now stores the deserialized JSValue inside the MessageEvent object instead of the SerializedScriptValue. As a result, when accessing MessageEvent.data from the main world, we'll just return the internal JSValue. When accessing MessageEvent.data from a non-main world, cachedPropertyValue() will detect that the internal JSValue is no compatible with this world and call cloneAcrossWorlds() on the internal JSValue to generate one suitable for the non-main world. Internally, cloneAcrossWorlds() creates a SerializedScriptValue from the JSValue and then deserializes that SerializedScriptValue in the target world. 2. As currently implemented, cloneAcrossWorlds() would drop transferrable objects such as OffscreenCanvas and MessagePort. To address the issue, we now introduce a new CloneAcrossWorlds SerializationContext. When in this context, SerializedScriptValue serialization will store OffscreenCanvas/MessagePort in the JSValue inside internal vectors and merely serialize indexes inside those vectors. Upon deserialization, we deserialize the index and lookup the OffscreenCanvas/MessagePort from the internal vector. Then, we call toJS() on the implementation object to get a JS wrapper for the target world. * Source/WebCore/bindings/js/JSDOMWrapper.cpp: (WebCore::cloneAcrossWorlds): * Source/WebCore/bindings/js/SerializedScriptValue.cpp: (WebCore::isTypeExposedToGlobalObject): (WebCore::CloneSerializer::serialize): (WebCore::CloneSerializer::CloneSerializer): (WebCore::CloneSerializer::dumpOffscreenCanvas): (WebCore::CloneSerializer::dumpIfTerminal): (WebCore::CloneDeserializer::deserialize): (WebCore::CloneDeserializer::CloneDeserializer): (WebCore::CloneDeserializer::readInMemoryOffscreenCanvas): (WebCore::CloneDeserializer::readTerminal): (WebCore::SerializedScriptValue::SerializedScriptValue): (WebCore::SerializedScriptValue::create): (WebCore::SerializedScriptValue::deserialize): * Source/WebCore/bindings/js/SerializedScriptValue.h: * Source/WebCore/dom/MessageEvent.cpp: (WebCore::MessageEvent::create): * Tools/TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj: * Tools/TestWebKitAPI/Tests/WebKitCocoa/UserContentWorld.mm: (-[UserContentWorldMessageHandler userContentController:didReceiveScriptMessage:]): (TEST): * Tools/TestWebKitAPI/Tests/WebKitCocoa/postMessage-various-types.html: Added. Canonical link: https://commits.webkit.org/266465@main
webkit-commit-queue
force-pushed
the
OffscreenCanvas_isolatedWorld
branch
from
c12afab
to
acece69
Compare
|
Committed 266465@main (acece69): https://commits.webkit.org/266465@main Reviewed commits have been landed. Closing PR #16003 and removing active labels. |
webkit-commit-queue
removed
the
merge-queue
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
acece69
c12afab