Andreea Matei [:AndreeaMatei] Reporter
	Description • 10 years ago

Started to fail today on all platforms, for Nightly en-US.

The test seems to fail at line 69, during a click:
http://hg.mozilla.org/qa/mozmill-tests/file/default/firefox/tests/remote/restartTests/testAddons_InstallAddonWithoutEULA/test1.js#l69

We might want to skip if it continues to fail this often.

	Henrik Skupin [:whimboo][⌚️UTC+2]
	Comment 1 • 10 years ago

Why is that not a P1? Have you checked for a regression range? We should do that right away if we get such massive failures. At least the one for mozilla-central, so someone could start a regression test.

	Andrei Eftimie Assignee
	Comment 2 • 10 years ago

I can reproduce this. Getting a regression range atm.

	Andrei Eftimie Assignee
	Comment 3 • 10 years ago

Regressor is bug 789261 with:
https://hg.mozilla.org/integration/mozilla-inbound/rev/82e3d6f25c5f

	Andrei Eftimie Assignee
	Comment 4 • 10 years ago

Peter could you please have a look at this failure.

The code that is failing is: https://github.com/mozilla/mozmill/blob/d8bffb97eee0a002562f2b2f77548869e77e4bc7/mozmill/mozmill/extension/resource/stdlib/EventUtils.js#L699-L700

With:
> aWindow.QueryInterface is not a function

I can manually see `QueryInterface`not being part of the window object. Just open up Web Developer and into Console type `window.QueryInterface`. This is not defined anymore after 789261 has landed.

	Andrei Eftimie Assignee
	Comment 5 • 10 years ago

We should skip this test for now.

	Andreea Matei [:AndreeaMatei] Reporter
	Comment 6 • 10 years ago

Comment on attachment 8430616 [details] [diff] [review]
skip.patch

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

Disabled:
http://hg.mozilla.org/qa/mozmill-tests/rev/540953714dbd (default)

	Peter Van der Beken [:peterv]
	Comment 7 • 10 years ago

(In reply to Andrei Eftimie from comment #4)
> I can manually see `QueryInterface`not being part of the window object. Just
> open up Web Developer and into Console type `window.QueryInterface`. This is
> not defined anymore after 789261 has landed.

QueryInterface is not exposed anymore to content, only to chrome. The test has a comment saying "this is assuming we are in chrome space", but if QueryInterface isn't available then the test isn't running in chrome and needs to be fixed. Usually that's done using SpecialPowers.

	Henrik Skupin [:whimboo][⌚️UTC+2]
	Comment 8 • 10 years ago

Andrei, why is only this single add-on test affected? What's different to the other ones?

	Andrei Eftimie Assignee
	Comment 9 • 10 years ago

(In reply to Henrik Skupin (:whimboo) from comment #8)
> Andrei, why is only this single add-on test affected? What's different to
> the other ones?

Best guess is that for other addons we return early _with_ SpecialPowers, while for this one we don't:
https://github.com/mozilla/mozmill/blob/d8bffb97eee0a002562f2b2f77548869e77e4bc7/mozmill/mozmill/extension/resource/stdlib/EventUtils.js#L681-L701

This should be easy to verify.

	Henrik Skupin [:whimboo][⌚️UTC+2]
	Comment 10 • 10 years ago

We don't have SpecialPowers in Mozmill, so no that's not the cause.

	Andrei Eftimie Assignee
	Comment 11 • 10 years ago

(In reply to Henrik Skupin (:whimboo) from comment #10)
> We don't have SpecialPowers in Mozmill, so no that's not the cause.

Yeah, seems we are not in "chrome space".

The addon we use in the failing test is Nightly Tester Tools. This is the only addon we install directly from the web[1], for the rest of the addons we install them directly from an XPI or from AMO (where we have a chrome window).

[1] https://addons.mozilla.org/en-US/firefox/addon/nightly-tester-tools/

	Andrei Eftimie Assignee
	Comment 12 • 10 years ago

This fixes the test failure. 
When I switched from using NodeCollector to findElement I got e ChromeWindow back.

The real problem might lie in NodeCollector, I'll file a new bug to investigate NC.

Testrun: http://mozmill-crowd.blargon7.com/#/remote/report/f2d338991b96dbc5b3013338f84f8334

	Henrik Skupin [:whimboo][⌚️UTC+2]
	Comment 13 • 10 years ago

Comment on attachment 8432439 [details] [diff] [review]
1.patch

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

::: firefox/lib/addons.js
@@ +1348,4 @@
>  
>      switch (type) {
>        case "install-button":
> +        elems = [findElement.Selector(root, '.install-button > a')]

You are operating on different elements here. Before it was an element with a class '.install-button'. Now it's an anchor below that element. Is that the problem?

	Andrei Eftimie Assignee
	Comment 14 • 10 years ago

(In reply to Henrik Skupin (:whimboo) from comment #13)
> Comment on attachment 8432439 [details] [diff] [review]
> 1.patch
> 
> Review of attachment 8432439 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: firefox/lib/addons.js
> @@ +1348,4 @@
> >  
> >      switch (type) {
> >        case "install-button":
> > +        elems = [findElement.Selector(root, '.install-button > a')]
> 
> You are operating on different elements here. Before it was an element with
> a class '.install-button'. Now it's an anchor below that element. Is that
> the problem?

Markup wise we have the following:
> <p class="install-button">
>   <a class="button  prominent  add installer" data-hash="sha256:632c594051f97f65de8dfa310390238838e0f539129d003ce281ce0df33b239a" href="/firefox/downloads/latest/6543/addon-6543-latest.xpi?src=dp-btn-primary">
>     <b></b>
>     <span>Add to Firefox</span>
>   </a>
> </p>
And we really want to click on the link.

This shouldn't have worked. But it does because of the way Mozmill synthesizes events. Since both the `p` and the `a` share the same area, when Mozmill computes the center of the `p` elements and triggers a `click` event there, the event actually hits the `a` element.

If you trigger a `click` event directly on the `p` element nothing happens.

	Henrik Skupin [:whimboo][⌚️UTC+2]
	Comment 15 • 10 years ago

(In reply to Andrei Eftimie from comment #14)
> If you trigger a `click` event directly on the `p` element nothing happens.

Exactly that we were doing with the nodeCollector selector. Shouldn't it work by simply updating it?

	Andrei Eftimie Assignee
	Comment 16 • 10 years ago

(In reply to Henrik Skupin (:whimboo) from comment #15)
> (In reply to Andrei Eftimie from comment #14)
> > If you trigger a `click` event directly on the `p` element nothing happens.
> 
> Exactly that we were doing with the nodeCollector selector. Shouldn't it
> work by simply updating it?

This is not the problem from this bug. The selector change is simply a nice improvement.

The problem is that we have a regular Window returned with NC, while using findElement we get a ChromeWindow returned. The change from bug 789261 is that a regular Window does not have QueryInterface. I'm still not sure _why_ NC (in this case) has a reference to a regular Window object.

	Henrik Skupin [:whimboo][⌚️UTC+2]
	Comment 17 • 10 years ago

Well, the NodeCollector is part of our mozmill-tests, and executed similarly to our tests within a Sandbox. So for security reasons this might be the problem. Does Marionette include SpecialPowers? If yes, we might want to do the same?

	Andreea Matei [:AndreeaMatei] Reporter
	Comment 18 • 10 years ago

Comment on attachment 8432439 [details] [diff] [review]
1.patch

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

This seems reasonable in order to fix the test. If there are more stuff to fix for nodeCollector, it should be a separate bug since this is for the test failure. Henrik, what do you think?

	Andrei Eftimie Assignee
	Comment 19 • 10 years ago

The difference seems to be the XrayWrapper which we miss with NC

NodeCollector:
> this._element: [object HTMLParagraphElement]
> this._element.ownerDocument: [object HTMLDocument]

ElementsLib:
> this._element: [object XrayWrapper [object HTMLParagraphElement]]
> this._element.ownerDocument: [object XrayWrapper [object HTMLDocument]]

	Andrei Eftimie Assignee
	Comment 20 • 10 years ago

And this is where NodeCollector unwraps the XrayWrapper:
http://hg.mozilla.org/qa/mozmill-tests/file/c4ecd47f1746/lib/dom-utils.js#l540

	Andrei Eftimie Assignee
	Comment 21 • 10 years ago

Seems NodeCollector has always done this.

Henrik, do you know why we need to unwrap NC's root?
I've run some tests without unwrapping NC's root and all appears fine:
http://mozmill-crowd.blargon7.com/#/functional/report/076ed54106439c677bbfdee32050fbba
http://mozmill-crowd.blargon7.com/#/remote/report/076ed54106439c677bbfdee320510395

	Henrik Skupin [:whimboo][⌚️UTC+2]
	Comment 22 • 10 years ago

No, I don't know that anymore. You might want to check the original implementation bug for NodeCollector. I think that I had a conversation with Blake at this time, and we decided to unwrap the node. It may not be appropriate anymore. If it is working we might be able to remove the unwrapping of the node.

I would propose you check that, and if questions remain you seek for needinfo from :mrbkap.

	Andrei Eftimie Assignee
	Comment 23 • 10 years ago

(In reply to Henrik Skupin (:whimboo) from comment #22)
> If it is working we might be able to remove the
> unwrapping of the node.

I agree. So far everything is looking great.
I'll run more tests across platforms and FF versions.
If all is good I'll also include that change in this patch.

	Andrei Eftimie Assignee
	Comment 24 • 10 years ago

Besides the initial changes, I've also amended NodeCollector to not unwrap the window object.

Ran multiple testruns across OSX and Windows and all looks fine.
Some functional and remote testruns on Nightly and Aurora:
http://mozmill-crowd.blargon7.com/#/functional/report/076ed54106439c677bbfdee32061155a
http://mozmill-crowd.blargon7.com/#/functional/report/076ed54106439c677bbfdee32060e327
http://mozmill-crowd.blargon7.com/#/remote/report/076ed54106439c677bbfdee320619673
http://mozmill-crowd.blargon7.com/#/remote/report/076ed54106439c677bbfdee320620f95
http://mozmill-crowd.blargon7.com/#/remote/report/076ed54106439c677bbfdee3205fb7c1

This patch is only for Nightly, will need backporting for the other branches.

	Henrik Skupin [:whimboo][⌚️UTC+2]
	Comment 25 • 10 years ago

I would still like to see some feedback from Blake here.

	Andrei Eftimie Assignee
	Comment 26 • 10 years ago

Blake, please see comment 21 and comment 22.

We want to stop unwrapping the XrayWrapper from NodeCollector.
Do you remember why it was done initially and if there are cases where we might still need it?

	Andreea Matei [:AndreeaMatei] Reporter
	Comment 27 • 10 years ago

Comment on attachment 8434138 [details] [diff] [review]
2.patch

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

Looks good to me. Waiting for Blake's answer though.

	Henrik Skupin [:whimboo][⌚️UTC+2]
	Comment 28 • 10 years ago

(In reply to Andrei Eftimie from comment #26)
> Blake, please see comment 21 and comment 22.

This is not what Blake might help. I requested that you find the original implementation bug for NodeCollector before asking for needinfo. So please do so.

	Henrik Skupin [:whimboo][⌚️UTC+2]
	Comment 29 • 10 years ago

Comment on attachment 8434138 [details] [diff] [review]
2.patch

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

Code-wise it looks fine to me. But please revert the changes in addons.js as mentioned inline. Lets wait for feedback from the devs before landing the patch.

::: firefox/lib/addons.js
@@ +1343,5 @@
>      var spec = aSpec || { };
>      var type = spec.type;
>  
>      var root = spec.parent ? spec.parent.getNode() : this._controller.tabs.activeTab;
> +    var elems = null;

Please keep the nodeCollector here. It was used on purpose, given that it might be necessary for other elements here. Also as more usage we have, as easier it will be to improve it. Goal is to also land this functionality in Mozmill base, if necessary before the transition to Marionette.

	Andrei Eftimie Assignee
	Comment 30 • 10 years ago

(In reply to Henrik Skupin (:whimboo) from comment #29)
> Please keep the nodeCollector here. It was used on purpose, given that it
> might be necessary for other elements here.

This makes no sense. We agreed to use NodeCollector _only_ when we need anonymous elements, which we can't access directly with `findElement`.

Based on that this is wrong as it uses NC where it is _not needed_, and we should change it to findElement. If we'll later need NC (which I highly doubt we'll need in the foreseeable future) we'll add it back. Why carry this baggage around?

> Also as more usage we have, as
> easier it will be to improve it. Goal is to also land this functionality in
> Mozmill base, if necessary before the transition to Marionette.

Land NodeCollector in Mozmill?
If you want this I hope only after a complete refactoring. NodeCollector / DomWalker is a mess right now. The _only_ useful part of NC is `queryAnonymousNode`. We should find a way to include this functionality into `findElement`.

	Henrik Skupin [:whimboo][⌚️UTC+2]
	Comment 31 • 10 years ago

(In reply to Andrei Eftimie from comment #30)
> Land NodeCollector in Mozmill?
> If you want this I hope only after a complete refactoring. NodeCollector /
> DomWalker is a mess right now. The _only_ useful part of NC is
> `queryAnonymousNode`. We should find a way to include this functionality
> into `findElement`.

No, that's not all. findElement() does also not support finding a list of elements. Because of all that I wanted to build the ui modules similarly, so it can easily be subclassed.

	Andrei Eftimie Assignee
	Comment 32 • 10 years ago

Why not enhance findEelement/elementslib to return the list then. This is at the moment hardcoded to return the requested index or the first element of the list:
https://github.com/mozilla/mozmill/blob/master/mozmill/mozmill/extension/resource/driver/elementslib.js#L139-L153

This is using `querySelectorAll` after all.

	Henrik Skupin [:whimboo][⌚️UTC+2]
	Comment 33 • 10 years ago

You cannot enhance findElement, given that it only returns a single node. We would need a method like findElements. But whatever is needed here, my goal is to have it similarly working like node collector in mozmill. I'm not sure if there is already a bug filed for it. You may want to check. Then lets move all the discussion over there.

	Andrei Eftimie Assignee
	Comment 34 • 10 years ago

I disagree with using nodeCollector where it is not needed (and we follow this guideline to _only_ use NC when we need Anonymous Elements), but I've made the changes as requested by Henrik in comment 29.

For the initial implementation of the unwrapping:
We have some changes done in bug 659487 http://hg.mozilla.org/qa/mozmill-tests/rev/6b5fb7dd4221#l2.12 where the implementation detail has been moved from the mozmill-tests library to mozmill proper.

THe initial code has been added here: http://hg.mozilla.org/qa/mozmill-tests/rev/4b3e6e75a9eb without any reference to a bugzilla bug, so I'm not sure how I can follow it deeper.

	Henrik Skupin [:whimboo][⌚️UTC+2]
	Comment 35 • 10 years ago

(In reply to Andrei Eftimie from comment #34)
> THe initial code has been added here:
> http://hg.mozilla.org/qa/mozmill-tests/rev/4b3e6e75a9eb without any
> reference to a bugzilla bug, so I'm not sure how I can follow it deeper.

Actually it seems like it happened with bug 569813.

	Andreea Matei [:AndreeaMatei] Reporter
	Comment 36 • 10 years ago

Comment on attachment 8437550 [details] [diff] [review]
3.patch

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

http://hg.mozilla.org/qa/mozmill-tests/rev/53ea632805de (default)

Due to the merge we need this in Aurora too, please check everything works fine there.

	Andrei Eftimie Assignee
	Comment 37 • 10 years ago

All good in Aurora: http://mozmill-crowd.blargon7.com/#/remote/report/c69af0ad3436e0d472c1ca8a06f3efe5

Transplanted:
https://hg.mozilla.org/qa/mozmill-tests/rev/146cba379d23 (mozilla-aurora)

	Andrei Eftimie Assignee
	Comment 38 • 10 years ago

I am closing this bug, but I'm leaving Blake's needinfo flag.
If there _might_ be issues with the current fix (to not unwrap the window object in NodeCollector) we should be aware.

	Blake Kaplan (:mrbkap) (inactive)
	Comment 39 • 10 years ago

(In reply to Andrei Eftimie from comment #26)
> We want to stop unwrapping the XrayWrapper from NodeCollector.
> Do you remember why it was done initially and if there are cases where we
> might still need it?

Sadly, I don't remember at all why these decisions were made. A couple of possibilities that I can think of would be either that we were running into limitations with the older versions of Xray wrappers (on* event handlers jump to mind as a likely candidate) or could older tests have relied on expando properties set in content? At this point, hg history would be much more useful than I would be.

	Henrik Skupin [:whimboo][⌚️UTC+2]
	Comment 40 • 10 years ago

Maybe, but I even wasn't able to find something about that decision. Anyway, it looks like we don't open up a hole here, and it works. We are in content and stay in content. So let it be as it is now. Thanks for the reply Blake!