_drupal_exception_handler() attempts to log and render uncaught exceptions.

If another exception is thrown during this process, then the following error is printed:
Fatal error: Exception thrown without a stack frame in Unknown on line 0

This doesn't provide much information about what happened.

The attached patch outputs details of both the original exception and the additional exception, without risking another exception being thrown.

This is analogous to how Apache handles errors that are generated when generating an error page.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

catch’s picture

Priority: Normal » Critical

Subscribing. This makes MySQL fatal errors impossible to debug, so bumping to critical.

Berdir’s picture

See also #710142: Handle exceptions in shutdown functions, maybe we should report these exceptions in a similiar way?

That could mean:
- Use _drupal_decode_exception as in the other issue to display the exception instead of simply printing them
- Check the error_level in _drupal_shutdown_function() as you did here

jbrown’s picture

Okay - here's a patch, but I have a better idea.

jbrown’s picture

Actually, my other idea was stupid. Please review the patch.

Anonymous’s picture

just applied this patch, and it printed the following, which is an improvement, but seems there is an underlying issue in the db layer:

Additional uncaught exception thrown while handling exception.

Original
PDOException: SQLSTATE[42S02]: Base table or view not found: 1146 Table 'glw.term_node' doesn't exist: SELECT tn.nid FROM term_node tn INNER JOIN {glwnews_issue_term} it ON it.tid = tn.tid WHERE it.issue_id = :issue_id; Array ( [:issue_id] => ) in glwnews_get_issue() (line 157 of /var/www/glw/drupal/sites/all/modules/glwnews/glwnews.module).

Additional
PDOException: SQLSTATE[42S02]: Base table or view not found: 1146 Table 'glw.term_node' doesn't exist: SELECT tn.nid FROM term_node tn INNER JOIN {glwnews_issue_term} it ON it.tid = tn.tid WHERE it.issue_id = :issue_id; Array ( [:issue_id] => ) in glwnews_get_issue() (line 157 of /var/www/glw/drupal/sites/all/modules/glwnews/glwnews.module).
Anonymous’s picture

actually, there isn't, it was an error thrown in theme('maintenance_page', ...) because of some of my yet-to-be-ported-to-D7 code.

catch’s picture

Status: Needs review » Reviewed & tested by the community

Fixes the issue, the way it does it is pretty self-documenting. RTBC. Would be nice not to see any more "Exception thrown without a stack frame in line 0".

Anonymous’s picture

just a note to say this is making it harder to fix update bugs: #757214: D6->D7 update.php failure: Fatal error: Class 'MergeQuery_mysql'.

Dries’s picture

Could we get a quick reroll minus the whitespace?

jbrown’s picture

jbrown’s picture

sorry - let me try that again

jbrown’s picture

jbrown’s picture

Status: Reviewed & tested by the community » Needs review
rfay’s picture

Issue tags: +D7 upgrade path
bcn’s picture

FileSize
4.21 KB

One last bit of whitespace removed, otherwise it's the same patch.

Anonymous’s picture

Status: Needs review » Reviewed & tested by the community

looks good to me.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Since Dries just asked for a no-whitespace re-roll, and this fixes the most flamingly critical bug I can think of, committed to HEAD. Great sleuthing work, jbrown!

webchick’s picture

Issue tags: +Needs tests

Oh, though one question. Is it possible to write tests for this at all so we don't EVER have this problem again?

jbrown’s picture

Great!

I'll look into testing this.

tobiasb’s picture

Status: Fixed » Needs work

I get this message in all tests now, when the default language isn't english. (here german)

Ein AJAX-HTTP-Fehler ist aufgetreten HTTP-Rückgabe-Code: 500 Im Folgenden finden Sie Debugging-Informationen. Pfad: /drupal7/batch?render=overlay&id=23&op=do StatusText: Service unavailable (with message) ResponseText: Additional uncaught exception thrown while handling exception.OriginalPDOException: SQLSTATE[42S02]: Base table or view not found: 1146 Table 'drupal7.simpletest253844locales_source' doesn't exist: SELECT s.source, s.context, t.translation, t.language FROM {locales_source} s LEFT JOIN {locales_target} t ON s.lid = t.lid AND t.language = :language WHERE s.textgroup = 'default' AND s.version = :version AND LENGTH(s.source) de [:version] => 7.0-dev ) in locale() (line 656 of F:\xampplite\htdocs\drupal7\modules\locale\locale.module).AdditionalPDOException: SQLSTATE[42S02]: Base table or view not found: 1146 Table 'drupal7.simpletest253844locales_source' doesn't exist: SELECT s.lid, t.translation, s.version FROM {locales_source} s LEFT JOIN {locales_target} t ON s.lid = t.lid AND t.language = :language WHERE s.source = :source AND s.context = :context AND s.textgroup = 'default'; Array ( [:language] => de [:source] => %type: %message in %function (line %line of %file). [:context] => ) in locale() (line 674 of F:\xampplite\htdocs\drupal7\modules\locale\locale.module). Fatal error: Exception thrown without a stack frame in Unknown on line 0

jbrown’s picture

Your checkout of locale.inc is not HEAD as the line numbers are wrong - I presume you are doing development in locale.inc .

The double exception reporting is working - its reporting an error. You seem to have some other bug.

I don't know where the final 'Fatal error: Exception thrown without a stack frame in Unknown on line 0' is coming from though. Its probably something to do with it occurring in simpletest.

tobiasb’s picture

FileSize
766 bytes

Same with a clean HEAD and only enabled german as default language.

An AJAX HTTP error occurred. HTTP Result Code: 500 Debugging information follows. Path: /drupal7/batch?render=overlay&id=2&op=do StatusText: Service unavailable (with message) ResponseText: Additional uncaught exception thrown while handling exception.OriginalPDOException: SQLSTATE[42S02]: Base table or view not found: 1146 Table 'drupal7.simpletest748206locales_source' doesn't exist: SELECT s.source, s.context, t.translation, t.language FROM {locales_source} s LEFT JOIN {locales_target} t ON s.lid = t.lid AND t.language = :language WHERE s.textgroup = 'default' AND s.version = :version AND LENGTH(s.source) de [:version] => 7.0-dev ) in locale() (line 656 of F:\xampplite\htdocs\drupal7\modules\locale\locale.module).AdditionalPDOException: SQLSTATE[42S02]: Base table or view not found: 1146 Table 'drupal7.simpletest748206locales_source' doesn't exist: SELECT s.lid, t.translation, s.version FROM {locales_source} s LEFT JOIN {locales_target} t ON s.lid = t.lid AND t.language = :language WHERE s.source = :source AND s.context = :context AND s.textgroup = 'default'; Array ( [:language] => de [:source] => %type: %message in %function (line %line of %file). [:context] => ) in locale() (line 674 of F:\xampplite\htdocs\drupal7\modules\locale\locale.module). Fatal error: Exception thrown without a stack frame in Unknown on line 0

Without t() , see drupal_742246.txt, I get this.

An AJAX HTTP error occurred. HTTP Result Code: 500 Debugging information follows. Path: /drupal7/batch?render=overlay&id=3&op=do StatusText: Service unavailable (with message) ResponseText: When called from JavaScript, simply output the error message. Fatal error: Exception thrown without a stack frame in Unknown on line 0

jbrown’s picture

I've replicated this. Looking into it right now.

Strangely, I can't get my interface text to translate into German.

jbrown’s picture

Status: Needs work » Fixed

This is not related to this issue.

Without the patch in this issue applied the following error still occurs:

An AJAX HTTP error occurred. HTTP Result Code: 500 Debugging information follows. Path: /drupal-7.x-dev/batch?id=8&op=do StatusText: Service unavailable (with message) ResponseText: Fatal error: Exception thrown without a stack frame in Unknown on line 0

The problem is that testing does not work if the locale module is enabled on the host installation.

jbrown’s picture

jbrown’s picture

Title: Uncaught exceptions thrown in the exception handler prevent error reporting » Handle uncaught exceptions in _drupal_exception_handler(), _drupal_shutdown_function() and _drupal_session_write()
Status: Fixed » Needs review
FileSize
3.65 KB

The "Fatal error: Exception thrown without a stack frame in Unknown on line 0" in #22 is caused by _drupal_session_write().

We need to be catching exceptions in _drupal_session_write() in addition to _drupal_exception_handler() and _drupal_shutdown_function() as the exception handler is not and cannot be active in any of these functions.

This will assist with debugging of #276153: Testing does not work at all when locale of host session is not English

jbrown’s picture

Title: Handle uncaught exceptions in _drupal_exception_handler(), _drupal_shutdown_function() and _drupal_session_write() » Handle uncaught exceptions in _drupal_session_write() like _drupal_exception_handler() and _drupal_shutdown_function()
Priority: Critical » Normal

Demoting this to normal to reduce the critical issue count.

Uncaught exceptions in _drupal_session_write() seem to be very rare.

rfay’s picture

I officially disagree with the idea of "demoting to reduce the critical issue count".

Please demote if it does not qualify as critical, but not to make our numbers look better. Maybe that's what you're saying in your second sentence. But please, if this is critical (and I suspect it may be) leave it as critical.

http://drupal.org/node/314328:

Critical: This status is used to designate broken functionality that makes the project unusable.

jbrown’s picture

I think this issue should be normal because it really wouldn't matter very much if D7 was released without this issue resolved.

chx’s picture

Title: Handle uncaught exceptions in _drupal_session_write() like _drupal_exception_handler() and _drupal_shutdown_function() » Handle uncaught exceptions
Priority: Normal » Critical

And, of course, that we need ungodly ugly code ($error_level == ERROR_REPORTING_DISPLAY_ALL || (defined('MAINTENANCE_MODE') && MAINTENANCE_MODE == 'update')) { such reflects my thoughts from #673050: Database bugs became fatals: we added exceptions and we have no strategy to really handle them. This issue IS critical and NOT just session.inc. Recently matt2000 raised the question in IRC: what happens if an exception is thrown in .install? Now that we will add fields in .install that will happen often and then you get a fatal error and then you have a broken install. And then what else do we have that we have not thought of?

Once again: catch database exceptions where PDO would throw them, remove every other exception throwing and switch core to non fatal static error collections before it's too late.

chx’s picture

Or if someone has a better overarching strategy then come ahead with it. "exceptions are awesome" is not a strategy to handle the sudden influx of fatals. In Drupal 6 a bad query was not a fatal it was a warning.

dhthwy’s picture

+1 chx in that I think it warrants further attention. IMHO whether a database query should be fatal depends on how critical that particular query is. *I* think it should be up to the caller to decide.

Berdir’s picture

I don't see how that ugly code (no discussion there, it *is* ugly) is related to using exceptions or not. It is just necessary to decide if we want to print an error message or not. We would need something similar if we want to properly handle errors in that function without exceptions there.

Re catching PDO exceptions and convert them to errors: This would imho totally break transaction handling as we have it now (Rollback if an exception happens during node_save() and other functions that use it), just to name an example. Also, queries are IMHO not supposed to fail, and if you really expect that they do, you can always catch the exception and handle it. I fail to see how it is bad that an broken query is fatal?

Oh, and I am very aware that our current situation is not perfect, there *are* issues with exceptions, both in PHP (that is why this issue exists) and in Drupal. But I don't think "remove all of them" is a valid solution, not at this point.

@32: And how is that not possible right now? The default simply changed from a warning to a fatal error, you can always catch the exception and handle it in any way you want.

chx’s picture

Status: Needs review » Reviewed & tested by the community

/me shrugs. Be it. I have already won't fixed my other issue and won't repeat all that I said there. I can only hope that someone in Drupal 8 will create a proper solution for this.

jbrown’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
7.62 KB

Okay - I made a pretty little function called error_displayable() .

chx’s picture

Status: Needs review » Needs work

error_displayable surely can be written without a switch.

jbrown’s picture

Status: Needs work » Needs review
FileSize
7.62 KB

Like this?

chx’s picture

Almost. I do not think it worths saving a variable_get in a rarely called function like that. And so then what about

function error_displayable($error = NULL) {
  $error_level = variable_get('error_level', ERROR_REPORTING_DISPLAY_ALL);
  return 
    (defined('MAINTENANCE_MODE') && MAINTENANCE_MODE == 'update') ||
    ($error_level == ERROR_REPORTING_DISPLAY_ALL) || 
    ($error_level == ERROR_REPORTING_DISPLAY_SOME && isset($error) && $error['%type'] != 'Notice' && $error['%type'] != 'Strict warning'));
}
jbrown’s picture

chx’s picture

Status: Needs review » Reviewed & tested by the community

I already RTBC'd this when it was uglier...

aspilicious’s picture

Please read doc style document.

Newline between @param and @return block.
Function doc has to start with 3th person verb.

Still rtbc

Berdir’s picture

Status: Reviewed & tested by the community » Needs work

Uh oh :)

+++ includes/errors.inc	11 May 2010 16:48:11 -0000
@@ -139,6 +139,27 @@
+ * When in mainenance mode or when error_level is ERROR_REPORTING_DISPLAY_ALL, ¶
+ * all errors should be displayed. For ERROR_REPORTING_DISPLAY_SOME, $error ¶
+ * will be examined to determine if it should be displayed.
+ *
+ * @param $error
+ *   Optional error to examine for ERROR_REPORTING_DISPLAY_SOME.
+ *
+ * @return
+ *   TRUE if an error should be displayed.
+ */
+function error_displayable($error = NULL) {
+  $error_level = variable_get('error_level', ERROR_REPORTING_DISPLAY_ALL);
+
+  return (defined('MAINTENANCE_MODE') && MAINTENANCE_MODE == 'update') ||
+    ($error_level == ERROR_REPORTING_DISPLAY_ALL) || ($error_level == ERROR_REPORTING_DISPLAY_SOME && ¶
+    isset($error) && $error['%type'] != 'Notice' && $error['%type'] != 'Strict warning');

There are trailing spaces...

Powered by Dreditor.

jbrown’s picture

Status: Needs work » Needs review
FileSize
7.55 KB
Berdir’s picture

Status: Needs review » Reviewed & tested by the community

Better, this was already RTBC multiple times :)

webchick’s picture

Status: Reviewed & tested by the community » Needs work

Wow, that error_displayable() wrapper function is certainly a vast improvement in terms of readability. I also asked catch to take a look, as resident benchmarker of extra function calls, and he's cool with it.

The only thing is the PHPDoc for that function needs a bit of love. For example, there is nothing there that tells me when I need to call it. And I can't quite figure out why drupal_shutdown_function and drupal_write_session use it, but other places in bootstrap do not from reading the code.

jbrown’s picture

The only functions that need to call error_displayable() are _drupal_log_error(), _drupal_exception_handler(), _drupal_shutdown_function() and _drupal_session_write().

Ideally all errors would be reported with _drupal_log_error(), but this function is not available from _drupal_exception_handler(), _drupal_shutdown_function() and _drupal_session_write() as it may trigger an exception so these functions handle reporting themselves.

Maybe there should be a _drupal_log_error_safe() that would call error_displayable() and output whatever it is given?

I don't know what else could be added to the error_displayable() docs. It should be called when it needs to be determined whether an error should be displayed or not.

Should error_displayable() be _error_displayable() ?

kanzer’s picture

Status: Needs work » Needs review

#43: catch-session-exceptions.patch queued for re-testing.

alexanderpas’s picture

edited the unholy return statement of error_displayable() a bit, no other changes.

Nick Lewis’s picture

Status: Needs review » Reviewed & tested by the community

If there was a drupal centric Seinfeld, this would have been a episode.

cchan’s picture

There is a typo in the errors.inc file. Maintenance is spelled incorrectly (missing a t) - "when in mainenance mode..."

Index: includes/errors.inc
===================================================================
RCS file: /cvs/drupal/drupal/includes/errors.inc,v
retrieving revision 1.5
diff -u -r1.5 errors.inc
--- includes/errors.inc	11 Apr 2010 18:33:43 -0000	1.5
+++ includes/errors.inc	13 May 2010 15:50:05 -0000
@@ -139,6 +139,29 @@
 }
 
 /**
+ * Determines whether an error should be displayed.
+ *
+ * When in mainenance mode or when error_level is ERROR_REPORTING_DISPLAY_ALL,
+ * all errors should be displayed. For ERROR_REPORTING_DISPLAY_SOME, $error
+ * will be examined to determine if it should be displayed.
+ *
+ * @param $error
+ *   Optional error to examine for ERROR_REPORTING_DISPLAY_SOME.
+ *
+ * @return
+ *   TRUE if an error should be displayed.
+ */
+function error_displayable($error = NULL) {
+  $error_level = variable_get('error_level', ERROR_REPORTING_DISPLAY_ALL);
+  $updating = (defined('MAINTENANCE_MODE') && MAINTENANCE_MODE == 'update');
+  $all_errors_displayed = ($error_level == ERROR_REPORTING_DISPLAY_ALL);
+  $error_needs_display = ($error_level == ERROR_REPORTING_DISPLAY_SOME &&
+    isset($error) && $error['%type'] != 'Notice' && $error['%type'] != 'Strict warning');
+
+  return ($updating || $all_errors_displayed || $error_needs_display);
+}
rfay’s picture

This fixes the typo; no other changes.

jbrown’s picture

I'm happy with this.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Nice clean-up and fixes a critical problem. Committed to CVS HEAD.

Status: Fixed » Closed (fixed)
Issue tags: -Needs tests, -D7 upgrade path

Automatically closed -- issue fixed for 2 weeks with no activity.