Closed Bug 1410799 Opened 2 years ago Closed 2 years ago

Error thrown looking up web element in listener.js clickElement is not caught

Categories

(Testing :: Marionette, defect)

58 Branch
defect
Not set

Tracking

(firefox-esr52 unaffected, firefox56 unaffected, firefox57 unaffected, firefox58 fixed)

RESOLVED FIXED
mozilla58
Tracking Status
firefox-esr52 --- unaffected
firefox56 --- unaffected
firefox57 --- unaffected
firefox58 --- fixed

People

(Reporter: barancev, Assigned: ato)

Details

(Keywords: hang, regression)

Attachments

(5 files)

User Agent: Mozilla/5.0 (Windows NT 6.1; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/61.0.3163.100 Safari/537.36

Steps to reproduce:

WebDriver driver = new FirefoxDriver();
driver.get("https://www.mozilla.org/en-US/");
WebElement link = driver.findElement(By.cssSelector("a"));
driver.get("https://www.mozilla.org/en-US/firefox/");
link.click();


Actual results:

click operation hands, no exception, just a message in the browser log:

JavaScript error: chrome://marionette/content/error.js, line 172: Error: The element reference of <a> is stale; either the element is no longer attached to the DOM, it is not in the current frame context, or the document has been refreshed



Expected results:

stale element reference exception should be thrown

Used to work in 58.0a1 (2017-09-28) (64-bit), started to fail about a week ago.
There have been some staleness changes lately. I will have a look shortly to see what caused it.
Component: Untriaged → Marionette
Product: Firefox → Testing
So for me this also started to fail but over the last weekend and not a week ago. I did a regression test with mozregression and here the result:

 9:11.71 INFO: Test command result: 10 (build is bad)
 9:11.71 INFO: Narrowed inbound regression window from [7a416cb1, d9034971] (3 builds) to [7a416cb1, 60454fbd] (2 builds) (~1 steps left)
 9:11.71 INFO: No more inbound revisions, bisection finished.
 9:11.71 INFO: Last good revision: 7a416cb1c15fc4dc877d9ab09ebddc710d91ebf3
 9:11.71 INFO: First bad revision: 60454fbd0eff2d747ae71c0ffef2c69ecf692662
 9:11.71 INFO: Pushlog:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=7a416cb1c15fc4dc877d9ab09ebddc710d91ebf3&tochange=60454fbd0eff2d747ae71c0ffef2c69ecf692662

So this is also a regression from bug 1400256. Andreas, we would need your eyes on this bug. Thanks.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(ato)
Keywords: hang, regression
Henrik, that seems more accurate. The selenium python tests started to complain on 21/10/2017
Henrik, is this something we should block the 58 release on?
Flags: needinfo?(hskupin)
(In reply to Mike Taylor [:miketaylr] (58 Regression Engineering Owner) from comment #4)
> Henrik, is this something we should block the 58 release on?

Mike, this is a Marionette bug and affects testing. So you should most likely not include the testing component in your search.
Flags: needinfo?(hskupin)
(In reply to Henrik Skupin (:whimboo) from comment #5)
> (In reply to Mike Taylor [:miketaylr] (58 Regression Engineering Owner) from
> comment #4)
> > Henrik, is this something we should block the 58 release on?
> 
> Mike, this is a Marionette bug and affects testing. So you should most
> likely not include the testing component in your search.

Regression triage covers everything that has the regression keyword and `affected-NN`, where -NN is an upcoming release. If it doesn't make sense for us to look at the Testing product, then we can modify our queries. I'll bring it up at our next triage session.
Ok, I think it's a question we should ask us outside of this bug. But just one note... In the past we had situations when our flags got removed by triage, and we had to add them back. Not sure what your exact plan here is.

Regards your question, yes we have to fix it for 58. Otherwise lots of people using Selenium and geckodriver will be affected by this regression. Andreas has to look into this bug.
For the record, I will look at this today.  It sounds like it is
either related to https://bugzil.la/1410796 or to the other recent
regression we had to do with the WindowProxy not being passed to
seenEls.get.
OS: Unspecified → All
Hardware: Unspecified → All
The lines to get the web element from the known element store were
moved outside a try…catch block [1], and because the clickElement
function the content frame script testing/marionette/listener.js
does not use the new-style dispatch technique, errors are not caught
automatically.

  [1] https://searchfox.org/mozilla-central/rev/aa1343961fca52e8c7dab16788530da31aab7c8d/testing/marionette/listener.js#1307-1308
Assignee: nobody → ato
Status: NEW → ASSIGNED
Flags: needinfo?(ato)
Summary: WebDriver: stale element reference exception is not thrown → Error thrown looking up web element in listener.js clickElement is not caught
It turns out that geckodriver is also returning the wrong HTTP
status code with ‘stale element reference’ error, so I will
also have to fix that before I can write a WPT test for this.
Comment on attachment 8924237 [details]
Bug 1410799 - Catch error from element.Store#get in clickElement

https://reviewboard.mozilla.org/r/195450/#review200882

::: commit-message-ee795:4
(Diff revision 1)
> +Bug 1410799 - Catch error from element.Store#get in clickElement r=whimboo
> +
> +Because the content frame script's clickElement function uses
> +the old-style despatch technique, all code lines that have the

`dispatch`
Attachment #8924237 - Flags: review?(hskupin) → review+
Comment on attachment 8924234 [details]
Bug 1410799 - Fix NameError for cls_name

https://reviewboard.mozilla.org/r/195444/#review200896
Attachment #8924234 - Flags: review?(james) → review+
Comment on attachment 8924235 [details]
Bug 1410799 - Fix API docs for assert_error and assert_success

https://reviewboard.mozilla.org/r/195446/#review200898
Attachment #8924235 - Flags: review?(james) → review+
Comment on attachment 8924236 [details]
Bug 1410799 - Fix HTTP status for stale element reference error

https://reviewboard.mozilla.org/r/195448/#review200900
Attachment #8924236 - Flags: review?(james) → review+
Comment on attachment 8924237 [details]
Bug 1410799 - Catch error from element.Store#get in clickElement

https://reviewboard.mozilla.org/r/195450/#review200882

> `dispatch`

I actually do mean ‘despatch’ here.
Comment on attachment 8924236 [details]
Bug 1410799 - Fix HTTP status for stale element reference error

https://reviewboard.mozilla.org/r/195448/#review200938

::: testing/webdriver/src/error.rs:205
(Diff revision 1)
>              NoSuchElement => NotFound,
>              NoSuchFrame => NotFound,
>              NoSuchWindow => NotFound,
>              ScriptTimeout => RequestTimeout,
>              SessionNotCreated => InternalServerError,
> -            StaleElementReference => BadRequest,
> +            StaleElementReference => NotFound,

Hm, I think we will want to update the geckodriver change log for this.
Comment on attachment 8924238 [details]
Bug 1410799 - Test for stale elements when clicking

https://reviewboard.mozilla.org/r/195452/#review200968
Attachment #8924238 - Flags: review?(mjzffr) → review+
Pushed by atolfsen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b4b544f4aaa4
Fix NameError for cls_name r=jgraham
https://hg.mozilla.org/integration/autoland/rev/f99477c499ab
Fix API docs for assert_error and assert_success r=jgraham
https://hg.mozilla.org/integration/autoland/rev/218cfcb9dbfd
Fix HTTP status for stale element reference error r=jgraham
https://hg.mozilla.org/integration/autoland/rev/23589b9af885
Catch error from element.Store#get in clickElement r=whimboo
https://hg.mozilla.org/integration/autoland/rev/7caf96c29235
Test for stale elements when clicking r=maja_zf
You need to log in before you can comment on or make changes to this bug.