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

Avoid modifying XHR proxy that no longer exists #1345

Closed
wants to merge 3 commits into from
Closed

Avoid modifying XHR proxy that no longer exists #1345

wants to merge 3 commits into from

Conversation

mihalik
Copy link

@mihalik mihalik commented Feb 19, 2018

address #761

Problem

The cannot set property 'aborted' of undefined issue in #761 stems from trying to set .aborted on a proxy that no longer exists. This seems to happen quite often with things that do long-polling like Firebase. This Firebase example has random failures almost every run prior to this update.

Solution

This update checks if a proxy exists before trying to abort it. I am not 100% why the proxies no longer exist when abort() is called but this prevents an exception from being thrown in that state and allows the tests to complete successfully.

Feel free to reject if this is hiding a more fundamental issue.

@CLAassistant
Copy link

CLAassistant commented Feb 19, 2018

CLA assistant check
Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ mihalik
❌ Dustin Mihalik


Dustin Mihalik seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

@mihalik
Copy link
Author

mihalik commented Feb 19, 2018

Oops. First commit was made from a computer without my email set. Let me know if the broken CLA is an issue and I'll cleanup and re-submit.

@bahmutov
Copy link
Contributor

@mihalik hmm, the CLA rechecking does not resolve the issue - it does need an email for the contribution, also, is it possible to make a unit test for this?

@brian-mann
Copy link
Member

brian-mann commented Feb 19, 2018

An integration/e2e test would be much better - but that means being able to reproduce the issue in a controlled manner. I think you've likely pointed us in a direction that I could investigate and figure out the root cause and then come up with a test.

This is one of those times where the test is only valuable as long as it mimics the real world conditions leading up to the point of failure which is why a unit test is not that great.

Also @bahmutov the network layer rewrite would naturally fix all these problems. Mucking with host objects has its problems (as demonstrated here).

@mihalik
Copy link
Author

mihalik commented Feb 20, 2018

Yeah, I've been unable to reproduce this issue in a reliable fashion so it seemed weird to write a test that was 'do this X times' to see if it fails. I was hoping the minimal

if proxy
  proxy.aborted = true

would be safe enough but it seems like it might not be.

I'm closing this due to my CLA failing and I've started preventing this error by loading an empty page between each test. And, it seems like you have a better handle on the full solution.

@mihalik mihalik closed this Feb 20, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants