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

Chrome Extensions that mutate the DOM, NotFoundError #11774

Closed
gwing33 opened this issue Dec 5, 2017 · 4 comments
Closed

Chrome Extensions that mutate the DOM, NotFoundError #11774

gwing33 opened this issue Dec 5, 2017 · 4 comments

Comments

@gwing33
Copy link

gwing33 commented Dec 5, 2017

Do you want to request a feature or report a bug?
Bug, but maybe this is a question...

What is the current behavior?
In chrome (and I'm sure other browsers), chrome extensions can mutate your DOM willy-nilly. In turn, when React goes to update the node, we get a critical error (NotFoundError), if you don't have an Error boundary in place, your whole app blows up.

How to reproduce

What is the expected behavior?
Not sure honestly. It's annoying that the entire page crashes rather than, maybe throwing a warning type error and skipping the corrupt node (I'm afraid of the cascading effect this type of change might have).

Maybe React shouldn't cater to this type of error, in that case, what can I do about it? I have added error boundaries and they help to track these issues down, but the amount of places I have to dangerouslySetInnerHTML is getting a bit absurd, and it's making it tricky for new feature development. At this rate I might as well just be setting any user input text as dangerouslySetInnerHTML but this doesn't seem like a good solution.

It's common enough that since I've added React 16 to my project, I've fixed about 10~15 different instances, and I find one or two each week right now. They do seem to be slowing down though, but I fear this will be an ongoing fight/maintenance issue.

Which versions of React, and which browser / OS are affected by this issue? Did this work in previous versions of React?
React 16.0 - Previous version of react also had similar issues, but they didn't seem to crash like react 16 does. Thankfully react 16 does have error boundaries and is more predictable in how it handles.
Chrome latest

@gaearon
Copy link
Collaborator

gaearon commented Dec 5, 2017

Maybe there's some particular kinds of mutations we could be more resilient to. But in general, React doesn't support something else modifying its DOM nodes.

@gaearon
Copy link
Collaborator

gaearon commented Dec 5, 2017

Could you look into which kind of mutations Google Voice extension is performing?

@gwing33
Copy link
Author

gwing33 commented Dec 5, 2017

Google voice was just the easiest example and beyond this example, assume it could be any type of mutation.

I'm not sure what types of mutations you're looking for, but in this case it seems to just wrap text in a span tag and then replaces the target node's HTML with it's updated version.

For example:

// If I don't wrap this variable in a span it will cause TaskTime  component to blow up.
const nextTaskName = 'Some phone number: (123) 123-1234';
return (
    <span>
        <TaskTime dateTime={val} />{' '}
        <span>{nextTaskName}</span>
    </span>
);

Actually, going through this did help me to realize that I don't need I don't need to use the dangerouslySetInnerHTML in this case because react doesn't have a need to update a specific node within the nextTaskName.

But there have been instances (with Ring Central) where they don't just update the containing node, they replace the containing node all together so it seems to be the safest to use dangerouslySetInnerHTML just to cover my grounds.

I have seen where extensions have removed DOM nodes as well, but this seems to be more rare.

I appreciate the feedback, I haven't seen much information or discussion around this topic and I at least wanted to voice this issue to see if there was something more I could do. Thankfully React 16 nicely logs these up to our server and I catch them pretty quick, it just always sucks when a user can't use your product because of some implicit chrome extension.

@gwing33 gwing33 closed this as completed Dec 5, 2017
@gaearon
Copy link
Collaborator

gaearon commented Dec 6, 2017

This might be related to #9836. (I'm not sure.)

If more users express concern over this and point out specific mutations they'd like us to be more resilient to, we can take a look.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants