feat(core): Add an option '__Zone_symbol_prefix' to set symbol prefix used in Zone.__symbol__(). by mleibman · Pull Request #1219 · angular/zone.js

mleibman · 2019-04-07T17:39:47Z

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 to __zone_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__.

… used in Zone.__symbol__(). 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 idetifier 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 to `__zone_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<X>' 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 · 2019-04-08T01:37:01Z

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

mleibman · 2019-04-08T01:50:03Z

@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 · 2019-04-08T02:05:03Z

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

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

is that correct?

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

parent window set some options such as __zone_symbol__on_properties=['scroll']
parent window load zone.js as normal
iframe will clear all properties startWith __zone_symbol__
iframe will set some other options such as __zone_symbol__on_properties=['mousemove']
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 · 2019-04-08T02:20:45Z

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 · 2019-04-08T02:35:24Z

@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 · 2019-04-08T02:37:54Z

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

JiaLiPassion · 2019-04-08T02:43:48Z

@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 · 2019-04-08T02:44:38Z

@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 · 2019-04-08T02:46:01Z

@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 · 2019-04-08T02:51:04Z

@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 · 2019-04-08T13:11:56Z

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

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

You will see the error is symbolPrefix is not defined, because

zone.js/lib/zone.ts

Line 1300 in 82a68f3

return symbolPrefix + name;

, when __symbol__ is called, the symbolPrefix in

zone.js/lib/zone.ts

Line 1299 in 82a68f3

function __symbol__(name: string) {

is not initialized yet, so we may move the line 1299 to the top

zone.js/lib/zone.ts

Line 675 in 82a68f3

mark('Zone');

around here.

mleibman · 2019-04-08T13:33:29Z

@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 · 2019-04-08T14:21:11Z

@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 · 2019-04-08T16:42:16Z

@mleibman, thanks, the CI is happy now!

JiaLiPassion · 2019-04-10T00:00:17Z

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

mleibman · 2019-04-10T00:48:17Z

@JiaLiPassion intermittent CI error?

JiaLiPassion · 2019-04-10T02:47:03Z

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

mhevery · 2019-04-12T23:02:26Z

 ]);

 function nodeTest(specFiles, cb) {
+  require('./build/test/node-env-setup');


this seems unrelated. Can you explain the reason for this?

In order to test the changes and protect against regressions, the tests are run with the symbol prefix changed to __zone_symbol_test__. This adds the environment setup that sets the __Zone_symbol_prefix optin.

mhevery · 2019-04-12T23:02:31Z

 module.exports = function(config) {
  require('./karma-base.conf.js')(config);
  config.files.push('node_modules/core-js-bundle/index.js');
+  config.files.push('build/test/browser-env-setup.js');


this seems unrelated. Can you explain the reason for this?

In order to test the changes and protect against regressions, the tests are run with the symbol prefix changed to __zone_symbol_test__. This adds the environment setup that sets the __Zone_symbol_prefix optin.

mhevery · 2019-04-12T23:03:29Z


-const EVENT_NAME_SYMBOL_REGX = /^__zone_symbol__(\w+)(true|false)$/;
-const IMMEDIATE_PROPAGATION_SYMBOL = ('__zone_symbol__propagationStopped');
+const EVENT_NAME_SYMBOL_REGX = new RegExp('^' + ZONE_SYMBOL_PREFIX + '(\\w+)(true|false)$');


Suggested change

const EVENT_NAME_SYMBOL_REGX = new RegExp('^' + ZONE_SYMBOL_PREFIX + '(\\w+)(true|false)$');

const EVENT_NAME_SYMBOL_REGX = new RegExp('^' + zoneSymbol('') + '(\\w+)(true|false)$');

I guess this file is a bit inconsistent. In some places it uses the ZONE_SYMBOL_PREFIX and string concatenation, while in other places it calls zoneSymbo()... In this case, IMHO the intent is clearer with the ZONE_SYMBOL_PREFIX.

mhevery · 2019-04-12T23:14:20Z

+  // Initialize before it's accessed below.
+  // __Zone_symbol_prefix global can be used to override the default zone
+  // symbol prefix with a custom one if needed.
+  const symbolPrefix = global['__Zone_symbol_prefix'] || '__zone_symbol__';


Not a fan of this now becoming part of the public API which needs to be documented etc...

One way to solve this would be:

Suggested change

const symbolPrefix = global['__Zone_symbol_prefix'] || '__zone_symbol__';

const symbolPrefix = '__zone_symbol__' + new Date().getTime() + '__;

But my guess is that this would be very breaking because even if it is private API people are probably relying on it.

That's my concern as well. Having a unique symbol prefix is exactly what I need. The customizable symbol prefix is a way to do this w/o introducing a breaking change.

mhevery · 2019-05-14T02:30:26Z

This has been already merged into g3

googlebot added the cla: yes label Apr 7, 2019

Fix failing karma test.

cd65d3b

Remove trailing whitespace.

c3a5659

JiaLiPassion approved these changes Apr 8, 2019

View reviewed changes

Merge branch 'master' into symbol-prefix

417df0c

Bump up the file size limiit.

5ff516f

mhevery reviewed Apr 12, 2019

View reviewed changes

Merge branch 'master' into symbol-prefix

766c461

mhevery merged commit 22b9937 into angular:master May 14, 2019

	const EVENT_NAME_SYMBOL_REGX = new RegExp('^' + ZONE_SYMBOL_PREFIX + '(\\w+)(true\|false)$');
	const EVENT_NAME_SYMBOL_REGX = new RegExp('^' + zoneSymbol('') + '(\\w+)(true\|false)$');

	const symbolPrefix = global['__Zone_symbol_prefix'] \|\| '__zone_symbol__';
	const symbolPrefix = '__zone_symbol__' + new Date().getTime() + '__;

Conversation

mleibman commented Apr 7, 2019

Uh oh!

JiaLiPassion commented Apr 8, 2019

Uh oh!

mleibman commented Apr 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JiaLiPassion commented Apr 8, 2019

Uh oh!

mleibman commented Apr 8, 2019

Uh oh!

JiaLiPassion commented Apr 8, 2019

Uh oh!

mleibman commented Apr 8, 2019

Uh oh!

JiaLiPassion commented Apr 8, 2019

Uh oh!

mleibman commented Apr 8, 2019

Uh oh!

mleibman commented Apr 8, 2019

Uh oh!

JiaLiPassion commented Apr 8, 2019

Uh oh!

JiaLiPassion commented Apr 8, 2019

Uh oh!

mleibman commented Apr 8, 2019

Uh oh!

mleibman commented Apr 8, 2019

Uh oh!

JiaLiPassion commented Apr 8, 2019

Uh oh!

JiaLiPassion commented Apr 10, 2019

Uh oh!

mleibman commented Apr 10, 2019

Uh oh!

JiaLiPassion commented Apr 10, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mhevery commented May 14, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

mleibman commented Apr 8, 2019 •

edited

Loading