Skip to content

fix-polyfill WebRTC-events-when-proto-is-non-writable-but-configurable#1194

Open
johnny-quesada-developer wants to merge 1 commit into
webrtcHacks:mainfrom
johnny-quesada-developer:fix-polyfill-WebRTC-events-when-proto-is-non-writable-but-configurable
Open

fix-polyfill WebRTC-events-when-proto-is-non-writable-but-configurable#1194
johnny-quesada-developer wants to merge 1 commit into
webrtcHacks:mainfrom
johnny-quesada-developer:fix-polyfill-WebRTC-events-when-proto-is-non-writable-but-configurable

Conversation

@johnny-quesada-developer
Copy link
Copy Markdown

Summary

Fix wrapPeerConnectionEvent so it does not skip event polyfilling when RTCPeerConnection.prototype event 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 on RTCPeerConnection.prototype:

proto.addEventListener = function(nativeEventName, cb) { ... };
proto.removeEventListener = function(nativeEventName, cb) { ... };

In some browsers or environments, these properties can behave as read-only, so direct assignment throws a TypeError:

Cannot assign to read only property 'addEventListener' of object '#<RTCPeerConnection>'

The latest adapter version avoids the crash by checking whether EventTarget.prototype.addEventListener is writable before applying the polyfill. However, that check is not enough to determine whether RTCPeerConnection.prototype can 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:

localStorage.setItem('error-test', true);

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.prototype descriptor 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:

(function () {
  'use strict';

  const object = {};

  Object.defineProperty(object, 'readonly', {
    value: false,
    writable: false,
    configurable: true,
  });

  // This throws because the property is read-only.
  object.readonly = someNewValue();
})();

However, even when a property is non-writable, it can still be redefined if it is configurable:

// This works because the property is configurable, even though it is read-only.
Object.defineProperty(object, 'readonly', {
  value: someNewValue(),
  writable: true,
  configurable: true,
});

This is the relevant case for RTCPeerConnection.prototype.addEventListener and RTCPeerConnection.prototype.removeEventListener: direct assignment can fail, but Object.defineProperty can still work when the property is configurable.

Root Cause

The old guard checked EventTarget.prototype.addEventListener.writable globally:

const addEventListener = Object.getOwnPropertyDescriptor(
  EventTarget.prototype,
  'addEventListener'
);

if (!addEventListener.writable) {
  return;
}

The main issue is that the guard checks the descriptor on EventTarget.prototype, but the polyfill modifies RTCPeerConnection.prototype.

Those descriptors do not necessarily have the same constraints. EventTarget.prototype may fail the writability check, while the actual patch target, RTCPeerConnection.prototype, can still be configurable and therefore patchable with Object.defineProperty.

In other words, writable: false prevents 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:

Object.defineProperty(target, property, {
  value: target[property],
  writable: true,
  configurable: true,
});

If the property can be redefined, the helper returns true. If the property is truly locked, it catches the error and returns false.

The polyfill now only proceeds when both required methods can be prepared:

  const isAddEventListenerWritable = tryMakePropertyWritable(proto, 'addEventListener');
  const isRemoveEventListenerWritable = tryMakePropertyWritable(proto, 'removeEventListener');
  const canPolyfill = isAddEventListenerWritable && isRemoveEventListenerWritable;

  if (!canPolyfill) {
    log('Unable to polyfill events');
    return;
  }

This makes the check more accurate because it validates the actual prototype being patched.

Behavior Before

const addEventListener = Object.getOwnPropertyDescriptor(
  EventTarget.prototype,
  'addEventListener'
);

if (!addEventListener.writable) {
  return;
}

The polyfill could be skipped based on the descriptor of EventTarget.prototype, even though the actual patch target was RTCPeerConnection.prototype.

Behavior After

const proto = window.RTCPeerConnection.prototype;

const canPolyfill =
  tryMakePropertyWritable(proto, 'addEventListener') &&
  tryMakePropertyWritable(proto, 'removeEventListener');

if (!canPolyfill) {
  log('Unable to polyfill events');
  return;
}

The polyfill now checks whether the actual target can be patched before replacing the event listener methods.

Test Plan

Added unit tests for wrapPeerConnectionEvent covering:

  • Configurable properties

    • addEventListener and removeEventListener can be redefined.
    • The polyfill is applied successfully.
    • Events are transformed before reaching the listener.
  • Non-configurable properties

    • The polyfill does not throw.
    • The prototype is left untouched.
    • Native callbacks continue to work without wrapping.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant