Permalink
Browse files

Check node index atomically to prevent race condition

Fixes Capybara::Webkit::InvalidResponseError when nodes are missing.
  • Loading branch information...
1 parent a29abf7 commit 6945ed52c7a2eb48d1718341ada747a03e6f8542 @eagletmt eagletmt committed with jferris Dec 6, 2013
@@ -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?
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)
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);
}
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);
}
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);
}
@@ -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;
}
@@ -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;
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);
}
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) {
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);
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;

0 comments on commit 6945ed5

Please sign in to comment.