Problem/Motivation

Steps to reproduce:

You will see the following error message:

If you have just changed code (for example deployed a new module or moved an existing one) read http://drupal.org/documentation/rebuild or run the rebuild script
Additional uncaught exception thrown while handling exception.

Original

LogicException: A stray drupal_render() invocation with $is_root_call = TRUE is causing bubbling of attached assets to break. in drupal_render() (line 3002 of /var/www/d8/core/includes/common.inc).

drupal_render(Array, 1)
drupal_render_root(Array)
Drupal\Core\Page\DefaultHtmlPageRenderer::renderPage('The website has encountered an error. Please try again later.', 'Error')
Drupal\Core\EventSubscriber\DefaultExceptionSubscriber->onHtml(Object)
Drupal\Core\EventSubscriber\DefaultExceptionSubscriber->onException(Object, 'kernel.exception', Object)
Drupal\Component\EventDispatcher\ContainerAwareEventDispatcher->dispatch('kernel.exception', Object)
Symfony\Component\HttpKernel\HttpKernel->handleException(Object, Object, 1)
Symfony\Component\HttpKernel\HttpKernel->handle(Object, 1, 1)
Drupal\Core\StackMiddleware\KernelPreHandle->handle(Object, 1, 1)
Drupal\Core\StackMiddleware\PageCache->handle(Object, 1, 1)
Drupal\Core\StackMiddleware\ReverseProxyMiddleware->handle(Object, 1, 1)
Stack\StackedHttpKernel->handle(Object, 1, 1)
Drupal\Core\DrupalKernel->handle(Object)
Additional

LogicException: A stray drupal_render() invocation with $is_root_call = TRUE is causing bubbling of attached assets to break. in drupal_render() (line 3002 of /var/www/d8/core/includes/common.inc).

drupal_render(Array, 1)
drupal_render_root(Array)
Drupal\Core\Page\DefaultHtmlPageRenderer::renderPage('The website has encountered an error. Please try again later.', 'Error')
_drupal_log_error(Array, 1)
_drupal_exception_handler(Object)

so what basically happens is that the exception rendering fails, this is quite bad, as there is no way to figure that out without going into _drupal_exception_handler()
and what not

Proposed resolution

TODO

Remaining tasks

User interface changes

API changes

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dawehner’s picture

.

Wim Leers’s picture

Ugh :(

Will fix this, of course.

Wim Leers’s picture

Note: I encountered this while working on #2352155: Remove HtmlFragment/HtmlPage already, or at least something very similar to this. I solved it there. Perhaps #2361681: drupal_render(): invert second argument ($is_recursive_call -> $is_root_call) => more strict, better DX/TX (the responsible issue/commit) being a child of #2352155 means that I didn't include the solution for that.

If I can't reproduce this in #2352155, then I'll consider that the solution.

dawehner’s picture

Wim Leers’s picture

I've reproduced this problem now that #2352155: Remove HtmlFragment/HtmlPage has landed. I'll fix it.

Wim Leers’s picture

Title: Exception thrown in drupal_render causes an exception during the rendering of exceptions » Exception thrown in drupal_render() causes an exception during the rendering of exceptions
Wim Leers’s picture

Also: #4: that issue title sounds hilarious :D

Wim Leers’s picture

Assigned: Unassigned » Fabianx

Root cause: if an exception occurs during rendering, the render stack does not get reset. When we then render a maintenance page with the error message, the error message above occurs, which is logically speaking correct.

AFAICT the best way to fix this is to have the ability to reset the render stack (in case an exception occurs)… and to be able to do that, we need the render stack not to be contained by drupal_render(), but for it to be a separate service.

I think Fabianx might have some thoughts on this — assigning to him for feedback.

AviiNL’s picture

I am getting the same error after generating nodes using devel-generate then visiting my home-page.

edit: figured out it is a problem with the basic theme, once i set bartik default again, the message goes away.

edit2: Okay, so it happens if I add any content to my site when using basic theme,
Not sure if still related to the OP.. (still the same error though)

Fabianx’s picture

Status: Active » Postponed

While a render stack is a good idea, it is way less useful once there is a Renderer service that holds the stack state and has a resetStack method.

So postponing briefly on #2346937: Implement a Renderer service; reduces drupal_render / _theme service container calls.

Wim Leers’s picture

Assigned: Fabianx » Unassigned
Status: Postponed » Active
Related issues: +#1855066: In the "menu" toolbar tray, clicking/tapping white space should show the child level

Hurray, #1855066: In the "menu" toolbar tray, clicking/tapping white space should show the child level landed,so we can now easily add a resetStack() method, hence this issue is now unblocked. Will roll patch, unless somebody beats me to it.

Wim Leers’s picture

Wim Leers’s picture

Status: Active » Needs review
FileSize
10.46 KB

Status: Needs review » Needs work

The last submitted patch, 13: exception_inception-2364381-13.patch, failed testing.

Wim Leers’s picture

Both of those failing tests pass locally! :( Re-testing.

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 13: exception_inception-2364381-13.patch, failed testing.

Wim Leers’s picture

I'm clueless. Help?

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 13: exception_inception-2364381-13.patch, failed testing.

Status: Needs work » Needs review
dawehner’s picture

I could reproduce that on my relative install on localhost/d8.

For the config import it set ups a batch process, which itself is processed
using the no_js option. For a simpletest it means that WebTestBase::checkForMetaRefresh() is used.
If you track the recursive calls in ConfigImportUiTest::testImport 115you will see that just the first request
has that meta tag, which forces the no_js refresh, every following one, no longer has it, aka. no more than step 0 is processed in the simpletest.

You will see similar behaviour when you disable JS, install it on localhost/d8
and import config changes AND extension changes.

Things I tried:

  • Don't reset the stack for batch requests
  • Render an absolute meta URL

Status: Needs review » Needs work

The last submitted patch, 13: exception_inception-2364381-13.patch, failed testing.

Fabianx’s picture

Well, it seems BareHtml Rendering does care about the state, however to fix the bug here, we only need to reset it in the exception rendering case, right?

Lets skip that and move to follow up or won't fix?

Thanks Daniel, great research!

Wim Leers’s picture

The inexplicable failure was fixed by dawehner & Fabianx in #2382503: Not possible to render self-contained render array while a render stack is active, which just got committed.

Root cause: if an exception occurs during rendering, the render stack does not get reset. When we then render a maintenance page with the error message, the error message above occurs, which is logically speaking correct.

This is actually wrong; there already is a safe guard to reset the stack in case an exception occurs during #pre_render. That then only leaves the following function calls inside Renderer::render() that would need similar safeguards:

  1. $elements['#children'] = $this->theme->render($elements['#theme'], $elements);
  2. $elements['#children'] = $this->theme->render($wrapper_hook, $wrapper_elements);
  3. #post_render callback execution
  4. #post_render_cache callback execution

I've attached 3 patches:

  1. one that will fail, with only the tests
  2. option 1, which proposes a solution based on the above patch
  3. option 2, which proposes a solution based on the new insight in this patch

I strongly prefer option 2, because it's the least hacky approach. Option 1 does still leave the main stack in its bad state. Option 2 resets it, preventing any and all problems.

The last submitted patch, 25: exception_inception-2364381-25-tests_only_FAIL.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 25: exception_inception-2364381-25-option_2.patch, failed testing.

Fabianx’s picture

I prefer option 2, too, but find it too verbose:

a) Wrap the whole function in a try { catch} after we early return for cache_get.
b) Add a helper tryCall which does the try-catch
c) Leave as is.

My vote is a) or c) as b) is performance problematic.

Besides that: Looks good.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
7.03 KB

Ok, option 2 it is :)

I think that if we're a bit smarter about b) that we can limit the damage? For /node/2, this implementation gives me 107,750 (HEAD) vs. 107,802 (patch) function calls. 52/107750 = 0.048% increase. i.e. negligible: a tenth of a half percent.

Fabianx’s picture

Status: Needs review » Needs work

As discussed in IRC, can add this to renderRoot / renderPlain, behavior of calling render() without not being in a stack / root call context is undefined anyway.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
2.44 KB
2.44 KB

Status: Needs review » Needs work

The last submitted patch, 31: exception_inception-2364381-31.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
6.19 KB

I uploaded the interdiff as the patch. D'oh. Again.

Status: Needs review » Needs work

The last submitted patch, 33: exception_inception-2364381-33.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
6.15 KB
1.57 KB

Wim cannot multitask.
Wim cannot multitask.
Wim cannot multitask.

Status: Needs review » Needs work

The last submitted patch, 35: exception_inception-2364381-35.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
106.11 KB

So I agreed with

As discussed in IRC, can add this to renderRoot / renderPlain, behavior of calling render() without not being in a stack / root call context is undefined anyway.

But I always had this feeling we were forgetting something. Now I know what :)

The problem is that we render the main content not with ::renderRoot(), but with ::render(). For good reason: it's necessary to allow the controller to generate a dynamic title.
However, the controller could also choose to throw an exception, like a 404, in case it dynamically decides that it cannot show any content. Views does this:

Here's a stack trace of the view in the failing NodeIntegrationTest being rendered:

which then hits the following code:

    if (!empty($this->view->build_info['fail'])) {
      throw new NotFoundHttpException();
    }

because this happens inside a ::render() call, the patches since #30 will not catch this exception and hence fail with the same error as the one in the IS.

This is absolutely valid, and hence the only possible conclusion is that we must catch exceptions on ::render(), and hence go back to the patch in #29.

Wim Leers’s picture

FileSize
7.03 KB

Straight reupload of #29. Still applies cleanly.

Fabianx’s picture

Status: Needs review » Reviewed & tested by the community

As calling render is valid and everything can throw an exception, this is RTBC.

Thanks, Wim!

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.0.x, thanks!

  • catch committed 599bad3 on 8.0.x
    Issue #2364381 by Wim Leers: Exception thrown in drupal_render() causes...

Status: Fixed » Closed (fixed)

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