Browser parsing race condition: JSON.parse sometimes called on partially streamed BigPipe placholder replacements, causes fatal JS error

I don't understand why #3 isn't tested by the bot. Adding the needs tests tag. But if this only happens after ~30 requests it will be really complicated to write a good test for this.

+++ b/core/modules/big_pipe/js/big_pipe.js
@@ -14,37 +14,28 @@
-   * @param {HTMLScriptElement} placeholderReplacement
+   * @param {HTMLScriptElement} placeholderReplacementSignal

Is this just a rename for clarity? Because if it's not needed, I think we should keep it as it was. To make it easier to see the actual changes here.

I don't believe however that just checking textContent is enough to ensure the output of the big pipe replacement has been received completely.

This sounds indeed plausible. Probably this was fine for the less complex/elaborate cases we tested, because we didn't have >16 KB of data to arrive via JSON, and you do (since you report that it fails at column 16683).

+++ b/core/modules/big_pipe/src/Render/BigPipe.php
@@ -157,6 +157,13 @@ class BigPipe {
+  const REPLACEMENT_SIGNAL = '<script type="application/vnd.drupal-ajax" data-big-pipe-event="replacement"></script>';

@@ -599,6 +606,7 @@ protected function sendPlaceholders(array $placeholders, array $placeholder_orde
     </script>
 EOF;
+      $output .= static::REPLACEMENT_SIGNAL . "\n";
       $this->sendChunk($output);

I like this!

#4: It doesn't happen every 30 requests for certain. This is a client-side (browser) race condition. We may not be able to write test coverage for it. But we definitely should try. Something like sending 16 KB of data, but sleeping for a second between every 1 KB would hopefully trigger it.

#5: this patch needs to be rolled against 8.5 for it to get committed. Also, no bug fixes happen for 8.3 anymore. They will happen for 8.4.

I had similar issues with my project. Chrome was ok, but firefox was complaining. Manually applying the patch on the 8.4.4 fixed the issue.

@zuuperman Thank you very much for confirming! Any chance you'd be up for writing test coverage as I described in #6? :) I'd promise fast reviews!

Sry, Wim. Did not have time.

But an update. I started noticing issues when a block was not cached yet. Some blocks randomly returned empty data, leading to empty blocks displayed. Removing the patch fixes this, but brings back my firefox problem. So the patch is not completely ok.
The moment blocks are cached, the empty block is gone and big pipe replacement behaves normally.

I created a patch that works for my project. To be 100% sure, I reverted a couple of times to the original patch. Every time I do, one of the blocks is not displayed. After some debugging I noticed it's related with following code:

  function bigPipeProcessPlaceholderReplacement(index, placeholderReplacementSignal) {
    var placeholderReplacement = placeholderReplacementSignal.previousElementSibling;

    var placeholderId = placeholderReplacement.getAttribute('data-big-pipe-replacement-for-placeholder-with-id');

The block that is going wrong, is always a block that comes after a form that has autocomplete enabled. Viewing with inspector

<div role="status" aria-live="assertive" aria-relevant="additions" class="ui-helper-hidden-accessible"></div>
<script type="application/vnd.drupal-ajax" data-big-pipe-event="replacement"></script>

So the previousElementSibling is not the replacement data. See my console.log of placeholderReplacement and the found response:

Included a patch that works for me, without the use of replacement signal.

Optionally we could tweak the replacement signal version, by searching the previous elements by id. But this leads to dom queries that can be avoided.

1. +++ b/core/modules/big_pipe/js/big_pipe.es6.js
   @@ -29,7 +29,17 @@
   +          // If json is not yet streamed completely. Parsing will fail.
   +          // Remove once so that it can be processed again later.
   

   This should follow our standards for multi-line comments.

2. +++ b/core/modules/big_pipe/js/big_pipe.es6.js
   @@ -29,7 +29,17 @@
   +          return;
   

   This is a bit confusing. Perhaps some additional documentation that we're going to return early, if JSON parsing fails?

@zuuperman++
@zuuperman++
@zuuperman++

Thank you so much for your detailed analysis and updated patch, that helps a lot! Your patch makes sense too :)

I addressed #14, and in doing so, I realized this could become a lot clearer by refactoring some of the existing code. Basically, the existing if/else makes sense, but this found another possible situation to go into the "if" branch. By moving the condition logic for the "if" branch in a helper method, we can make this much easier to understand.

Curious to hear what you think @drpal.

Some minor feedback,

1. +++ b/core/modules/big_pipe/js/big_pipe.es6.js
   @@ -20,16 +20,15 @@
   +        // Mark as unprocessed so this will be retried later.
   +        // @see bigPipeProcessDocument()
   

   Multiline comment format.

2. +++ b/core/modules/big_pipe/js/big_pipe.es6.js
   @@ -46,6 +45,31 @@
   +   * @returns {Array|boolean}
   

   This should be @return instead of @returns

3. +++ b/core/modules/big_pipe/js/big_pipe.es6.js
   @@ -46,6 +45,31 @@
   +    }
   

   You don't need an else{} block here since you are returning inside the if.

Thanks! Done! ✔️

So … Needs tests …

But this bug only occurs in a browser/network race condition.

I could try to trigger it manually by completely hacking the server side to send A) a huge JSON payload, B) cut that JSON payload in half and send the first half, then pause for 2 seconds to hopefully trigger the browser, then send the second half. But that is a LOT of work and likely it would still not reliably test this (read: random test failure).

I can manually send malformed JSON and test it that way (also lots of work, but less crazy brittle). But it'd still not be testing what we actually want to test: we'd not be testing the race condition, but a branch in the JS code because we know that that JS code branch exists, i.e. whitebox testing rather than blackbox testing, i.e. an integration test testing unit behavior.

So, even though I want BigPipe to have as-solid-as-possible test coverage, and a regression test for every bug, I don't think that's actually reasonable and feasible here. Removing Needs tests.

@Wim Leers ➕1️⃣

I originally added the needs tests tag, and I agree with the reasoning in #18. Great job Wim!

Committed/pushed to 8.6.x and cherry-picked to 8.5.x. Thanks!