Problem/Motivation

After BigPipe is enabled, the scripts added twice into page that will cause the script executes twice.

reproduce:
- @see #5
- Enables BigPipe and locate to any content node/*

Proposed resolution

Excluded loaded scripts on further page loading.

Remaining tasks

- (Needs to idenify if it affected CSS)
- Patch

User interface changes

-

API changes

-

Data model changes

-

Members fund testing for the Drupal project. Drupal Association Learn more

Comments

droplet created an issue. See original summary.

Wim Leers’s picture

Title: BigPipe re-add the scripts twice » BigPipe adds scripts twice
Status: Active » Postponed (maintainer needs more info)
Issue tags: +Needs steps to reproduce

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 :)

Wim Leers’s picture

Issue tags: +Needs tests

We should be able to create a failing test.

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

Wim Leers’s picture

Priority: Major » Normal

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.

droplet’s picture

Status: Postponed (maintainer needs more info) » Active
FileSize
556.48 KB
41.87 KB

Hmm.. So frustrated... I can't reproduce it 100% now.... But.. I can reproduce with simplytest.me without patch.

23 hrs left: https://d2ikq.ply.st

Here's what I do (recall from my memory):
1. https://simplytest.me/project/drupal/8.5.x
2. Install
3. Enable big pipe
4. Create a new node/1
5. reload page
6. add new comment
7. reload page
8. hit the bug.

Note 1: Clear browser caches no help
Note 2: Clear Drupal caches no help
Note 3: Clear Cookies helps but after login reload the page few time, the bug comes back
Note 4: I created node/2 and can't reproduce the bug :S
Note 5: The first time I found the bug in my local env, which is disabled "Aggregate JavaScript files" all time. (using settings.local.php)
Note 6: I've tested on private mode without any plugins

I don't know how bigPipe works. Could the script executions time affect it? the Ajax call posting `ajax_page_state[libraries]` data

Attached network .har

Hope it helps :)

Wim Leers’s picture

Status: Active » Postponed (maintainer needs more info)

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’s picture

BigPipe determines which scripts need to be loaded on the server side.

But the 2nd loads come from /quickedit/attachments?_wrapper_format=drupal_ajax. Look at the HAR, ajax_page_state[libraries] didn't include `ckeditor*`.

** HAR captured from a non-patch env. :s

Is it possible the Quickedit Ajax request sent before BigPipe Ajax (bigPipeProcessPlaceholderReplacement)?

If the assumption is true, we need to check the drupalSetting.ajaxPageState before script insertion. (Or change big_pipe.js weights)

Let me see if I can delay the script loading with proxy tools.

droplet’s picture

Issue summary: View changes
FileSize
499.5 KB

Cool I can reproduce it with my macbook also. Both Chrome & (fresh installed) Firefox also this time.

Keeping press `Command + Shift + R` to reload the page. And try to clean drupal caches and create a new node and keeping reload until catch it.


and my assumption is correct. the ajax `attachments` called before others.

Non BigPipe:

Page Rendering (have correct data in drupalSetting) -> `attachments` ajax

BigPipe:

non-buggy path:
Page Rendering (part of drupalSetting) -> Keep to render and merge setting (async) -> `attachments` ajax (async)

buggy path:
Page Rendering (part of drupalSetting) -> `attachments` ajax (async) -> Keep to render and merge setting (async)

By theory, it makes sense now.

droplet’s picture

FileSize
1.12 KB

Cool. Apply the patch and find an auto-reload browser extension and set `Preserve log` in your console.

For some reason, it's easier to reproduce the bug with Chrome Canary Build. (But my Firefox is a stable version)

Version 62.0.3194.0 (Official Build) canary (64-bit)

Add `sleep(2)` to a particular point that delay sending data to frontend in BigPipe PHP side would easier to reproduce it I thought. BigPipe guys turn.

Wim Leers’s picture

Assigned: Unassigned » Wim Leers
Status: Postponed (maintainer needs more info) » Active

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

Investigating :)

Wim Leers’s picture

Title: BigPipe adds scripts twice » [PP-1] 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
Version: 8.5.x-dev » 8.4.x-dev
Assigned: Wim Leers » Unassigned

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.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.4 was released on January 3, 2018 and is the final full bugfix release for the Drupal 8.4.x series. Drupal 8.4.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.5.0 on March 7, 2018. (Drupal 8.5.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.5.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.