Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Fix random Capybara::Webkit::InvalidResponseError failure #595

Closed
wants to merge 4 commits into from

3 participants

Kohei Suzuki Matthew Horan Joe Ferris
Kohei Suzuki

This pull-request fixes the random Capybara::Webkit::InvalidResponseError failure described in #550.

The problem is a kind of "time of check to time of use" issue. The current webkit checks the validity of node index before actual Node command.
https://github.com/thoughtbot/capybara-webkit/blob/f35e2df0df3b2e253d78ab96afa670662f862037/lib/capybara/webkit/node.rb#L128-L134
But it could be broken in the following scenario:

  1. Check the validity of index by sending Node isAttached.
  2. The index becomes invalid by DOM mutation or asynchronous redirection, etc..
  3. Send actual Node command to the index which is no longer valid.

My pull-request makes the validity check atomic. By checking each time in JavaScript and throw NodeNotAttachedError from JavaScript, "time of check to time of use" issue should be fixed.

Matthew Horan
Collaborator

This looks great! One problem with NodeNotAttached is that it's not super informative to the user. Perhaps we could provide a more informative message to the user, e.g. "element at [index] no longer present in the DOM". Even better, providing an xpath to that element (though that may not be possible).

Thoughts, @jferris?

Kohei Suzuki

I tweaked error message as suggested by @mhoran .

I found another fix of my pull-request.
dragTo and equals command take a node index as an argument, but it isn't checked.
If an argument node index is invalid, NodeNotAttachedError should be raised.

Joe Ferris
Admin

Awesome work! Thanks for submitting.

I've rebased, squashed, and merged this as 6945ed5 on master. It will go out with the next release.

Joe Ferris jferris closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
6 lib/capybara/webkit/node.rb
View
@@ -126,11 +126,7 @@ def find_css(selector)
end
def invoke(name, *args)
- if allow_unattached_nodes? || attached?
- browser.command "Node", name, native, *args
- else
- raise Capybara::Webkit::NodeNotAttachedError
- end
+ browser.command "Node", name, allow_unattached_nodes?, native, *args
end
def allow_unattached_nodes?
36 spec/driver_spec.rb
View
@@ -2328,6 +2328,42 @@ def log
end
end
+ context 'unattached node app' do
+ let(:driver) do
+ driver_for_html(<<-HTML)
+ <html><body>
+ <p id="remove-me">Remove me</p>
+ <a href="#" id="remove-button">remove</a>
+ <script type="text/javascript">
+ document.getElementById('remove-button').addEventListener('click', function() {
+ var p = document.getElementById('remove-me');
+ p.parentNode.removeChild(p);
+ });
+ </script>
+ </body></html>
+ HTML
+ end
+
+ it 'raises NodeNotAttachedError' do
+ visit '/'
+ remove_me = driver.find_css('#remove-me').first
+ expect(remove_me).not_to be_nil
+ driver.find_css('#remove-button').first.click
+ expect { remove_me.text }.to raise_error(Capybara::Webkit::NodeNotAttachedError)
+ end
+
+ it 'raises NodeNotAttachedError if the argument node is unattached' do
+ visit '/'
+ remove_me = driver.find_css('#remove-me').first
+ expect(remove_me).not_to be_nil
+ remove_button = driver.find_css('#remove-button').first
+ expect(remove_button).not_to be_nil
+ remove_button.click
+ expect { remove_button == remove_me }.to raise_error(Capybara::Webkit::NodeNotAttachedError)
+ expect { remove_me == remove_button }.to raise_error(Capybara::Webkit::NodeNotAttachedError)
+ end
+ end
+
context "version" do
let(:driver) do
driver_for_html(<<-HTML)
2  src/FindCss.cpp
View
@@ -7,7 +7,7 @@ FindCss::FindCss(WebPageManager *manager, QStringList &arguments, QObject *paren
}
void FindCss::start() {
- InvocationResult result = page()->invokeCapybaraFunction("findCss", arguments());
+ InvocationResult result = page()->invokeCapybaraFunction("findCss", true, arguments());
finish(&result);
}
2  src/FindXpath.cpp
View
@@ -7,7 +7,7 @@ FindXpath::FindXpath(WebPageManager *manager, QStringList &arguments, QObject *p
}
void FindXpath::start() {
- InvocationResult result = page()->invokeCapybaraFunction("findXpath", arguments());
+ InvocationResult result = page()->invokeCapybaraFunction("findXpath", true, arguments());
finish(&result);
}
2  src/InvocationResult.cpp
View
@@ -24,6 +24,8 @@ ErrorMessage *InvocationResult::errorMessage() {
if (error["name"] == "Capybara.ClickFailed")
return new ErrorMessage("ClickFailed", message);
+ else if (error["name"] == "Capybara.NodeNotAttachedError")
+ return new ErrorMessage("NodeNotAttachedError", message);
else
return new ErrorMessage(message);
}
7 src/JavascriptInvocation.cpp
View
@@ -5,8 +5,9 @@
#include <QEvent>
#include <QContextMenuEvent>
-JavascriptInvocation::JavascriptInvocation(const QString &functionName, const QStringList &arguments, WebPage *page, QObject *parent) : QObject(parent) {
+JavascriptInvocation::JavascriptInvocation(const QString &functionName, bool allowUnattached, const QStringList &arguments, WebPage *page, QObject *parent) : QObject(parent) {
m_functionName = functionName;
+ m_allowUnattached = allowUnattached;
m_arguments = arguments;
m_page = page;
}
@@ -15,6 +16,10 @@ QString &JavascriptInvocation::functionName() {
return m_functionName;
}
+bool JavascriptInvocation::allowUnattached() {
+ return m_allowUnattached;
+}
+
QStringList &JavascriptInvocation::arguments() {
return m_arguments;
}
5 src/JavascriptInvocation.h
View
@@ -10,12 +10,14 @@ class InvocationResult;
class JavascriptInvocation : public QObject {
Q_OBJECT
Q_PROPERTY(QString functionName READ functionName)
+ Q_PROPERTY(bool allowUnattached READ allowUnattached)
Q_PROPERTY(QStringList arguments READ arguments)
Q_PROPERTY(QVariant error READ getError WRITE setError)
public:
- JavascriptInvocation(const QString &functionName, const QStringList &arguments, WebPage *page, QObject *parent = 0);
+ JavascriptInvocation(const QString &functionName, bool allowUnattached, const QStringList &arguments, WebPage *page, QObject *parent = 0);
QString &functionName();
+ bool allowUnattached();
QStringList &arguments();
Q_INVOKABLE void leftClick(int x, int y);
Q_INVOKABLE void rightClick(int x, int y);
@@ -31,6 +33,7 @@ class JavascriptInvocation : public QObject {
private:
QString m_functionName;
+ bool m_allowUnattached;
QStringList m_arguments;
WebPage *m_page;
QVariant m_error;
3  src/Node.cpp
View
@@ -9,7 +9,8 @@ Node::Node(WebPageManager *manager, QStringList &arguments, QObject *parent) : J
void Node::start() {
QStringList functionArguments(arguments());
QString functionName = functionArguments.takeFirst();
- InvocationResult result = page()->invokeCapybaraFunction(functionName, functionArguments);
+ QString allowUnattached = functionArguments.takeFirst();
+ InvocationResult result = page()->invokeCapybaraFunction(functionName, allowUnattached == "true", functionArguments);
finish(&result);
}
8 src/WebPage.cpp
View
@@ -143,14 +143,14 @@ bool WebPage::shouldInterruptJavaScript() {
return false;
}
-InvocationResult WebPage::invokeCapybaraFunction(const char *name, const QStringList &arguments) {
+InvocationResult WebPage::invokeCapybaraFunction(const char *name, bool allowUnattached, const QStringList &arguments) {
QString qname(name);
- JavascriptInvocation invocation(qname, arguments, this);
+ JavascriptInvocation invocation(qname, allowUnattached, arguments, this);
return invocation.invoke(currentFrame());
}
-InvocationResult WebPage::invokeCapybaraFunction(QString &name, const QStringList &arguments) {
- return invokeCapybaraFunction(name.toLatin1().data(), arguments);
+InvocationResult WebPage::invokeCapybaraFunction(QString &name, bool allowUnattached, const QStringList &arguments) {
+ return invokeCapybaraFunction(name.toLatin1().data(), allowUnattached, arguments);
}
void WebPage::javaScriptConsoleMessage(const QString &message, int lineNumber, const QString &sourceID) {
4 src/WebPage.h
View
@@ -16,8 +16,8 @@ class WebPage : public QWebPage {
public:
WebPage(WebPageManager *, QObject *parent = 0);
- InvocationResult invokeCapybaraFunction(const char *name, const QStringList &arguments);
- InvocationResult invokeCapybaraFunction(QString &name, const QStringList &arguments);
+ InvocationResult invokeCapybaraFunction(const char *name, bool allowUnattached, const QStringList &arguments);
+ InvocationResult invokeCapybaraFunction(QString &name, bool allowUnattached, const QStringList &arguments);
QString failureString();
QString userAgentForUrl(const QUrl &url ) const;
void setUserAgent(QString userAgent);
68 src/capybara.js
View
@@ -20,11 +20,11 @@ Capybara = {
},
findXpathWithin: function (index, xpath) {
- return this.findXpathRelativeTo(this.nodes[index], xpath);
+ return this.findXpathRelativeTo(this.getNode(index), xpath);
},
findCssWithin: function (index, selector) {
- return this.findCssRelativeTo(this.nodes[index], selector);
+ return this.findCssRelativeTo(this.getNode(index), selector);
},
findXpathRelativeTo: function (reference, xpath) {
@@ -55,8 +55,16 @@ Capybara = {
document.evaluate("ancestor-or-self::html", this.nodes[index], null, XPathResult.FIRST_ORDERED_NODE_TYPE, null).singleNodeValue != null;
},
+ getNode: function(index) {
+ if (CapybaraInvocation.allowUnattached || this.isAttached(index)) {
+ return this.nodes[index];
+ } else {
+ throw new Capybara.NodeNotAttachedError(index);
+ }
+ },
+
text: function (index) {
- var node = this.nodes[index];
+ var node = this.getNode(index);
var type = (node.type || node.tagName).toLowerCase();
if (type == "textarea") {
return node.innerHTML;
@@ -66,35 +74,36 @@ Capybara = {
},
allText: function (index) {
- var node = this.nodes[index];
+ var node = this.getNode(index);
return node.textContent;
},
attribute: function (index, name) {
+ var node = this.getNode(index);
switch(name) {
case 'checked':
- return this.nodes[index].checked;
+ return node.checked;
break;
case 'disabled':
- return this.nodes[index].disabled;
+ return node.disabled;
break;
case 'multiple':
- return this.nodes[index].multiple;
+ return node.multiple;
break;
default:
- return this.nodes[index].getAttribute(name);
+ return node.getAttribute(name);
}
},
hasAttribute: function(index, name) {
- return this.nodes[index].hasAttribute(name);
+ return this.getNode(index).hasAttribute(name);
},
path: function(index) {
- return this.pathForNode(this.nodes[index]);
+ return this.pathForNode(this.getNode(index));
},
pathForNode: function(node) {
@@ -130,11 +139,11 @@ Capybara = {
},
tagName: function(index) {
- return this.nodes[index].tagName.toLowerCase();
+ return this.getNode(index).tagName.toLowerCase();
},
submit: function(index) {
- return this.nodes[index].submit();
+ return this.getNode(index).submit();
},
expectNodeAtPosition: function(node, pos) {
@@ -186,7 +195,7 @@ Capybara = {
},
click: function (index, action) {
- var node = this.nodes[index];
+ var node = this.getNode(index);
node.scrollIntoViewIfNeeded();
var pos = this.clickPosition(node);
CapybaraInvocation.hover(pos.relativeX, pos.relativeY);
@@ -208,7 +217,7 @@ Capybara = {
},
hover: function (index) {
- var node = this.nodes[index];
+ var node = this.getNode(index);
node.scrollIntoViewIfNeeded();
var pos = this.clickPosition(node);
@@ -218,11 +227,11 @@ Capybara = {
trigger: function (index, eventName) {
var eventObject = document.createEvent("HTMLEvents");
eventObject.initEvent(eventName, true, true);
- this.nodes[index].dispatchEvent(eventObject);
+ this.getNode(index).dispatchEvent(eventObject);
},
visible: function (index) {
- return this.isNodeVisible(this.nodes[index]);
+ return this.isNodeVisible(this.getNode(index));
},
isNodeVisible: function(node) {
@@ -237,26 +246,26 @@ Capybara = {
},
selected: function (index) {
- return this.nodes[index].selected;
+ return this.getNode(index).selected;
},
value: function(index) {
- return this.nodes[index].value;
+ return this.getNode(index).value;
},
getInnerHTML: function(index) {
- return this.nodes[index].innerHTML;
+ return this.getNode(index).innerHTML;
},
setInnerHTML: function(index, value) {
- this.nodes[index].innerHTML = value;
+ this.getNode(index).innerHTML = value;
return true;
},
set: function (index, value) {
var length, maxLength, node, strindex, textTypes, type;
- node = this.nodes[index];
+ node = this.getNode(index);
type = (node.type || node.tagName).toLowerCase();
textTypes = ["email", "number", "password", "search", "tel", "text", "textarea", "url"];
@@ -292,16 +301,16 @@ Capybara = {
},
focus: function(index) {
- this.nodes[index].focus();
+ this.getNode(index).focus();
},
selectOption: function(index) {
- this.nodes[index].selected = true;
+ this.getNode(index).selected = true;
this.trigger(index, "change");
},
unselectOption: function(index) {
- this.nodes[index].selected = false;
+ this.getNode(index).selected = false;
this.trigger(index, "change");
},
@@ -338,7 +347,7 @@ Capybara = {
},
dragTo: function (index, targetIndex) {
- var element = this.nodes[index], target = this.nodes[targetIndex];
+ var element = this.getNode(index), target = this.getNode(targetIndex);
var position = this.centerPosition(element);
var options = {
clientX: position.x,
@@ -364,7 +373,7 @@ Capybara = {
},
equals: function(index, targetIndex) {
- return this.nodes[index] === this.nodes[targetIndex];
+ return this.getNode(index) === this.getNode(targetIndex);
}
};
@@ -390,3 +399,10 @@ Capybara.UnpositionedElement = function(path, visible) {
};
Capybara.UnpositionedElement.prototype = new Error();
Capybara.UnpositionedElement.prototype.constructor = Capybara.UnpositionedElement;
+
+Capybara.NodeNotAttachedError = function(index) {
+ this.name = 'Capybara.NodeNotAttachedError';
+ this.message = 'Element at ' + index + ' no longer present in the DOM';
+};
+Capybara.NodeNotAttachedError.prototype = new Error();
+Capybara.NodeNotAttachedError.prototype.constructor = Capybara.NodeNotAttachedError;
Something went wrong with that request. Please try again.