### Problem/Motivation

When using the big_pipe module with JavaScript, under certain circumstances, placeholders generated from #lazy_builder callbacks to history_attach_timestamp in comments cause Drupal.behaviors for the entire document to be detached. This issue became apparent after custom theming the comments field to put the comment form above the list of comments rather than below it. The user-facing issue was that, shortly after being attached to the comment_body field, the CKEditor was subsequently detached, leaving a plain textarea field, and producing the following warning in the browser's developer console:

[CKEDITOR] Error code: editor-incorrect-destroy.

The specific reason for this issue is as follows. After making a theme change to move the comment form above the list of existing comments, the order of content with placeholders changed in the document, putting the placeholder for the comment form above the history_attach_timestamp. Big Pipe uses the order of the placeholders in the markup to set the order in which it renders the <script> tags it appends to the document body, and Big Pipe's JavaScript, in turn, reads the script tags in the order they appear. The result in this case was that the placeholder for history_attach_timestamp was evaluated after the one for the comment form.

#### Steps to reproduce:

1. Ensure comment, big_pipe, and history modules are enabled.
2. Create a node with a few comments.
3. Edit field--comment.html.twig to move the comment form above the list of comments on a node
4. Rebuild caches and note that the WYSIWYG is gone from the form, and the errors above are in the Chrome or Firefox console
5. Attach patch and note the issue is fixed

Here is a sample of the script tags that Big Pipe appended to the end of the document body after the comment form was moved above the comment list:

<script type="application/json" data-big-pipe-event="start"></script>
<script type="application/json" data-big-pipe-placeholder="callback=Drupal%5CCore%5CRender%5CElement%5CStatusMessages%3A%3ArenderMessages&amp;args[0]&amp;token=a8c34b5e" data-drupal-ajax-processor="big_pipe">
</script>    <script type="application/json" data-big-pipe-placeholder="callback=history_attach_timestamp&amp;args[0]=2&amp;token=15cf9f98" data-drupal-ajax-processor="big_pipe">
</script>    <script type="application/json" data-big-pipe-placeholder="callback=history_attach_timestamp&amp;args[0]=2&amp;token=15cf9f98" data-drupal-ajax-processor="big_pipe">
</script><script type="application/json" data-big-pipe-event="stop"></script>


As the above code sample demonstrates, the script tags with the identifier data-big-pipe-placeholder="callback=history_attach_timestamp&amp;args[0]=2&amp;token=15cf9f98" appear after the one with the identifier data-big-pipe-placeholder="callback=comment.lazy_builders%3ArenderForm&amp;args[0]=node&amp;args[1]=2&amp;args[2]=field_question_comments&amp;args[3]=comment&amp;token=6313718a", and as a result the code in the big_pipe.js file evaluates the script tags with callback=history_attach_timestamp after the script tag containing the JSON for the comment form placeholder. This logic appears in the following code excerpt from bigPipeProcessDocument() in big_pipe.js:

    $(context).find('script[data-big-pipe-replacement-for-placeholder-with-id]') .once('big-pipe') .each(bigPipeProcessPlaceholderReplacement);  When a given node has more than one comment (the above example has two, which is why there are two tags with data-big-pipe-placeholder="callback=history_attach_timestamp&amp;args[0]=2&amp;token=15cf9f98"), the comment module generates identical #lazy_builder callbacks to history_attach_timestamp for all of them, which big_pipe in turn uses to generate identical placeholders. The identical placeholders become an issue when the bigPipeProcessPlaceholderReplacement() function simulates an ajax success response using the identical script tags. The success() method in the ajax system invokes the simulated command defined in the JSON content of the script tags, which uses the replaceWith method. This triggers invocation of Drupal.AjaxCommands.prototype.insert() command, which detaches behaviors attached to any DOM elements in the context of the placeholder: Drupal.detachBehaviors(wrapper.get(0), settings);  The first time this executes on a placeholder generated for the #lazy_builder callback for history_attach_timestamp, the value of wrapper.get(0) is <div data-big-pipe-placeholder-id="callback=history_attach_timestamp&amp;args[0]=2&amp;token=15cf9f98"></div>. However, a few lines below that, the jQuery replaceWith method is triggered:  // Add the new content to the page. wrapper[method](new_content);  After this line executes, the DOM node <div data-big-pipe-placeholder-id="callback=history_attach_timestamp&amp;args[0]=2&amp;token=15cf9f98"></div> is replaced with an empty div (<div></div>), because the value of response.data in the simulated ajax response is an empty string. The problem is that the next time this code executes on an identical placeholder from the script tags at the bottom of the document body, all instances of these placeholder divs in the DOM have been replaced with empty divs. The value for the wrapper variable is now an empty object, because the selector in response.selector refers to DOM nodes that have been removed: // The value of response.selector is [data-big-pipe-placeholder-id="callback=history_attach_timestamp&args[0]=2&token=15cf9f98"] var wrapper = response.selector ?$(response.selector) : $(ajax.wrapper);  Then, a few lines down, when Drupal.detachBehaviors() runs, the value of wrapper.get(0) evaluates to undefined: Drupal.detachBehaviors(wrapper.get(0), settings);  Drupal.detachBehaviors() sets the context in which it operates to the entire document if the context is undefined; see the first line of the function:  Drupal.detachBehaviors = function (context, settings, trigger) { context = context || document; settings = settings || drupalSettings; trigger = trigger || 'unload'; var behaviors = Drupal.behaviors; // Execute all of them. for (var i in behaviors) { if (behaviors.hasOwnProperty(i) && typeof behaviors[i].detach === 'function') { // Don't stop the execution of behaviors in case of an error. try { behaviors[i].detach(context, settings, trigger); } catch (e) { Drupal.throwError(e); } } } };  The result is that behaviors are detached from the entire document, causing behaviors that were attached earlier in the execution process to be removed. ### Proposed resolution Proposed solution is to modify \Drupal\comment\CommentViewBuilder::buildComponents() to prevent duplicate copies of the placeholder from being generated. The #lazy_builder callback in the render array that causes Big Pipe to generate the placeholder seems to have one purpose: to add the timestamp for the comments' parent node to the drupalSettings object. The #lazy_builder for the history_attach_timestamp in the comment view builder is:  // Embed the metadata for the comment "new" indicators on this node.$build[$id]['history'] = [ '#lazy_builder' => ['history_attach_timestamp', [$commented_entity->id()]],
'#create_placeholder' => TRUE,
];


The callback function for history_attach_timestamp() is:

function history_attach_timestamp($node_id) {$element = [];
$element['#attached']['drupalSettings']['history']['lastReadTimestamps'][$node_id] = (int) history_read($node_id); return$element;
}


The history_attach_timestamp() callback function always returns a render array that is empty, except for the settings being added to drupalSettings. Since the settings are per node ID and are the same for all comments attached to the same node, subsequent invocations of this callback function merely result in this property of the drupalSettings object being overwritten with the same value.

Proposed code change is to set this #lazy_builder only once for the entire comment list:

diff --git a/core/modules/comment/src/CommentViewBuilder.php b/core/modules/comment/src/CommentViewBuilder.php
index 9b47b7c..435bae5 100644
--- a/core/modules/comment/src/CommentViewBuilder.php
+++ b/core/modules/comment/src/CommentViewBuilder.php
@@ -148,18 +148,18 @@ public function buildComponents(array &$build, array$entities, array $displays,$build[$id]['#attached']['library'][] = 'comment/drupal.comment-by-viewer'; if ($this->moduleHandler->moduleExists('history') && $this->currentUser->isAuthenticated()) {$build[$id]['#attached']['library'][] = 'comment/drupal.comment-new-indicator'; - - // Embed the metadata for the comment "new" indicators on this node. -$build[$id]['history'] = [ - '#lazy_builder' => ['history_attach_timestamp', [$commented_entity->id()]],
-          '#create_placeholder' => TRUE,
-        ];
}
}
if ($build[$id]['#comment_threaded']) {
// The final comment must close up some hanging divs.
$build[$id]['#comment_indent_final'] = $current_indent; } + + // Embed the metadata for the comment "new" indicators on this node. +$build['history'] = [
+      '#lazy_builder' => ['history_attach_timestamp', [$commented_entity->id()]], + '#create_placeholder' => TRUE, + ]; } /**  Patch file attached. ### Remaining tasks Patch needs review. ### User interface changes None. ### API changes None. ### Data model changes None. Files: CommentFileSizeAuthor #321.02 KBdanmuzyka #3012.95 KBdanmuzyka #2413.85 KBWim Leers ## Comments danmuzyka created an issue. See original summary.  Status: Needs review » Needs work FileSize 1.43 KB 945 bytes Corrected patch attached.  Status: Needs work » Needs review  Version: 8.0.x-dev » 8.1.x-dev Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0. Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle. FileSize 1.43 KB 290 bytes Updating patch file for 8.1.x branch.  Issue summary: View changes I've updated the IS with steps to reproduce, and also verified myself that this patch fixes the issue using those steps. +++ b/core/modules/comment/src/CommentViewBuilder.php @@ -142,18 +142,23 @@ public function buildComponents(array &$build, array $entities, array$displays,
if ($this->moduleHandler->moduleExists('history') &&$this->currentUser->isAuthenticated()) {
$build[$id]['#attached']['library'][] = 'comment/drupal.comment-new-indicator';


Can this bit now be moved below as well since it is the same if check?

FileSize
550 bytes

This patch is for manual testing purposes only. It moves the comment form above the list of comments in Bartik.

 Version: 8.1.x-dev » 8.2.x-dev
FileSize
4.57 KB

I started on a javascript test, but did not get far enough to reproduce the error. For some reason, the custom test theme declared here isn't being used, so the comment form is still below the comments.

 Component: comment.module » big_pipe.module

After further investigation, this issue seems to be related more generally to the way that Big Pipe processes placeholders. There are circumstances under which the original patch to the comment module won't fix the issue (e.g., if there are multiple comment fields on the same page), so a better solution seems to be for Big Pipe not to add duplicate copies of the placeholder JSON in the page footer.

I'm attaching a new patch that fixes this in Big Pipe instead of Comment, and includes JavaScript functional tests.

 Status: Needs review » Needs work
 Status: Needs work » Needs review
FileSize
6.79 KB
183 bytes

Rerolled patch.

@danmuzyka could you attach patch that just contains the new test to illustrate the failure without the fix?

Sure, here is the patch file with the test only.

 Status: Needs review » Needs work
 Status: Needs work » Needs review Issue tags: +BigPipe in Core

This is looking good to me, both the fix and the test. Back to NR and tagging in hopes of getting a few more eyes on this for review.

 Status: Needs review » Needs work

Nice work on the patch!

I would like to skip re-rendering placeholders that have already been replaced though - if possible and not just skip the JS replacement part.

 Title: Duplicate placeholders generated from #lazy_builder callbacks to history_attach_timestamp in comments cause JavaScript errors when using Big Pipe » BigPipe unnecessarily renders and sends multiple-occurrence placeholders multiple times when using JS — can cause JS errors Issue tags: +JavaScript, +Needs tests
1. Reproduced, thanks for the clear STR!
2. The solution in the IS is wrong: we should not modify CommentViewBuilder. Having multiple lazy builders map to the same placeholder is very much intentional, and is an architectural requirement: the whole point is that if independent code paths need the same placeholder, that we only replace that placeholder once.
3. The problem is that there really is only a single placeholder for history_attach_timestamp() (verify yourself by putting a breakpoint at \Drupal\big_pipe\Render\Placeholder\BigPipeStrategy::doProcessPlaceholders()), yet BigPipe wants to render each occurrence of a placeholder separately. i.e. the problem is \Drupal\big_pipe\Render\BigPipe::getPlaceholderOrder(). That code relies on parsing to determine which placeholders to replace. This means the solution in #14 is also wrong.

Patch attached.

This should still get a \Drupal\big_pipe\Tests\BigPipeTest::testBigPipeJsMultipleOccurrencePlaceholders() test. That would be a generic test. However, it's awesome that the current patch already contains a functional JS regression test! I'd like to keep that, but rename it to be specifically for regression tests.

1. index 3e67737..6f32c6c 100644
--- a/core/modules/big_pipe/src/Render/BigPipe.php


The changes here should be reverted.

2. +++ b/core/modules/big_pipe/tests/src/FunctionalJavascript/BigPipePlaceholderTest.php
@@ -0,0 +1,116 @@
+ * @group comment


s/comment/big_pipe/

3. +++ b/core/modules/big_pipe/tests/src/FunctionalJavascript/BigPipePlaceholderTest.php
@@ -0,0 +1,116 @@
+class BigPipePlaceholderTest extends JavascriptTestBase {


BigPipeRegressionTest

4. +++ b/core/modules/big_pipe/tests/src/FunctionalJavascript/BigPipePlaceholderTest.php
@@ -0,0 +1,116 @@
+    // Clear the theme registry.
+    $this->container->set('theme.registry', NULL);  This should not be necessary. 5. +++ b/core/modules/big_pipe/tests/src/FunctionalJavascript/BigPipePlaceholderTest.php @@ -0,0 +1,116 @@ + // Ensure an article node type exists. +$this->createContentType(['type' => 'article']);
+    $this->addDefaultCommentField('node', 'article'); + + // Enable CKEditor. + FilterFormat::create([ + 'format' => 'full_html', + 'name' => 'Full HTML', + 'weight' => 1, + 'filters' => [], + ])->save(); +$settings['toolbar']['rows'] = [
+      [
+        [
+          'items' => [
+          ],
+        ],
+      ],
+    ];
+    $editor = Editor::create([ + 'format' => 'full_html', + 'editor' => 'ckeditor', + ]); +$editor->setSettings($settings); +$editor->save();
+
+    $admin_user =$this->drupalCreateUser([
+      'use text format full_html',
+    ]);
+    $this->drupalLogin($admin_user);


Can you move this into the test method?

6. +++ b/core/modules/big_pipe/tests/src/FunctionalJavascript/BigPipePlaceholderTest.php
@@ -0,0 +1,116 @@
+  public function testCommentForm() {


testCommentFormRegression_2698811()

FileSize
1.07 KB

Oops, forgot the patch.

 Status: Needs work » Needs review
FileSize
9.81 KB
8.83 KB

Here's an updated patch file with the new, generic test, and an interdiff of it relative to the patch I posted in #12.

Question:

Will this also work when JS is off?

As there we can only replace one placeholder at a time ...

 Issue tags: -Needs tests
FileSize
10.22 KB
7.34 KB

#20: splendid work! :) Thanks so much!

Here's first a reroll that fixes all my nits. That avoids needless back-and-forth. Zero code changes, just renaming, simplifying, strictifying.

FileSize
11.55 KB
4.05 KB

Will this also work when JS is off?

Yes, this already was only a problem when JS was turned on. My analysis in #18 already applies only to JS placeholders: getPlaceholderOrder() only runs for JS placeholders.

We could look into the no-JS placeholder case too… and I decided to do so here.

In order to test that better, it's important that we can prove that no-JS placeholders are rendered multiple times. An easy way to test that, is by doing something evil: using a static inside a lazy builder, so we can increment a counter. Then the JS version would show "Count=1. Count=1. Count=1." The no-JS version would show "Count=1. Count=2. Count=3." And that's exactly what happens.

Attached patch expands that test coverage, and fails.

FileSize
13.85 KB
2.63 KB

And here are then the changes for \Drupal\big_pipe\Render\BigPipe::sendNoJsPlaceholders().

As far as I'm concerned, this is RTBC. If Fabian agrees, he can RTBC this right away.

 Version: 8.2.x-dev » 8.1.x-dev

Oh, and this should go in 8.1, not 8.2. BigPipe is experimental, so we can commit this to 8.1 :)

Also testing #24 against 8.1.

@danmuzyka & @jhedstrom: I just wanted to say thanks again for your excellent work here. These are much higher quality comments and patches than usual, so please know it is very much appreciated. I've repaid the favor by helping to land this issue swiftly. I'm sorry for not having seen it sooner, if I would have, then I'd have helped land it right then!

The last submitted patch, 23: big_pipe_multi_occurence_js_placeholder-2698811-23.patch, failed testing.

 Status: Needs review » Reviewed & tested by the community

RTBC - Yes, indeed fantastic work on the patches and tests.

It is great to have both a JS regression test (yeah!) and functional tests.

Also good that we now no longer re-replace multiple occurences, which will help for all "token" based placeholders, so a nice perf improvement, too.

It is great to have both a JS regression test (yeah!) and functional tests.

Indeed!

This seems like a perfect model:

1. have thorough, comprehensive tests that are abstract (unit tests and functional tests where necessary)
2. have concrete regression tests for every regression ever encountered, and ensure they have a test in a fully functional environment (and thus functional JS tests)

At least, I personally think that's the perfect model. It allows me to have very strong confidence that it works as intended.

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

Thanks @Wim Leers and @Fabianx !

If you feel like fast-tracking any other core bugs, I have two other open ones at the moment: ;-)

I think in the latest patch, the callback function placeholderContent() is no longer needed. I'm attaching an updated patch that removes it.

#30: Could you make interdiffs with git diff OR with diff -u?

They are very hard to read atm. ...

FileSize
1.02 KB

Sorry, this one should be better...

 Status: Needs review » Reviewed & tested by the community

Indeed, back to RTBC

 Status: Reviewed & tested by the community » Needs work

The last submitted patch, 30: big_pipe_multi_occurence_js_placeholder-2698811-30.patch, failed testing.

 Status: Needs work » Reviewed & tested by the community

Those fails do not look related to this patch.

• catch committed 5e86905 on 8.2.x
Issue #2698811 by danmuzyka, Wim Leers, jhedstrom, Fabianx: BigPipe...
• catch committed f9cd621 on 8.1.x
Issue #2698811 by danmuzyka, Wim Leers, jhedstrom, Fabianx: BigPipe...
 Status: Reviewed & tested by the community » Fixed

Agreed the patch and tests look great. Committed/pushed to 8.2.x and cherry-picked to 8.1.x. Thanks!

Also committed & pushed to the contrib module for Drupal 8.0: http://drupalcode.org/project/big_pipe.git/commit/f60bceb.

 Status: Fixed » Closed (fixed)

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