904577 - Method exists() from elementslib.Elem returns true even after the node has been removed

Status: RESOLVED FIXED

(Testing Graveyard :: Mozmill, defect)

Description

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: testCase.js

Elements instantiated using elementslib.Elem will catch the node and the exists method will return true even after the element was removed from DOM.
See the test case attached.

Comment 1

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

This looks like a Mozmill specific bug. Cosmin, does that happen with both Mozmill 1.5 and 2.0?

Comment 2

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

Comment on attachment 789546 [details]
testCase.js

>  var linkNode = controller.tabs.activeTab.getElementsByTagName('a')[0];
>  var link = new elementslib.Elem(linkNode);
>
>  mozmill.utils.waitFor(function () {
>    return link.exists();
>  }, "Link has been found");
>
>  var linkParent = link.getNode().parentNode;
>  linkParent.removeChild(link.getNode());
>
>  mozmill.utils.waitFor(function () {
>    return !link.exists();
>  }, "Link has not been found");
>}

Comment 3

[Cosmin Malutan, [:cosmin-malutan]]

(In reply to Henrik Skupin (:whimboo) from comment #1)
> This looks like a Mozmill specific bug. Cosmin, does that happen with both
> Mozmill 1.5 and 2.0?
Yes it happens with both mozmill.

Comment 4

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

Ok, so nothing which blocks us from releasing 2.0 here. Just in fact that this is a long-time malfunction. We can try to get it for 2.1.

Comment 5

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch_v1.0

As for mozelements that are  instances of elemenstlib.Elem the locator is the element we have to check if node is still attached to DOM for DOM-elements or if they have parentBox attribute on their boxObject attribute for XUL-elements.
I also added a mutt test to check this functionality. 
I run all mutt tests on all machines.
I will come with test-run reports ASAP.

Comment 6

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

Comment on attachment 803090 [details] [diff] [review]
patch_v1.0

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

::: mozmill/mozmill/extension/resource/driver/mozelement.js
@@ +125,5 @@
> +        // Check if element is XUL element
> +        if (element instanceof Components.interfaces.nsIDOMXULElement) {
> +          // Check if XULElement node has been removed by checking if he has a parentBox
> +          if (element.boxObject && !element.boxObject.parentBox)
> +            element = undefined;

Also elements in chrome space have an ownerDocument. So why aren't you handling both cases identical? I don't like that way, bot the other one with contains().

@@ +130,5 @@
> +        }
> +        else {
> +          // Check if DOM node has been removed by checking if he is still attached to DOM
> +          if (element.ownerDocument && !element.ownerDocument.contains(element))
> +            element = undefined;

We always put brackets around if conditions. Please check our coding-styles.

::: mutt/mutt/tests/js/testElementsLib/manifest.ini
@@ +6,4 @@
>  [testMozElement.js]
>  [testMozElementWindow.js]
>  [testRadioButtons.js]
> +[testRemoveElements.js]

This should be named with a correlation to the class. I would suggest testMozElementElem.js.

::: mutt/mutt/tests/js/testElementsLib/testRemoveElements.js
@@ +3,5 @@
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +const TEST_DATA = [
> +  "chrome://mozmill/content/test/chrome_elements.xul",
> +  "https://mozqa.com/data/firefox/tabbedbrowsing/data_url.html"

Don't use an external URL. We have local testcases available.

@@ +17,5 @@
> +
> +  var menulistNode =  controller.tabs.activeTab.getElementById('menulist');
> +  var menulist = new elementslib.Elem(menulistNode);
> +
> +  assert.ok(menulist.exists(), "Element has been found !");

Which element has been found? Also please no planking (space before the exclamation mark).

@@ +29,5 @@
> +  var link = new elementslib.Elem(linkNode);
> +
> +  assert.ok(link.exists(), "Link has been found");
> +  link.getNode().parentNode.removeChild(link.getNode());
> +  assert.ok(!link.exists(), "Link has not been found");

Mostly identical code here. If we can get this element via id too, we can have a loop over enhanced TEST_DATA elements.

Comment 7

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch_v1.1

(In reply to Henrik Skupin (:whimboo) from comment #6)
> Also elements in chrome space have an ownerDocument. So why aren't you
> handling both cases identical? I don't like that way, bot the other one with
> contains().
I don't know how I missed that, I guess I handled the xul elements first, and when I found the solution for DOM nodes I didn't check if it applies for XUL elements too. Thanks for catch, you were right it works fine. 
> > +          if (element.ownerDocument && !element.ownerDocument.contains(element))
> > +            element = undefined;
> 
> We always put brackets around if conditions. Please check our coding-styles.
I updated my patch to use brackets, but on https://developer.mozilla.org/en-US/docs/Mozmill_Tests/Mozmill_Style_Guide it says that we should't add brackets for single-line blocks, so should I update the wiki page ?
> Mostly identical code here. If we can get this element via id too, we can
> have a loop over enhanced TEST_DATA elements.
Did that too thanks.

Comment 8

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

(In reply to Cosmin Malutan from comment #7)
> > We always put brackets around if conditions. Please check our coding-styles.
> I updated my patch to use brackets, but on
> https://developer.mozilla.org/en-US/docs/Mozmill_Tests/Mozmill_Style_Guide
> it says that we should't add brackets for single-line blocks, so should I
> update the wiki page ?

We are in Mozmill code here, not mozmill-tests.

Comment 9

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

Comment on attachment 813709 [details] [diff] [review]
patch_v1.1

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

::: mozmill/mozmill/extension/resource/driver/mozelement.js
@@ +120,4 @@
>      if (elementslib[this._locatorType]) {
>        this._element = elementslib[this._locatorType](this._document, this._locator);
>      } else if (this._locatorType == "Elem") {
> +      // Check if node has been removed by checking if he is still attached to DOM

'he' is not used for things. In those cases it's always 'it'.

@@ +120,5 @@
>      if (elementslib[this._locatorType]) {
>        this._element = elementslib[this._locatorType](this._document, this._locator);
>      } else if (this._locatorType == "Elem") {
> +      // Check if node has been removed by checking if he is still attached to DOM
> +      if (this._locator.ownerDocument && !this._locator.ownerDocument.contains(this._locator)) {

So what if the element is already a document? If it has been replaced by a reload of the page, we would miss that it has been deleted. Please also enhance the mutt test for that. Same applies to the window as element.

@@ +123,5 @@
> +      // Check if node has been removed by checking if he is still attached to DOM
> +      if (this._locator.ownerDocument && !this._locator.ownerDocument.contains(this._locator)) {
> +        this._element = undefined;
> +      }
> +      else {

nit: no hanging else please.

Comment 10

[Andreea Matei [:AndreeaMatei]]

(In reply to Henrik Skupin (:whimboo) from comment #8)
> (In reply to Cosmin Malutan from comment #7)
> > > We always put brackets around if conditions. Please check our coding-styles.
> > I updated my patch to use brackets, but on
> > https://developer.mozilla.org/en-US/docs/Mozmill_Tests/Mozmill_Style_Guide
> > it says that we should't add brackets for single-line blocks, so should I
> > update the wiki page ?
> 
> We are in Mozmill code here, not mozmill-tests.
Do we have a different documentation for Mozmill code style guide? If not we should create one to remove the confusions.

Comment 11

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch_v1.2

Hi, I fixed the nits changes .
(In reply to Henrik Skupin (:whimboo) from comment #9)
> Comment on attachment 813709 [details] [diff] [review]
> patch_v1.1
> @@ +120,5 @@
> >      if (elementslib[this._locatorType]) {
> >        this._element = elementslib[this._locatorType](this._document, this._locator);
> >      } else if (this._locatorType == "Elem") {
> > +      // Check if node has been removed by checking if he is still attached to DOM
> > +      if (this._locator.ownerDocument && !this._locator.ownerDocument.contains(this._locator)) {
> 
> So what if the element is already a document? 
We cannot instantiate an MozElem with a document or window as node.

> If it has been replaced by a reload of the page, we would miss that it has been deleted.
After a reload we have to recreate the elements or get the new nodes otherwise we get "can't access dead object".

Comment 12

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

Comment on attachment 814071 [details] [diff] [review]
patch_v1.2

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

(In reply to Cosmin Malutan from comment #11)
> > So what if the element is already a document? 
> We cannot instantiate an MozElem with a document or window as node.

Why not? When you claim that you should give a reasoning. Check the following test, which exactly makes use of it. 
 
> > If it has been replaced by a reload of the page, we would miss that it has been deleted.
> After a reload we have to recreate the elements or get the new nodes
> otherwise we get "can't access dead object".

There should be not a case when we get the dead object. We should catch that and ensure that the element is not existent anymore.

Unless we got all this solved, you will not get my r+ for it. And please refrain from new review requests until the discussion has been finished and we know what exactly has to be done.

Comment 13

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: testCase_MozElements_with_document_as_node.js

(In reply to Henrik Skupin (:whimboo) from comment #12)
> > We cannot instantiate an MozElem with a document or window as node.
> 
> Why not? When you claim that you should give a reasoning. Check the
> following test, which exactly makes use of it. 
Hi Henrik, we cannot instantiate MozElements with a document or a window object as nodes because we don't handle that type of nodes and will get TypeError: "node.localName is undefined/null", I attached a minimized test case for this. 

> There should be not a case when we get the dead object. We should catch that
> and ensure that the element is not existent anymore.
I can check if the node is attached to the DOM in a try-catch and if I get a "dead object error" to fallback at searching in DOM for current node attributes(id, class, name), but for this I would still have to find a way to check that is the same node and not a node with the same class/name or even id.
What do you think, should I continue with this approach ?

Comment 14

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

(In reply to Cosmin Malutan from comment #13)
> > Why not? When you claim that you should give a reasoning. Check the
> > following test, which exactly makes use of it. 
> Hi Henrik, we cannot instantiate MozElements with a document or a window
> object as nodes because we don't handle that type of nodes and will get
> TypeError: "node.localName is undefined/null", I attached a minimized test
> case for this. 

So please explain what we are doing differently in this test which is passing:
https://github.com/mozilla/mozmill/blob/master/mutt/mutt/tests/js/testElementsLib/testMozElementWindow.js#L14

> > There should be not a case when we get the dead object. We should catch that
> > and ensure that the element is not existent anymore.
> I can check if the node is attached to the DOM in a try-catch and if I get a
> "dead object error" to fallback at searching in DOM for current node
> attributes(id, class, name), but for this I would still have to find a way
> to check that is the same node and not a node with the same class/name or
> even id.
> What do you think, should I continue with this approach ?

Why would a simple ´===´ not work?

Comment 15

[Cosmin Malutan, [:cosmin-malutan]]

(In reply to Henrik Skupin (:whimboo) from comment #14)
> So please explain what we are doing differently in this test which is
> passing:
> https://github.com/mozilla/mozmill/blob/master/mutt/mutt/tests/js/
> testElementsLib/testMozElementWindow.js#L14
When calling "new elementslib.Elem(node)" it calls a factory method, which checks some attributes from the node, that's why my testcase is failing, I guess I missed that, sorry.

> > What do you think, should I continue with this approach ?
> 
> Why would a simple ´===´ not work?
Because nodes are objects and '===' works only on primitive types, we could use node.isEqualNode but I would have to find a way to keep the node alive trough a page refresh.

Comment 16

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

Well, then we should simply check if the document or window of the node is still valid.

Comment 17

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch_v2.0

Hi Henrik, I updated the patch, and it checks if the nodes are attached to dom, and for document and window it checks that the window hasn't been closed.

Comment 18

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

Comment on attachment 8335928 [details] [diff] [review]
patch_v2.0

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

::: mozmill/mozmill/extension/resource/driver/mozelement.js
@@ +125,5 @@
> +          // Check if node has been removed by verifying if it's still attached to the DOM
> +          if (this._locator.ownerDocument.contains(this._locator))
> +            this._element = this._locator;
> +        } else if (this._locator.ownerDocument === null ||
> +                   typeof this._locator.ownerDocument === "undefined") {

I don't see a need for null and undefined checks. The else condition can be a catch all, given that if the object doesn't exist it will be taken as false.

@@ +126,5 @@
> +          if (this._locator.ownerDocument.contains(this._locator))
> +            this._element = this._locator;
> +        } else if (this._locator.ownerDocument === null ||
> +                   typeof this._locator.ownerDocument === "undefined") {
> +          if (this._locator instanceof Ci.nsIDOMWindow && !this._locator.closed) {

Is that a check for the Window as specified via Elem()? Then please add a comment to make this understandable. Also I wouldn't combine those conditions into a single if statement, given that it makes the code hard to read. I think its better to have ?: expressions here.

@@ +135,5 @@
> +          }
> +        }
> +      } catch (e) {
> +        // If the element doesn't exist anymore it will rise a
> +        // "can't access dead object" error

What type of exception is it? We should only catch that one. Also we should set this._element to null/undefined, to indicate it doesn't exist.

::: mutt/mutt/tests/js/testElementsLib/testMozElementElem.js
@@ +1,1 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public

Personally I would be happy if we could move both tests into the general testMozElement.js test module.

@@ +19,5 @@
> +function setupModule(aModule) {
> +  aModule.controller = mozmill.getBrowserController();
> +}
> +
> +var testMozElementElem = function () {

Please stop introducing 'var' declarations again. We already removed all of them.

@@ +25,5 @@
> +    controller.open(TEST_DATA[elementData]["url"]);
> +    controller.waitForPageLoad();
> +
> +    var node = controller.tabs.activeTab.getElementById(TEST_DATA[elementData]["id"]);
> +    var element = new elementslib.Elem(node);

I would suggest you use findElement.Elem() here.

@@ +26,5 @@
> +    controller.waitForPageLoad();
> +
> +    var node = controller.tabs.activeTab.getElementById(TEST_DATA[elementData]["id"]);
> +    var element = new elementslib.Elem(node);
> +    var id = node.getAttribute("id");

This is superfluous given that you already have the id via the config.

@@ +28,5 @@
> +    var node = controller.tabs.activeTab.getElementById(TEST_DATA[elementData]["id"]);
> +    var element = new elementslib.Elem(node);
> +    var id = node.getAttribute("id");
> +
> +    assert.ok(element.exists(), "Element with id: " + id + " has been found");

I don't call this a complete test. Ensure that elem.getNode() equals node too.

@@ +29,5 @@
> +    var element = new elementslib.Elem(node);
> +    var id = node.getAttribute("id");
> +
> +    assert.ok(element.exists(), "Element with id: " + id + " has been found");
> +    element.getNode().parentNode.removeChild(element.getNode());

Here I would operate on node directly, so element stays around as reference object.

@@ +40,5 @@
> +  }
> +}
> +var testDocumentAndWindow = function () {
> +
> +  // Add event listener to wait until the tab has been closed

Please check new line usage in the above 4 lines.

@@ +51,5 @@
> +
> +  var chromeWindow = new elementslib.MozMillElement("Elem", controller.window);
> +  var chromeDocument = new elementslib.MozMillElement("Elem", controller.window.document);
> +  var document = new elementslib.MozMillElement("Elem", controller.tabs.activeTab);
> +  var tabWindow = new elementslib.MozMillElement("Elem", controller.tabs.activeTab.defaultView);

Again, please use findElement.Elem(). Also rename the last two variables to contentWindow, and contentDocument.

@@ +59,5 @@
> +  assert.ok(tabWindow.exists(), "Window has been found");
> +  assert.ok(document.exists(), "Document has been found");
> +
> +  // Close the window
> +  tabWindow.getNode().close();

You are not closing the window here, it's the tab.

@@ +68,5 @@
> +  } finally {
> +    controller.window.removeEventListener("TabClose", checkTabClosed);
> +  }
> +  // Spin the event loop once to be sure the window gets removed
> +  controller.sleep(0);

Why? That should not be necessary with the event listener.

@@ +73,5 @@
> +
> +  assert.ok(chromeWindow.exists(), "Chrome window element has been found");
> +  assert.ok(chromeDocument.exists(), "Chrome document element has been found");
> +  assert.ok(!tabWindow.exists(), "Window has not been found");
> +  assert.ok(!document.exists(), "Document has not been found");

Shouldn't we check that no exceptions are thrown for dead objects?

Comment 19

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch_v3.0

(In reply to Henrik Skupin (:whimboo) from comment #18)
> > +  var tabWindow = new elementslib.MozMillElement("Elem", controller.tabs.activeTab.defaultView);
> 
> Again, please use findElement.Elem(). Also rename the last two variables to
> contentWindow, and contentDocument.
I cannot use findElement.Elem() for window and document objects, see comment #15

> > +  assert.ok(!document.exists(), "Document has not been found");
> 
> Shouldn't we check that no exceptions are thrown for dead objects?
No, because we handle the exception in the try-catch and the node returned inside exists method is undefined.

Comment 20

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

Comment on attachment 8343714 [details] [diff] [review]
patch_v3.0

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

(In reply to Cosmin Malutan from comment #19)
> (In reply to Henrik Skupin (:whimboo) from comment #18)
> > > +  var tabWindow = new elementslib.MozMillElement("Elem", controller.tabs.activeTab.defaultView);
> > 
> > Again, please use findElement.Elem(). Also rename the last two variables to
> > contentWindow, and contentDocument.
> I cannot use findElement.Elem() for window and document objects, see comment
> #15

Ok, this is bug 956753. We might want to update this line later.

::: mozmill/mozmill/extension/resource/driver/mozelement.js
@@ +126,5 @@
> +          if (this._locator.ownerDocument.contains(this._locator))
> +            this._element = this._locator;
> +        } else {
> +          // If element is an instance of window we check if it's closed
> +          let exists = this._locator instanceof Ci.nsIDOMWindow && !this._locator.closed;

This module uses mostly 'var' declarations. Lets do it here too.

@@ +131,5 @@
> +
> +          // If element is an instance of document we check if it's attached to a window
> +          exists = (exists) ? exists :
> +                              this._locator instanceof Ci.nsIDOMDocument &&
> +                              !this._locator.defaultView.closed

Please separate this check out to its own line similar for nsIDOMWINDOW.

::: mutt/mutt/tests/js/testElementsLib/testMozElement.js
@@ +4,4 @@
>  
>  const BASE_URL = collector.addHttpResource("../../data/");
>  const TEST_DATA = BASE_URL + "form.html";
> +const ELEMENTS = {

Please separate this constant to its own block.

@@ +42,5 @@
>                 "Loaded URL has to be equal to the initially loaded one.");
>  };
> +
> +
> +var testMozElementElem = function () {

Please don't use the var declaration method.

@@ +43,5 @@
>  };
> +
> +
> +var testMozElementElem = function () {
> +  for (var elementData in ELEMENTS) {

Please use ELEMENTS.forEach().

@@ +73,5 @@
> +
> +  // Add event listener to wait until the tab has been closed
> +  var self = { closed: false };
> +  function checkTabClosed() { self.closed = true; }
> +  controller.window.addEventListener("TabClose", checkTabClosed);

Please move those lines down to the lines when they are used.

@@ +90,5 @@
> +  assert.ok(contentWindow.exists(), "Content window has been found");
> +  assert.ok(contentDocument.exists(), "Content document has been found");
> +
> +  // Close the window
> +  contentWindow.getNode().close();

You do not close a window here but the tab.

@@ +101,5 @@
> +    controller.window.removeEventListener("TabClose", checkTabClosed);
> +  }
> +
> +  assert.ok(chromeWindow.exists(), "Chrome window element has been found");
> +  assert.ok(chromeDocument.exists(), "Chrome document element has been found");

This is a non-complete check for chrome elements. I would suggest to separate out chrome checks into its own test method, and operate on a real browser window.

Comment 21

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

No update recently. So this bug will not be taken for version 2.1. We might want to re-evaluate for 2.2.

Comment 22

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch_v3.1

I moved the windows checks in a different tests, and I made use of fat arrow functions.

Comment 23

[Andrei Eftimie]

Comment on attachment 8399906 [details] [diff] [review]
patch_v3.1

This is mozmill code. Changing review flag as appropriate.

Comment 24

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

Comment on attachment 8399906 [details] [diff] [review]
patch_v3.1

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

::: mozmill/mozmill/extension/resource/driver/mozelement.js
@@ +127,5 @@
> +            this._element = this._locator;
> +        } else {
> +          // If element is an instance of window we check if it's closed
> +          var window = this._locator instanceof Ci.nsIDOMWindow &&
> +                       !this._locator.closed;

This variable does not hold a window but its open state.

@@ +131,5 @@
> +                       !this._locator.closed;
> +
> +          // If element is an instance of document we check if it's attached to a window
> +          var document = this._locator instanceof Ci.nsIDOMDocument &&
> +                         !this._locator.defaultView.closed

Same for document.

I think it would be better to combine all that with the outer if/else block, so we have distinct blocks for each type.

::: mutt/mutt/tests/js/testElementsLib/testMozElement.js
@@ +69,5 @@
> +                                 " has not been found");
> +  });
> +}
> +
> +function testDocument() {

testContentDocumentAndWindow?

@@ +75,5 @@
> +  controller.mainMenu.click("#menu_newNavigatorTab");
> +  controller.waitForPageLoad();
> +
> +  let contentDocument = new elementslib.MozMillElement("Elem", controller.tabs.activeTab);
> +  let contentWindow = new elementslib.MozMillElement("Elem", controller.tabs.activeTab.defaultView);

No need for the content prefix. Please use findElement, which works now.

@@ +83,5 @@
> +
> +
> +
> +
> +  try {

why such a big amount of empty lines above?

@@ +103,5 @@
> +  assert.ok(!contentWindow.exists(), "Content window has not been found");
> +  assert.ok(!contentDocument.exists(), "Content document has not been found");
> +}
> +
> +function testWindow() {

testChromeDocumentAndWindow?

@@ +104,5 @@
> +  assert.ok(!contentDocument.exists(), "Content document has not been found");
> +}
> +
> +function testWindow() {
> +  let initialWindows = mozmill.utils.getWindows("navigator:browser").length;

Those are not the initial windows but a counter. Please make sure to stay close with the naming as what the variable contains.

@@ +116,5 @@
> +    return windows.length === (initialWindows + 1);
> +  }, "A new window has been opened");
> +
> +  // Get the controller of the new opened window
> +  controller = mozmill.getBrowserController();

This will overwrite the original controller.

@@ +119,5 @@
> +  // Get the controller of the new opened window
> +  controller = mozmill.getBrowserController();
> +
> +  let chromeWindow = new elementslib.MozMillElement("Elem", controller.window);
> +  let chromeDocument = new elementslib.MozMillElement("Elem", controller.window.document);

No need for the chrome prefix. Also use findElement

@@ +124,5 @@
> +
> +  assert.ok(chromeWindow.exists(), "Chrome window has been found");
> +  assert.ok(chromeDocument.exists(), "Chrome document has been found");
> +
> +  // Close the tab

Wrong comment.

@@ +134,5 @@
> +  }, "A new window has been opened");
> +
> +  assert.ok(!chromeWindow.exists(), "Chrome window element has been found");
> +  assert.ok(!chromeDocument.exists(), "Chrome document element has been found");
> +}
\ No newline at end of file

I don't see a teardownModule which cares about closing additionally opened windows.

Comment 25

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch_v4.0

(In reply to Henrik Skupin (:whimboo) from comment #24)
> > +  assert.ok(!chromeWindow.exists(), "Chrome window element has been found");
> > +  assert.ok(!chromeDocument.exists(), "Chrome document element has been found");
> > +}
> \ No newline at end of file
> 
> I don't see a teardownModule which cares about closing additionally opened
> windows.
We close the new opened windows and check that they are closed in test. If we really  need code in teardown to close all windows I can add that.
Otherwise I updated everything else.

Comment 26

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

Comment on attachment 8402716 [details] [diff] [review]
patch_v4.0

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

(In reply to Cosmin Malutan from comment #25)
> We close the new opened windows and check that they are closed in test. If
> we really  need code in teardown to close all windows I can add that.
> Otherwise I updated everything else.

I think we said it a couple of times that the tests have to properly clean-up. Therefore teardownModule exists.

::: mozmill/mozmill/extension/resource/driver/mozelement.js
@@ +125,5 @@
> +          // Check if node has been removed by verifying if it's still attached to the DOM
> +          if (this._locator.ownerDocument.contains(this._locator))
> +            this._element = this._locator;
> +        } else if (this._locator instanceof Ci.nsIDOMWindow &&
> +                   !this._locator.closed){

The closed part here should not make it into the condition, which is about the type of the node. If it is closed it would unnecessarily go through the next if. So raise TypeError if it is closed.

@@ +129,5 @@
> +                   !this._locator.closed){
> +          // If element is an instance of window we check if it's closed
> +          this._element = this._locator;
> +        } else if (this._locator instanceof Ci.nsIDOMDocument &&
> +                   !this._locator.defaultView.closed) {

Same here.

Comment 27

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch_v4.1

I added the teardown module, and I separated the type from the closed condition.

Comment 28

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch_v4.2

I had an extra empty line.

Comment 29

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

Comment on attachment 8403200 [details] [diff] [review]
patch_v4.2

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

We are getting closer. Two things remaining here.

::: mozmill/mozmill/extension/resource/driver/mozelement.js
@@ +125,5 @@
> +      try {
> +        if (this._locator.ownerDocument) {
> +          // Check if node has been removed by verifying if it's still attached to the DOM
> +          if (!this._locator.ownerDocument.contains(this._locator)) {
> +            throw new TypeError("Element doesn't exists");

No need for a message here. We do not forward it but handle it internally. Same for the other two instances.

::: mutt/mutt/tests/js/testElementsLib/testMozElement.js
@@ +30,5 @@
> +    assert.waitFor(() => !mozmill.controller.windowMap.contains(windowId),
> +                   "Window '" + windowId + "' has been closed.");
> +    windows = mozmill.utils.getWindows("navigator:browser");
> +  }
> +}

Why all this extra code? Just save off the controller of the new window in a global variable, which you can then use in teardownModule() to actually close this extra window.

Comment 30

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch_v4.3

I fixed the nits, thanks for review Henrik.

Comment 31

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

Comment on attachment 8403945 [details] [diff] [review]
patch_v4.3

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

::: mutt/mutt/tests/js/testElementsLib/testMozElement.js
@@ +22,5 @@
>  
> +function teardownModule(aModule) {
> +  if (!aModule.newWindowController.window.closed) {
> +    aModule.newWindowController.window.close();
> +  }

So what happens in the following cases:

* The test failed before aModule.newWindowController has been defined
* The window is already closed. Don't we really work on a dead object here, with a TypeError raised?

@@ +119,5 @@
> +    return windows.length === (initialWindowsCount + 1);
> +  }, "A new window has been opened");
> +
> +  // Get the controller of the new opened window
> +  newWindowController = mozmill.getBrowserController();

This will define the variable in the global scope, which you don't wanna do. Make sure you define it as null in setupModule/Test. Check the following test, which is doing something similar:

http://hg.mozilla.org/qa/mozmill-tests/file/default/firefox/tests/functional/testTabbedBrowsing/testNewWindow.js

Comment 32

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch_v4.4

I changed the teardown cleaning method, and I set the controller2 as null in setupmodule.

Comment 33

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

Comment on attachment 8404661 [details] [diff] [review]
patch_v4.4

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

::: mutt/mutt/tests/js/testElementsLib/testMozElement.js
@@ +22,5 @@
> +}
> +
> +function teardownModule(aModule) {
> +  if (aModule.controller2 && aModule.controller2.window)
> +    aModule.controller2.window.close();

I don't see why we have to check for controller2.window here. If we have a controller, the window property is always present. It's not getting reset. Also wouldn't this always raise a TypeError when the window has already been closed? In such a case close() would work on a non-existent window. Have you tested each condition?

Also why the removal of the enclosing brackets around the if condition section?

Comment 34

[Cosmin Malutan, [:cosmin-malutan]]

(In reply to Henrik Skupin (:whimboo) from comment #33)
> Comment on attachment 8404661 [details] [diff] [review]
> patch_v4.4
> 
> Review of attachment 8404661 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mutt/mutt/tests/js/testElementsLib/testMozElement.js
> @@ +22,5 @@
> > +}
> > +
> > +function teardownModule(aModule) {
> > +  if (aModule.controller2 && aModule.controller2.window)
> > +    aModule.controller2.window.close();
> 
> I don't see why we have to check for controller2.window here. If we have a
> controller, the window property is always present. It's not getting reset.
This was the way it was implemented in the example you gave in comment 31.
> Also wouldn't this always raise a TypeError when the window has already been
> closed? In such a case close() would work on a non-existent window. Have you
> tested each condition?
It won't raise a TypeError, that would happen only for dom nodes, windows objects keeps a strong reference. That's why In mozmill code a I check the same if the window is closed and then I throw an exception, is not thrown by default.
I checked all the cases the window object always exists even if it was closed, so I would do:
>if (!aModule.controller2.window.closed) {
>  aModule.controller2.window.close();
>}

Comment 35

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

(In reply to Cosmin Malutan from comment #34)
> > I don't see why we have to check for controller2.window here. If we have a
> > controller, the window property is always present. It's not getting reset.
> This was the way it was implemented in the example you gave in comment 31.

Was which an example as you said. We should always think about if the code, which we have created in the past, is still up-to-date, or shouldn't get improved.

> I checked all the cases the window object always exists even if it was
> closed, so I would do:
> >if (!aModule.controller2.window.closed) {
> >  aModule.controller2.window.close();
> >}

No, this is not enough. You missed to check for a valid controller2, which could be null.

Comment 36

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch_v4.5

I changed only the teardown code, as discussed.
Thanks

Comment 37

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

https://github.com/mozilla/mozmill/commit/c305151b137dc7325a3b153e8b385389551389b7