Make WordPress Core

Opened 3 years ago

Closed 2 years ago

Last modified 2 years ago

#54825 closed defect (bug) (fixed)

Heartbeat minimalInterval option causing incorrect value of mainInterval

Reported by: tabrisrp's profile tabrisrp Owned by: peterwilsoncc's profile peterwilsoncc
Milestone: 6.0 Priority: normal
Severity: normal Version:
Component: Autosave Keywords: has-patch needs-testing dev-reviewed
Focuses: javascript Cc:

Description

Looking at the code of heartbeat.js, I think there is a bug in it when minimalInterval is used.

In the initialize function:

  • If options.minimalInterval is set, and is between 0 and 120, it's multiplicated by 1000 and assigned to settings.minimalInterval
  • After that, if settings.minimalInterval is superior to settings.mainInterval (which is likely always the case with the multiplication), the new value of settings.mainInterval is settings.minimalInterval
  • And then a bit further down, the value of settings.mainInterval is multiplied by 1000 to convert to microseconds. But it was already the case when using options.minimalInterval, so the value ends up being incorrect 120 000 000 microseconds instead of the expected 120 000 ms

As settings.minimalInterval is used against values in microseconds in other parts, a solution could be to do the following when assigning the value to mainInterval:

if ( settings.minimalInterval && settings.mainInterval < settings.minimalInterval ) {
	settings.mainInterval = settings.minimalInterval / 1000;
}

Attachments (1)

54825.diff (1.0 KB) - added by peterwilsoncc 2 years ago.

Download all attachments as: .zip

Change History (9)

#1 @sabernhardt
3 years ago

  • Component changed from General to Autosave
  • Focuses javascript added

This ticket was mentioned in PR #2361 on WordPress/wordpress-develop by Tabrisrp.


2 years ago
#2

  • Keywords has-patch added

Move the minimalInterval valu conversion to microseconds later to avoid setting an incorrect value of mainInterval when mainInterval is inferior to minimalInterval

Trac ticket: https://core.trac.wordpress.org/ticket/54825

#3 @tabrisrp
2 years ago

  • Keywords needs-testing added

#4 @SergeyBiryukov
2 years ago

  • Milestone changed from Awaiting Review to 6.0

#5 @audrasjb
2 years ago

  • Keywords dev-feedback added

The PR provided by @tabrisrp looks good to me and fixes the underlined issue.
If think it could use a second pair of eyes, just to check the overall logic, then we can ship it.

@peterwilsoncc
2 years ago

#6 @peterwilsoncc
2 years ago

  • Keywords dev-reviewed added; dev-feedback removed

54825.diff is @tabrisrp's pull request with the code moved a few lines to group the conversions to milliseconds together.

@audrasjb I agree, the logic looks good. I've marked dev reviewed as a result.

#7 @peterwilsoncc
2 years ago

  • Owner set to peterwilsoncc
  • Resolution set to fixed
  • Status changed from new to closed

In 53226:

Autosave: Compare heartbeat intervals in same unit.

Move the conversion of minimalInterval to milliseconds to be grouped with the other time conversions from seconds to milliseconds.

This fixes a bug in which the minimal and main intervals were compared after the former had been converted to milliseconds but the latter had not. This could cause the heatbeat interval to blow out to unexpectedly high values if the minimal interval was set.

Props tabrisrp, sabernhardt, SergeyBiryukov, audrasjb.
Fixes #54825.

peterwilsoncc commented on PR #2361:


2 years ago
#8

Committed in https://core.trac.wordpress.org/changeset/53226 with some slight shuffling of the code.

Note: See TracTickets for help on using tickets.