facebook / lexical

Lexical is an extensible text editor framework that provides excellent reliability, accessibility and performance.
https://lexical.dev
MIT License
17.5k stars 1.45k forks source link

Bug: #4119

Closed Alex-Github-Account closed 1 year ago

Alex-Github-Account commented 1 year ago

lexical-list/src)/LexicalListItemNode.js contains a loop at lines 300-something while (currentIndent !== indent) { which, if indent is not a number, becomes endless and freezes the browser shold be: / checking for correctness before starting dangerous loop / if (!(indent > -1) or typeof indent != 'number'){ throw error 'system error: incorrect data passed' }else{ while (currentIndent !== indent) {

Lexical version: idk lastest

Steps To Reproduce

  1. feed json with indent not been a number to lexical-list/src)/LexicalListItemNode.js
  2. it would freeze the browser in infinite loop instead of error or warn

Link to code example:

The current behavior

app containing lexical freezes

The expected behavior

a error or warning to be thrown. infinite loop is never a solution

acywatson commented 1 year ago

I think I understand the problem, but what do you mean by this?

feed json with indent not been a number to lexical-list/src)/LexicalListItemNode.js

How are you "feeding json" to that node class? Why is the JSON malformed in the first place?

Alex-Github-Account commented 1 year ago

I do

  useEffect(() => { 
        editor.update(() => { 
                    let state =  editor.parseEditorState(json)
                    editor.setEditorState(state)
                } 
        });
    }, 
    [json]);

to put data into editor (from DB) and user encountered a bug where on opening our app, browser freezes upon investigating, during freeze, I clicked 'pause execution' in browser console and it leads me to this file to this specific 'while' where 'current indent' was like -999999999999999999 so I searched this 'while' for a while and while it may not be that exact while I seen in console (it was line ~966 in browser thus I doubt), it looks exactly the same, so I pointed it out.

acywatson commented 1 year ago

yea good call - I would also probably look into how that JSON got malformed, but we can put a check in to fix this.

Alex-Github-Account commented 1 year ago

Why is the JSON malformed in the first place?

One doing programming should always. always assume that input data may be wrong. One doing good programming should assume input data may be malformed. In my particular case it was because JS '0' is ~same as 'false' and thus went to '' (empty string). But the exact reason is irrelevant. I wonder if code like one I highlighted comes from one working at facebook. Like, no sanity check in the potential infinite loop? Are we that low? Explains recent layoffs /s

Alex-Github-Account commented 1 year ago

yea good call - I would also probably look into how that JSON got malformed, but we can put a check in to fix this.

this https://github.com/facebook/lexical/issues/4104 people go different ways trying to minimize json output - since in a cloud we pay for amount of data read from DB, and as mentoined in issue, output json contains all possible default values (whyyyyyy?) unofficial minification is among the reasons leads to bugs

acywatson commented 1 year ago

Wow, I'm not sure the personal attacks are warranted here. I'd point out that you're free to build your own rich text editor and then you can make it work however you like! I'll be watching with great interest to see how far you get :)

I'll fix this for you anyway, even though you're not being very nice.

Alex-Github-Account commented 1 year ago

I'd point out that you're free to build your own rich text editor and then you can make it work however you like! I'll be watching with great interest to see how far you get :)

Thanks already did that before even contentEditable was available to use in browsers (textarea width:2px; position: absolute floating above text, imitating cursor)

acywatson commented 1 year ago

Thanks already did that before even contentEditable was available to use in browsers (textarea width:2px; position: absolute floating above text, imitating cursor)

Oh sweet! I guess that code isn't available for me to look at though? Would love to see your ideas!

Alex-Github-Account commented 1 year ago

Thanks already did that before even contentEditable was available to use in browsers (textarea width:2px; position: absolute floating above text, imitating cursor)

Oh sweet! I guess that code isn't available for me to look at though? Would love to see your ideas!

You guess wrong assuming places to share code like github existed at time when I build such editor. My mentoining of lack of contentEditable support in browsers didn't clicked for you that it was quite a long time ago? Yep there was some ideas. It was CMS I've built with website content been editable directly on front end in any place, with no competitors with such feature on market at time. Together with content-scaling support on a client while no css zoom or transform was available. We used to think and find solutions, IE 4.0 and 64mb of ram was enough for everything in right hands, and seeing how we come to browser tab eating 4gb of ram trying to render an text editor... quite sad.

acywatson commented 1 year ago

Awesome. Sounds like you're a very talented person. Thanks for filing this issue.

Alex-Github-Account commented 1 year ago

Awesome. Sounds like you're a very talented person. Thanks for filing this issue.

Don't be offended. Yes age has some perks like always been most expirienced guy in the room and having 'tried it all already decades ago', but young age is the biggest advantage no one can gain no matter what. thanks for quick responses. BTW no need a fix I fixed already in my fork.

Alex-Github-Account commented 1 year ago

@acywatson

we'd do this everywhere, which I'm not sure is practically necessary. https://github.com/facebook/lexical/pull/4120

It is simple. After rich text editing is done, JSON stored and processed out-of-control from lexical editor (in a file, DB, other apps, whatever). So we don't need checks everywhere, only in parts of code that are related/handle to importing of JSON, and they are easy to track.

acywatson commented 1 year ago

So we don't need checks everywhere, only in parts of code that are related/handle to importing of JSON, and they are easy to track.

That depends on how you define the responsibility of the library with respect to these issues. Deserialization is one of the scenarios where type-checking isn't sufficient, but, as I pointed out, another one is where the consumer of the library simply isn't using compile-time type-checking. It's not clear to me why we'd take responsibility for someone bungling their JSON, but not for them bungling a method invocation.

Alex-Github-Account commented 1 year ago

typechecking would solve if indent: -1 passed? typeof(-1) === number but still likely will cause infinite loop/issue. Typecheck is not a silver bullet, it is typesctipt marketing claim. Regarding 'too custom' importing solution - it is taken from stackoverflow answer, so I am far not the only one ended in need of it.

acywatson commented 1 year ago

Typecheck is not a silver bullet

This is exactly what I'm saying. This is why deserialization isn't the only scenario we need to worry about here.

Alex-Github-Account commented 1 year ago

@acywatson

This is exactly what I'm saying

no, you not. You said " isn't using compile-time type-checking" --> like if opposite will fix the potential problem of nan and -1. It will not (because of the reason I outlined above).