feat(core): Add an option '__Zone_symbol_prefix' to set symbol prefix used in Zone.__symbol__().

[mleibman]

Add a global environment option __Zone_symbol_prefix to allow configuring the symbol prefix Zone.js uses to store its state data on objects.

This is needed in situations where multiple instances of Zone.js may 'touch' the same object or DOM node, accessing and clobbering the other instance's data stored there, and causing unpredictable behavior. Ultimately, it would be nice if Zone.js used proper ES6 symbols or some unique identifier as a prefix, but many products rely on being able to access the data stored by it.

This change adds the env option and cleans up all hard-coded references to 'zone_symbol_XXX' to go through `Zone.symbol(). In order to test the changes and protect against regressions, the tests are run with the symbol prefix changed tozone_symbol_test__`.

While the primary purpose of this change is to be able to isolate data stored on the objects & DOM nodes, some global environment options are also specified with the __zone_symbol__ prefix and not the usual __Zone_ one. The current API for providing env options isn't very consistent. For example, disabling 'on' property patching is done via __Zone_ignore_on_properties, w/o using the symbol prefix, while disabling event patching is done via __zone_symbol__UNPATCHED_EVENTS, using the symbol prefix. This change only affects the options that are prefixed with __zone_symbol__.

[JiaLiPassion]

@mleibman, thanks for the PR, could you explain the use case why we need to load multiple Zone.js instances?

[mleibman]

@JiaLiPassion I have an scenario where a separate Angular application with its own copy of Zone.js will be loaded in an IFrame on the same domain and will access the DOM elements (as well as document and window) in the parent window directly, potentially adding event listeners to them. When that happens, the two separate instances of Zone.js interfere with each other since they store the data using the same hard-coded symbol names.

[JiaLiPassion]

@mleibman, thanks for the explanation, so with this PR, the application will load like this,

1. parent window set some options such as __zone_symbol__on_properties=['scroll']
2. parent window load zone.js as normal
3. iframe will set a global __zone_symbol__prefix to some other value such as __zone_symbol2__.
4. iframe will set some other options such as __zone_symbol2__on_properties=['mousemove']
5. iframe will load another copy of zone.js.

is that correct?

If this is correct, we can do like this without have this prefix.

1. parent window set some options such as __zone_symbol__on_properties=['scroll']
2. parent window load zone.js as normal
3. iframe will clear all properties startWith __zone_symbol__
4. iframe will set some other options such as __zone_symbol__on_properties=['mousemove']
5. iframe will load another copy of zone.js.

But I totally agree the code of __zone_symbol__ usage is not consistent in current codebase. I just want to make sure I understand the use case correctly.

[mleibman]

That is one of the scenarios, namely specifying options for Zone.js prior to loading.

However the bigger issue is that Zone.js stores the state data like event handlers, original unpatched properties, configuration data, etc. on various objects at runtime. (And other code like Angular also reads from there for some optimizations around event listeners.)

Consider this scenario:

// loads zone.js
document.onclick = () => console.log('global click handler');

const iframe = document.body.appendChild(
  document.createElement('iframe'));

iframe.contentWindow.eval(`
  // load zone.js
  window.parent.document.onclick = () => console.log('iframe click handler');
`);

Clicking on the document will result in the iframe click handler executing twice, and the main click handler not executing at all, all because the two instances of Zone.js interfere with each other. With this change, the issue can be avoided by having the iframe code set a different __Zone_symbol_prefix global option before loading Zone.js.

[JiaLiPassion]

@mleibman, currently, if two instances of zone.js are going to be loaded in main window, the 2nd loading will be ignored, so only one instance of zone.js, should we also do this in iframe? So in one mainwindow + multiple iframes, only one copy of zone.js is allowed, although we need to do some other work to let Zone can be inherit from Main window to iframe contentWindow.

[mleibman]

@JiaLiPassion In my example above, the iframe loads its own copy of zone.js.

[JiaLiPassion]

@mleibman, got it, maybe zone.js can be smart enough to do this automatically when it is being loaded in iframe, but for now, this LGTM, I have restarted the Travis CI, not sure why it failed.

[mleibman]

@JiaLiPassion Thanks! It has done that several times... Do you think I'm missing something in the test configs? The local runs of yarn ci all passed.

[mleibman]

@JiaLiPassion As for Zone.js automatically doing it, as I mentioned, ultimately the symbol prefix should be unique (auto-generated? ES6 symbols?), but a lot of code assumes the hard-coded one, so this would be a breaking change.

[JiaLiPassion]

@mleibman, yeah, the Travis CI will test multiple browsers in different environments, so maybe some config is not there, I will check it.

As for Zone.js automatically doing it, as I mentioned, ultimately the symbol prefix should be unique (auto-generated? ES6 symbols?), but a lot of code assumes the hard-coded one, so this would be a breaking change.

You are right, Zone.__symbol__ is some kind of polyfill for the Symbol, every instance of zone.js should have a unique ZoneSymbol, like you said, we can use autogenerated, es6 symbol or some random number, but it will be a breaking change, so we can discuss that later.

[JiaLiPassion]

@mleibman, the CI failed because there is an error in zone-evergreen.js, you can reproduce in your local by run

1. gulp build.
2. karma start karma-evergreen-dist-jasmine.conf.js

You will see the error is symbolPrefix is not defined, because https://github.com/angular/zone.js/blob/82a68f3560224aa23445c728072a6a6d5a455c61/lib/zone.ts#L1300, when __symbol__ is called, the symbolPrefix in https://github.com/angular/zone.js/blob/82a68f3560224aa23445c728072a6a6d5a455c61/lib/zone.ts#L1299 is not initialized yet, so we may move the line 1299 to the top https://github.com/angular/zone.js/blob/82a68f3560224aa23445c728072a6a6d5a455c61/lib/zone.ts#L675 around here.

[mleibman]

@JiaLiPassion Thanks! Not sure why that didn't fail the other tests though...

I'm still getting a bunch of errors though:

Chrome 73.0.3683 (Mac OS X 10.14.3) ERROR { "message": "Uncaught ReferenceError: Zone is not defined\nat dist/zone-patch-canvas.js:21:1\n\nReferenceError: Zone is not defined\n at dist/zone-patch-canvas.js:21:1\n at dist/zone-patch-canvas.js:11:3\n at dist/zone-patch-canvas.js:12:2", "str": "Uncaught ReferenceError: Zone is not defined\nat dist/zone-patch-canvas.js:21:1\n\nReferenceError: Zone is not defined\n at dist/zone-patch-canvas.js:21:1\n at dist/zone-patch-canvas.js:11:3\n at dist/zone-patch-canvas.js:12:2" }

[mleibman]

@JiaLiPassion I guess that's just my environment. The remaining error on Travis is

Edge 15.15063.0 (Windows 10.0.0) fetch should work for text response FAILED Error: Timeout of 5000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves. Edge 15.15063.0 (Windows 10.0.0) fetch should work for text response FAILED Error: Timeout of 5000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves.

which could be a fluke.

[JiaLiPassion]

@mleibman, thanks, the CI is happy now!

[JiaLiPassion]

@mleibman, there are some conflicts due to the other merges, could you rebase your PR? thanks!

[mleibman]

@JiaLiPassion intermittent CI error?

[JiaLiPassion]

@mleibman, yeah, you need to update the file-size-limit.json too, update the number from 44000 to 44200

[mhevery]

This has been already merged into g3