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

Ensure that EventTarget is patched prior to legacy property descriptor patch #1214

Merged
merged 4 commits into from
Apr 8, 2019

Conversation

jwKimo
Copy link
Contributor

@jwKimo jwKimo commented Apr 3, 2019

If the event that EventTarget is defined on a device that also does not support patch via property descriptor, the result is that the XMLHttpRequest class would be patched prior to the event target being patched (due to returning in event-target-legacy if EventTarget is defined). This results in the wrapper that replaces XMLHttpRequest missing the patched listener fields. E.g. __zone_symbol__addEventListener and __zone_symbol__removeEventListener
event-target-legacy.

The result is that for any attempted http requests, a type error occurs in scheduleTask in browse.ts -> patchXhr, due to attempting call to non-existent __zone_symbol__addEventListener and __zone_symbol__removeEventListener, causing the requests to fail.

Ensuring that patching of event target for legacy path takes place before legacy property descriptor patch eliminates this problem.

@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

1 similar comment
@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@jwKimo
Copy link
Contributor Author

jwKimo commented Apr 3, 2019

I signed it

@JiaLiPassion
Copy link
Collaborator

@jwKimo, could you describe which browser will have this issue? And what is the error of this issue? Thanks

@jwKimo
Copy link
Contributor Author

jwKimo commented Apr 3, 2019

@JiaLiPassion This was discovered on internal Google application running on an LG WebOS 3.0 SmartTV device. The error is that XMLHttpRequests fail to process due to a type error occurring due to attempting to make an call on undefined for "__zone_symbol__addEventListener" at https://github.com/angular/zone.js/blob/master/lib/browser/browser.ts#L166

This is due to patchClass of "XMLHttpRequest" (https://github.com/angular/zone.js/blob/master/lib/browser/property-descriptor-legacy.ts#L25) taking place before patching of event target(This was not the case in v0.8.21). Which does does take place in event-target-legacy as it returns if EventTarget is defined: https://github.com/angular/zone.js/blob/master/lib/browser/event-target-legacy.ts#L26

@googlebot
Copy link

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

1 similar comment
@googlebot
Copy link

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@googlebot googlebot added cla: yes and removed cla: no labels Apr 3, 2019
@JiaLiPassion
Copy link
Collaborator

@jwKimo, thanks, so this error occurs before https://github.com/angular/zone.js/blob/master/lib/browser/browser.ts#L78 is called, is that right?

@jwKimo
Copy link
Contributor Author

jwKimo commented Apr 3, 2019

@JiaLiPassion No. The error occurs when attempting to create and send an XMLHttpRequest. The send triggers scheduleTask(https://github.com/angular/zone.js/blob/master/lib/browser/browser.ts#L117) which will attempt to call undefined variable (https://github.com/angular/zone.js/blob/master/lib/browser/browser.ts#L166)

@JiaLiPassion
Copy link
Collaborator

@jwKimo, Sorry for the late reply, I know the error is because https://github.com/angular/zone.js/blob/master/lib/browser/browser.ts#L166.
what I don't understand is I don't think this issue has relationship with https://github.com/angular/zone.js/blob/master/lib/browser/property-descriptor-legacy.ts#L25

So the current situation is,

  • EventTarget is available
  • canPatchViaPropertyDescriptor return false.

And the load order is

  1. browser-legacy.ts -> event-target-legacy.ts (https://github.com/angular/zone.js/blob/master/lib/browser/event-target-legacy.ts#L26)
    So EventTarget is not patched at this time.
  2. browser.ts -> event-target.ts (https://github.com/angular/zone.js/blob/master/lib/browser/event-target.ts#L28),
    So EventTarget will be patched here.

And the error is XMLHttpRequest.prototype.addEventListener is not patched, so I am not sure the reason is https://github.com/angular/zone.js/blob/master/lib/browser/event-target.ts#L28.

After your fix, the problem is gone, is that right?

Sorry for the long text, I just want to make sure I understand the whole situation, Thank you!

@jwKimo
Copy link
Contributor Author

jwKimo commented Apr 3, 2019

Ah yes should have elaborated on that part.

  1. The patching of event target yields the fields __zone_symbol__addEventListener and __zone_symbol__removeEventListener
  2. It is expected that these fields exist on the XMLHttpRequest prototype when creating and sending requests: https://github.com/angular/zone.js/blob/master/lib/browser/browser.ts#L166
  3. property-descriptor-legacy makes a call to patchClass('XMLHttpRequest') which replaces the original XMLHttpRequest` with a wrapper with copied fields: https://github.com/angular/zone.js/blob/master/lib/browser/property-descriptor-legacy.ts#L25
  4. Thus if patchClass('XMLHttpRequest') runs before patching of event target, the patched event target fields mentioned in item 1 above, will be missing from the wrapper that replaces XMLHttpRequest.
  5. Result is that XMLHttpRequests will all fail to send due to type error as it will attempt to retrieve and then call undefined function on XMLHttpRequest, "__zone_symbol__addEventListener": https://github.com/angular/zone.js/blob/master/lib/browser/browser.ts#L103

@JiaLiPassion
Copy link
Collaborator

@jwKimo, thank you for the detail information, I understand the reason now, so the underlying reason is patchClass only copy the fields, not set the prototype. I think the current fix is ok, I will try to add the prototype copy logic later, thanks!

@jwKimo
Copy link
Contributor Author

jwKimo commented Apr 8, 2019

Thanks! I don't have write access so someone else would need to merge the pull request.

@JiaLiPassion
Copy link
Collaborator

@jwKimo, yeah, I am asking @vikerman to check this PR, thanks!

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

Successfully merging this pull request may close these issues.

4 participants