fix-polyfill WebRTC-events-when-proto-is-non-writable-but-configurable#1194
Open
johnny-quesada-developer wants to merge 1 commit into
Conversation
This file contains hidden or 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
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.
Summary
Fix
wrapPeerConnectionEventso it does not skip event polyfilling whenRTCPeerConnection.prototypeevent listener methods are non-writable but still configurable.Background
I found this issue while testing a WebRTC application embedded in Wix.
The current implementation checks the descriptor of
EventTarget.prototype.addEventListener, but the polyfill actually patches methods onRTCPeerConnection.prototype:In some browsers or environments, these properties can behave as read-only, so direct assignment throws a
TypeError:The latest adapter version avoids the crash by checking whether
EventTarget.prototype.addEventListeneris writable before applying the polyfill. However, that check is not enough to determine whetherRTCPeerConnection.prototypecan actually be patched.This is a known issue that has been reported in other contexts as well:
Reproduction
This can be reproduced with this Wix test page:
By default, the page loads with the polyfill fix applied. To disable the fix and reproduce the original error, run this in the browser console:
Then reload the page. You will see the crash in the console.
Important: Because the test page uses an old version that does not have the
EventTarget.prototypedescriptor check, the issue was loud and visible. The latest version of adapter does not crash, but it also does not apply the polyfill.A minimal code example to illustrate the underlying JavaScript behavior:
However, even when a property is non-writable, it can still be redefined if it is configurable:
This is the relevant case for
RTCPeerConnection.prototype.addEventListenerandRTCPeerConnection.prototype.removeEventListener: direct assignment can fail, butObject.definePropertycan still work when the property is configurable.Root Cause
The old guard checked
EventTarget.prototype.addEventListener.writableglobally:The main issue is that the guard checks the descriptor on
EventTarget.prototype, but the polyfill modifiesRTCPeerConnection.prototype.Those descriptors do not necessarily have the same constraints.
EventTarget.prototypemay fail the writability check, while the actual patch target,RTCPeerConnection.prototype, can still be configurable and therefore patchable withObject.defineProperty.In other words,
writable: falseprevents direct assignment, but it does not prevent redefining the property when the property is configurable.Fix
Introduce
tryMakePropertyWritable, which attempts to redefine the target property as writable before applying the polyfill.Instead of relying on direct assignment, it uses
Object.defineProperty:If the property can be redefined, the helper returns
true. If the property is truly locked, it catches the error and returnsfalse.The polyfill now only proceeds when both required methods can be prepared:
This makes the check more accurate because it validates the actual prototype being patched.
Behavior Before
The polyfill could be skipped based on the descriptor of
EventTarget.prototype, even though the actual patch target wasRTCPeerConnection.prototype.Behavior After
The polyfill now checks whether the actual target can be patched before replacing the event listener methods.
Test Plan
Added unit tests for
wrapPeerConnectionEventcovering:Configurable properties
addEventListenerandremoveEventListenercan be redefined.Non-configurable properties