638876 - Failure opening new tab via toolbar after closing Panorama

Status: VERIFIED FIXED

(Testing :: Mozbase, defect)

Description

[Dave Hunt [:davehunt] [he/him] ⌚BST]

If a test opens/closes Panorama, and then attempts to open a new tab via the toolbar button, the following failure occurs:


ERROR | Test Failure: {"exception": {"message": "Expression \"id(\"navigator-toolbox\")\" returned null. Anonymous == false", "lineNumber": 486, "stack": "([object Array],\"id(\\\"navigator-toolbox\\\")\",4,[object Array])@resource://mozmill/modules/elementslib.js:486\n()@resource://mozmill/modules/elementslib.js:501\n([object Object])@resource://mozmill/modules/controller.js:490\n@:0\ntabBrowser_openTab([object Proxy])@resource://mozmill/stdlib/securable-module.js -> file:///Users/dave/workspace/mozmill-tests/default/shared-modules/tabs.js:442\n@:0\ntestOpenNewTabs()@resource://mozmill/modules/frame.js -> file:///Users/dave/workspace/mozmill-tests/default/firefox/testTabView/testOpenNewTab.js:14\n(testOpenNewTabs)@resource://mozmill/modules/frame.js:542\n([object Object])@resource://mozmill/modules/frame.js:611\n([object Object])@resource://mozmill/modules/frame.js:654\n(\"/Users/dave/workspace/mozmill-tests/default/firefox/testTabView/testOpenNewTab.js\")@resource://mozmill/modules/frame.js:491\n(\"/Users/dave/workspace/mozmill-tests/default/firefox/testTabView/testOpenNewTab.js\")@resource://mozmill/modules/frame.js:666\n((function (filename, invokedFromIDE) {var runner = new Runner(new Collector, invokedFromIDE);runner.runTestFile(filename);runner.end();return true;}),[object Proxy])@resource://jsbridge/modules/server.js:164\n(\"794e2c60-4692-11e0-98c3-c42c03350c60\",(function (filename, invokedFromIDE) {var runner = new Runner(new Collector, invokedFromIDE);runner.runTestFile(filename);runner.end();return true;}),[object Proxy])@resource://jsbridge/modules/server.js:168\n@:0\n@resource://jsbridge/modules/server.js:249\n", "fileName": "resource://mozmill/modules/elementslib.js"}}

Comment 1

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Attachment: Simple test to replicate the issue. v1

Added a simple test that replicates the issue.

Comment 2

[u279076]

This is similar (if not the same) failure we saw after landing bug 620536 and bug 620538 (both Panorama tests).  This failure was happening in all tests following the Panorama tests execution.  Something happens in the browser once Panorama is first instantiated.

I'm going to bring in Ian Gilman on this bug and see if he might be able to help us identify what might be happening here.

Comment 3

[u279076]

Adding blocking for the Panorama tests tracking bug (bug 629050).  I think we need to get this fixed before any tests can reliably land.

Comment 4

[Ian Gilman [:iangilman]]

Raymond, can you take a look at this?

Comment 5

[Aaron Train [:aaronmt]]

(In reply to comment #4)
> Raymond, can you take a look at this?

For reference, how we're opening Tab View http://hg.mozilla.org/qa/mozmill-tests/file/fec1bce1e6db/shared-modules/tabview.js#l101, and how we're closing Tab View http://hg.mozilla.org/qa/mozmill-tests/file/fec1bce1e6db/shared-modules/tabview.js#l157

Comment 6

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

I'm a bit concerned here if this can also be reproduced manually. In case like those it could break add-on compatibility, at least for those which are using lookup strings to get references to elements.

Comment 7

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

Attachment: minimized test

That's a minimized test without any external requirements. You will be able to execute it via the Mozmill IDE (add-on). So as it looks like the opening and closing of Panorama is changing the something in the lookup string, which causes us to fail. The navigator-toolbox is around but we can't access it via a lookup string.

Comment 8

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

This is kinda strange and it looks like a bug in our Lookup implementation. CC'ing Andrew and Clint to get their feedback on it.

I will attach one more simplified testcase, which makes it even more clear. As it looks like the vbox below the tab-view-deck returns a completely broken element node which is the reason we are failing:

** Lookup: [object XULElement]
** Child nodes: vbox
[open / close Panorama]
** Lookup: [object XULElement],[object XULElement]
** Child nodes: undefined

As you can see our object is built-up from two XULElement objects which is clearly wrong.

Comment 9

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

Attachment: minimized test v2

Comment 10

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

Attachment: minimized test v2

Now qrefreshed.

Comment 11

[u279076]

Seems like we definitely have a Firefox regression...

Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b13pre) Gecko/20110303 Firefox/4.0b13pre
** Lookup: [object XULElement]
** Child nodes: vbox
** Lookup: [object XULElement],[object XULElement]
** Child nodes: undefined

Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b12pre) Gecko/20110204 Firefox/4.0b12pre
** Lookup: [object XULElement]
** Child nodes: vbox
** Lookup: [object XULElement],[object XULElement]
** Child nodes: undefined

Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b9pre) Gecko/20110103 Firefox/4.0b9pre
** Lookup: [object XULElement]
** Child nodes: vbox
** Lookup: [object XULElement]
** Child nodes: vbox

Based on the above results, it happened between Jan 3 and Feb 4.  I'll try to narrow it down to a day.

Comment 12

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

Moving over to Panorama, because it's not our fault.

Comment 13

[u279076]

24-hour regression window:

20110116 -> 20110117

Comment 14

[Andrew Halberstadt [:ahal]]

Actually it may be Mozmill's fault:

I've figured out what's going on. When you first start Minefield you get an
element hierarchy like this:
-window
--deck
---vbox

To walk through the vbox we need to use "{'flex'= 1}" because that is its only
DOM attribute.

When you open and close Panorama, it leaves behind an iframe so you get a
hierarchy like so:
-window
--deck
---vbox
---iframe

Now the iframe also has the DOM attribute "flex = 1" so what is happening is
that the lookup code is returning a list of nodes rather than a single node.

I don't know a whole lot about lookup expressions, so I'm not sure if this is
expected behaviour or not. I do know that you can specify something like
".../[0]/..." in a lookup expression... is this used for selecting an index in
a list?

Comment 15

[Andrew Halberstadt [:ahal]]

It depends if leaving behind that iframe when you open/close Panorama is a bug or not.

Comment 16

[Tim Taubert [:ttaubert] (inactive)]

(In reply to comment #15)
> It depends if leaving behind that iframe when you open/close Panorama is a bug
> or not.

No, that's not a bug. That _is_ panorama :) It's lazy-loaded in an iframe.

Comment 17

[Andrew Halberstadt [:ahal]]

That's what I figured.. moving back to Mozmill land.

Comment 18

[Andrew Halberstadt [:ahal]]

So what needs to be determined now is whether returning a list of elements that all match a certain property is expected behaviour or a bug.

If it isn't then the test should be fixed, otherwise this belongs in Mozmill core.

Comment 19

[u279076]

I'm not sure how to proceed, Henrik.  There is a pref, browser.panorama.experienced_first_run, which we might be able to utilize in some way so that we know what to expect...I don't know.

Comment 20

[Tim Taubert [:ttaubert] (inactive)]

(In reply to comment #19)
> I'm not sure how to proceed, Henrik.  There is a pref,
> browser.panorama.experienced_first_run, which we might be able to utilize in
> some way so that we know what to expect...I don't know.

That is set when panorama was run/used once. But it does not indicate whether panorama (the iframe) is loaded.

Comment 21

[Andrew Halberstadt [:ahal]]

Looking at the source code for the lookup expression parser, it seems that
returning the list is expected behaviour. Pretty sure this will need to be
worked around in the tests.

Can you not just check to see if the iframe exists before setting the lookup expression?  Or since the test controls when it opens panorama couldn't it just explicitly change the lookup expression after this event occurs?

Comment 22

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

(In reply to comment #14)
> I don't know a whole lot about lookup expressions, so I'm not sure if this is
> expected behaviour or not. I do know that you can specify something like
> ".../[0]/..." in a lookup expression... is this used for selecting an index in
> a list?

Yes, that's exactly what we need here. I haven't noticed that the iframe also has a flex attribute set. Patch upcoming.

Comment 23

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

Attachment: Patch v1

Lets go with the simplest approach. We don't want to refactor the tabbrowser class right now, so simply update the lookup string to access elements below the deck. That patch is only necessary for default and mozilla2.0.

In the future we are using querySelector and wont be able to get into situations like those again.

Comment 24

[u279076]

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

This seems to have fixed the problem originally reported on bug 635305.

Please land ASAP.

Comment 25

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

Landed as:
http://hg.mozilla.org/qa/mozmill-tests/rev/1423d15983c5 (default)
http://hg.mozilla.org/qa/mozmill-tests/rev/4d03d21bd353 (2.0)

Comment 26

[u279076]

> -const TABS_VIEW = '/id("main-window")/id("tab-view-deck")/{"flex":"1"}';
> +const TABS_VIEW = '/id("main-window")/id("tab-view-deck")/[0]';

We will need to port this change to all shared modules which have "tab-view-deck" in their path. For example, in the Search shared module:

- const NAV_BAR = '/id("main-window")/id("tab-view-deck")/{"flex":"1"}/id("navigator-toolbox")/id("nav-bar")';
+ const NAV_BAR = '/id("main-window")/id("tab-view-deck")/[0]/id("navigator-toolbox")/id("nav-bar")';

Henrik, would you mind if I write a patch to fix this?

Comment 27

[u279076]

Attachment: Patch v2

Patch to push the same change to other affected shared modules.

Comment 28

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

Comment on attachment 517889 [details] [diff] [review]
Patch v2

Good catch! Totally missed to check other modules and tests. Beside the above to modules we also have to fix testFindInPage.js

Comment 29

[u279076]

Attachment: Patch v2.1 [checked-in]

Adds support to testFindInPage. Having looked at the test though, I'm wondering if we should move all those lookup strings into a Findbar section in the Toolbars Shared Module?

Comment 30

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

Comment on attachment 517917 [details] [diff] [review]
Patch v2.1 [checked-in]

Not yet. We will move everything with the refactoring. We shouldn't spend time on that part now.

Comment 31

[u279076]

Comment on attachment 517917 [details] [diff] [review]
Patch v2.1 [checked-in]

Landed:
http://hg.mozilla.org/qa/mozmill-tests/rev/022f06d5314c [default]
http://hg.mozilla.org/qa/mozmill-tests/rev/d2f9e79a67cc [mozilla2.0]

Comment 32

[u279076]

Attachment: Patch v2.1a [checked-in]

Typo in Patch v2 corrected.

Comment 33

[u279076]

Comment on attachment 517927 [details] [diff] [review]
Patch v2.1a [checked-in]

Landed:
http://hg.mozilla.org/qa/mozmill-tests/rev/a813db6f7f81 [default]
http://hg.mozilla.org/qa/mozmill-tests/rev/83fc6650a4d5 [mozilla2.0]

Comment 34

[u279076]

(In reply to comment #33)
> Comment on attachment 517927 [details] [diff] [review]
> Patch v2.1a [checked-in]
> 
> Landed:
> http://hg.mozilla.org/qa/mozmill-tests/rev/a813db6f7f81 [default]
> http://hg.mozilla.org/qa/mozmill-tests/rev/83fc6650a4d5 [mozilla2.0]

I've tested this and it corrects the issues that were causing us to back out the 2 previous Panorama tests.  I'll revise those patches for bitrot and attach them to their respective bugs for review.  Marking verified fixed for now.