Fix #546, #554, #555, the error should keep prototype chain and can be called without new #559

JiaLiPassion · 2016-12-20T19:17:40Z

With the commit #547, the #546, error prototype issue was fixed, but a bigger issue occurs, that
Error object can be created with direct call.

let error = Error('message');
And the #547 PR will break it, cause a lot of errors, such as #554, #555

Sorry for the buggy commit in #547.

In this new PR, I checked the this exists or not and checked the this is global or not (non strict mode). So it should fix both #546 and #554, #555.

Please review, Thank you.

…otype chain and can be called without new

mhevery · 2016-12-21T17:30:38Z

lib/zone.ts


  // Copy the prototype so that instanceof operator works as expected
-  ZoneAwareError.prototype = NativeError.prototype;
+  ZoneAwareError.prototype = Object.create(NativeError.prototype);


Why is Object.create necessary here? What is wrong with having it the way it was before?

I just want to create a new object to let ZoneAwareError.prototype be different with NativeError's, so in the future, if someday modify ZoneAwareError's prototype will not impact NativeError's, for this issue #546, we don't need to use Object.create

mhevery · 2016-12-21T17:31:15Z

lib/zone.ts

    }
-    return error;
-  }
+    return (this && this !== global) ? this : error;


The fact that sometimes we return this and sometimes errror concerns me. We have two seperate code path and I fear that things will only work in one or the other code path.

Yes, it is a little risky code, I will modify it.

mhevery · 2016-12-21T17:32:19Z

lib/zone.ts

@@ -1318,13 +1318,24 @@ const Zone: ZoneType = (function(global: any) {
  function ZoneAwareError() {
    // Create an Error.
    let error: Error = NativeError.apply(this, arguments);
-
+    if (this && this !== global) {


I think a better strategy would be to always assume that we were called without the new operator, that way we don't have to decide.

Yes, I understand your concern, you are right. I will change it.

JiaLiPassion · 2016-12-22T09:01:15Z

@mhevery , Thank you for your review, I'll consider it again and reply to you soon.

JiaLiPassion · 2016-12-22T15:39:57Z

found another problem, In typescript 2.0.8, the class extends will be compiled into

var __extends = (this && this.__extends) || function (d, b) {
    for (var p in b) if (b.hasOwnProperty(p)) d[p] = b[p];
    function __() { this.constructor = d; }
    d.prototype = b === null ? Object.create(b) : (__.prototype = b.prototype, new __());
};

var MyError = (function (_super) {
    __extends(MyError, _super);
    function MyError() {
        _super.apply(this, arguments);
    }
    return MyError;
}(Error));

This is not correct, In typescript 2.1.4 , it will be compiled into

        var MyError = (function (_super) {
            __extends(MyError, _super);
            function MyError() {
                return _super.apply(this, arguments) || this;
            }
            return MyError;
        }(Error));

This is the correct one.
And it will behave just like the error case in #546.

…typescript to 2.1.4

JiaLiPassion · 2016-12-23T16:08:50Z

@mhevery , I have changed my PR, please review it is correct or not, Thanks a lot.
The package.json was updated because typescript 2.0.8 will treat 'class extends' in a wrong way.

mhevery · 2017-01-03T19:28:36Z

package.json

@@ -69,7 +69,7 @@
    "ts-loader": "^0.6.0",
    "tslint": "^3.15.1",
    "tslint-eslint-rules": "^3.1.0",
-    "typescript": "^2.0.2",


Let's not upgrade to TS 2.1.4 since we have to make sure that the generated .d.ts is compatible with angular which is on alder version on TS. The easiest is to wait until angular is ready.

Ok, I see, I will rollback the typescript version.

mhevery · 2017-01-03T19:31:55Z

lib/zone.ts

+    // and not global (non-strict mode call Error())
+    // and it is an error not an unrelated object
+    if (this && this !== global && this instanceof Error) {
+      error = setPrototypeOf(error, getPrototypeOf(this));


Calling setProtypeOf will make this object slow. Not sure if that is a problem since it is a Error object.

Thank you for pointing out this one, I didn't realize that setPrototypeOf will make javascript engine optimization become slow. how about change the code above to

// if this is valid, means not undefined // and not global (non-strict mode call Error()) // and it is an error not an unrelated object if (this && this !== global && this instanceof Error) { assignAll(this, error); return this; }

-Pro: don't use setPrototypeOf
-Con:if/else will return different object

I just make a new commit to describe it, please review.

mhevery · 2017-01-04T23:46:13Z

lib/zone.ts

@@ -1387,8 +1387,8 @@ const Zone: ZoneType = (function(global: any) {
    // if this is valid, means not undefined
    // and not global (non-strict mode call Error())
    // and it is an error not an unrelated object
-    if (this && this !== global && this instanceof Error
-      && Object.getPrototypeOf(this) !== NativeError.prototype && assignAll(this, error)) {
+    if (this && this !== global && this instanceof Error &&


I was thinking about this some more, and I think the best way to do this would be to place this guard at the top of the function

if (! this instanceof ZoneAwareError) { return ZoneAwareError.apply(Object.create(ZoneAwareError), arguments); }

This way we force the new and only have one code path. What do you think?

Thank you for the review, in my understanding, your code will guarantee that 'this' is a valid ZoneAwareError, and we can set all properties from error to this and then return this. Is my understanding correct?

I have modified the PR based on your idea, please review. Thank you.

mhevery · 2017-01-06T17:28:17Z

Thanks for all of your hard work.

Fix angular#546, angular#554, angular#555, the error should keep prot…

1815d81

…otype chain and can be called without new

googlebot added the cla: yes label Dec 20, 2016

JiaLiPassion added 5 commits December 21, 2016 04:28

modify test cases

b961f50

remove test-util detect browser function, modify error test case

4d212c8

remove test-util detect browser function, modify error test case

7c85fe6

modify error test case

c5866da

use Object.create to create prototype

e71c487

mhevery suggested changes Dec 21, 2016

View reviewed changes

JiaLiPassion added 5 commits December 23, 2016 22:39

use setPrototypeOf to fix angular#546, modify package.json to update …

4fee5d3

…typescript to 2.1.4

reformat code and add comment

5ddb051

reformat with gulp format

b195518

add blank line

15f3962

use Object.getPrototypeOf insteadof this.constructor.prototype

0429f9e

JiaLiPassion added 4 commits December 24, 2016 11:18

use Obejct.setPrototypeOf and add testcase

8989d83

reformat

bf13636

should always be error

f65dfff

reformat

0dbbf4d

mhevery suggested changes Jan 3, 2017

View reviewed changes

JiaLiPassion added 2 commits January 5, 2017 01:35

don't use setPrototypeOf

4a6a171

reformat

3d7c6ef

mhevery suggested changes Jan 4, 2017

View reviewed changes

JiaLiPassion added 3 commits January 5, 2017 09:33

add check code to prevent invalid this when create ZoneAwareError

d48acae

refactor

9ee3ba5

remove duplicate commemt

ae9301a

mhevery merged commit 82722c3 into angular:master Jan 6, 2017

JiaLiPassion deleted the issue-555 branch January 7, 2017 00:29

JiaLiPassion restored the issue-555 branch January 12, 2017 01:25

JiaLiPassion deleted the issue-555 branch January 12, 2017 01:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #546, #554, #555, the error should keep prototype chain and can be called without new #559

Fix #546, #554, #555, the error should keep prototype chain and can be called without new #559

JiaLiPassion commented Dec 20, 2016 •

edited

Loading

mhevery Dec 21, 2016

JiaLiPassion Dec 23, 2016

mhevery Dec 21, 2016

JiaLiPassion Dec 23, 2016

mhevery Dec 21, 2016

JiaLiPassion Dec 23, 2016

JiaLiPassion commented Dec 22, 2016

JiaLiPassion commented Dec 22, 2016 •

edited

Loading

JiaLiPassion commented Dec 23, 2016 •

edited

Loading

mhevery Jan 3, 2017

JiaLiPassion Jan 4, 2017

mhevery Jan 3, 2017

JiaLiPassion Jan 4, 2017 •

edited

Loading

mhevery Jan 4, 2017

JiaLiPassion Jan 5, 2017

mhevery commented Jan 6, 2017

Fix #546, #554, #555, the error should keep prototype chain and can be called without new #559

Fix #546, #554, #555, the error should keep prototype chain and can be called without new #559

Conversation

JiaLiPassion commented Dec 20, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JiaLiPassion commented Dec 22, 2016

JiaLiPassion commented Dec 22, 2016 • edited Loading

JiaLiPassion commented Dec 23, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JiaLiPassion Jan 4, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhevery commented Jan 6, 2017

JiaLiPassion commented Dec 20, 2016 •

edited

Loading

JiaLiPassion commented Dec 22, 2016 •

edited

Loading

JiaLiPassion commented Dec 23, 2016 •

edited

Loading

JiaLiPassion Jan 4, 2017 •

edited

Loading