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

Fallback to legacy set/get in old versions of FF #6930

Merged
merged 1 commit into from
Jun 2, 2016

Conversation

andrewimm
Copy link
Contributor

#5746 introduced some issues with older (<22) versions of Firefox that prevent inputs from being properly updated, and fails to fire componentDidMount and any ref callbacks.

In these old versions of Firefox, accessing the property descriptor of a prototype calculated the value, which caused it to throw exceptions in certain cases related to the DOM (see: https://bugzilla.mozilla.org/show_bug.cgi?id=520882). In this specific case, fetching the value of an input or textarea would fail with NS_ERROR_XPC_BAD_OP_ON_WN_PROTO, which cascaded upwards, eventually getting caught by the transaction queue. This would prevent any other enqueued actions, such as running componentDidMount or establishing refs.

This fix uses Firefox's deprecated __lookupSetter__ and __lookupGetter__ methods as a fallback, which are supported in all versions where the getOwnPropertyDescriptor will fail. It has been spot tested on Firefox 4, 17, 21, and 46: all managed to run all lifecycle hooks and establish refs, and were able to set and update the value of inputs.

One remaining question is what to do when the catch block is reached and the lookup methods don't exist. In this case, descriptor will be undefined and will break below. This actually seems like reasonable behavior to me, because if we get that far we're in a bizarre, unsupported browser environment, and nothing should work.

@jimfb jimfb self-assigned this Jun 1, 2016
@jimfb jimfb merged commit 8216125 into facebook:master Jun 2, 2016
@jimfb
Copy link
Contributor

jimfb commented Jun 2, 2016

I'll start doing a sync now, so we can get this internally.

cc @spicyj

@sophiebits
Copy link
Collaborator

Wait. Should we just polyfill getOwnPropertyDescriptor better? es5-sham seems to do something with lookupGetter or lookupSetter:

https://github.com/es-shims/es5-shim/blob/0001a373c540b47701043b5ff476b8e9179acea7/es5-sham.js#L142

I don't think we should keep this fix.

@andrewimm
Copy link
Contributor Author

@spicyj that'd certainly avoid this from happening again. Happy to reconsider

@jimfb
Copy link
Contributor

jimfb commented Jun 2, 2016

@andrewimm Want to submit a followup PR and then I'll proceed with the sync?

@sophiebits
Copy link
Collaborator

Yeah, fine to sync this in. Looks like we don't even polyfill getOwnPropertyDescriptor in www?

@andrewimm
Copy link
Contributor Author

Hmm, so should we move ahead with this and consider the value of a more permanent polyfill down the road? We have only 2 instances of using it in this codebase, and this is the only one that references a native property.

@sophiebits
Copy link
Collaborator

No, I don't want to ship this in a React release.

@zpao
Copy link
Member

zpao commented Jun 6, 2016

What's happening here? I'll hold off cherry-picking to stable but want to make sure we don't forget about following up.

@sophiebits
Copy link
Collaborator

We're fixing our polyfill internally (D3382014) so we should be good to revert this.

@sophiebits
Copy link
Collaborator

Revert in #6976.

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

Successfully merging this pull request may close these issues.

4 participants