Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Avoid synchronous xhr by using window.sendBeacon when available #14994

Merged
merged 1 commit into from
Apr 23, 2019

Conversation

talldan
Copy link
Contributor

@talldan talldan commented Apr 16, 2019

Description

While looking at #14986 I noticed an error occurring with one of the preview tests:

DOMException: Failed to execute 'send' on 'XMLHttpRequest': 
Failed to load 'http://localhost:8889/wp-admin/admin-ajax.php': Synchronous XHR in page dismissal.

      at PostLockedModal.releasePostLock (../../http:/localhost:8889/wp-content/plugins/gutenberg/build/editor/index.js?ver=1555400628:8006:11)

Upon some investigation, it looks like this is a change in newer versions of chrome/chromium. Use of synchronous xhr requests have been disallowed in beforeunload. Not a great deal of evidence for this change, I found the stack overflow question first and then the thread on the chromium group after that:

We're using synchronous xhr requests in beforeunload to unlock a post when the page is closed. This fix detects whether the navigator.sendBeacon is available, and uses that instead to make the request.

How has this been tested?

Use browser dev tool network tab

  1. Load up a post
  2. Open chrome dev tools network tab
  3. Filter to 'admin-ajax'.
  4. Enabled the 'Preserve Logs' option.
  5. Refresh the page
  6. Check that a request to /wp-admin/admin-ajax.php was made. The request should have an action of 'wp-remove-post-lock' in its form data.

Tail logs

  1. Tail server access logs (for the docker dev env I used docker logs -f <container_id>
  2. Load up a post in your browser
  3. Close the browser tab
  4. Check that there's a request to /wp-admin/admin-ajax.php in the logs when closing the tab (when I tested I added a temporary query string to the url to verify that it was the right request in the logs).

Other ways of testing.

  • Trigger the beforeUnload event from the browser console (window.dispatchEvent(new Event('beforeunload'));) and monitor the network requests
  • Use a network sniffer like Charles to catch the requests.

Types of changes

Bug fix (non-breaking change which fixes an issue)

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.

@talldan talldan added the [Type] Task Issues or PRs that have been broken down into an individual action to take label Apr 16, 2019
@talldan talldan self-assigned this Apr 16, 2019
@talldan
Copy link
Contributor Author

talldan commented Apr 16, 2019

Also seeing this error message appear in my browser console when refreshing the page since upgrading to Chrome 73.0.3683.103.

@talldan talldan added this to the 5.5 (Gutenberg) milestone Apr 16, 2019
@talldan talldan marked this pull request as ready for review April 16, 2019 10:38
@ocean90
Copy link
Member

ocean90 commented Apr 16, 2019

@talldan
Copy link
Contributor Author

talldan commented Apr 18, 2019

Not sure why the CI jobs aren't running for this PR. Will try some force pushing/rebasing to see if I can trigger it.

@talldan talldan force-pushed the fix/synchronous-xhr-in-beforeunload branch from 9158bcc to 99526fd Compare April 18, 2019 00:43
@draganescu
Copy link
Contributor

draganescu commented Apr 23, 2019

The problem is no matter what I do, with latest Chrome even, I don't see any error in the console.
Yes the StackOverflow link got updated and it states:

Update: You can again, in v73 and v74, but it's slated (for now) to go away again in v75 unless there's more pushback

This change should make us safe from the incoming updates though

@draganescu
Copy link
Contributor

In the Chromium forum:

To close the loop here: this change has been in experiment for last several months.
We didn't find any reports of problems and have seen a 7% improvement (i.e. reduction) in FCP at 75th percentile, a clear UX win. It is being turned on by default in M75

So we should merge this as bulletproofing.

Copy link
Contributor

@draganescu draganescu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested the code locally and the posts get unlock fine when the user exits.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Task Issues or PRs that have been broken down into an individual action to take
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants