Closed Bug 915554 Opened 11 years ago Closed 11 years ago

Fix mozmill 2.0 failures on ESR17

Categories

(Testing Graveyard :: Mozmill, defect, P2)

defect

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: andrei, Assigned: andrei)

References

()

Details

(Whiteboard: [mozmill-2.0+][ateamtrack: p=mozmill q=2013q3 m=3])

Attachments

(4 files, 4 obsolete files)

We're seeing lots of failures on ESR17 while running mozmill-tests under mozmill-2.0

Most of the failures have probably the same issue, and fail with:
> controller(): Window has been initialized.

This should probably be fixed before the 2.0 release.
ESR17 will still be around for a couple of months, if we'll want to transition to using mozmill 2.0 in CI before that, we need to get this fixed.

This is a testrun report:
http://mozmill-crowd.blargon7.com/#/functional/report/dc3d4d2f66af31b9e75e291fa0151bb8
These tests pass when run independently, we probably have a hang earlier, and we fail to properly register newer controllers later on.
Assignee: nobody → andrei.eftimie
Some of these failures are going to be fixed (as expected) by bug 885221
(In reply to Andrei Eftimie from comment #0)
> http://mozmill-crowd.blargon7.com/#/functional/report/
> dc3d4d2f66af31b9e75e291fa0151bb8

It will probably not change something, but as I have said already early last week, you are using a very outdated version of ESR17. Just in case if there was a possible regression.
(In reply to Andrei Eftimie from comment #2)
> Some of these failures are going to be fixed (as expected) by bug 885221

Which failures will  remain? Lets get this bug done ASAP.
Attached file testcase.js (obsolete) —
I've found the major culprit: the old Private Browsing Mode

Most of the failures are gone when skipping the PB tests:
functional: http://mozmill-crowd.blargon7.com/#/remote/report/dc3d4d2f66af31b9e75e291fa0b40cd7
remote: http://mozmill-crowd.blargon7.com/#/functional/report/dc3d4d2f66af31b9e75e291fa0b50b64

It seems once we've initiated (then existed) PB, mozmill is unable to register new controllers.

The attached testcase exhibits the problem. Run under mozmill 2.0 with ESR17

I'll now look into mozmill to find out exactly what is happening.
Attached file mozmill1.5.log
windowMap log from mozmill 1.5
Attached file mozmill2.log (obsolete) —
windowMap log from mozmill 2.0

==

There are some differences in how we treated the Normal windows when in PB in 1.5
Notice that "3" is never losing its "loaded" state (even if it probably should have while we were in PB).

For 2.0 we start with two windowID's: 3 and 7
At the end 7 is updating its loaded status (since it is probably reloading) while 3 isn't (and that is where we fail, since we are waiting for 3 to be loaded).

I'm not sure of the status of the 3. Might that be a "special" window? the chrome?.

If that is the case we might want to do what we do in mozmill 1.5, never "unload" 3.

If it is not, we should restore its loaded status once we return from PB.

Henrik, Dave any insights into this?
Flags: needinfo?(hskupin)
Flags: needinfo?(dave.hunt)
Attached file testcase.js
small update to the testcase to work in both mozmill 1.5 and 2.0
Attachment #805284 - Attachment is obsolete: true
Attached file mozmill2.log
I've included more information in the log, and it is clear that we unload browser.xul when starting PB, and we never reload it.

The initial question still remains, should we never unload it (like mozmill 1.5) or should we reload it when exiting PB mode? (I'm inclined to go for that latter)

I assume we load another browser.xul for PB, but we don't see it referenced due to the nature of PB?
Attachment #805842 - Attachment is obsolete: true
I've tried (hacky hardcode) to never unload browser.xul, but that completely throws mozmill off: we fail with a Disconnect Error, so we most probably need to reload it once we exit PB mode.
So I've re-attached all EventListeners whenever we enter and exit Private Browsing Mode using an observer for that.

Not sure if this is overkill.

Testrun with this patch (+ bug 885221) applied:
http://mozmill-crowd.blargon7.com/#/functional/report/dc3d4d2f66af31b9e75e291fa0d011e4

Only 1 failure remaining.

We don't seem to regress anything on Nightly:
http://mozmill-crowd.blargon7.com/#/functional/report/dc3d4d2f66af31b9e75e291fa0d0b85c
Attachment #805927 - Flags: feedback?(hskupin)
Attachment #805927 - Flags: feedback?(dave.hunt)
Flags: needinfo?(hskupin)
Flags: needinfo?(dave.hunt)
Depends on: 917245
Attachment #805838 - Attachment mime type: text/x-log → text/plain
Attachment #805848 - Attachment mime type: text/x-log → text/plain
Comment on attachment 805927 [details] [diff] [review]
0001-Bug-915554-Mozmill-2.0-to-handle-the-old-Private-Bro.patch

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

That sounds like the proper solution for it. At least when seeing the observer notifications as listed here:
https://developer.mozilla.org/en-US/docs/Observer_Notifications#Private_browsing

Beside the comments inline I would also like to see a simple Mutt test. Good work Andrei!

::: mozmill/mozmill/extension/resource/modules/windows.js
@@ +157,5 @@
>    }
>  };
>  
> +// Observer when Private Browsing Mode is enabled/disabled
> +var privateProwsingMode = {

It's 'Browsing'. Beside that please also add Enter/Leave to the observer name.
Attachment #805927 - Flags: feedback?(hskupin)
Attachment #805927 - Flags: feedback?(dave.hunt)
Attachment #805927 - Flags: feedback+
ESR17 is still supported for a while, so we cannot let this regression slip through Mozmill 2.0.
Whiteboard: [mozmill-2.0?] → [mozmill-2.0+]
> I would also like to see a simple Mutt test.

I've also added a mutt test.
Given that we need to skip it for all newer FF versions since the PB Mode is not available anymore, I'm not sure of its usefulness.

I've corrected the typo, enhanced the observer name, and added a comment to remove the code once we no longer support ESR17 referencing this bug.
Attachment #805927 - Attachment is obsolete: true
Attachment #806480 - Flags: review?(hskupin)
Attachment #806480 - Flags: review?(dave.hunt)
Comment on attachment 806480 [details] [diff] [review]
0001-Bug-915554-Mozmill-2.0-to-handle-the-old-Private-Bro.patch

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

More style nits. Once done we can get it landed. r=me with those updates.

::: mozmill/mozmill/extension/resource/modules/windows.js
@@ +158,5 @@
>  };
>  
> +// Bug 915554
> +// Support for the old Private Browsing Mode,
> +// remove once ESR17 is no longer supported

Make this a ´TODO´ item.

@@ +287,5 @@
>    observerService.addObserver(windowCloseObserver, "outer-window-destroyed", false);
> +  // Bug 915554
> +  // Support for the old Private Browsing Mode,
> +  // remove once ESR17 is no longer supported
> +  observerService.addObserver(enterLeavePrivateBrowsingObserver, "private-browsing", false);

No need to do the comment twice. I would keep it at the other location.

::: mutt/mutt/tests/js/testController/testEnterLeavePrivateBrowsing.js
@@ +5,5 @@
> +"use strict";
> +
> +Cu.import("resource://gre/modules/Services.jsm");
> +
> +const PB_NO_PROMPT_PREF = 'browser.privatebrowsing.dont_prompt_on_enter';

PREF should be the prefix.

@@ +15,5 @@
> +  aModule.prefBranch = Services.prefs.QueryInterface(Ci.nsIPrefBranch);
> +  aModule.prefBranch.setBoolPref(PB_NO_PROMPT_PREF, true);
> +}
> +
> +var teardownModule = function(aModule) {

nit: you know that there should be a blank beofre the opening bracket.

@@ +19,5 @@
> +var teardownModule = function(aModule) {
> +  aModule.prefBranch.clearUserPref(PB_NO_PROMPT_PREF);
> +}
> +
> +var test = function() {

Why this inconsistency between the function definitions? Don't use var.

@@ +63,5 @@
> +  }
> +}
> +
> +// Bug 915554
> +// Only run this test against builds that have the old Private Browsing Mode (eg. ESR17)

Make it a ´TODO´ item.

@@ +69,5 @@
> +  var _pbs = Cc["@mozilla.org/privatebrowsing;1"].getService(Ci.nsIPrivateBrowsingService);
> +}
> +catch (error) {
> +  setupModule.__force_skip__ = "Bug 915554 - This test needs the old PB Mode available"
> +  teardownModule.__force_skip__ = "Bug 915554 - This test needs the old PB Mode available"

I would mention Firefox 17 here, so it's clear that we have to remove the test once it has been desupported.
Attachment #806480 - Flags: review?(hskupin)
Attachment #806480 - Flags: review?(dave.hunt)
Attachment #806480 - Flags: review+
Addressed the issues from the review.

Updated the function declarations from the mutt test, and raised bug 918243 to change all of them (atm we have 9 tests using `function setupModule` and 30 using `var setupModule = function`)

Still green run for ESR17:
http://mozmill-crowd.blargon7.com/#/functional/report/1039ea48a9d69a5a1cc4fd228c3d22e7
Attachment #806480 - Attachment is obsolete: true
Attachment #807120 - Flags: review?(hskupin)
Comment on attachment 807120 [details] [diff] [review]
0001-Bug-915554-Mozmill-2.0-to-handle-the-old-Private-Bro.patch

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

Looks fine. I will get this landed.
Attachment #807120 - Flags: review?(hskupin) → review+
Landed as:
https://github.com/mozilla/mozmill/commit/2f85a34e36a150ad521eab04ca5148abbdac991b (master)
https://github.com/mozilla/mozmill/commit/b0c40ced128fd3811eb9e32baf1e9c9099a5402f (hotfix-2.0)
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Whiteboard: [mozmill-2.0+] → [mozmill-2.0+][ateamtrack: p=mozmill q=2013q3 m=3]
Product: Testing → Testing Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: