Closed
Bug 915554
Opened 11 years ago
Closed 11 years ago
Fix mozmill 2.0 failures on ESR17
Categories
(Testing Graveyard :: Mozmill, defect, P2)
Testing Graveyard
Mozmill
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
Assignee | ||
Comment 1•11 years ago
|
||
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
Assignee | ||
Comment 2•11 years ago
|
||
Some of these failures are going to be fixed (as expected) by bug 885221
Comment 3•11 years ago
|
||
(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.
Comment 4•11 years ago
|
||
(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.
Assignee | ||
Comment 5•11 years ago
|
||
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.
Assignee | ||
Comment 6•11 years ago
|
||
windowMap log from mozmill 1.5
Assignee | ||
Comment 7•11 years ago
|
||
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)
Assignee | ||
Comment 8•11 years ago
|
||
small update to the testcase to work in both mozmill 1.5 and 2.0
Attachment #805284 -
Attachment is obsolete: true
Assignee | ||
Comment 9•11 years ago
|
||
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
Assignee | ||
Comment 10•11 years ago
|
||
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.
Assignee | ||
Comment 11•11 years ago
|
||
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)
Assignee | ||
Comment 12•11 years ago
|
||
I've file bug 917245 for the 1 remaining issue. We've got green run on remote: http://mozmill-crowd.blargon7.com/#/remote/report/dc3d4d2f66af31b9e75e291fa0d2f531 and functional: http://mozmill-crowd.blargon7.com/#/functional/report/dc3d4d2f66af31b9e75e291fa0d2cb1c
Updated•11 years ago
|
Attachment #805838 -
Attachment mime type: text/x-log → text/plain
Updated•11 years ago
|
Attachment #805848 -
Attachment mime type: text/x-log → text/plain
Comment 13•11 years ago
|
||
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+
Comment 14•11 years ago
|
||
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+]
Assignee | ||
Comment 15•11 years ago
|
||
> 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 16•11 years ago
|
||
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+
Assignee | ||
Comment 17•11 years ago
|
||
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 18•11 years ago
|
||
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+
Comment 19•11 years ago
|
||
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]
Updated•8 years ago
|
Product: Testing → Testing Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•