Race condition results in same CSS+JS being loaded twice: race between BigPipe's server-side dynamic asset loading and Quick Edit's client-side dynamic asset loading

I can't reproduce this.

EDIT: to be clear: great find! Thank you! But of course, we need to be able to reproduce the bug to understand the root cause, and to be able to fix it :)

We should be able to create a failing test.

All regression tests for BigPipe live at \Drupal\Tests\big_pipe\FunctionalJavascript\BigPipeRegressionTest.

Also, this does not meet the criteria for a major bug: https://www.drupal.org/core/issue-priority#major-bug — but it does meet the definition of a normal bug: https://www.drupal.org/core/issue-priority#normal-bug.

I still can't reproduce it, even if I follow your steps exactly. Sorry 😥

BigPipe determines which scripts need to be loaded on the server side. So script execution times do not affect it. I appreciate you adding #1988968: Drupal.ajax does not guarantee that "add new JS file to page" commands have finished before calling said JS, which is indeed a conceptually related issue, but it's technically impossible for that issue to cause this. Impressive association though :)

See \Drupal\big_pipe\Render\BigPipe::sendPlaceholders() for how sending BigPipe placeholders works. The class docblock at the top of \Drupal\big_pipe\Render\BigPipe explains the entire process.

@droplet+++++++++++++++

Investigating :)

I now understand the problem, and why it's hard to reproduce (and why using a particular browser may make it more likely to be reproducible): this is a race condition.

Analysis

Explained very simply: this is a race condition between BigPipe and Quick Edit.

Explained in more detail: this is a race condition on the client-side, but affected by network latency and server response times. Specifically:

1. BigPipe on the server is flushing content to the client, and on the client, big_pipe.js is checking every 50 milliseconds whether new content has arrived.
2. Quick Edit on the client is first checking which entities on the page are in-place editable (this requires DOM parsing), then determining whether those fields are in-place editable (access checking) and if so, which in-place editors scripts are necessary for them (CSS+JS for @InPlaceEditor plugins). For this, metadata needs to be available, which is first attempted to be found in localStorage, but if the metadata doesn't exist yet there, then a request is made to the server. Finally, all the CSS+JS for those in-place editor plugins is loaded, which requires a request. That's up to 2 requests, but at minimum 1 request (the latter), if the necessary metadata is cached on the client side.

The race condition then is:

1. If BigPipe on the server side already flushed the content for the comment form, and the BigPipe JS already picked it up, editor.js + ckeditor.js will already have been loaded by BigPipe. Which means that when Quick Edit runs, it will no longer load those files.
2. But if the server side didn't flush content yet, or the BigPipe JS didn't process it yet, then Quick Edit will load editor.js + ckeditor.js. The BigPipe server side doesn't ever know which additional asset libraries have been loaded on the client side though… so it never will respect JS loaded on the client side, because BigPipe on the server side isn't ever informed of changes on the client side (which also doesn't make sense, because 99% of the time BigPipe on the server side is already rendering the content to flush by the time that JS is executing).

Root cause

The root cause of this problem is not BigPipe though. The root cause is this in \Drupal\Core\Ajax\AjaxResponseAttachmentsProcessor::buildAttachmentsCommands():

    if ($css_assets) {
      $css_render_array = $this->cssCollectionRenderer->render($css_assets);
      $resource_commands[] = new AddCssCommand($this->renderer->renderPlain($css_render_array));
    }
    if ($js_assets_header) {
      $js_header_render_array = $this->jsCollectionRenderer->render($js_assets_header);
      $resource_commands[] = new PrependCommand('head', $this->renderer->renderPlain($js_header_render_array));
    }
    if ($js_assets_footer) {
      $js_footer_render_array = $this->jsCollectionRenderer->render($js_assets_footer);
      $resource_commands[] = new AppendCommand('body', $this->renderer->renderPlain($js_footer_render_array));
    }

Note that we have a specific "add CSS" command, but no specific "add JS" commands. For JS, we're simply inserting HTML. So for CSS, we have a semantically accurate command, which we can make "smarter". For JS, we do not.

If we'd have "add CSS" and "add JS" commands, then we'd be able to make Drupal.AjaxCommands.prototype.add_css and Drupal.AjaxCommands.prototype.add_js take into account CSS and JS that was just loaded already, due to a race condition between two independent pieces of logic.

Next steps

This technically belongs to the ajax system component. Would you agree?

Given the root cause, this is actually more closely related to #1988968: Drupal.ajax does not guarantee that "add new JS file to page" commands have finished before calling said JS than I would've anticipated earlier! That wouldn't resolve the race condition in this particular case (because one piece of logic is wholly client-side and the other is wholly server-side), but it'd introduce the basic infrastructure we need to solve this bug. And it would solve the race condition if the two pieces of logic determining which additional assets to load were solely JS.

Once #1988968: Drupal.ajax does not guarantee that "add new JS file to page" commands have finished before calling said JS is solved, we'd be able to further enhance the add_js command, and make it smart enough to not again load the same JS.

Per #11.

After applying the fix in https://www.drupal.org/project/drupal/issues/1988968 to allow us to attach a library as ajax, other scripts began firing twice.

We are not using Quick edit, but also disabling BigPipe fixes the race condition.

We have the .once function to prevent issues with multiple triggers, does you code use that and you're still seeing a problem?

to apply on top of #1988968-185: Drupal.ajax does not guarantee that "add new JS file to page" commands have finished before calling said JS.

Patch isn't following coding standards to minimize the diff.

@nicxvan: with the two patch is it better?

I couldn't reproduce the issue so hard to test on my end.

If there are uncertainties we can restrict possibilities, for exemple you have to load scripts through loadjs, using the ajax command or not. I started with .filter(script => !document.querySelector(`script[src="${script.src}"]`)) before going with loadjs features. But that works just as well (or doesn't work just as well :p)

I'm not sure what are the other ways to add script to the page that would cause an issue? Bigpipe uses the ajax object. just like we can only attach whole libraries and not single JS or CSS files anymore we can restrict things so that it'll work as expected. Also I question the need to load a js file twice on purpose, what's the use case?

@nod_ sorry for the delay and thanks for the quick reply. .once with the patch took care of the issue, everything is working as expected.

Rewrite for current 9.1.x, now depending on #1988968-213: Drupal.ajax does not guarantee that "add new JS file to page" commands have finished before calling said JS

Corrected to apply on latest patch on #1988968-216: Drupal.ajax does not guarantee that "add new JS file to page" commands have finished before calling said JS

Bumping to PP-2 since #1988968: Drupal.ajax does not guarantee that "add new JS file to page" commands have finished before calling said JS is PP-1.

Tha add_js command made it in

Now that Quick Edit no longer lives in Drupal core … is this really still relevant as a core bug? 🤔

Is this still a problem? 🤔