1126744 - Test failure 'tab.getNode(...) is null' in testAddons/testPluginDisableAffectsAboutPlugins.js

Status: RESOLVED FIXED

(Mozilla QA Graveyard :: Mozmill Tests, defect)

Description

[Daniela Domnici]

Module:  testDisableEnablePlugin
Test:    /testAddons/testPluginDisableAffectsAboutPlugins.js
Errors:  tab.getNode(...) is null 
         aIndex is not defined 
Branches:Nightly(38.0a1), Aurora(37.0a2),esr31(31.4.0esrpre) 
Platforms: All

Reports: http://mozmill-daily.blargon7.com/#/functional/failure?app=Firefox&branch=All&platform=All&from=2015-01-27&to=2015-01-28&test=%2FtestAddons%2FtestPluginDisableAffectsAboutPlugins.js&func=testDisableEnablePlugin

Comment 1

[Daniela Domnici]

Attachment: patch_V1

created skip patch for the nightly branch

Comment 2

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8555802 [details] [diff] [review]
patch_V1

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

::: firefox/tests/functional/testAddons/manifest.ini
@@ +8,5 @@
>  [testInstallUninstallSoftBlocklistedExtension.js]
>  [testInstallUninstallTheme.js]
>  [testManagerKeyboardShortcut.js]
>  [testPluginDisableAffectsAboutPlugins.js]
> +disabled = Bug 1126744 -Bug 1126744 - Test failure 'tab.getNode(...) is null'

Please remove one bug no.

Comment 3

[Daniela Domnici]

Attachment: patch_V2

Updated patch.Also, the patch applies to aurora branch.

Comment 4

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8555804 [details] [diff] [review]
patch_V2

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

https://hg.mozilla.org/qa/mozmill-tests/rev/5220ed19cea0 (default)
https://hg.mozilla.org/qa/mozmill-tests/rev/5808a4355e88 (aurora)

Comment 5

[Andreea Matei [:AndreeaMatei]]

Teodor is looking into this, might be related to his changes in tabs.js

Comment 6

[Teodor Druta]

Attachment: fixtestplugindisabled.patch

:( As Cosmin pointed out the Addons Manager (about:addons) will open differently in two cases:
* If the active tab is a 'clean' about:newtab, about:home or about:blank page with no history, it will open in the active tab directly.
* If the active tab is any another page or either an about:* page with history it will open a new tab.

So, the closeAllTabs() method now works differently in the way that it opens a new clean tab with no history and then closes all the other tabs. The old method closed all tabs besides one and then just opened 'about:newtab' in the last remaining tab, resulting in a 'dirty' with history 'about'newtab' page.

No other tests seem to be affected by this change.

Comment 7

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

Comment on attachment 8555834 [details] [diff] [review]
fixtestplugindisabled.patch

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

This should really be handled in the addon manager class. It's just a workaround here for this specific test.

Comment 8

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

Well, disregard my last comment. I did a quick check on this test and it assumes that about:addons is handled in a second tab, right? We should kill this assumption, given that we have a clean state at the beginning. In any case if we want to flip back to about:newtab, the situation will be different again.

So lets open about:blank explicitly before opening the addon manager, and adding a comment why we have to do that. This will be cleaner and more stable.

Comment 9

[Teodor Druta]

Attachment: fixtestplugindisabledv2.patch

(In reply to Henrik Skupin (:whimboo) from comment #8)
> So lets open about:blank explicitly before opening the addon manager, and
> adding a comment why we have to do that. This will be cleaner and more
> stable.

Actually opening about:blank in a clean about:newtab page will leave the page history empty, so I decided to open about:home instead, before opening the addon manager.

Comment 10

[Teodor Druta]

Attachment: fixtestplugindisabledv2.patch

sorry, forgot to save the file in my text editor after a change.

Comment 11

[Mihaela Velimiroviciu (:mihaelav)]

Comment on attachment 8556394 [details] [diff] [review]
fixtestplugindisabledv2.patch

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

LGTM

Comment 12

[Teodor Druta]

Please either land the fix patch or skip this test on mozilla-release and mozilla-beta ASAP.

Comment 13

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

Comment on attachment 8556394 [details] [diff] [review]
fixtestplugindisabledv2.patch

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

::: firefox/tests/functional/testAddons/testPluginDisableAffectsAboutPlugins.js
@@ +63,1 @@
>    addonsManager.open();

I think you misunderstood my last review comments. The AOM does not need any kind of history. Loading about:blank was meant that the AOM opens in the exact same tab, but no new tabs gets opened.

Comment 14

[Teodor Druta]

(In reply to Henrik Skupin (:whimboo) from comment #13)
> Comment on attachment 8556394 [details] [diff] [review]
> fixtestplugindisabledv2.patch
> 
> Review of attachment 8556394 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> :::
> firefox/tests/functional/testAddons/testPluginDisableAffectsAboutPlugins.js
> @@ +63,1 @@
> >    addonsManager.open();
> 
> I think you misunderstood my last review comments. The AOM does not need any
> kind of history. Loading about:blank was meant that the AOM opens in the
> exact same tab, but no new tabs gets opened.

In that case I don't think we need to open about:blank here, we can just open directly the AOM in the about:newtab tab, that remains from calling the closeAllTabs() method.

Comment 15

[Daniela Domnici]

Attachment: patch_V1_beta

Created the skip patch for the beta branch to avoid the multiple failures for the beta day.

Comment 16

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8556483 [details] [diff] [review]
patch_V1_beta

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

https://hg.mozilla.org/qa/mozmill-tests/rev/d058bbfd87b1 (beta)

Comment 17

[Daniela Domnici]

Attachment: patch_V1_release

Created the skip patcch for the release branch.

Comment 18

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

(In reply to Teodor Druta from comment #14)
> In that case I don't think we need to open about:blank here, we can just
> open directly the AOM in the about:newtab tab, that remains from calling the
> closeAllTabs() method.

This does not work, because a new tab will be opened in case of about:newtab. But with about:blank no new tab will be opened. Exactly this is the problem I'm trying to tell three times now. We won't hit this problem right now, but if there is a case when we have to disable about:newtab, this test will start to fail because of wrong tab indexes.

Comment 19

[Teodor Druta]

(In reply to Henrik Skupin (:whimboo) from comment #18)
> This does not work, because a new tab will be opened in case of
> about:newtab. But with about:blank no new tab will be opened. 

A new (AMO) tab will not open for 'about:newtab' (CLEAN) as well, their functionality is the same on this matter.

Steps:
1. Open firefox
2. Open a new tab (At this point we have about:newtab open with no history at all)
3. Open the Addons Manager (Using shortcut or menu entry)
- The addons manager will open in the current active tab

1. -/-
2. Open a new tab
3. Navigate to any remote/local resource in the same tab
4. Open about:newtab in the same tab
5. Open the Addons Manager
- The addons manager will open in a different new tab

1. -/-
2. Open a new tab
3. Open 'about:blank' in the same tab
4. Open the Addons Manager
- The addons manager will open in the same tab

1. -/-
2. Open a new tab
3. Navigate to any remote/local resource in the same tab
4. Open 'about:blank' in the same tab
5. Open the Addons Manager
- The addons manager will open in a different new tab

Comment 20

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

Very interesting. Something might have been changed here. Thanks for the information.

Comment 21

[Teodor Druta]

Comment on attachment 8555834 [details] [diff] [review]
fixtestplugindisabled.patch

After looking more into this problem, I think that this is the best solution so far, we will open a new tab, and will use it for the AMO page, this tab will have index 1, while the 'about:plugins' page will be handled in the tab with index 0.

Comment 22

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8556506 [details] [diff] [review]
patch_V1_release

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

Don't need it in release, will be merged.

Comment 23

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8555834 [details] [diff] [review]
fixtestplugindisabled.patch

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

lgtm. Henrik, do you agree?

Comment 24

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

Comment on attachment 8555834 [details] [diff] [review]
fixtestplugindisabled.patch

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

Looks good to me, yes. Lets just add a comment why we have to open a new tab here.

Comment 25

[Teodor Druta]

Attachment: fixtestplugindisabled.patch

Added an explanatory comment to the openTab line.

Comment 26

[Andreea Matei [:AndreeaMatei]]

Actually, please add the unskip also in this patch, so we don't land 2 sets (fix and unskip) for each branch. Thanks.

Comment 27

[Teodor Druta]

Attachment: fixtestplugindisabled.patch

(In reply to Andreea Matei [:AndreeaMatei] from comment #26)
> Actually, please add the unskip also in this patch, so we don't land 2 sets
> (fix and unskip) for each branch. Thanks.

Done.

Comment 28

[Teodor Druta]

Attachment: fixtestplugindisabled.patch

...

Comment 29

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8559103 [details] [diff] [review]
fixtestplugindisabled.patch

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

https://hg.mozilla.org/qa/mozmill-tests/rev/3f0391299059 (default)

Comment 30

[Teodor Druta]

Attachment: fixtestplugindisabled_beta.patch

I think we should backport. The patch applies cleanly and runs well on aurora.
Attached is the patch for beta.

Comment 31

[Teodor Druta]

Attachment: fixtestplugindisabled_release.patch

This will be the patch for the release branch.

Comment 32

[Andreea Matei [:AndreeaMatei]]

https://hg.mozilla.org/qa/mozmill-tests/rev/eb467322462e (aurora)

Comment 33

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8559183 [details] [diff] [review]
fixtestplugindisabled_beta.patch

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

https://hg.mozilla.org/qa/mozmill-tests/rev/d8c4e6c54b8e (beta)

Comment 34

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8559701 [details] [diff] [review]
fixtestplugindisabled_release.patch

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

https://hg.mozilla.org/qa/mozmill-tests/rev/6a9d5f025ae5 (release)