Suggestion to remove the silencing try-except handlers

[hvassbotn]

I see that you tried to address the problem with the exception handling, but I don't think the current solution is very good.

The change you added to OutputDebugMessage does not re-raise the original exception. And it is very confusing behavior of a innocent looking function name. I notice you do call it outside of exception handler, also.

Code: Select all

procedure OutputDebugMessage(const aMessage : string);
const
  DEFAULT_LINE = 1;
begin
  {$IFDEF DEBUG}
  OutputDebugString({$IFDEF DELPHI12_UP}PWideChar{$ELSE}PAnsiChar{$ENDIF}(aMessage + chr(0)));

  if (GlobalCEFApp <> nil) and GlobalCEFApp.LibLoaded then
    CefLog('CEF4Delphi', DEFAULT_LINE, CEF_LOG_SEVERITY_ERROR, aMessage);
  {$ENDIF}

  if (GlobalCEFApp <> nil) and GlobalCEFApp.ReRaiseExceptions then
    raise Exception.Create(aMessage);
end;

A better solution would be:

Code: Select all

  except
    on e : exception do
    begin
      OutputDebugMessage('TCefApplication.CreateInternalApp error: ' + e.Message);
      if ReRaiseExceptions then
        raise;
    end;
  end;

The best solution is to remove the try-excepts completely, or at least IFDEF them out:

Code: Select all

{$IFDEF NO_EXCEPTION_SILENCE}
  finally
  end;
{$ELSE}
  except
    on e : exception do
      OutputDebugMessage('TCefApplication.MultiExeProcessing error: ' + e.Message);
  end;
{$ENDIF}

[salvadordf]

I'll take a look at this over the weekend.

[salvadordf]

I need to have the try...except blocks for BriskBard and I'm not convinced that adding so many {$IFDEF} blocks would result in a clean code base.

I've created a new function to handle all exceptions, different than OutputDebugMessage.
I've tested it and it raises the exceptions if you set GlobalCEFApp.ReRaiseExceptions to True.

It will be included in the next version of CEF4Delphi.

[hvassbotn]

But why do you need the exception handlers for BriskBard?

What purpose do they serve?

Do you expect exceptions here in normal cases?

If so, it would be better to handle specific exception classes instead of the generic Exception class.

The problem with this, is that you're hiding programming bugs - like access violations etc.

Much better to use an exception logging system like madExcept or similar.

Refrences:

https://www.thoughtco.com/handling-exce ... ng-1058215

Note that the else part would grab all (other) exceptions, including those you know nothing about. In general, your code should handle only exceptions you actually know how to handle and expect to be thrown.

Also, you should never "eat" an exception

https://pascal.today/2016/12/15/try-or-not-to-try/

Try to never suppress the exceptions with the empty except end clause. This is just bad behavior!

[salvadordf]

This is getting a philosophical discussion about coding styles

I prefer to show customized error messages for some applications instead of raw error messages.

Anyway, please try the new CEF4Delphi version and set GlobalCEFApp.ReRaiseExceptions := True to get all missing exceptions.

If you still need to remove a given try..except block just let me know what function/procedure and I'll set it as virtual.

Notice that I moved all private declarations to the protected section because I assumed some people would like to have full access in their inherited classes.

[hvassbotn]

> I prefer to show customized error messages for some applications instead of raw error messages.

Maybe. But then that should be done in the application level, not in a general component meant to be reused in many different kinds of applications.

[hvassbotn]

I'm looking into the latest version of CEF4Delphi now. I'm still not happy with the exception handling.

It's now:

Code: Select all

procedure CustomExceptionHandler(const aMessage : string);
begin
  OutputDebugMessage(aMessage);

  if (GlobalCEFApp <> nil) and GlobalCEFApp.ReRaiseExceptions then
    raise Exception.Create(aMessage);
end;

  try
    // Do actual work
  except
    on e : exception do
      CustomExceptionHandler('TCefApplication.CreateInternalApp error: ' + e.Message);
  end;

Problems with this:

• Original exception type and any extra Exception property values is lost
• Original stack trace is lost
• OutputDebugMessage is always called for exceptions

The proper way of re-raising exceptions can be read about here:

http://docwiki.embarcadero.com/RADStudi ... Exceptions

http://stackoverflow.com/questions/2923 ... logging-it

If you still insist on having this unnecessary exception handling in all applications that uses the library, at least do it properly.

My suggestion is:

Code: Select all

function CustomHandleException(const Source: string; E: Exception): boolean;
begin
  if Assigned(GlobalCEFApp) and GlobalCEFApp.LogExceptionsToOutputDebugMessage then
    OutputDebugMessage(Source + E.Message);
  Result := not Assigned(GlobalCEFApp) or GlobalCEFApp.ReRaiseExceptions;
end;

  try
    // Do actual work
  except
    on E: Exception do
      if not CustomHandleException('TCefApplication.CreateInternalApp error: ', E) then
        raise;
  end;

This requires a new boolean LogExceptionsToOutputDebugMessage property on the TCefApplication class.

/H

[salvadordf]

I'll take a look as soon as I have time.