Skip to content
This repository has been archived by the owner on Feb 26, 2024. It is now read-only.

Zone.js 0.8.13+ breaks addEventListener in Safari 11.0, 11.1 #883

Closed
markandrus opened this issue Aug 21, 2017 · 12 comments · Fixed by #905
Closed

Zone.js 0.8.13+ breaks addEventListener in Safari 11.0, 11.1 #883

markandrus opened this issue Aug 21, 2017 · 12 comments · Fixed by #905

Comments

@markandrus
Copy link
Contributor

Hi,

I'm a WebRTC developer, and I noticed an issue with addEventListener in Safari using Zone.js. I doubt this issue is specific to WebRTC—I just happened to notice it when using the RTCPeerConnection APIs. Try for example, the following in the console for Chrome, Firefox, and Safari (11.0 or greater). They should all yield the same result.

Example without Zone.js

(async () => {
  console.log('Create RTCPeerConnection')
  const pc = new RTCPeerConnection()

  console.log('Add "track" event listener')
  let event
  pc.addEventListener('track', _event => event = _event)

  console.log('Dispatch "track" event')
  pc.dispatchEvent(new Event('track'))

  if (event) {
    console.log('Success!')
  } else {
    console.error('Failure!')
  }
})().catch(console.error)

Results

Create RTCPeerConnection
Add "track" event listener
Dispatch "track" event
Success!

Now, try loading Zone.js first and re-running the code sample.

Example with Zone.js

(async () => {
  console.log('Load Zone.js')
  const script = document.createElement('script')
  script.src = 'https://cdnjs.cloudflare.com/ajax/libs/zone.js/0.8.16/zone.js'
  document.body.appendChild(script)
  await new Promise(resolve => script.onload = resolve)

  console.log('Create RTCPeerConnection')
  const pc = new RTCPeerConnection()

  console.log('Add "track" event listener')
  let event
  pc.addEventListener('track', _event => event = _event)

  console.log('Dispatch "track" event')
  pc.dispatchEvent(new Event('track'))

  if (event) {
    console.log('Success!')
  } else {
    console.error('Failure!')
  }
})().catch(console.error)

Expected Results

Load Zone.js
Create RTCPeerConnection
Add "track" event listener
Dispatch "track" event
Success!

Actual Results (Chrome, Firefox)

Chrome and Firefox work fine.

Load Zone.js
Create RTCPeerConnection
Add "track" event listener
Dispatch "track" event
Success!

Actual Results (Safari 11.0, Safari 11.1)

Safari 11.0 and Safari 11.1 do not.

Load Zone.js
Create RTCPeerConnection
Add "track" event listener
Dispatch "track" event
Failure!

If you re-run the tests, changing out the version of Zone.js used, 0.8.12 is the last version of Zone.js that worked without error in Safari. Zone.js 0.8.13 introduced the error.

@markandrus markandrus changed the title Zone.js breaks addEventListener in Safari 11.0, 11.1 Zone.js 0.8.13+ breaks addEventListener in Safari 11.0, 11.1 Aug 21, 2017
@markandrus
Copy link
Contributor Author

Something must've changed here: v0.8.12...v0.8.13 — perhaps b3a76d3?

@markandrus
Copy link
Contributor Author

Interestingly, if I use the on-prefixed EventListener, things work OK in Safari (although they fail in Chrome and Firefox even without Zone.js for reasons I don't yet understand...).

Example without Zone.js

(async () => {
  console.log('Create RTCPeerConnection')
  const pc = new RTCPeerConnection()

  console.log('Add "track" event listener')
  let event
  pc.ontrack = _event => event = _event

  console.log('Dispatch "track" event')
  pc.dispatchEvent(new Event('track'))

  if (event) {
    console.log('Success!')
  } else {
    console.error('Failure!')
  }
})().catch(console.error)

Example with Zone.js

(async () => {
  const script = document.createElement('script')
  script.src = 'https://cdnjs.cloudflare.com/ajax/libs/zone.js/0.8.16/zone.js'
  document.body.appendChild(script)
  await new Promise(resolve => script.onload = resolve)

  console.log('Create RTCPeerConnection')
  const pc = new RTCPeerConnection()

  console.log('Add "track" event listener')
  let event
  pc.ontrack = _event => event = _event

  console.log('Dispatch "track" event')
  pc.dispatchEvent(new Event('track'))

  if (event) {
    console.log('Success!')
  } else {
    console.error('Failure!')
  }
})().catch(console.error)

Actual Results (Safari 11.0, Safari 11.1)

Create RTCPeerConnection
Add "track" event listener
Dispatch "track" event
Success!

@JiaLiPassion
Copy link
Collaborator

@markandrus , thank you for posting the issue, I will check it.

@JiaLiPassion
Copy link
Collaborator

JiaLiPassion commented Aug 21, 2017

@markandrus , I think it is a weird safari behavior. I create a html for easier test.

<html>
  <head>
    <!--<script src="https://cdnjs.cloudflare.com/ajax/libs/zone.js/0.8.16/zone.js"></script>-->
    <script>
      window.onload = function () {
          console.log('Create RTCPeerConnection')
          const pc = new RTCPeerConnection()

          console.log('Add "track" event listener')
          let event
          pc.addEventListener('track', function(_event) {
            console.log('this === pc', this === pc);
            console.log('track event', _event);
            event = _event
          });
          /*pc.ontrack = function(_event) {
            console.log('this === pc', this === pc);
            console.log('track event', _event);
            event = _event
          };*/

          console.log('Dispatch "track" event')
          pc.dispatchEvent(new Event('track'))

          if (event) {
            console.log('Success!')
          } else {
            console.error('Failure!')
       }
      }
    </script>
  </head>
</html>

without zone.js, in the handler of pc.addEventListener, this is not equal with pc, so the instance changed when event triggered. current version of zone.js rely on this, so it will not work.

And pc.ontrack work because current zone.js not patch ontrack, so it just work as native version.

I have created an issue in webkit bug tracker, https://bugs.webkit.org/show_bug.cgi?id=175802

@markandrus
Copy link
Contributor Author

@JiaLiPassion thank you for your analysis and for opening the WebKit bug. I spoke too soon and this is indeed an RTCPeerConnection-specific issue in WebKit.

current version of zone.js rely on this, so it will not work.

What path forward do you recommend? Although I could workaround this using ontrack and other on-prefixed properties in my code, should Zone.js workaround this, too? I suppose both 11.0 and 11.1 are not yet stable, but I don't know how quickly we'll get a bug fix...

@JiaLiPassion
Copy link
Collaborator

@markandrus , if this is a webkit bug, I think we can wait for the fix, and at that time, you can use ontrack , if after safari 11 released, and the bug still exists , zone.js may provide a walkaround patch for this case. (monkey-patch RTCPeerConnection to preserve this)

@markandrus
Copy link
Contributor Author

@JiaLiPassion OK, that seems reasonable. I've reviewed my code, though, and I see I'd have to change a non-trivial amount of code to use the on-prefixed versions instead of the addEventListener/removeEventListener combo I use today. Can you explain what the "monkey-patch RTCPeerConnection to preserve this" technique would look like? Is it something I could implement outside of Zone.js? Is there another way I can bypass Zone.js's behavior for users of my library?

@JiaLiPassion
Copy link
Collaborator

@markandrus , about monkey-patch, I just have some ideas. just create a wrapper to patch RTCPeerConnection, like WrappedRTCPeerConnection and reference a real RTCPeerConnection inside, so the wrapped one can keep this.

Is it something I could implement outside of Zone.js?
yes, but I am not sure all needed API is exposed or not, such as patchClass in zone.js.

Is there another way I can bypass Zone.js's behavior for users of my library?
yes, but it will be a little tricky and risky, because you have to check all APIs to make sure they are not using zone.js.

@treyrich
Copy link

@JiaLiPassion with Safari 11's release now looming, and there no one assigned to the webkit bug that you filed, are there any plans to implement a patch at the zone.js level? This is essentially forcing us to select between using zone.js in our project or WebRTC in Safari, since the two cannot be used together.

@markandrus
Copy link
Contributor Author

markandrus commented Sep 15, 2017

Here is one potential workaround, but it precludes using a mixture of ontrack and addEventListener('track', ...):

(async () => {
  console.log('Load Zone.js')
  const script = document.createElement('script')
  script.src = 'https://cdnjs.cloudflare.com/ajax/libs/zone.js/0.8.16/zone.js'
  document.body.appendChild(script)
  await new Promise(resolve => script.onload = resolve)

  // NOTE(mroberts): Workaround for https://github.com/angular/zone.js/issues/883
  const originalAddEventListener = RTCPeerConnection.prototype.addEventListener
  RTCPeerConnection.prototype.addEventListener = function addEventListener(event, listener) {
    if ('on' + event in this) {
      this._eventListeners = this._eventListeners || new Map()
      const listeners = this._eventListeners.get(event) || []
      listeners.push(listener)
      this._eventListeners.set(event, listeners)
      if (typeof this['on' + event] !== 'function') {
        var self = this
        this['on' + event] = function applyListeners() {
          const listeners = self._eventListeners.get(event) || []
          listeners.forEach(listener => {
            try {
              listener.apply(self, arguments)
            } catch (error) {
              console.error(error)
            }
          })
        }
      }
      return
    }
    originalAddEventListener.call(this, event, listener)
  }

  const originalRemoveEventListener = RTCPeerConnection.prototype.removeEventListener
  RTCPeerConnection.prototype.removeEventListener = function removeEventListener(event, listener) {
    if ('on' + event in this) {
      this._eventListeners = this._eventListeners || new Map()
      const listeners = this._eventListeners.get(event) || []
      this._eventListeners.set(event, listeners.filter(_listener => _listener !== listener))
      return
    }
    originalRemoveEventListener.call(this, event, listener)
  }

  console.log('Create RTCPeerConnection')
  const pc = new RTCPeerConnection()

  console.log('Add "track" event listener')
  let event
  pc.addEventListener('track', _event => event = _event)

  console.log('Dispatch "track" event')
  pc.dispatchEvent(new Event('track'))

  if (event) {
    console.log('Success!')
  } else {
    console.error('Failure!')
  }
})().catch(console.error)

@markandrus
Copy link
Contributor Author

@JiaLiPassion I may land this code in my library, but would you consider landing this workaround in Zone.js? I would prefer not to have to workaround this in my own library. Can you think of any other ways to implement this workaround?

@JiaLiPassion
Copy link
Collaborator

@markandrus , sure , I will post a PR to fix this one.
and you can load the patched js in the following order.

 <script src="node_modules/zone.js/dist/zone.js"></script>
 <script src="node_modules/zone.js/dist/webapis-rtc-peer-connection.js"></script>

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants