Spun off from bug 381269 comment 29. http://mxr.mozilla.org/comm-central/source/suite/modules/test/browser_sanitizer.js#242 http://mxr.mozilla.org/comm-central/source/suite/modules/Sanitizer.jsm#248 http://mxr.mozilla.org/comm-central/source/mozilla/toolkit/components/passwordmgr/test/browser/browser_passwordmgrdlg.js What I found out is that 1) If I comment out http://mxr.mozilla.org/comm-central/source/suite/modules/test/browser_sanitizer.js?mark=302-304#300 then I get the same exception there... 2) This refers to bug 400238, which is marked as fixed in NSS 3.12rc1, and SM 2.0 (now) uses NSS 3.12.3. Robert, was it the right bug number? Or could our /suite/ files need to be updated (for Core fixes that would have happened in the meantime)?

Bug 381269 comment 24: { TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/toolkit/components/passwordmgr/test/browser/browser_passwordmgrdlg.js | Exception thrown - [Exception... "'User canceled master password entry, login not added.' when calling method: [nsILoginManagerStorage::addLogin]" nsresult: "0x8057001e (NS_ERROR_XPC_JS_THREW_STRING)" location: "JS frame :: file:///e:/builds/slave/comm-1.9.1-win32-unittest/build/objdir/mozilla/dist/bin/components/nsLoginManager.js :: anonymous :: line 454" data: no] }

(In reply to comment #0) > Robert, was it the right bug number? > Or could our /suite/ files need to be updated (for Core fixes that would have > happened in the meantime)? No idea, but Mark could know something on LoginManager.

Maybe this test has more threading issues and would need to be converted to more "executeSoon(callbacks)" steps? Anyway, someone with more LoginManager knowledge should look at this.

Comment on attachment 405330 [details] [diff] [review] (Av1) browser_sanitizer.js: Add needed executeSoon(), Swap disabled 'passwords' part from one loop to the other (ftb) I can't dig my head into this right now, need to care about release stuff. Neil, can you please take a look?

Is this a 1.9.1 fix only? I only have a trunk test-enabled build, and passwordmgrdlg.js fails for me for a full test run with or without this patch.

I'm testing with [Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.1.5pre) Gecko/20091008 SeaMonkey/2.0pre] (comm-1.9.1-win32-unittest/1255010081) (W2Ksp4) (http://hg.mozilla.org/releases/mozilla-1.9.1/rev/bcfccb9bfd02 +http://hg.mozilla.org/comm-central/rev/7de852d0812e) You're right: patch Av1 fixed running the 2 tests alone only. But, to pass the whole suite, browser_feed_tab.js needs to be disabled too. Again, I couldn't understand what the (underlying) relation between these 3 tests is...

(In reply to comment #6) > You're right: patch Av1 fixed running the 2 tests alone only. Oh? How is it possible to do just those two tests?

(In reply to comment #7) > Oh? How is it possible to do just those two tests? Manually backup/delete all the others ;->

Comment on attachment 405816 [details] [diff] [review] (Av1a) browser_sanitizer.js: Add needed executeSoon(), Swap disabled 'passwords' part from one loop to the other (ftb); (Improve but) Disable browser_feed_tab.js (ftb) So, browser_sanitizer breaks browser_passwordmgrdlg; I can confirm this by disabling browser_sanitizer, but I didn't even have to disable browser_feed_tab!

(In reply to comment #5) > Is this a 1.9.1 fix only? I only have a trunk test-enabled build, and > passwordmgrdlg.js fails for me for a full test run with or without this patch. My two patches are based on what I reproduced locally with the cited 2.0 Windows build. (In reply to comment #9) > (From update of attachment 405816 [details] [diff] [review]) > So, browser_sanitizer breaks browser_passwordmgrdlg; I can confirm this by > disabling browser_sanitizer, but I didn't even have to disable > browser_feed_tab! Should you un-obsolete patch Av1 and r+ it, then? Otherwise, could you tell me explicitly what kind of patch you expect?

(In reply to comment #10) > (In reply to comment #9) > > (From update of attachment 405816 [details] [diff] [review] [details]) > > So, browser_sanitizer breaks browser_passwordmgrdlg; I can confirm this by > > disabling browser_sanitizer, but I didn't even have to disable > > browser_feed_tab! > Should you un-obsolete patch Av1 and r+ it, then? No; neither patch allows browser_passwordmgrdlg to work for me.

(In reply to comment #10) > (In reply to comment #9) > > (From update of attachment 405816 [details] [diff] [review]) > > So, browser_sanitizer breaks browser_passwordmgrdlg > could you tell me explicitly what kind of patch you expect? Given that browser_passwordmgrdlg works if you do a full test run without browser_sanitizer the patch would have to modify browser_sanitizer to do as many sanitizer tests as possible without breaking browser_passwordmgrdlg.

Actually from reading the dependent bugs it looks like there's an issue whereby if you log in to the SDR and then logout and tear down it won't always log back in correctly :-(

Av1, with comment 12 suggestion(s).

(In reply to comment #13) > Actually from reading the dependent bugs it looks like there's an issue whereby > if you log in to the SDR and then logout and tear down it won't always log back > in correctly :-( Yeah, I read something like that. Yet, it feels easier to disable SeaMonkey tests ftb, though the cause could likely be in Toolkit (test) :-/ (In reply to comment #14) > (Av1b) browser_sanitizer.js: Add needed executeSoon(), (Explicitly) Disable all > 'passwords' parts (ftb) Fwiw, I had already tried this for my two previous patches and it didn't help me. But, if it helps you, we can check this in then see what happens.

This sounds a lot like bug 437904.

With { prefs.setBoolPref("signon.debug", true); } added to browser-test.js.

(In reply to comment #17) > signon.debug cmd console log (1) [Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.1.5pre) Gecko/20091008 SeaMonkey/2.0pre] (comm-1.9.1-win32-unittest/1255010081) (W2Ksp4) Running the two tests only.

Just by chance, I discovered that this function call will log you out if you have a master password, but won't confuse NSS if you don't.

Comment on attachment 406889 [details] [diff] [review] Proposed patch [Checkin: Comment 23] Question is, is this designed functionality or accidental? If the latter should we be relying on it?

(In reply to comment #20) > (From update of attachment 406889 [details] [diff] [review]) > Question is, is this designed functionality or accidental? If the latter should > we be relying on it? I'll ask in the crypto newsgroup.

(In reply to comment #20) > (From update of attachment 406889 [details] [diff] [review]) > Question is, is this designed functionality or accidental? If the latter should > we be relying on it? It seems to be by design, see PK11_CheckUserPassword's comment.

Pushed changeset ad433d192a3d to comm-central. I guess we need to wait for the tests to cycle...

OK, browser_passwordmgrdlg tests seem to be passing now.

Comment on attachment 408242 [details] [diff] [review] (Av2) browser_sanitizer.js: Add needed executeSoon(), Re-enable disabled 'passwords' part; Improve browser_feed_tab.js too [Split into patch Av3 and bug 735143] >+ let testTab = gBrowser.addTab(); [Suite's addTab's first parameter isn't really optional; omitting it leads to "undefined" appearing in the URLbar... I wonder whether we should change that, or alternatively try passing the http URL directly in here.] > var observer = { > observe: function(win, topic, data) { >- if (topic != "page-info-dialog-loaded") >- return; >+ obs.removeObserver(observer, "page-info-dialog-loaded"); > >- obs.removeObserver(observer, "page-info-dialog-loaded"); >+ // Proceed with test. Don't bother with these changes. >+ is(feedRowsNum, 3, "Number of feeds listed"); As is() doesn't log its parameters (at least, not on success), maybe the message should be "3 feeds listed"? [I don't know whether it would be a good idea to change is() to log the passing value too.] >+ is(feedItem.getAttribute("name"), i+1, "Name " + i); This is logging the wrong value. >+ // Close 'Page Info' window. Did you outsource your comments to Captain Obvious? >+ // executeSoon() prevents (in the "pass" case only) leaking 'Console message's (...) to the next test. Console messages

(In reply to comment #26) > [Suite's addTab's first parameter isn't really optional; omitting it leads to > "undefined" appearing in the URLbar... I wonder whether we should change that, > or alternatively try passing the http URL directly in here.] If the Firefox version allows that parameter to be optional, we probably should match that as well as the behavior they follow there, even if "just" for add-on compatibility.

Av2, browser_sanitizer.js part only, with comment 26 suggestion(s).

Comment on attachment 416337 [details] [diff] [review] (Av3) browser_sanitizer.js: Add needed executeSoon(), Re-enable disabled 'passwords' part [Checkin: Comment 30 & 31] http://hg.mozilla.org/comm-central/rev/d06dfd029724 "approval-seamonkey2.0.2=?": No risk, test only.

Comment on attachment 416337 [details] [diff] [review] (Av3) browser_sanitizer.js: Add needed executeSoon(), Re-enable disabled 'passwords' part [Checkin: Comment 30 & 31] http://hg.mozilla.org/releases/comm-1.9.1/rev/b7d6282b3b10

(In reply to Robert Kaiser (:kairo@mozilla.com) from comment #27) > (In reply to comment #26) > > [Suite's addTab's first parameter isn't really optional; omitting it leads to > > "undefined" appearing in the URLbar... I wonder whether we should change that, > > or alternatively try passing the http URL directly in here.] > > If the Firefox version allows that parameter to be optional, we probably > should match that as well as the behavior they follow there, even if "just" > for add-on compatibility. Eventually fixed by bug 536940.

Comment on attachment 408242 [details] [diff] [review] (Av2) browser_sanitizer.js: Add needed executeSoon(), Re-enable disabled 'passwords' part; Improve browser_feed_tab.js too [Split into patch Av3 and bug 735143] (In reply to neil@parkwaycc.co.uk from comment #26) I moved browser_feed_tab.js part to bug 735143.