864375 - createInstance() should fallback to MozMillElement if element does not exist yet

Status: RESOLVED FIXED

(Testing Graveyard :: Mozmill, defect, P2)

Description

[Andreea Matei [:AndreeaMatei]]

This is currently a blocker for our Mozmill 2.0 release, giving that in some special cases we declare elements before they are really available on the content and that is now failing.
Applies for ID, lookup and there are other cases where we check the existence of an element and we fail with "can't access dead object".

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to Andreea Matei [:AndreeaMatei] from comment #0)
> existence of an element and we fail with "can't access dead object".

The dead object failure is for stale elements (bug 761600) and not related to this bug.

Comment 2

[Andreea Matei [:AndreeaMatei]]

Attachment: work in progress

This has fixed the problem with Lookup (added also the changes from bug 864268 and my owns) and have progressed with element ID. We no longer invoke the element there, but there are tests where after declaring the element ID, we have waitForElement() and that is failing. Still investigating that, cause with the testcase from bug 760699 is not reproducing.
Report:
http://mozmill-crowd.blargon7.com/#/functional/report/49feb8c5f96a46e9037cffdde1607189

I think is somewhere a race condition as well, because at least for testTabbedBrowsing/testOpenInBackground.js we are switching between tabs very quickly or between pages for testToolbar/testBackForwardButtons.js. If I add a sleep call, we pass waitForElement().

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 741866 [details] [diff] [review]
work in progress

Review of attachment 741866 [details] [diff] [review]:
-----------------------------------------------------------------

::: mozmill/mozmill/extension/resource/driver/mozelement.js
@@ +46,5 @@
> +    throw new Error("could not find element " + locatorType + ": " + locator);
> +  } catch (ex) {
> +      var args = {"element" : false,
> +                  "document": document };
> +      dump("document " + document.title + "\n");

I do not see a reason why we have to encapsulate all the code in a try/catch. elem is only used to determine if we have a special kind of MozElement and nothing more. That means only that part should be inside the `if (elem)` condition. If it doesn't exist we should always fallback to MozMillElement, which then should always create a new instance.

Also please check if we need a new test for this or if there is already one around. It might be good to have two of them, one for the base MozMillElement and one for a specific subclass.

@@ +82,5 @@
> +  } catch (ex) { }
> +
> +  var elem = createInstance("Lookup", expression, element, document);
> +  elem.expression = expression;
> +  return elem;

Nothing here should change. I assume it's testing code for now.

@@ +101,5 @@
> +  if (this._element) {
> +    this.isElement = true;
> +  } else {
> +    this.isElement = false;
> +  }

No change should be necessary here.

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Andreea, can we please get a new patch ASAP? This is blocking nearly all of my work. It should have priority compared to other patches for Mozmill 2. Thanks.

Comment 5

[Henrik Skupin [:whimboo][⌚️UTC+1]]

I will take this given it blocks our further work and should be in the next RC we want to release on Monday.

Comment 6

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: Patch v1

This gives us quite good results for our mozmill-tests repository. Once we handle waitForPageLoad correctly we should check how we can create elements of a specific MozMillElement subclass. But for now this handles the case when the element does not exist yet.

Comment 7

[cmtalbert]

Comment on attachment 745086 [details] [diff] [review]
Patch v1

Review of attachment 745086 [details] [diff] [review]:
-----------------------------------------------------------------

should work.

Comment 8

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Pushed to master:
https://github.com/mozilla/mozmill/commit/6d0bbcb09c443bdb2017012d346c6dd289734b15