Silent try-catch blocks that covers more than a native code should log caught errors

[Piotrek Koszuliński]

​http://dev.ckeditor.com/ticket/9706#comment:4

Again we were close to miss some important error because it was thrown in silent try-catch (this time in selectionChange listener).

We should review all try-catches in code and:

1. if that's a try-catch that covers only one critical line of native code that can throw an error which we cannot handle differntly, then it's ok.
2. all other try-catches should be:
   1. removed (may be unsafe, because we'd have to test what'd be the impact).
   2. logged - we need logs (at least in DEV mode - we can introduce CKEDITOR.DEV flag with %REMOVE_LINE% annotation) of caught errors. Now it'd be unsafe to unwrap e.g. selectionChange listeners, so at least we should know about what was caught.

[Piotrek Koszuliński]

Another example of undiscovered issue - #9847.

[Piotrek Koszuliński]

Different case - #9848. Test is silently failing because of selectionChange being an errorProof event.

There are two solutions - we can completely get rid of errorProof flag and see what'll happen. Or error logger can forward errors when running tests (simplest solution - tests runner can overwrite error logger or set some flag).

[Piotrek Koszuliński]

We need some solution ASAP. I and Olek have already spent a lot more time on issues caused by errorProof than we would spend on fixing this issue.

Also, I have completely removed errorProof flag when working on #9764, because it makes testing and developing code extremely hard and risky. Every issue inside selectionChange is drowned by empty try-catch.

[Piotrek Koszuliński]

Pushed t/9786 with removed errorProof flag from selectionChange listener. This is not a final solution of course - I was just curious whether there are some hidden issues that will come out. Fortunately, tests pass. Also, I'm still unsure whether it is better to remove errorProof from selectionChange or to log those errors.

Anyway, some kind of logger is needed. In general, I'm thinking about more comprehensive tool:

// Allways will be logged in dev mode.
CKEDITOR.debug.error( err, 'Some message' ); %REMOVE_LINE%

// Will be logged only when CKEDITOR.debug.level is set to CKEDITOR.DEBUG_PEDANTIC.
CKEDITOR.debug.error( err, 'Some message', CKEDITOR.DEBUG_PEDANTIC ); %REMOVE_LINE%

// Will be logged even in release mode.
CKEDITOR.debug.log( 'Button "alignleft" does not exist', CKEDITOR.DEBUG_SHOUT ); 

// Will be logged in dev mode when value !== true.
CKEDITOR.debug.assert( foo === 'bar', 'Foo always equals bar' ); %REMOVE_LINE%

// Will be logged in dev mode and will break execution.
CKEDITOR.debug.assert( foo === 'bar', 'Foo always equals bar', CKEDITOR.DEBUG_CRITICAL ); %REMOVE_LINE%

These are very simple functions and we often miss them.

[Olek Nowodziński]

I'm totally for this feature. As soon as possible.

[Alfonso Martínez de Lizarrondo]

I guess that everyone that has tried to find a problem with something that doesn't work as expected in CKEditor has suffered this problem more than once.

Please, remove as many try-catch blocks as possible.

[Piotrek Koszuliński]

Actually, only selectionChange with errorProof covers more than few lines of non-native code (in fact - huge amount of it). Some of other try-catches are documented that they are needed because of browser issues. Few are not documented and empty and to these we should add logging, so we'll learn what they really mean. The most important to log are these which contains non-native code.

Anyway - our goal is to not break anything in production, but to have more control in dev mode and remove try-catches which contain non-native code, unless that's a correct usage of this block.