810301 - AddonsManager.close in lib/addon.js closes the current tab without checking if it is the addons manager

Status: RESOLVED FIXED

(Mozilla QA Graveyard :: Mozmill Tests, defect, P2)

Description

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

Currently, the close method simply assumes that the current tab is the addons manager, and closes it. If the addons manager is not open, then this could cause an unexpected tab to close or even the final tab to close - causing a disconnect error.

We should instead close only (all) addons manager tabs, or throw an exception if the addons manager is not open.

Comment 1

[Alex Lakatos[:AlexLakatos]]

Attachment: patch v1.0

closing all Add-ons Manager tabs in all windows, and throwing an error if the Add-ons Manager is not opened in the first place.

Comment 2

[Alex Lakatos[:AlexLakatos]]

here is a testrun with the patch applied:
http://mozmill-crowd.blargon7.com/#/functional/report/190d57cf64e3c5c3996c0c1059fe730c
http://mozmill-crowd.blargon7.com/#/functional/report/190d57cf64e3c5c3996c0c1059fe9cfb

Comment 3

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

Comment on attachment 681013 [details] [diff] [review]
patch v1.0

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

What we need here is that close() should close the tab which gets returned by getTabs(). This method here is too complicated.

Comment 4

[Alex Lakatos[:AlexLakatos]]

(In reply to Henrik Skupin (:whimboo) from comment #3)
> Comment on attachment 681013 [details] [diff] [review]
> patch v1.0
> 
> Review of attachment 681013 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> What we need here is that close() should close the tab which gets returned
> by getTabs(). This method here is too complicated.

The function does just that. getTabs() returns an array of opened tabs with the url "about:addons". But that tabs may be on a different window, that's we instantiate the tabBrowser every time. If we close them starting from the smallest index, then the next index does not work, and we would have to call getTabs() all over again and use just the first entry. Also, if the tab is the single tab in an window, then we get an error(something that closeTab() does not handle btw), and that's why we open about:blank in those cases.
The function is complicated, but covers all cases.

Comment 5

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

This is not a sprint entry for next week given that we have bug 812435 to fix and this bug seems to be nearly ready.

Comment 6

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

(In reply to Alex Lakatos[:AlexLakatos] from comment #4)
> > What we need here is that close() should close the tab which gets returned
> > by getTabs(). This method here is too complicated.
> 
> The function does just that. getTabs() returns an array of opened tabs with
> the url "about:addons". But that tabs may be on a different window, that's
> we instantiate the tabBrowser every time. If we close them starting from the
> smallest index, then the next index does not work, and we would have to call
> getTabs() all over again and use just the first entry. Also, if the tab is
> the single tab in an window, then we get an error(something that closeTab()
> does not handle btw), and that's why we open about:blank in those cases.
> The function is complicated, but covers all cases.

It still stands. The code here is way to complicated and unnecessary. Why not doing something like:

while (this.getTabs()) {
  ...
}

Sure that you have to special case the last tab, but it's not that complicated. Further don't use 'middleClick' for closing a tab but 'menu'.

Comment 7

[Alex Lakatos[:AlexLakatos]]

(In reply to Henrik Skupin (:whimboo) from comment #6)
> (In reply to Alex Lakatos[:AlexLakatos] from comment #4)
> > > What we need here is that close() should close the tab which gets returned
> > > by getTabs(). This method here is too complicated.
> > 
> > The function does just that. getTabs() returns an array of opened tabs with
> > the url "about:addons". But that tabs may be on a different window, that's
> > we instantiate the tabBrowser every time. If we close them starting from the
> > smallest index, then the next index does not work, and we would have to call
> > getTabs() all over again and use just the first entry. Also, if the tab is
> > the single tab in an window, then we get an error(something that closeTab()
> > does not handle btw), and that's why we open about:blank in those cases.
> > The function is complicated, but covers all cases.
> 
> It still stands. The code here is way to complicated and unnecessary. Why
> not doing something like:
> 
> while (this.getTabs()) {
>   ...
> }
That keeps calling getTabs() each iteration. It's going to only use one entry in the returned array, and then get a new array. I don't really like the performance of that approach.
> 
> Sure that you have to special case the last tab, but it's not that
> complicated. Further don't use 'middleClick' for closing a tab but 'menu'.
'middleClick' is the only way I can specify an index to the closed tab, whereas 'menu' only closes the current tab. That would need an extra action, switching to the Addons Manager tab before closing it.

Comment 8

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

(In reply to Alex Lakatos[:AlexLakatos] from comment #7)
> > while (this.getTabs()) {
> >   ...
> > }
> That keeps calling getTabs() each iteration. It's going to only use one
> entry in the returned array, and then get a new array. I don't really like
> the performance of that approach.

The add-on manager will only exist a single time usually. It's getting re-used across windows. Means if it is open in window 1 and you also want it to open from window 2 the tab from window 1 will be focused. Only if you explicitly enter 'about:addons' into the location bar you are able to open multiple instances. Given that this will nearly never happen I don't worry about the little loss in performance compared to the code quality in our test.

> > Sure that you have to special case the last tab, but it's not that
> > complicated. Further don't use 'middleClick' for closing a tab but 'menu'.
> 'middleClick' is the only way I can specify an index to the closed tab,
> whereas 'menu' only closes the current tab. That would need an extra action,
> switching to the Addons Manager tab before closing it.

Hm, I see. Ok, so we should leave it as it is currently.

Comment 9

[Alex Lakatos[:AlexLakatos]]

Attachment: patch v2.0

Closing the first instance of the addons manager. Trowing if the addons manager is not opened. Soft failing if we closed the addons manager once but another instance still exists.

Comment 10

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

Comment on attachment 686161 [details] [diff] [review]
patch v2.0

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

::: lib/addons.js
@@ +209,3 @@
>     */
> +  close : function AddonsManager_close() {
> +    var allTabs = this.getTabs();

`allTabs` is misleading to what you want to store here. Most likely you want to have `aomTabs` here.

@@ +209,4 @@
>     */
> +  close : function AddonsManager_close() {
> +    var allTabs = this.getTabs();
> +    var allTabsLength = allTabs.length;

It's used once. So I don't see a need for this variable.

@@ +210,5 @@
> +  close : function AddonsManager_close() {
> +    var allTabs = this.getTabs();
> +    var allTabsLength = allTabs.length;
> +
> +    if (allTabsLength > 0) {

Instead of `if` we should use a try/catch and throw the Error from inside the catch clause.

@@ +225,5 @@
> +      throw new Error(arguments.callee.name + ": Add-ons Manager has been closed.");
> +    }
> +
> +    if (this.isOpen)
> +      expect.fail("Add-ons Manager has been closed.");

This message uses a wrong assumption. We have closed the AOM but there were more than a single instance open. You have to call that out here.

Comment 11

[Alex Lakatos[:AlexLakatos]]

Attachment: patch v3.0

used aomTabs, a try/catch block and used a better comment for the soft fail

Comment 12

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

Comment on attachment 686595 [details] [diff] [review]
patch v3.0

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

This looks good, however in order to test it I added some lines to lib/tests/testAddonsManager.js. Could we include a few assertions alongside this patch? Below is what I used:

  am.close();
  expect.ok(!am.isOpen);

  try {
    am.close();
  }
  catch (e) {
    expect.equal(e.message, "AddonsManager_close: Add-ons Manager has been closed.")
  }

I don't know if we can test the soft assertion when multiple AOMs are open. I verified it locally using:

  am.open();
  tabBrowser.openTab();
  controller.open("about:addons");
  am.waitForOpened();
  am.close();

r- for now to see if we can get a couple of additions to the standard module tests.

Comment 13

[Alex Lakatos[:AlexLakatos]]

Attachment: patch v4.0

added unit tests for the throw path. we can't test for the soft fail, it will mark the test as failed, even if we wrap it in a try/catch.

Comment 14

[Alex Lakatos[:AlexLakatos]]

(In reply to Alex Lakatos[:AlexLakatos] from comment #13)
> Created attachment 689842 [details] [diff] [review]
> patch v4.0
>-  addon = am.getAddons()[0];
>+  addon = am.getAddons()[1];
The existing unit test was failing, we were not retrieving the correct add-on here. Fixed that as well so we're having a passing unit test

Comment 15

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

Comment on attachment 689842 [details] [diff] [review]
patch v4.0

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

We are getting closer. Lets get the mentioned issues fixed and hashed out.

::: lib/addons.js
@@ +224,5 @@
> +      throw new Error(arguments.callee.name + ": Add-ons Manager has been closed.");
> +    }
> +
> +    if (this.isOpen)
> +      expect.fail("Add-ons Manager has been opened multiple times.");

This should state: "Not all Add-on Manager instances have been closed".

::: lib/tests/testAddonsManager.js
@@ +70,5 @@
>    // Mozmill should be marked as installed
>    expect.ok(am.isAddonInstalled({addon: addon}), "MozMill is marked as being installed");
>  
>    // Get first search result and check it is not installed
> +  addon = am.getAddons()[1];

Why this change? What is the 'not correct addon' in this case?

@@ +86,5 @@
> +    am.close();
> +  }
> +  catch (ex) {
> +    expect.equal(ex.message, "AddonsManager_close: Add-ons Manager has been closed.",
> +                 "Add-ons Manager was opened.")

This message is not clear. It should state what you have not expected.

Comment 16

[Alex Lakatos[:AlexLakatos]]

(In reply to Henrik Skupin (:whimboo) from comment #15)
> Comment on attachment 689842 [details] [diff] [review] 
> >    // Get first search result and check it is not installed
> > +  addon = am.getAddons()[1];
> 
> Why this change? What is the 'not correct addon' in this case?
We are searching for Mozmill Crowd but retrieving Mozmill instead with am.getAddons()[0]. And right after that we check that am.getAddons()[0] is not installed, which fails.

Comment 17

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

If we are working with Mozmill Crowd, then please kill that dependency given that it is no longer compatible. Instead make sure to use an addon we can trust to be updated regularly, like NTT or MemChaser.

Comment 18

[Andrei Eftimie]

Attachment: patch v5.0

As asked:
- swapped the Mozmill dependency with MemChaser
- changed 2 messages to be more clear

Testruns:
http://mozmill-crowd.blargon7.com/#/functional/report/a11aa7b15f4e571fd6fe21a2db70ea80

Comment 19

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

Comment on attachment 692983 [details] [diff] [review]
patch v5.0

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

::: lib/tests/testAddonsManager.js
@@ +88,5 @@
> +    am.close();
> +  }
> +  catch (ex) {
> +    expect.equal(ex.message, "AddonsManager_close: Add-ons Manager has been closed.",
> +                 "Add-ons Manager should have not remained open.")

This message should really be related to expecting the exception. Something like 'Exception thrown because Add-ons Manager has already been closed.'

Comment 20

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

Comment on attachment 692983 [details] [diff] [review]
patch v5.0

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

::: lib/tests/testAddonsManager.js
@@ +88,5 @@
> +    am.close();
> +  }
> +  catch (ex) {
> +    expect.equal(ex.message, "AddonsManager_close: Add-ons Manager has been closed.",
> +                 "Add-ons Manager should have not remained open.")

Well, you should never compare the message here. That would be a hell of maintenance. If we see an exception we are good, that's all.

Comment 21

[Andrei Eftimie]

Attachment: Patch v5.1

- changed message
- always throw the message, don't compare any error messages into the exception

Comment 22

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

Comment on attachment 694269 [details] [diff] [review]
Patch v5.1

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

::: lib/tests/testAddonsManager.js
@@ +87,5 @@
> +  try {
> +    am.close();
> +  }
> +  catch (ex) {
> +    expect.pass("Exception thrown because Add-ons Manager has already been closed.");

So actually this test doesn't make that much sense. Sorry that I missed it last time. But what we really want to test here is the failing case and not when it is passing.

Comment 23

[Andrei Eftimie]

Attachment: Patch v5.2

Enhanced the logic to:

Try closing the Addons Manager.
If failed => all good
If succeeded => raise exception since we shouldn't be able to do this

Comment 24

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

Comment on attachment 694394 [details] [diff] [review]
Patch v5.2

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

Looks great, landed as:
http://hg.mozilla.org/qa/mozmill-tests/rev/35c8434196a0

Comment 25

[Andrei Eftimie]

Cleanly applies to all branches.

Testruns:
mozilla-aurora
http://mozmill-crowd.blargon7.com/#/functional/report/a11aa7b15f4e571fd6fe21a2dbb66e40
http://mozmill-crowd.blargon7.com/#/functional/report/a11aa7b15f4e571fd6fe21a2dbb6d6d4

mozilla-beta
http://mozmill-crowd.blargon7.com/#/functional/report/a11aa7b15f4e571fd6fe21a2dbb82f4d
http://mozmill-crowd.blargon7.com/#/functional/report/a11aa7b15f4e571fd6fe21a2dbb86574

mozilla-release
http://mozmill-crowd.blargon7.com/#/functional/report/a11aa7b15f4e571fd6fe21a2dbb8d05e
http://mozmill-crowd.blargon7.com/#/functional/report/a11aa7b15f4e571fd6fe21a2dbb904e1

mozilla-esr17
http://mozmill-crowd.blargon7.com/#/functional/report/a11aa7b15f4e571fd6fe21a2dbb983cf
http://mozmill-crowd.blargon7.com/#/functional/report/a11aa7b15f4e571fd6fe21a2dbb9a989

mozilla-esr10
http://mozmill-crowd.blargon7.com/#/functional/report/a11aa7b15f4e571fd6fe21a2dbba1129
http://mozmill-crowd.blargon7.com/#/functional/report/a11aa7b15f4e571fd6fe21a2dbba42a5

Comment 26

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

Comment on attachment 694394 [details] [diff] [review]
Patch v5.2

>+  var closeError = false;
>+  try {
>+    am.close();
>+  }
>+  catch (ex) {
>+    closeError = true;
>+  }
>+  expect.ok(closeError, "Exception thrown because Add-ons Manager has already been closed.")
> }

Ideally this should have been a call to expect.throws(). It's not important and we can fix that later, given that it is a simple test for the libs.

Comment 27

[Andrei Eftimie]

At the moment expect does not have a throws method.

Would you like me to create that method?
I am not sure how it should behave.

If we'll want it to always fail:
expect.throws('Message')

We do have:
expect.fail('Message')

If we are going to check for a value, this will be very similar to ok():
expect.throws(aValue, 'Message')

I am not sure how this will help us, won't this just dilute the API?

If you are having something entirely different on you mind, please share how throws() should work.

Comment 28

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

Ouch. Yeah, please file a new bug Andrei! We have to port that code from Mozmill's assertion module to our one similar to bug 642081. But make sure we have the latest version from:

https://github.com/mozilla/mozmill/blob/master/mozmill/mozmill/extension/resource/modules/assertions.js#L416

Comment 29

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

http://hg.mozilla.org/qa/mozmill-tests/rev/9d56fd65db73 (aurora)
http://hg.mozilla.org/qa/mozmill-tests/rev/e13e5fb40fa7 (beta)
http://hg.mozilla.org/qa/mozmill-tests/rev/dedffe5c2e60 (release)
http://hg.mozilla.org/qa/mozmill-tests/rev/303dd16d91f0 (esr17)
http://hg.mozilla.org/qa/mozmill-tests/rev/5ae6196cb6b4 (esr10)

Keep in mind that release is already on 18.0.

Comment 30

[Andrei Eftimie]

(In reply to Henrik Skupin (:whimboo) from comment #28)
> Ouch. Yeah, please file a new bug Andrei! We have to port that code from
> Mozmill's assertion module to our one similar to bug 642081. But make sure
> we have the latest version from:
> 
> https://github.com/mozilla/mozmill/blob/master/mozmill/mozmill/extension/
> resource/modules/assertions.js#L416

Filed bug 826645