Closed jwKimo closed 5 years ago
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).
:memo: 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.
ℹ️ Googlers: Go here for more info.
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).
:memo: 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.
ℹ️ Googlers: Go here for more info.
I signed it
@jwKimo, could you describe which browser
will have this issue? And what is the error of this issue? Thanks
@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
@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?
@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)
@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 availablecanPatchViaPropertyDescriptor
return false.And the load order is
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.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!
Ah yes should have elaborated on that part.
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#L25patchClass('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.
@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!
Thanks! I don't have write access so someone else would need to merge the pull request.
@jwKimo, yeah, I am asking @vikerman to check this PR, thanks!
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_symboladdEventListener and zone_symbolremoveEventListener 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_symboladdEventListener and zone_symbolremoveEventListener, causing the requests to fail.
Ensuring that patching of event target for legacy path takes place before legacy property descriptor patch eliminates this problem.