Closed Bug 780957 Opened 8 years ago Closed 8 years ago

Failure in /testAddons_pluginDisabledAfterRestart/test2.js | Shockwave Flash is enabled - 'false' should equal 'true'

Categories

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

defect

Tracking

(firefox18 fixed, firefox19 fixed, firefox20 unaffected, firefox21 unaffected, firefox-esr17 fixed)

RESOLVED FIXED
Tracking Status
firefox18 --- fixed
firefox19 --- fixed
firefox20 --- unaffected
firefox21 --- unaffected
firefox-esr17 --- fixed

People

(Reporter: AlexLakatos, Assigned: andrei)

References

()

Details

(Whiteboard: [mozmill-test-failure] s=121203 u=failure c=addons p=1)

Attachments

(5 files, 5 obsolete files)

TEST: /testAddons_pluginDisabledAfterRestart/test2.js
ERROR: Shockwave Flash is enabled - 'false' should equal 'true'
WHEN: 08-07-2012
FIRST: 08-02-2012
FREQ: 11
There seems to be a problem with addons.getInstalledAddons and Flash. Looking into it further.
Assignee: nobody → alex.lakatos
Status: NEW → ASSIGNED
Hardware: x86_64 → x86
(In reply to Alex Lakatos[:AlexLakatos] from comment #1)
> There seems to be a problem with addons.getInstalledAddons and Flash.
> Looking into it further.
Ran multiple tests, and they all fail, regardless of the chosen plugin. But only on XP. 
May not be a problem with addons.getInstalledAddons as I initially thought.
Rethinking this through.
Are you able to replicate the locally? Are any of the plugins actually enabled? Can we get a regression range for this failure?
(In reply to Dave Hunt (:davehunt) from comment #3)
> Are you able to replicate the locally? Are any of the plugins actually
> enabled? Can we get a regression range for this failure?

Alex has the week of. If you need this addressed quickly, I can have a look
Yes please. Test failures should always be high priority.
(In reply to Dave Hunt (:davehunt) from comment #5)
> Yes please. Test failures should always be high priority.

At least today I am going to start some runs on a XP VM. Without Alex being here to summarize his results so far I need to study the problem before I can refer some metrics. Stay tuned please!
Hasn't happened anymore in the last month. Closing as WFM for now.
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Priority: -- → P1
Resolution: --- → WORKSFORME
Happened last night 11/26 on Aurora Ubuntu 12.04:
http://mozmill-ci.blargon7.com/#/functional/report/674977957b923f4905160d1b9ae9b30c
Status: RESOLVED → REOPENED
Resolution: WORKSFORME → ---
This was on a new node. Flash is installed so not sure what happened. We have to watch it.
Priority: P1 → P3
Happened again on ESR17 Firefox 17.0esrpre (17.0esrpre, en-US, 20121127034503) Ubuntu 12.04 (x86_64) - same machine: mm-ub-1204-64-2
http://mozmill-ci.blargon7.com/#/functional/report/674977957b923f4905160d1b9af122e6
Duplicate of this bug: 815592
Assignee: alex.lakatos → nobody
Status: REOPENED → NEW
OS: Windows XP → All
Priority: P3 → P2
Hardware: x86 → All
Happened again today on Aurora FR with Ubuntu 12.04 x86_64:
http://mozmill-ci.blargon7.com/#/functional/report/674977957b923f4905160d1b9afc5734
Lets see if that has been changed (fixed) now that I moved back to the Ubuntu 64 machine in the ESX cluster. Please recheck later today.
Anthony, please see comment 15 where it has already been mentioned. It's still the temporary VM I have disabled. I assume this failure will not happen anymore tomorrow.
Whiteboard: [mozmill-test-failure] → [mozmill-test-failure] s=121203 u=failure c=addons p=1
(In reply to Henrik Skupin (:whimboo) from comment #18)
> Anthony, please see comment 15 where it has already been mentioned. It's
> still the temporary VM I have disabled. I assume this failure will not
> happen anymore tomorrow.
This has'n happened starting with 2012-11-30. I think it's ok if we close it. http://mozmill-ci.blargon7.com/#/functional/failure?branch=All&platform=All&from=2012-11-30&to=&test=%2FrestartTests%2FtestAddons_pluginDisabledAfterRestart%2Ftest2.js&func=test2.js%3A%3AtestPluginDisabled
Which machines were affected here? Would be good to know.
Closing as WFM for now, given that we will not be able to retrieve the information I have asked for in my last comment. Alex, next time please try to respond immediately so that the logs will be available.
Status: NEW → RESOLVED
Closed: 8 years ago8 years ago
Resolution: --- → WORKSFORME
Happened again on ESR17 with Shockwave Flash
http://mozmill-ci.blargon7.com/#/functional/report/64349dc5ab196179e1debdbdac02b598
(machine: mm-ub-1204-64-1.qa.scl3.mozilla.com)
http://mozmill-ci.blargon7.com/#/functional/report/64349dc5ab196179e1debdbdac0548c3
(machine: mm-ub-1204-32-1.qa.scl3.mozilla.com)
And on ESR17 with Java:
http://mozmill-ci.blargon7.com/#/functional/report/64349dc5ab196179e1debdbdac054dfa
(machine: hostname": "mm-ub-1210-32-1.qa.scl3.mozilla.com)
http://mozmill-ci.blargon7.com/#/functional/report/64349dc5ab196179e1debdbdac02b47a
(machine: mm-ub-1210-32-1.qa.scl3.mozilla.com)
Status: RESOLVED → REOPENED
Resolution: WORKSFORME → ---
What I think is that there might be too many plugins installed. Once deactivated the plugin will move at the very end of the list and outside of the current view. So we might have to scroll down to the element first. Please try to reproduce and fix by reducing the window size on startup for the test.
Status: REOPENED → ASSIGNED
Please get this assigned.
Status: ASSIGNED → NEW
Three failures appeared on ESR17 branch today:
For Java with Ubuntu x86:
http://mozmill-ci.blargon7.com/#/functional/report/72e15dc943833b8fcba70aeb511a2dcb
For Shockwave with Ubuntu x86:
http://mozmill-ci.blargon7.com/#/functional/report/72e15dc943833b8fcba70aeb511a3cef
For Shockwave with Ubuntu x86_64:
http://mozmill-ci.blargon7.com/#/functional/report/72e15dc943833b8fcba70aeb511a1bc5
Assignee: nobody → andrei.eftimie
This also affects beta. We should make it a top item for P2 bugs for the next two days to get fixed.

http://mozmill-ci.blargon7.com/#/functional/report/72e15dc943833b8fcba70aeb512c6d7c
Status: NEW → ASSIGNED
Andrei, what is the status of this bug? It's the most appearing failure at the moment.
I can indeed reproduce this by resizing the window so that the Shockwave plugin gets out of the visible viewport.
Am working on a fix.
Attached patch patch v1 (obsolete) — Splinter Review
It seems Mozmill (or Firefox) can not click on an element which is outside its visible viewport.

This test would fail to click on the button to enable a plugin.

The proposed fix does directly the command that is on the button instead of trying to click on it.
Attachment #707036 - Flags: review?(hskupin)
Attachment #707036 - Flags: review?(dave.hunt)
Attachment #707036 - Flags: review?(andreea.matei)
Since this change affects all tests that rely on the Addon Manager, here is also a passing testrun:
http://mozmill-crowd.blargon7.com/#/functional/report/72e15dc943833b8fcba70aeb51bdc6f1
Comment on attachment 707036 [details] [diff] [review]
patch v1

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

::: lib/addons.js
@@ +364,5 @@
>      var spec = aSpec || { };
>      spec.button = "enable";
>  
>      var button = this.getAddonButton(spec);
> +    this._controller.window.document.getBindingParent(button.getNode()).userDisabled = false;

Problem here might be that this is a XBL binding which doesn't let through the click to the button which is located inside this element. It might help to focus the listbox entry first. That should automatically scroll the list to it. We should not call the command directly, because that's not testing the ui.
Attachment #707036 - Flags: review?(hskupin)
Attachment #707036 - Flags: review?(dave.hunt)
Attachment #707036 - Flags: review?(andreea.matei)
Attachment #707036 - Flags: review-
Attached patch patch v2 (obsolete) — Splinter Review
Changed approach.
We do click on the button so we test the UI, but we trigger the click event directly on the DOM element.

I am not sure what our controller does, but (when element out of the viewport):

this._controller.click(button); //fails
button.getNode().click(); // works 

Testrun:
http://mozmill-crowd.blargon7.com/#/functional/report/72e15dc943833b8fcba70aeb51ce307e
Attachment #707036 - Attachment is obsolete: true
Attachment #707498 - Flags: review?(hskupin)
Attachment #707498 - Flags: review?(dave.hunt)
Attachment #707498 - Flags: review?(andreea.matei)
Comment on attachment 707498 [details] [diff] [review]
patch v2

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

::: lib/addons.js
@@ +364,5 @@
>      var spec = aSpec || { };
>      spec.button = "enable";
>  
>      var button = this.getAddonButton(spec);
> +    button.getNode().click();

All the events we fire have to go through the controller. If it's broken we have to get it investigated and fixed. But not sure at the moment if it's worth for Mozmill 1.5.

So please do not make this workaround but please check what happens if we select the appropriate plugin entry first by clicking on the list entry itself. It should bring it into focus.
Attachment #707498 - Flags: review?(hskupin)
Attachment #707498 - Flags: review?(dave.hunt)
Attachment #707498 - Flags: review?(andreea.matei)
Attachment #707498 - Flags: review-
I was just reaching the same conclusion that we need to patch mozmill.
I just reproduced bug 783484. Exactly the same issue.

So it might be worth it to fix this in mozmill 1.5 since we might get rid of more sporadical issues that are hard to reproduce.
Basically any controller.click event that is in the browser window might not fire if the window is to small.

I am looking now into mozmill
Horizontal scrolling problem. Can not access elements in the far right of the list, such as the "Disable" button
Vertical scrolling is problematic here. Can not get the last item of the list into view.
Horizontal scrolling issue.
Vertical scrolling issue. Can't reach 2nd last item completely
Please keep in mind that we do not have those minimal window sizes when running tests with Mozmill. So please create screenshots based on the default window size of Firefox on Linux.

If those elements are still cut-off we might consider to maximize the window for this specific test, but only on esr17. As Andrei mentioned more recent versions of Firefox are not affected.
We shouldn't have those minimal sizes in our tests, but from the looks of our failures it appears to happen from time to time.
I am not sure sure of the exact threshold needed. (on OSX somewhere around the width of 300px).

Regarding trying to scroll the element into view, our controller tries to do that:
https://github.com/mozilla/mozmill/blob/hotfix-1.5/mozmill/mozmill/extension/resource/modules/controller.js#L494
And looking at the comment in that code, this problem was known from before.

The Addon Manager page appears to have multiple boxes with different overflow settings one inside the other, and that might be the cause of our problem. (It might be that element.scrollIntoView fails at that point... I don't know how I could test that).

We could increase the window size in our tests when working with the Addon Manager, and we would need to do it on all branches, not just esr17 (I just said the problem has been ameliorated on newer versions, but it still persists, just to a smaller degree). I am not sure that this would be the right solution.

It could be that the Addon Manager itself should get its scrolling problems fixed, because that will improve the use of that page on very small resolutions (although I don't think there are any screens that small in the wild).

Still looking into this at the controller level, because we are asking the controller to make a click, it should be its job to handle how to do it, not the tests.
The controller cannot do anything here. It's simply a defect in the addons manager. So we have to workaround it until it has been fully fixed. There should be at least one bug filed for this issue. I will CC Blair because he was mostly working on the AOM front-end. I hope we can get the scrolling issues fixed at some point, but until that happened lets do the workaround by exercising real UI elements and not doing API calls.
Attached patch patch v3 (obsolete) — Splinter Review
As a workaround we can force the window size whenever we open the Addons Manager to ensure we have enough screen realestate for the UI elements.

I have hardcoded the resize to 1024x768.
Should we instead try to maximize the window?
Attachment #707498 - Attachment is obsolete: true
Attachment #708551 - Flags: feedback?(hskupin)
Attachment #708551 - Flags: feedback?(dave.hunt)
Attachment #708551 - Flags: feedback?(andreea.matei)
(In reply to Henrik Skupin (:whimboo) from comment #41)
> The controller cannot do anything here. It's simply a defect in the addons
> manager. So we have to workaround it until it has been fully fixed. There
> should be at least one bug filed for this issue. I will CC Blair because he
> was mostly working on the AOM front-end. I hope we can get the scrolling
> issues fixed at some point, but until that happened lets do the workaround
> by exercising real UI elements and not doing API calls.

This isn't a priority for me - such small window sizes don't represent real-world usage, and even if scrolling did work correctly there the UI would still be next to unusable at such a small window size. Saying that, I do see a really easy win to make the UI adapt better to small horizontal sizes - filed bug 837008, which will help a bit with making the UI fit into a narrow window (but doesn't change how scrolling is done).
Comment on attachment 708551 [details] [diff] [review]
patch v3

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

Please do not resize but maximize the window. We can return more easily to the original size that way.
Attachment #708551 - Flags: feedback?(hskupin)
Attachment #708551 - Flags: feedback?(dave.hunt)
Attachment #708551 - Flags: feedback?(andreea.matei)
Attachment #708551 - Flags: feedback-
Have we tried taking a screenshot when this failure occurs, so we can identify if Firefox is running a small window or perhaps the addon manager content is otherwise corrupt? Either sounds like a bug to me.
We have not.

We might get more information this way...
Should I prepare a patch in which to wrap relevant code in a try/catch and make a screenshot when it fails?
Attached patch patch v4 (obsolete) — Splinter Review
We maximize the browser window whenever we open the Addons Manager, and restore it whenever we close the Addons Manager.

This should prevent this error from happening again, as well as fix bug 783484
Attachment #708551 - Attachment is obsolete: true
Attachment #709701 - Flags: review?(hskupin)
Attachment #709701 - Flags: review?(dave.hunt)
Attachment #709701 - Flags: review?(andreea.matei)
Comment on attachment 709701 [details] [diff] [review]
patch v4

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

::: lib/addons.js
@@ +159,5 @@
>          }, "Selected category has been loaded.", timeout);
>        }
>      } finally {
> +
> +      // XXX bug 837008

There is no need for the XXX here. Remove it please and add one more line what is that bug about. Also please add the dependency to this bug if it hasn't been done yet.

@@ +228,5 @@
>        throw new Error(arguments.callee.name + ": Add-ons Manager has been closed.");
>      }
>  
> +    // XXX bug 837008
> +    // we restore the window when closing the Addons Manager

Same here. And please always start with a capital letter for comments.
Attachment #709701 - Flags: review?(hskupin)
Attachment #709701 - Flags: review?(dave.hunt)
Attachment #709701 - Flags: review?(andreea.matei)
Attachment #709701 - Flags: review-
Depends on: 837008
No longer blocks: 783484
Attached patch patch v5 (obsolete) — Splinter Review
Fixed the comments.
Attachment #709701 - Attachment is obsolete: true
Attachment #710102 - Flags: review?(andreea.matei)
Blocks: 783484
Comment on attachment 710102 [details] [diff] [review]
patch v5

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

Lets see how this goes and if everything is ok, please check for backport since this failure is on all branches.
Attachment #710102 - Flags: review?(andreea.matei) → review+
Comment on attachment 710102 [details] [diff] [review]
patch v5

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

Just a note for you Andrei. Please make sure to stick to our commit message format, including capital letters at the beginning.

> bug 780957. resize the window whenever we open the Addons Manager to ensure enough screen realestate for all UI elements. r=hskupin, r=dhunt, r=amatei

Bug 780957 - Resize...

Landed on default as:
http://hg.mozilla.org/qa/mozmill-tests/rev/662f8d763942
Attachment #710102 - Flags: checkin+
Please check tomorrows results so we can get this backported. Thanks.
Landed on esr17 for now so we can see if it is fixed:
http://hg.mozilla.org/qa/mozmill-tests/rev/0d19f9e6e63a (esr17)

If that's the case we can port it across all the other branches.
Looks like it hasn't fixed the problem. I leave the patch in for now so Andrei can investigate.
We need to back this out since it didn't really fix the problem, investigating other avenues like:
- using keyboard shortcuts to make the element scroll

The problem persists in esr17, release, beta but works in aurora and nightly.
Here is a pushlog on when scrollIntoView() started correctly working on the items in the Addons Manager:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=d2fbc67f69f5&tochange=0352a32fde64

(For some reason I am unable to build these said versions on OS X "ImportError: No module named buildconfig")

scrollIntoView() appears not to be present on <richlistitems> elements up to that point.
(In reply to Andrei Eftimie from comment #57)
> scrollIntoView() appears not to be present on <richlistitems> elements up to
> that point.

If that's the case you might want to have a look at MXR. You can blame the lines in the appropriate file to see when it has been added. It might correlate with the pushlog you pointed out above.
Attached patch patch v6Splinter Review
Added a new method to the Addons Manager class, to scroll the Addon into view via the ensureElementIsVisible method.

Fixes the problem for older versions of FF where the richlistitem element does not have the scrollIntoView() method.

Needed only on esr17, release and beta branches.
Attachment #710102 - Attachment is obsolete: true
Attachment #711720 - Flags: feedback?(hskupin)
Attachment #711720 - Flags: feedback?(dave.hunt)
Attachment #711720 - Flags: feedback?(andreea.matei)
(In reply to Andrei Eftimie from comment #59)
> Fixes the problem for older versions of FF where the richlistitem element
> does not have the scrollIntoView() method.

I would still love to see which fix made this available in the addons manager. Blair, can you remember?
Flags: needinfo?(bmcbride)
Comment on attachment 711720 [details] [diff] [review]
patch v6

That's even a way better solution. Great find, Andrei! I'm fully behind that for the older branches.
Attachment #711720 - Flags: feedback?(hskupin)
Attachment #711720 - Flags: feedback?(dave.hunt)
Attachment #711720 - Flags: feedback?(andreea.matei)
Attachment #711720 - Flags: feedback+
Comment on attachment 711720 [details] [diff] [review]
patch v6

This appears to be where scrollIntoView() is defined.

Some more information about scrollIntoview() and XUL:
https://developer.mozilla.org/en-US/docs/XUL/scrollbox#_Scrolling_a_child_element_into_view_
https://bugzilla.mozilla.org/show_bug.cgi?id=660561
https://bugzilla.mozilla.org/show_bug.cgi?id=364612

Have not found exactly where and when it was implemented...
Attachment #711720 - Flags: review?(hskupin)
Attachment #711720 - Flags: review?(dave.hunt)
Attachment #711720 - Flags: review?(andreea.matei)
(In reply to Henrik Skupin (:whimboo) [away 02/09 - 02/017] from comment #60)
> I would still love to see which fix made this available in the addons
> manager. Blair, can you remember?

As comment 62 and 63 alluded, scrollIntoView() isn't specific to the Add-ons Manager. AFAIK, all elements have it now days (originally it was only HTML elements), thanks to bug 804950, which seems to have landed in Fx19.
Flags: needinfo?(bmcbride)
Comment on attachment 711720 [details] [diff] [review]
patch v6

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

Landed on ESR17, I say we wait for those results to make sure it's fixed before landing on the other two branches.

http://hg.mozilla.org/qa/mozmill-tests/rev/ba024b6c3410 (esr17)
Attachment #711720 - Flags: review?(hskupin)
Attachment #711720 - Flags: review?(dave.hunt)
Attachment #711720 - Flags: review?(andreea.matei)
Attachment #711720 - Flags: review+
No more failures on ESR17, landed on beta and release as well:
http://hg.mozilla.org/qa/mozmill-tests/rev/22508faadc59 (release)
http://hg.mozilla.org/qa/mozmill-tests/rev/bf43ce6fe639 (beta)

Thanks Andrei!
Status: ASSIGNED → RESOLVED
Closed: 8 years ago8 years ago
Resolution: --- → FIXED
(In reply to Blair McBride [:Unfocused] (Back from the dead. Mostly.) from comment #64)
> As comment 62 and 63 alluded, scrollIntoView() isn't specific to the Add-ons
> Manager. AFAIK, all elements have it now days (originally it was only HTML
> elements), thanks to bug 804950, which seems to have landed in Fx19.

Andrei, can you please tell me why we landed the workaround for firefox19 given the information from Blair?
Flags: needinfo?(andrei.eftimie)
Based on testing the actual feature, it has been enabled only in 20.
It does not exist in 19.

Retested now and both the Release and Beta channel which run on the 19 branch miss this feature.
So we're good with which branches our fix landed.
Flags: needinfo?(andrei.eftimie)
Thanks Andrei. So that means another patch made this possible beside the one from bug 804950. I don't see a reason to further investigate. Lets mark it as done.
Product: Mozilla QA → Mozilla QA Graveyard
You need to log in before you can comment on or make changes to this bug.