Skip to content

Commit

Permalink
Reducing JavaScript errors during findElement(s) for IE
Browse files Browse the repository at this point in the history 
In most cases, executing JavaScript is an atomic action for the IE
driver. However, in the off chance that there is a page refresh during
the execution of JavaScript, converting the result of the script to a
JSON object for serialization might cause an error, in which case we
should return the appropriate error to the caller. One place, however,
where this might be common is executing findElement(s) across a page
navigation. This change makes the driver return the proper error in the
post-processing of a script execution. It also recognizes the
findElement(s) case, ignoring JavaScript errors in finding elements, and
returning the proper structure (an empty list or a "no such element"
error) in its place. This lets things like explicit waits retry the
find, which may succeed once the page navigation has completed and the
DOM available for querying again.
  • Loading branch information
@jimevans
jimevans committed Dec 20, 2018
1 parent 2344ff1 commit 0a7b9a2
Show file tree
Hide file tree
Showing 2 changed files with 78 additions and 33 deletions.
75 changes: 51 additions & 24 deletions cpp/iedriver/ElementFinder.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -50,10 +50,15 @@ int ElementFinder::FindElement(const IECommandExecutor& executor,
LOG(DEBUG) << "Element location strategy is CSS selectors, but "
<< "document does not support CSS selectors. Falling back "
<< "to using the Sizzle JavaScript CSS selector engine.";
return this->FindElementUsingSizzle(executor,
parent_wrapper,
criteria,
found_element);
status_code = this->FindElementUsingSizzle(executor,
parent_wrapper,
criteria,
found_element);
if (status_code != WD_SUCCESS) {
LOG(WARN) << "A JavaScript error was encountered finding elements using Sizzle.";
status_code = ENOSUCHELEMENT;
}
return status_code;
}
}

Expand All @@ -77,13 +82,17 @@ int ElementFinder::FindElement(const IECommandExecutor& executor,
status_code = script_wrapper.Execute();
if (status_code == WD_SUCCESS) {
Json::Value atom_result;
script_wrapper.ConvertResultToJsonValue(executor, &atom_result);
int atom_status_code = atom_result["status"].asInt();
Json::Value atom_value = atom_result["value"];
status_code = atom_status_code;
*found_element = atom_result["value"];
}
else {
int converted_status_code = script_wrapper.ConvertResultToJsonValue(executor, &atom_result);
if (converted_status_code != WD_SUCCESS) {
LOG(WARN) << "Could not convert return from findElements atom to JSON value";
status_code = ENOSUCHELEMENT;
} else {
int atom_status_code = atom_result["status"].asInt();
Json::Value atom_value = atom_result["value"];
status_code = atom_status_code;
*found_element = atom_result["value"];
}
} else {
// Hitting a JavaScript error with the atom is an unrecoverable
// error. The most common case of this for IE is when there is a
// page refresh, navigation, or similar, and the driver is polling
Expand All @@ -94,8 +103,8 @@ int ElementFinder::FindElement(const IECommandExecutor& executor,
// after the page transition is completed. Note carefully that this
// is an extreme hack, and has the potential to be papering over a
// very serious problem in the driver.
status_code = ENOSUCHELEMENT;
LOG(WARN) << "A JavaScript error was encountered executing the findElement atom.";
status_code = ENOSUCHELEMENT;
}
} else {
LOG(WARN) << "Unable to get browser";
Expand All @@ -118,10 +127,16 @@ int ElementFinder::FindElements(const IECommandExecutor& executor,
LOG(DEBUG) << "Element location strategy is CSS selectors, but "
<< "document does not support CSS selectors. Falling back "
<< "to using the Sizzle JavaScript CSS selector engine.";
return this->FindElementsUsingSizzle(executor,
parent_wrapper,
criteria,
found_elements);
status_code = this->FindElementsUsingSizzle(executor,
parent_wrapper,
criteria,
found_elements);
if (status_code != WD_SUCCESS) {
LOG(WARN) << "A JavaScript error was encountered finding elements using Sizzle.";
status_code = WD_SUCCESS;
*found_elements = Json::Value(Json::arrayValue);
}
return status_code;
}
}

Expand All @@ -145,11 +160,17 @@ int ElementFinder::FindElements(const IECommandExecutor& executor,
status_code = script_wrapper.Execute();
if (status_code == WD_SUCCESS) {
Json::Value atom_result;
script_wrapper.ConvertResultToJsonValue(executor, &atom_result);
int atom_status_code = atom_result["status"].asInt();
Json::Value atom_value = atom_result["value"];
status_code = atom_status_code;
*found_elements = atom_result["value"];
int converted_status_code = script_wrapper.ConvertResultToJsonValue(executor, &atom_result);
if (converted_status_code != WD_SUCCESS) {
LOG(WARN) << "Could not convert return from findElements atom to JSON value";
status_code = WD_SUCCESS;
*found_elements = Json::Value(Json::arrayValue);
} else {
int atom_status_code = atom_result["status"].asInt();
Json::Value atom_value = atom_result["value"];
status_code = atom_status_code;
*found_elements = atom_result["value"];
}
} else {
// Hitting a JavaScript error with the atom is an unrecoverable
// error. The most common case of this for IE is when there is a
Expand All @@ -159,9 +180,9 @@ int ElementFinder::FindElements(const IECommandExecutor& executor,
// this means that the error will be transitory, and will sort
// itself out once the DOM returns to normal after the page transition
// is completed. Return an empty array, and a success error code.
LOG(WARN) << "A JavaScript error was encountered executing the findElements atom.";
status_code = WD_SUCCESS;
*found_elements = Json::Value(Json::arrayValue);
LOG(WARN) << "A JavaScript error was encountered executing the findElements atom.";
}
} else {
LOG(WARN) << "Unable to get browser";
Expand Down Expand Up @@ -322,8 +343,14 @@ bool ElementFinder::HasNativeCssSelectorEngine(const IECommandExecutor& executor
browser->GetDocument(&doc);

Script script_wrapper(doc, script_source, 0);
script_wrapper.Execute();
int status_code = script_wrapper.Execute();
if (status_code != WD_SUCCESS) {
// If executing the script yields an error, then falling back to
// Sizzle will never work, so assume there is a native CSS selector
// engine.
return true;
}
return script_wrapper.result().boolVal == VARIANT_TRUE;
}

} // namespace webdriver
} // namespace webdriver
36 changes: 27 additions & 9 deletions cpp/iedriver/VariantUtilities.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -219,12 +219,21 @@ int VariantUtilities::ConvertVariantToJsonValue(IElementManager* element_manager

long length = 0;
status_code = GetArrayLength(variant_value.pdispVal, &length);
if (status_code != WD_SUCCESS) {
LOG(WARN) << "Did not successfully get array length.";
return EUNEXPECTEDJSERROR;
}

for (long i = 0; i < length; ++i) {
CComVariant array_item;
int array_item_status = GetArrayItem(variant_value.pdispVal,
i,
&array_item);
if (array_item_status != WD_SUCCESS) {
LOG(WARN) << "Did not successfully get item with index "
<< i << " from array.";
return EUNEXPECTEDJSERROR;
}
Json::Value array_item_result;
ConvertVariantToJsonValue(element_manager,
array_item,
Expand All @@ -239,15 +248,21 @@ int VariantUtilities::ConvertVariantToJsonValue(IElementManager* element_manager
ConvertVariantToJsonValue(element_manager, json_serialized, &result_object);
} else {
std::vector<std::wstring> property_names;
status_code = GetPropertyNameList(variant_value.pdispVal,
&property_names);
GetPropertyNameList(variant_value.pdispVal, &property_names);

for (size_t i = 0; i < property_names.size(); ++i) {
CComVariant property_value_variant;
GetVariantObjectPropertyValue(variant_value.pdispVal,
property_names[i],
&property_value_variant);

bool property_value_retrieved =
GetVariantObjectPropertyValue(variant_value.pdispVal,
property_names[i],
&property_value_variant);
if (!property_value_retrieved) {
LOG(WARN) << "Did not successfully get value for property '"
<< StringUtilities::ToString(property_names[i])
<< "' from object.";
return EUNEXPECTEDJSERROR;
}

Json::Value property_value;
ConvertVariantToJsonValue(element_manager,
property_value_variant,
Expand All @@ -269,7 +284,7 @@ int VariantUtilities::ConvertVariantToJsonValue(IElementManager* element_manager
// TODO: We need to track window objects and return a custom JSON
// object according to the spec, but that will require a fair
// amount of refactoring.
LOG(DEBUG) << "Returning window object from JavaScript is not supported";
LOG(WARN) << "Returning window object from JavaScript is not supported";
return EUNEXPECTEDJSERROR;
}

Expand Down Expand Up @@ -383,8 +398,11 @@ bool VariantUtilities::GetVariantObjectPropertyValue(IDispatch* variant_object_d
LOCALE_USER_DEFAULT,
&dispid_property);
if (FAILED(hr)) {
LOGHR(WARN, hr) << "Unable to get dispatch ID (dispid) for property "
<< StringUtilities::ToString(property_name);
// Only log failures to find dispid to debug level, not warn level.
// Querying for the existence of a property is a normal thing to
// want to accomplish.
LOGHR(DEBUG, hr) << "Unable to get dispatch ID (dispid) for property "
<< StringUtilities::ToString(property_name);
return false;
}

Expand Down

0 comments on commit 0a7b9a2

Please sign in to comment.