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

Browser tab freezes, unresponsive, plus possible memory leak with form in modal #4018

Closed
SharkFourSix opened this issue Mar 2, 2024 · 16 comments · Fixed by #4021
Closed

Comments

@SharkFourSix
Copy link

SharkFourSix commented Mar 2, 2024

Overview of the problem

Buefy version: 0.9.28
Vuejs version: 2.7.16
OS/Browser: Ubuntu 22, Firefox, Brave

Description

Whenever the modal opens and captures focus or when you start typing into the form fields, the tab will freeze.

Steps to reproduce

  1. Navigate to https://jsbin.com/baluzoquvo/edit?html,output
  2. Open your browser's "Task Manager". (If you can, mark the window to stay ontop of other windows for better access)
  3. Click on "Launch card modal (Component)" button to open the modal
  4. If you are lucky, the tab will immediately freeze. If not, simply just start typing into the input fields, pressing tab to advance to the next field. By the time you get to the "Phone" field, your tab will freeze. Another way is to click into any field the first time the modal is open. It seems like the issue has to do with field focus inside the modal?
  5. Observe the CPU and memory usage of the tab. Clicking in the unresponsive fields will also increase the memory usage even higher.

Expected behavior

Tab should not lock up or have memory spike

Actual behavior

Inspecting CPU usage in the browser's Task Manager shows memory usage growing rapidly and CPU usage stays at 100%

@wesdevpro
Copy link
Member

@kikuomax could this be related to #3992?

@wesdevpro
Copy link
Member

@kikuomax @SharkFourSix

I was able to reproduce this issue under the following conditions:
OS: Windows 10
Browser: Chrome
@: https://jsbin.com/baluzoquvo/edit?html,output
Buefy version: 0.9.28
Vuejs version: 2.7.16

Before:
image

After(I had to select a form input to get the tab to freeze and start using more CPU than necessary):
image

@kikuomax
Copy link
Collaborator

kikuomax commented Mar 2, 2024

@SharkFourSix @wesdevpro I was also able to reproduce the error on Safari and Chrome on my M1 mac.

@kikuomax
Copy link
Collaborator

kikuomax commented Mar 2, 2024

@SharkFourSix @wesdevpro It seems the validation message on a horizontally grouped field is causing the problem.

@wesdevpro
Copy link
Member

wesdevpro commented Mar 2, 2024

@SharkFourSix @wesdevpro It seems the validation message on a horizontally grouped field is causing the problem.

Interesting. How were you able to pin it down to the grouped field?

@SharkFourSix
Copy link
Author

@SharkFourSix @wesdevpro It seems the validation message on a horizontally grouped field is causing the problem.

I removed the grouped attribute and rearranged the form and the issue disappeared.

@kikuomax
Copy link
Collaborator

kikuomax commented Mar 4, 2024

Interesting. How were you able to pin it down to the grouped field?

@wesdevpro @SharkFourSix

  1. I found it froze when I focused the first or last name field and immediately moved the focus to another element. No validation message was shown before freeze.
  2. If I removed required attributes from the first and last name fields, it no longer froze.
  3. If I removed the grouped and horizontal attributes from the first and last name fields, and rearranged the form, like @SharkFourSix did, it did not freeze either.

@kikuomax
Copy link
Collaborator

kikuomax commented Mar 4, 2024

@wesdevpro @SharkFourSix So Modal does not matter to this issue, the following is kind of minimal reproduction code: I was wrong, the following code did not reproduce the problem. I was correct, but it did not happen in Buefy v0.9.2 or earlier.

<template>
    <b-field grouped horizontal label="Name">
        <b-input placeholder="First name" required></b-input>
        <b-input placeholder="Last name" required></b-input>
    </b-field>
</template>

@kikuomax
Copy link
Collaborator

kikuomax commented Mar 4, 2024

@wesdevpro @SharkFourSix I confirmed that the problem did not happen in Buefy-next v0.1.2.

@kikuomax
Copy link
Collaborator

kikuomax commented Mar 8, 2024

I confirmed that the problem did not happen in Buefy-next v0.1.2.

@wesdevpro @SharkFourSix I was wrong, it happens in Buefy-next v0.1.2 too. But the browser did not freeze thanks to Vue 3's detecting infinite updates.

@kikuomax
Copy link
Collaborator

kikuomax commented Mar 8, 2024

The following watch got infinite updates:

newMessage(value) {
if (this.parent && this.parent.hasInnerField) {
if (!this.parent.type) {
this.parent.newType = this.newType
}
if (!this.parent.message) {
this.parent.newMessage = value
}
}
}

@kikuomax
Copy link
Collaborator

kikuomax commented Mar 8, 2024

The slot of the horizontally grouped <b-field> will be wrapped in <b-field-body> when it is rendered:

<div class="field is-horizontal">
    <b-field-body :message="formattedMessage" :type="newType">
        <b-input placeholder="First name" required></b-input>
        <b-input placeholder="Last name" required></b-input>
    </b-field-body>
</div>

<b-field-body> will wrap each <b-input> in <b-field> under a <div> element:

<div class="field is-horizontal">
    <div class="field-body">
        <b-field :message="formattedMessage" :type="newType">
            <b-input placeholder="First name" required></b-input>
        </b-field>
        <b-field :type="newType">
            <b-input placeholder="Last name" required></b-input>
        </b-field>
    </div>
</div>

Please note that the above snippets are simplified to make explanation easier.

Here are steps happening in the infinite updates:

  1. When <b-input> reports a validation error, the error message is put into the newMessage data field of its immediate parent <b-field> (inner <b-field>).
  2. The inner <b-field> puts the updated newMessage value into the newMessage data field of its parent <b-field> (outer <b-field>).
  3. The outer <b-field> updates the formattedMessage computed when its newMessage data field is changed. formattedMessage produces a fresh new array every time it is updated.
  4. The outer <b-field> supplies the updated formattedMessage to the message prop of the inner <b-field>.
  5. The inner <b-field> updates its newMessage data field when the message prop is changed.
  6. The inner <b-field> puts the updated newMessage value into the newMessage data field of the outer <b-field>.
  7. The outer <b-field> updates the formattedMessage computed value because its newMessage data field is updated. Because formattedMessage produces a brand new array every time it is updated, the resulting formattedMessage is different from the previous one although containing the same message.
  8. The outer <b-field> supplies the updated formattedMessage to the message prop of the inner <b-field>.
  9. Repeat from Step 5

@SharkFourSix
Copy link
Author

  1. Repeat from Step 5

So how can the cycle be broken given that it's a property change event rather a function call?

@kikuomax
Copy link
Collaborator

kikuomax commented Mar 8, 2024

  1. Repeat from Step 5

So how can the cycle be broken given that it's a property change event rather a function call?

@SharkFourSix @wesdevpro Possible solutions are

  1. Let watch of the message prop make sure the contents of the updated value are different from those of newMessage before replacing newMessage:

    message(value) {
    this.newMessage = value
    },

  2. Let formattedMessage return newMessage as is (no new array) if newMessage is an array containing only strings:

    formattedMessage() {
    if (this.parent && this.parent.hasInnerField) {
    return '' // Message will be displayed in parent field
    }
    if (typeof this.newMessage === 'string') {
    return [this.newMessage]
    }
    let messages = []
    if (Array.isArray(this.newMessage)) {
    this.newMessage.forEach((message) => {
    if (typeof message === 'string') {
    messages.push(message)
    } else {
    for (let key in message) {
    if (message[key]) {
    messages.push(key)
    }
    }
    }
    })
    } else {
    for (let key in this.newMessage) {
    if (this.newMessage[key]) {
    messages.push(key)
    }
    }
    }
    return messages.filter((m) => { if (m) return m })
    },

The first one needs deep comparison of two arbitrary values, which is not as easy as people might think. However, JSON.stringify should be sufficient here. I like the second one, but it might be too "clever".

@kikuomax
Copy link
Collaborator

kikuomax commented Mar 9, 2024

@SharkFourSix Btw, do you have any specific reason for combining grouped and horizontal? Simply removing grouped may work around the issue:

<b-field horizontal label="Name">
    <b-input placeholder="First name" required></b-input>
    <b-input placeholder="Last name" required></b-input>
</b-field>

I found Buefy was not designed to combine grouped and horizontal. Even if we fixed the infinite recursion issue, there would be another issue that once the validation error state is set, it won't be reset after correcting the value.

@SharkFourSix
Copy link
Author

SharkFourSix commented Mar 9, 2024

@kikuomax

Well, for organizing the input fields.

I have since changed the layout because the issue would have stalled my progress 😁.

The problem along with its solution is worth a note though as I'm sure that someone else crazier than me will do the same thing at some point.

kikuomax added a commit that referenced this issue Mar 14, 2024
…h form in modal (#4018) (#4021)

* fix(lib): infinite Field updates

- Fixes the bug that `Field` caused infinite recursions when both
  `grouped` and `horizontal` attributes were `true`, and a validation
  error was set. There were circular updates of the `newMessage` data
  field and the `message` prop of the child `Field`s. To break the loop,
  `watch` of the `message` prop makes sure that the contents of the new
  value are different from the `newMessage` data field before updating
  `newMessage`. Simple identity check won't work because `message`
  receives a brand new array every time.

* test(lib): add test cases for Field with grouped and horizontal

- Adds new test cases that test `Field` with `grouped` and `horizontal`
  attributes `true`. The test would never stop due to infinite
  recursions if the previous fix were not applied.

* docs(CHANGELOG): update unreleased log

- Adds the fix of `Field` freezing due to infinite recursions.
- Explains the issue that remains after fixing the frozen Field bug.

* chore(docs): add warning on grouped and horizontal (Field)

- Adds warnings of not mixing `grouped` and `horizontal` props of
  `Field`.
kikuomax added a commit to kikuomax/buefy that referenced this issue Apr 2, 2024
…h form in modal (buefy#4018) (buefy#4021)

* fix(lib): infinite Field updates

- Fixes the bug that `Field` caused infinite recursions when both
  `grouped` and `horizontal` attributes were `true`, and a validation
  error was set. There were circular updates of the `newMessage` data
  field and the `message` prop of the child `Field`s. To break the loop,
  `watch` of the `message` prop makes sure that the contents of the new
  value are different from the `newMessage` data field before updating
  `newMessage`. Simple identity check won't work because `message`
  receives a brand new array every time.

* test(lib): add test cases for Field with grouped and horizontal

- Adds new test cases that test `Field` with `grouped` and `horizontal`
  attributes `true`. The test would never stop due to infinite
  recursions if the previous fix were not applied.

  Includes adaptation for @vue/test-utils V2:
  - `find(Component)` → `findComponent(Component)`
  - `findAll(Component)` → `findAllComponents(Component)`
  - `.at(i)` → `[i]` when accessing an item in `findAllComponents`
    results. @vue/test-utils V2 returns an ordinary array
  - `localVue` → `global: { components }`: @vue/test-utils V2 no longer
    supports `localVue`
  - `propsData` → `props`: `propsData` still works, but prepares for
    possible future deprecation

* docs(CHANGELOG): update unreleased log

- Adds the fix of `Field` freezing due to infinite recursions.
- Explains the issue that remains after fixing the frozen Field bug.

* chore(docs): add warning on grouped and horizontal (Field)

- Adds warnings of not mixing `grouped` and `horizontal` props of
  `Field`.
kikuomax added a commit to ntohq/buefy-next that referenced this issue Apr 3, 2024
…h form in modal (buefy#4018) (buefy#4021)

* fix(lib): infinite Field updates

- Fixes the bug that `Field` caused infinite recursions when both
  `grouped` and `horizontal` attributes were `true`, and a validation
  error was set. There were circular updates of the `newMessage` data
  field and the `message` prop of the child `Field`s. To break the loop,
  `watch` of the `message` prop makes sure that the contents of the new
  value are different from the `newMessage` data field before updating
  `newMessage`. Simple identity check won't work because `message`
  receives a brand new array every time.

* test(lib): add test cases for Field with grouped and horizontal

- Adds new test cases that test `Field` with `grouped` and `horizontal`
  attributes `true`. The test would never stop due to infinite
  recursions if the previous fix were not applied.

  Includes adaptation for @vue/test-utils V2:
  - `find(Component)` → `findComponent(Component)`
  - `findAll(Component)` → `findAllComponents(Component)`
  - `.at(i)` → `[i]` when accessing an item in `findAllComponents`
    results. @vue/test-utils V2 returns an ordinary array
  - `localVue` → `global: { components }`: @vue/test-utils V2 no longer
    supports `localVue`
  - `propsData` → `props`: `propsData` still works, but prepares for
    possible future deprecation

* docs(CHANGELOG): update unreleased log

- Adds the fix of `Field` freezing due to infinite recursions.
- Explains the issue that remains after fixing the frozen Field bug.

* chore(docs): add warning on grouped and horizontal (Field)

- Adds warnings of not mixing `grouped` and `horizontal` props of
  `Field`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Pending Release🔦
3 participants