Log exceptions caught in polymer expressions

Log exceptions caught in polymer expressions
BUG=https://code.google.com/p/dart/issues/detail?id=12130
R=justinfagnani@google.com

Committed: https://code.google.com/p/dart/source/detail?r=28805

[Siggi Cherem (dart-lang), 2013-10-08 23:41:08]

I'm new to this code, so I might not be fixing things the right way

https://codereview.chromium.org/26619002/diff/1/pkg/polymer_expressions/lib/p...
File pkg/polymer_expressions/lib/polymer_expressions.dart (right):

https://codereview.chromium.org/26619002/diff/1/pkg/polymer_expressions/lib/p...
pkg/polymer_expressions/lib/polymer_expressions.dart:103: } catch (e) {
should I merge this and the catch above together? I saw in many places we only
check for EvalException, but I wonder if we want to consistently catch
everything or only catch EvalExceptions?

[Jennifer Messerly, 2013-10-15 22:47:14]

looks good if catch is changed back to only catch EvalException :)

https://codereview.chromium.org/26619002/diff/1/pkg/polymer_expressions/lib/p...
File pkg/polymer_expressions/lib/polymer_expressions.dart (right):

https://codereview.chromium.org/26619002/diff/1/pkg/polymer_expressions/lib/p...
pkg/polymer_expressions/lib/polymer_expressions.dart:103: } catch (e) {
On 2013/10/08 23:41:08, Siggi Cherem (dart-lang) wrote:
> should I merge this and the catch above together? I saw in many places we only
> check for EvalException, but I wonder if we want to consistently catch
> everything or only catch EvalExceptions?

IMO: only EvalException in both places.

[justinfagnani, 2013-10-16 00:37:07]

https://codereview.chromium.org/26619002/diff/1/pkg/polymer_expressions/lib/p...
File pkg/polymer_expressions/lib/polymer_expressions.dart (right):

https://codereview.chromium.org/26619002/diff/1/pkg/polymer_expressions/lib/p...
pkg/polymer_expressions/lib/polymer_expressions.dart:103: } catch (e) {
On 2013/10/15 22:47:15, John Messerly wrote:
> On 2013/10/08 23:41:08, Siggi Cherem (dart-lang) wrote:
> > should I merge this and the catch above together? I saw in many places we
only
> > check for EvalException, but I wonder if we want to consistently catch
> > everything or only catch EvalExceptions?
> 
> IMO: only EvalException in both places.

same, other exceptions are real application exceptions and not just nulls in the
binding path.

[Siggi Cherem (dart-lang), 2013-10-16 00:45:53]

yeah - sorry I had put this aside for a bit after talking with Justin last week.

I initially wanted to address the fact that some exceptions were being swallowed
(dartbug.com/12130), only using EvalException is not fixing the bug reported
there, so I think there are other places where this is happening, so I need to
look more closely into it.

But I agree, it seems that here we should only be catching the EvalExceptions

[Jennifer Messerly, 2013-10-16 02:46:33]

On 2013/10/16 00:45:53, Siggi Cherem (dart-lang) wrote:
> yeah - sorry I had put this aside for a bit after talking with Justin last
week.
> 
> I initially wanted to address the fact that some exceptions were being
swallowed
> (dartbug.com/12130), only using EvalException is not fixing the bug reported
> there, so I think there are other places where this is happening, so I need to
> look more closely into it.
> 
> But I agree, it seems that here we should only be catching the EvalExceptions

I wonder if the bug is in path_observer.dart? specifically _getObjectProperty.
It should likely be catching only noSuchMethod.

[justinfagnani, 2013-10-16 02:55:17]

On 2013/10/16 02:46:33, John Messerly wrote:
> On 2013/10/16 00:45:53, Siggi Cherem (dart-lang) wrote:
> > yeah - sorry I had put this aside for a bit after talking with Justin last
> week.
> > 
> > I initially wanted to address the fact that some exceptions were being
> swallowed
> > (dartbug.com/12130), only using EvalException is not fixing the bug reported
> > there, so I think there are other places where this is happening, so I need
to
> > look more closely into it.
> > 
> > But I agree, it seems that here we should only be catching the
EvalExceptions
> 
> I wonder if the bug is in path_observer.dart? specifically _getObjectProperty.
> It should likely be catching only noSuchMethod.

Catching noSuchMethod can be nasty too, since you can't tell if it's a "local"
NSM (originating on the object that you invoked on), or an uncaught NSM from
some other code that indicates a non-binding error. dartbug.com/6252 is the
relevant bug for adding receiver to NSME

[Jennifer Messerly, 2013-10-16 03:18:29]

On 2013/10/16 02:55:17, justinfagnani wrote:
> On 2013/10/16 02:46:33, John Messerly wrote:
> > On 2013/10/16 00:45:53, Siggi Cherem (dart-lang) wrote:
> > > yeah - sorry I had put this aside for a bit after talking with Justin last
> > week.
> > > 
> > > I initially wanted to address the fact that some exceptions were being
> > swallowed
> > > (dartbug.com/12130), only using EvalException is not fixing the bug
reported
> > > there, so I think there are other places where this is happening, so I
need
> to
> > > look more closely into it.
> > > 
> > > But I agree, it seems that here we should only be catching the
> EvalExceptions
> > 
> > I wonder if the bug is in path_observer.dart? specifically
_getObjectProperty.
> > It should likely be catching only noSuchMethod.
> 
> Catching noSuchMethod can be nasty too, since you can't tell if it's a "local"
> NSM (originating on the object that you invoked on), or an uncaught NSM from
> some other code that indicates a non-binding error. dartbug.com/6252 is the
> relevant bug for adding receiver to NSME

yeah, in the meantime maybe PathObserver should either:
A. only call getField if it can find the field/getter, or 
B. same as A, but also call getField if the target implements noSuchMethod (and
it isn't Object.noSuchMethod)

[Jennifer Messerly, 2013-10-16 03:36:22]

On 2013/10/16 03:18:29, John Messerly wrote:
> On 2013/10/16 02:55:17, justinfagnani wrote:
> > On 2013/10/16 02:46:33, John Messerly wrote:
> > > On 2013/10/16 00:45:53, Siggi Cherem (dart-lang) wrote:
> > > > yeah - sorry I had put this aside for a bit after talking with Justin
last
> > > week.
> > > > 
> > > > I initially wanted to address the fact that some exceptions were being
> > > swallowed
> > > > (dartbug.com/12130), only using EvalException is not fixing the bug
> reported
> > > > there, so I think there are other places where this is happening, so I
> need
> > to
> > > > look more closely into it.
> > > > 
> > > > But I agree, it seems that here we should only be catching the
> > EvalExceptions
> > > 
> > > I wonder if the bug is in path_observer.dart? specifically
> _getObjectProperty.
> > > It should likely be catching only noSuchMethod.
> > 
> > Catching noSuchMethod can be nasty too, since you can't tell if it's a
"local"
> > NSM (originating on the object that you invoked on), or an uncaught NSM from
> > some other code that indicates a non-binding error. dartbug.com/6252 is the
> > relevant bug for adding receiver to NSME
> 
> yeah, in the meantime maybe PathObserver should either:
> A. only call getField if it can find the field/getter, or 
> B. same as A, but also call getField if the target implements noSuchMethod
(and
> it isn't Object.noSuchMethod)

fyi -- I'm working on a change to fix this (along with many other issues in
pkg:observe)

[Siggi Cherem (dart-lang), 2013-10-17 17:06:13]

OK, I trim this down to just catch EvalException

PTAL

[Siggi Cherem (dart-lang), 2013-10-17 17:06:29]

On 2013/10/17 17:06:13, Siggi Cherem (dart-lang) wrote:
> OK, I trim this down to just catch EvalException
> 
> PTAL

err: *trimmed

[justinfagnani, 2013-10-17 17:11:46]

lgtm

[Siggi Cherem (dart-lang), 2013-10-17 17:18:01]

Committed patchset #3 manually as r28805 (presubmit successful).