Problem/Motivation

In HtmlRenderer::prepare, the main content is rendered by wrapping the execution in the following:

$this->renderer->executeInRenderContext();

The page attachments are generated in the same method, but without executing the rendering in a context:

$this->invokePageAttachmentHooks($page);

The issue is that big_pipe_page_attachments calls Url::toString which invokes RendererInterface:render in the background, trying to render something without a render context. This causes the following exception to be thrown:

The controller result claims to be providing relevant cache metadata, but leaked metadata was detected.

Steps to reproduce

1. Enable bigpipe
2. In a custom route controller use this:

    $build = [
      '#title' => 'A title',
      'content' => ['#markup' => 'Some content'],
    ];
    return \Drupal::service('main_content_renderer.html')->renderResponse($build, \Drupal::requestStack()->getCurrentRequest(), \Drupal::routeMatch());

3. Visit the URL of the custom route
4. Notice the error.

Proposed resolution

Two solutions:
- Adjust HtmlRenderer::prepare so it invokes invokePageAttachmentHooks inside a render context.
- Adjust big_pipe_page_attachments so it calls Url::toString inside a render context.

Remaining tasks

n/a

User interface changes

n/a

API changes

n/a

Data model changes

n/a

Release notes snippet

n/a

Comments

alecsmrekar created an issue. See original summary.

wim leers’s picture

Status: Active » Postponed (maintainer needs more info)
Issue tags: +Needs tests

🤯

How has this never been an issue so far?!

Based on the description, we should be able to reproduce this with any response? Or … only if your controller calls main_content_renderer.html directly, rather than just returning the render array? Or can you even reproduce this when your controller returns hardcoded HTML?

alecsmrekar’s picture

StatusFileSize
new4.47 KB

I think this only happens when a controller returns a Response object. It doesn't happen if you return a render array.
You can see this in EarlyRenderingControllerWrapperSubscriber::wrapControllerExecutionInRenderContext.

I'm attaching an initial patch.

alecsmrekar’s picture

Status: Postponed (maintainer needs more info) » Needs review

Status: Needs review » Needs work

The last submitted patch, 3: 3303874-early-rendering-issue.patch, failed testing. View results

alecsmrekar’s picture

StatusFileSize
new4.51 KB

Adjusted the patch to resolve the deprecation notices.

alecsmrekar’s picture

StatusFileSize
new4.5 KB
alecsmrekar’s picture

StatusFileSize
new4.44 KB
alecsmrekar’s picture

Status: Needs work » Needs review

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.9 was released on December 7, 2022 and is the final full bugfix release for the Drupal 9.4.x series. Drupal 9.4.x will not receive any further development aside from security fixes. Drupal 9 bug reports should be targeted for the 9.5.x-dev branch from now on, and new development or disruptive changes should be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

smustgrave’s picture

Status: Needs review » Needs work
Issue tags: -Needs tests +Needs Review Queue Initiative, +Needs issue summary update

This issue is being reviewed by the kind folks in Slack, #needs-review-queue-initiative. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge request as a guide.

One of the proposed solutions

- Adjust big_pipe_page_attachments so it calls Url::toString inside a render context.

Still appears to need to be done.

wim leers’s picture

Title: Early rendering issue in big_pipe_page_attachments » Early rendering issue in big_pipe_page_attachments() for controllers returning Response objects
Status: Needs work » Needs review
Issue tags: +D8 cacheability

Clarifying based on #3.

#11: no, the patch just implements the other possible solution. And that solution is actually preferable because it solves the problem generically.

Testing against 10.1.x

smustgrave’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs issue summary update

Thanks @Wim Leers! D10.1 didn't have any failures

  • catch committed 1bca2d9b on 10.0.x
    Issue #3303874 by alecsmrekar, Wim Leers, smustgrave: Early rendering...

  • catch committed d6d5051f on 10.1.x
    Issue #3303874 by alecsmrekar, Wim Leers, smustgrave: Early rendering...
catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 10.1.x, cherry-picked to 10.0.x and 9.5.x, thanks!

  • catch committed 397bc37b on 9.5.x
    Issue #3303874 by alecsmrekar, Wim Leers, smustgrave: Early rendering...

Status: Fixed » Closed (fixed)

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