fix(change_detection): _throwError should not mask the original exception

vsavkin · vsavkin · commit cec4b36d9bd3 · 2015-09-30T18:18:28.000-07:00
diff --git a/modules/angular2/src/core/change_detection/abstract_change_detector.ts b/modules/angular2/src/core/change_detection/abstract_change_detector.ts
@@ -287,11 +287,19 @@ export class AbstractChangeDetector<T> implements ChangeDetector {
   }
 
   private _throwError(exception: any, stack: any): void {
-    var c = this.dispatcher.getDebugContext(this._currentBinding().elementIndex, null);
-    var context = isPresent(c) ? new _Context(c.element, c.componentElement, c.context, c.locals,
-                                              c.injector, this._currentBinding().debug) :
-                                 null;
-    throw new ChangeDetectionError(this._currentBinding().debug, exception, stack, context);
+    var error;
+    try {
+      var c = this.dispatcher.getDebugContext(this._currentBinding().elementIndex, null);
+      var context = isPresent(c) ? new _Context(c.element, c.componentElement, c.context, c.locals,
+                                                c.injector, this._currentBinding().debug) :
+                                   null;
+      error = new ChangeDetectionError(this._currentBinding().debug, exception, stack, context);
+    } catch (e) {
+      // if an error happens during getting the debug context, we throw a ChangeDetectionError
+      // without the extra information.
+      error = new ChangeDetectionError(null, exception, stack, null);
+    }
+    throw error;
   }
 
   throwOnChangeError(oldValue: any, newValue: any): void {
diff --git a/modules/angular2/src/core/compiler/view.ts b/modules/angular2/src/core/compiler/view.ts
@@ -254,7 +254,7 @@ export class AppView implements ChangeDispatcher, RenderEventDispatcher {
 
     } catch (e) {
       // TODO: vsavkin log the exception once we have a good way to log errors and warnings
-      // if an error happens during getting the debug context, we return an empty map.
+      // if an error happens during getting the debug context, we return null.
       return null;
     }
   }
diff --git a/modules/angular2/test/core/change_detection/change_detector_spec.ts b/modules/angular2/test/core/change_detection/change_detector_spec.ts
@@ -11,6 +11,8 @@ import {
   fakeAsync
 } from 'angular2/test_lib';
 
+import {SpyChangeDispatcher} from '../spies';
+
 import {
   CONST_EXPR,
   isPresent,
@@ -92,8 +94,8 @@ export function main() {
 
 
       function _createChangeDetector(expression: string, context = _DEFAULT_CONTEXT,
-                                     registry = null) {
-        var dispatcher = new TestDispatcher();
+                                     registry = null, dispatcher = null) {
+        if (isBlank(dispatcher)) dispatcher = new TestDispatcher();
         var testDef = getDefinition(expression);
         var protoCd = _getProtoChangeDetector(testDef.cdDef);
         var cd = protoCd.instantiate(dispatcher);
@@ -797,6 +799,22 @@ export function main() {
             expect(e.location).toEqual('invalidFn(1) in location');
           }
         });
+
+        it('should handle unexpected errors in the event handler itself', () => {
+          var throwingDispatcher = new SpyChangeDispatcher();
+          throwingDispatcher.spy("getDebugContext")
+              .andCallFake((_, __) => { throw new BaseException('boom'); });
+
+          var val =
+              _createChangeDetector('invalidFn(1)', _DEFAULT_CONTEXT, null, throwingDispatcher);
+          try {
+            val.changeDetector.detectChanges();
+            throw new BaseException('fail');
+          } catch (e) {
+            expect(e).toBeAnInstanceOf(ChangeDetectionError);
+            expect(e.location).toEqual(null);
+          }
+        });
       });
 
       describe('Locals', () => {
@@ -1398,5 +1416,5 @@ class TestDispatcher implements ChangeDispatcher {
 }
 
 class _ChangeDetectorAndDispatcher {
-  constructor(public changeDetector: any, public dispatcher: TestDispatcher) {}
+  constructor(public changeDetector: any, public dispatcher: any) {}
 }
diff --git a/modules/angular2/test/core/spies.dart b/modules/angular2/test/core/spies.dart
@@ -35,6 +35,11 @@ class SpyProtoChangeDetector extends SpyObject implements ProtoChangeDetector {
   noSuchMethod(m) => super.noSuchMethod(m);
 }
 
+@proxy
+class SpyChangeDispatcher extends SpyObject implements ChangeDispatcher {
+  noSuchMethod(m) => super.noSuchMethod(m);
+}
+
 @proxy
 class SpyIterableDifferFactory extends SpyObject
     implements IterableDifferFactory {
diff --git a/modules/angular2/test/core/spies.ts b/modules/angular2/test/core/spies.ts
@@ -40,6 +40,8 @@ export class SpyProtoChangeDetector extends SpyObject {
   constructor() { super(DynamicChangeDetector); }
 }
 
+export class SpyChangeDispatcher extends SpyObject {}
+
 export class SpyIterableDifferFactory extends SpyObject {}
 
 export class SpyRenderCompiler extends SpyObject {

Original file line number	Diff line number	Diff line change
`@@ -254,7 +254,7 @@ export class AppView implements ChangeDispatcher, RenderEventDispatcher {`
`254`	`254`
`255`	`255`	`} catch (e) {`
`256`	`256`	`// TODO: vsavkin log the exception once we have a good way to log errors and warnings`
`257`		`- // if an error happens during getting the debug context, we return an empty map.`
	`257`	`+ // if an error happens during getting the debug context, we return null.`
`258`	`258`	`return null;`
`259`	`259`	`}`
`260`	`260`	`}`
Original file line number	Diff line number	Diff line change
`@@ -40,6 +40,8 @@ export class SpyProtoChangeDetector extends SpyObject {`
`40`	`40`	`constructor() { super(DynamicChangeDetector); }`
`41`	`41`	`}`
`42`	`42`
	`43`	`+export class SpyChangeDispatcher extends SpyObject {}`
	`44`	`+`
`43`	`45`	`export class SpyIterableDifferFactory extends SpyObject {}`
`44`	`46`
`45`	`47`	`export class SpyRenderCompiler extends SpyObject {`