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

Login page can explode #13557

Closed
turt2live opened this issue May 6, 2020 · 10 comments · Fixed by #13669
Closed

Login page can explode #13557

turt2live opened this issue May 6, 2020 · 10 comments · Fixed by #13669
Labels
T-Defect Z-Rageshake Has attached rageshake (not for log submission process) Z-Soft-Crash React soft crash caught by an error boundary

Comments

@turt2live
Copy link
Member

2020-05-06T10:18:31.994Z I newscreen login
2020-05-06T10:18:34.338Z E Failed to execute 'removeChild' on 'Node': The node to be removed is not a child of this node.
Error: Failed to execute 'removeChild' on 'Node': The node to be removed is not a child of this node.
    at us (https://riot.im/app/bundles/9f769ac7a648e2ec9507/vendors~init.js:2:1919176)
    at pc (https://riot.im/app/bundles/9f769ac7a648e2ec9507/vendors~init.js:2:1932365)
    at t.unstable_runWithPriority (https://riot.im/app/bundles/9f769ac7a648e2ec9507/vendors~init.js:2:1959256)
    at Vr (https://riot.im/app/bundles/9f769ac7a648e2ec9507/vendors~init.js:2:1877531)
    at hc (https://riot.im/app/bundles/9f769ac7a648e2ec9507/vendors~init.js:2:1930225)
    at Zs (https://riot.im/app/bundles/9f769ac7a648e2ec9507/vendors~init.js:2:1926379)
    at https://riot.im/app/bundles/9f769ac7a648e2ec9507/vendors~init.js:2:1877822
    at t.unstable_runWithPriority (https://riot.im/app/bundles/9f769ac7a648e2ec9507/vendors~init.js:2:1959256)
    at Vr (https://riot.im/app/bundles/9f769ac7a648e2ec9507/vendors~init.js:2:1877531)
    at Wr (https://riot.im/app/bundles/9f769ac7a648e2ec9507/vendors~init.js:2:1877767)
2020-05-06T10:18:34.341Z E Failed to execute 'removeChild' on 'Node': The node to be removed is not a child of this node.
Error: Failed to execute 'removeChild' on 'Node': The node to be removed is not a child of this node.
    at us (https://riot.im/app/bundles/9f769ac7a648e2ec9507/vendors~init.js:2:1919176)
    at pc (https://riot.im/app/bundles/9f769ac7a648e2ec9507/vendors~init.js:2:1932365)
    at t.unstable_runWithPriority (https://riot.im/app/bundles/9f769ac7a648e2ec9507/vendors~init.js:2:1959256)
    at Vr (https://riot.im/app/bundles/9f769ac7a648e2ec9507/vendors~init.js:2:1877531)
    at hc (https://riot.im/app/bundles/9f769ac7a648e2ec9507/vendors~init.js:2:1930225)
    at Zs (https://riot.im/app/bundles/9f769ac7a648e2ec9507/vendors~init.js:2:1926379)
    at https://riot.im/app/bundles/9f769ac7a648e2ec9507/vendors~init.js:2:1877822
    at t.unstable_runWithPriority (https://riot.im/app/bundles/9f769ac7a648e2ec9507/vendors~init.js:2:1959256)
    at Vr (https://riot.im/app/bundles/9f769ac7a648e2ec9507/vendors~init.js:2:1877531)
    at Wr (https://riot.im/app/bundles/9f769ac7a648e2ec9507/vendors~init.js:2:1877767)
2020-05-06T10:18:34.341Z E The above error occured while React was rendering the following components: 
    in input
    in form
    in div
    in Sr
    in div
    in ir
    in div
    in div
    in div
    in f
    in Login
    in Hc
    in MatrixChat
@turt2live turt2live added T-Defect Z-Rageshake Has attached rageshake (not for log submission process) Z-Soft-Crash React soft crash caught by an error boundary 🔥 Fire 🔥 labels May 6, 2020
@turt2live
Copy link
Member Author

This is new in 1.6

@turt2live turt2live added this to In Progress in Web App Team via automation May 6, 2020
@turt2live turt2live moved this from In Progress to Raging Inferno 🔥🔥🔥 in Web App Team May 6, 2020
@turt2live
Copy link
Member Author

Possibly related to #11793 ? Same stacktrace, different area. Maybe React needs updating or we're doing something anti-patternish?

@turt2live turt2live assigned turt2live and unassigned turt2live May 6, 2020
@turt2live
Copy link
Member Author

This is possibly a case of us using React.Fragment wrong, though I'm not able to reproduce the failure in any code path which triggers creation or destruction of fragments :(

Might be a race condition or some other environment-specific condition.

@bwindels
Copy link
Contributor

bwindels commented May 8, 2020

Would be great to know the actual stack trace, but looks like we don't ship sourcemaps with /app.

@t3chguy
Copy link
Member

t3chguy commented May 8, 2020

Isn't this what the new decoder ring is for?

@turt2live
Copy link
Member Author

That decoder ring isn't shipped on /app, but it could maybe be convinced to work

@jryans
Copy link
Collaborator

jryans commented May 12, 2020

A few details I've noticed so far:

  • Component Sr on the component stack is PasswordLogin, so it's something to do with a field input in the login form.
  • The code stack trace is all within React, so it's not much use.
  • Every rageshake is from Chrome 80 - 81 on Windows (as well as a few on Android), and 81 is the current stable version

@dbkr
Copy link
Member

dbkr commented May 13, 2020

It's not clear what has changed that's suddenly made this a problem, but this is due to facebook/react#11538 (closed as unfixable) / https://bugs.chromium.org/p/chromium/issues/detail?id=872770 (open but they've essentially said it's React's problem).

Options we have for fixing this:

  • Add the monkey patches that make removeNode not throw an exception. Come with some perf hit according to react and is also a terrible hack.
  • Work around the bug on a case-by-case basis by wrapping buttons in span tags. Is a very specific workaround for a very specific issue and will probably be a game of whack-a-mole as we find more elements like this)
  • Add class="notranslate" to make the whole app untranslatable to google translate. Obviously means people can't use google to translate Riot into languages we don't have i18n for (although one of the above reports is from a French speaker...).

In the course of writing this up I've switched between favouring all three solutions above. I think right now I'm on 1. since it's the closest to fixing the actual failed assumption in React that causes this. I'd be surprised if the perf hit was that much, and I would assume we could add a try/catch instead which may be less overhead (or not).

@dbkr
Copy link
Member

dbkr commented May 13, 2020

Never mind: option 1. is terrible because it just doesn't remove the element, which means in this case you get another submit button each time you press it...

Edit: that said, you can use:

    const originalRemoveChild = Node.prototype.removeChild;
    Node.prototype.removeChild = function(...args) {
        try {
            return originalRemoveChild.apply(this, args);
        } catch (e) {
            console.log("Caught exception from removeChild", e);
            return originalRemoveChild.apply(args[0].parentNode, args);
        }
    }

...which seems to work fine.

dbkr added a commit that referenced this issue May 13, 2020
Hopefully all the info for this is in the comment.

Fixes #13557
@dbkr dbkr moved this from Raging Inferno 🔥🔥🔥 to In Review in Web App Team May 13, 2020
dbkr added a commit that referenced this issue May 14, 2020
…Translate

Google Translate manipulates the DOM which is fundamentally incomaptible with
React and causes exceptions to be thrown when React tries to manipulate the DOM
based on its VDOM and the DOM methods throw exceptions because the DOM structure
is not what React thinks it is.

Riot has an i18n system, although it doesn't cover all strings and all languages.

Fixes #13557
Web App Team automation moved this from In Review to In Test May 14, 2020
@turt2live
Copy link
Member Author

For anyone who is curious: this got 26 rageshakes (for now).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-Defect Z-Rageshake Has attached rageshake (not for log submission process) Z-Soft-Crash React soft crash caught by an error boundary
Projects
None yet
5 participants