Follow-up to #1825952: Turn on twig autoescape by default

Problem/Motivation

In #1825952: Turn on twig autoescape by default, we add a known list of "safe" strings that is populated from everything processed by String::checkPlain() (and therfore every @ or % placeholder passed to t() and String::format()), Xss::filter(), various Views and other rendering functions, etc. This means in many cases we may have nested lists of safe strings, i.e. one safe string that is composed of several other safe strings that in turn is composed of several others... all in the static list. We have no idea how big this list might get at present.

Additionally, for form and batch processing, we are storing the known list of safe strings in the form/batch state to transfer it between requests. This has the potential to exacerbate existing form cache issues, especially if the list is very large.

Proposed resolution

Ensure that the list of strings does not become dangerously bloated for cache size or the memory footprint.

Remaining tasks

Do some profiling, particularly once other critical issues related to the form cache are resolved. Suggestions for profiling:

  • The permissions page with more than 10 roles and standard profile.
  • A large view, especially one with exposed filters.
    • admin/people with more than 10 roles and 100+ users
    • admin/content with 100+ nodes
  • The block placement page.
  • The menu page for a large menu

User interface changes

N/A

API changes

N/A unless something is broken.

Postponed until

CommentFileSizeAuthor
#17 2295823.17.patch62.54 KBalexpott
#13 2295823.13.patch22.85 KBalexpott
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

yched’s picture

Do we really need to persist the list across form rebuilds ? Was performance really bad without it or is that some just-in-case / possibly premature optimization ?

Also not fully convinced of the interest of persisting across batch steps. Batches are mostly about processing rather than outputing strings than need to be sanitized, so the gain seems minor. OTOH, on a batch with thousands of steps that stores messages, serializing, unserializing the growing cache of safe strings could easily slow down processing as the batch progress.

xjm’s picture

Status: Postponed » Active
xjm’s picture

Issue tags: +revisit before release candidate

Just making sure this one doesn't slip through the cracks forever.

xjm’s picture

Issue tags: +SafeMarkup
xjm’s picture

xjm’s picture

Issue summary: View changes

Adding some other/more specific scenarios.

xjm’s picture

xjm’s picture

xjm’s picture

Category: Task » Plan
star-szr’s picture

Looks like that comment was meaning to reference #2506035: Do not add placeholders from SafeMarkup::format() to the safe list :)

alexpott’s picture

Status: Active » Needs review
FileSize
22.85 KB

The patch attached combines:

On my test scenario of 11 roles and rendering admin/people/permissions it has the following affect:

Run #557f553bef71a Run #557f56d10d013 Diff Diff%
Number of Function Calls 762,886 744,726 -18,160 -2.4%
Incl. MemUse (bytes) 51,700,240 43,454,272 -8,245,968 -15.9%
Incl. PeakMemUse (bytes) 63,862,320 55,666,088 -8,196,232 -12.8%

SafeMarkup::set() is called 11,903 times less!

[edit removed wall time because it is misleading]

Status: Needs review » Needs work

The last submitted patch, 13: 2295823.13.patch, failed testing.

xjm’s picture

Status: Needs work » Active

@alexpott, awesome work.

Wim Leers’s picture

\o/ alexpott \o/

alexpott’s picture

Status: Active » Needs review
FileSize
62.54 KB

Here's the combination of patches #13 not that they are all green.

Run #558042caa0842 Run #55804264bcc9a Diff Diff%
Number of Function Calls 762,887 742,608 -20,279 -2.7%
Incl. Wall Time (microsec) 2,054,154 1,946,305 -107,849 -5.3%
Incl. MemUse (bytes) 51,683,416 42,025,448 -9,657,968 -18.7%
Incl. PeakMemUse (bytes) 63,867,592 54,250,088 -9,617,504 -15.1%
Function Name Calls Diff Calls
Diff%
Incl. Wall
Diff
(microsec)
IWall
Diff%
Excl. Wall
Diff
(microsec)
EWall
Diff%
Incl.
MemUse
Diff
(bytes)
IMemUse
Diff%
Excl.
MemUse
Diff
(bytes)
EMemUse
Diff%
Incl.
PeakMemUse
Diff
(bytes)
IPeakMemUse
Diff%
Excl.
PeakMemUse
Diff
(bytes)
EPeakMemUse
Diff%
Drupal\Component\Utility\SafeMarkup::set -11,906 -58.7% -29,708 -27.5% -29,708 -27.5% -22,104,592 -228.9% -22,104,592 -228.9% -5,494,392 -57.1% -5,494,392 -57.1%
Drupal\Core\Render\SafeString::__toString 6,125 30.2% 50 0.0% 50 0.0% 19,256 0.2% 19,256 0.2% 1,112 0.0% 1,112 0.0%
Drupal\Core\Render\SafeString::__construct 6,105 30.1% 1,645 1.5% 1,645 1.5% 11,692,048 121.1% 11,692,048 121.1% 1,766,112 18.4% 1,766,112 18.4%
is_scalar -5,698 -28.1% 2 0.0% 2 0.0% 8 0.0% 8 0.0% -24 -0.0% -24 -0.0%
Drupal\Component\Utility\SafeMarkup::isSafe -5,697 -28.1% -3,432 -3.2% -2,585 -2.4% 488,264 5.1% 518,320 5.4% -1,118,408 -11.6% -1,118,408 -11.6%
is_object -4,612 -22.7% -107 -0.1% -107 -0.1% -8 -0.0% -8 -0.0% -1,456 -0.0% -1,456 -0.0%
method_exists -4,612 -22.7% -120 -0.1% -120 -0.1% -2,000 -0.0% -2,000 -0.0% -1,168 -0.0% -1,168 -0.0%
htmlspecialchars 2,589 12.8% 211 0.2% 211 0.2% 369,760 3.8% 369,760 3.8% -4,000 -0.0% -4,000 -0.0%
Drupal\Component\Utility\SafeMarkup::checkPlain -757 -3.7% -1,622 -1.5% -1,554 -1.4% 355,408 3.7% 471,096 4.9% -1,200 -0.0% -928 -0.0%
Drupal\Component\Utility\UrlHelper::stripDangerousProtocols -733 -3.6% -3,485 -3.2% -2,401 -2.2% -720 -0.0% 1,562,576 16.2% -22,728 -0.2% -1,824 -0.0%
array_flip -733 -3.6% -1,848 -1.7% -1,848 -1.7% -1,358,384 -14.1% -1,358,384 -14.1% -152,896 -1.6% -152,896 -1.6%
Drupal\Component\Utility\SafeMarkup::escape -107 -0.5% -1,147 -1.1% -145 -0.1% -38,552 -0.4% 3,424 0.0% 0 0.0% 0 0.0%
Drupal\Core\Template\AttributeValueBase::render -94 -0.5% -4,875 -4.5% -2,299 -2.1% -64,896 -0.7% 51,808 0.5% -30,584 -0.3% -2,728 -0.0%
Drupal\Core\Template\AttributeString::__toString -80 -0.4% -638 -0.6% -289 -0.3% -43,992 -0.5% -17,616 -0.2% -5,320 -0.1% -160 -0.0%
Drupal\Component\Utility\SafeMarkup::doPlaceholder 42 0.2% 109 0.1% 82 0.1% 8,128 0.1% 1,864 0.0% 0 0.0% 0 0.0%
Drupal\Component\Utility\SafeMarkup::placeholder -39 -0.2% -128 -0.1% -62 -0.1% -13,520 -0.1% -10,504 -0.1% 0 0.0% 0 0.0%
Drupal\Core\Template\Attribute::__toString -27 -0.1% -17,790 -16.5% -7,642 -7.1% -1,999,952 -20.7% 786,760 8.1% -84,720 -0.9% -6,448 -0.1%
Drupal\Core\Template\AttributeArray::__toString -14 -0.1% -1,918 -1.8% -972 -0.9% -44,544 -0.5% 18,552 0.2% -22,888 -0.2% -1,008 -0.0%
array_unique -14 -0.1% -332 -0.3% -332 -0.3% -28,672 -0.3% -28,672 -0.3% -16,016 -0.2% -16,016 -0.2%
array_filter -14 -0.1% -102 -0.1% -107 -0.1% -22,784 -0.2% -22,784 -0.2% -5,720 -0.1% -5,720 -0.1%
implode -14 -0.1% -35 -0.0% -35 -0.0% 68,600 0.7% 68,600 0.7% 864 0.0% 864 0.0%
Drupal\Core\Template\AttributeBoolean::render -2 -0.0% -9 -0.0% -5 -0.0% -248 -0.0% 0 0.0% 0 0.0% 0 0.0%
Drupal\Core\Template\AttributeBoolean::__toString -2 -0.0% -4 -0.0% -2 -0.0% -248 -0.0% -96 -0.0% 0 0.0% 0 0.0%

Status: Needs review » Needs work

The last submitted patch, 17: 2295823.17.patch, failed testing.

Berdir’s picture

// Store the known list of safe strings for form re-use.
// @todo Ensure we are not storing an excessively large string list in:
// https://www.drupal.org/node/2295823
$form_state->addBuildInfo('safe_strings', SafeMarkup::getAll());

How exactly are we *ensuring* this here?

I noticed that our form_state entries on a big page are *huge* (3-6MB), tracked it down to this list for a form that's rendered in post_render_cache/callback, so safe markup is full of viewed entities, views and so on. I guess #2506581: Remove SafeMarkup::set() from Renderer::doRender is going to help specifically with that.

xjm’s picture

Issue tags: -revisit before release candidate

This is currently covered in the scope of #2549943: [plan] Remove as much of the SafeMarkup class's methods as possible, so untagging.

joelpittet’s picture

Status: Needs work » Closed (duplicate)

We're no longer storing lists of strings. They are all passed around as Value Objects. This was done as a barrage of criticals just before RC.