Last Comment Bug 521263 - After bug 381269 landing, browser_sanitizer.js breaks browser_passwordmgrdlg.js
: After bug 381269 landing, browser_sanitizer.js breaks browser_passwordmgrdlg.js
Status: RESOLVED FIXED
: fixed-seamonkey2.0.1, fixed-seamonkey2.0.3, regression
Product: SeaMonkey
Classification: Client Software
Component: Passwords & Permissions (show other bugs)
: Trunk
: All All
: -- normal with 1 vote (vote)
: ---
Assigned To: neil@parkwaycc.co.uk
:
:
Mentors:
: 523345 (view as bug list)
Depends on: 394686 416233 437904
Blocks: 381269
  Show dependency treegraph
 
Reported: 2009-10-08 09:55 PDT by Serge Gautherie (:sgautherie)
Modified: 2012-03-12 20:32 PDT (History)
5 users (show)
bugzillamozillaorg_serge_20140323: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
(Av1) browser_sanitizer.js: Add needed executeSoon(), Swap disabled 'passwords' part from one loop to the other (ftb) (2.17 KB, patch)
2009-10-08 14:12 PDT, Serge Gautherie (:sgautherie)
no flags Details | Diff | Splinter 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) (4.85 KB, patch)
2009-10-11 19:20 PDT, Serge Gautherie (:sgautherie)
neil: review-
Details | Diff | Splinter Review
(Av1b) browser_sanitizer.js: Add needed executeSoon(), (Explicitly) Disable all 'passwords' parts (ftb) (2.53 KB, patch)
2009-10-13 09:58 PDT, Serge Gautherie (:sgautherie)
no flags Details | Diff | Splinter Review
signon.debug cmd console log (1) (3.35 KB, text/plain)
2009-10-13 16:51 PDT, Serge Gautherie (:sgautherie)
no flags Details
Proposed patch [Checkin: Comment 23] (2.14 KB, patch)
2009-10-17 16:11 PDT, neil@parkwaycc.co.uk
iann_bugzilla: review+
kairo: approval‑seamonkey2.0.1+
Details | Diff | Splinter 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] (4.48 KB, patch)
2009-10-24 19:13 PDT, Serge Gautherie (:sgautherie)
neil: review-
Details | Diff | Splinter Review
(Av3) browser_sanitizer.js: Add needed executeSoon(), Re-enable disabled 'passwords' part [Checkin: Comment 30 & 31] (1.94 KB, patch)
2009-12-06 09:32 PST, Serge Gautherie (:sgautherie)
neil: review+
kairo: approval‑seamonkey2.0.3+
Details | Diff | Splinter Review

Description Serge Gautherie (:sgautherie) 2009-10-08 09:55:53 PDT
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)?
Comment 1 Serge Gautherie (:sgautherie) 2009-10-08 10:11:37 PDT
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]
}
Comment 2 Robert Kaiser 2009-10-08 11:44:32 PDT
(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.
Comment 3 Serge Gautherie (:sgautherie) 2009-10-08 14:12:00 PDT
Created attachment 405330 [details] [diff] [review]
(Av1) browser_sanitizer.js: Add needed executeSoon(), Swap disabled 'passwords' part from one loop to the other (ftb)

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 4 Robert Kaiser 2009-10-08 14:16:33 PDT
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?
Comment 5 neil@parkwaycc.co.uk 2009-10-11 16:26:32 PDT
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.
Comment 6 Serge Gautherie (:sgautherie) 2009-10-11 19:20:49 PDT
Created 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)

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...
Comment 7 neil@parkwaycc.co.uk 2009-10-12 05:10:49 PDT
(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?
Comment 8 Serge Gautherie (:sgautherie) 2009-10-12 06:05:10 PDT
(In reply to comment #7)
> Oh? How is it possible to do just those two tests?

Manually backup/delete all the others ;->
Comment 9 neil@parkwaycc.co.uk 2009-10-12 09:44:50 PDT
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!
Comment 10 Serge Gautherie (:sgautherie) 2009-10-12 11:06:32 PDT
(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?
Comment 11 neil@parkwaycc.co.uk 2009-10-13 01:29:37 PDT
(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.
Comment 12 neil@parkwaycc.co.uk 2009-10-13 01:32:40 PDT
(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.
Comment 13 neil@parkwaycc.co.uk 2009-10-13 02:01:34 PDT
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 :-(
Comment 14 Serge Gautherie (:sgautherie) 2009-10-13 09:58:54 PDT
Created attachment 406036 [details] [diff] [review]
(Av1b) browser_sanitizer.js: Add needed executeSoon(), (Explicitly) Disable all 'passwords' parts (ftb)

Av1, with comment 12 suggestion(s).
Comment 15 Serge Gautherie (:sgautherie) 2009-10-13 10:05:00 PDT
(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.
Comment 16 Justin Dolske [:Dolske] 2009-10-13 13:48:04 PDT
This sounds a lot like bug 437904.
Comment 17 Serge Gautherie (:sgautherie) 2009-10-13 16:51:04 PDT
Created attachment 406138 [details]
signon.debug cmd console log (1)

With
{
  prefs.setBoolPref("signon.debug", true);
}
added to browser-test.js.
Comment 18 Serge Gautherie (:sgautherie) 2009-10-13 16:54:03 PDT
(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.
Comment 19 neil@parkwaycc.co.uk 2009-10-17 16:11:12 PDT
Created attachment 406889 [details] [diff] [review]
Proposed patch
[Checkin: Comment 23]

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 20 Ian Neal 2009-10-17 16:30:11 PDT
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?
Comment 21 neil@parkwaycc.co.uk 2009-10-17 16:31:53 PDT
(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.
Comment 22 neil@parkwaycc.co.uk 2009-10-19 05:32:39 PDT
(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.
Comment 23 neil@parkwaycc.co.uk 2009-10-22 03:41:50 PDT
Pushed changeset ad433d192a3d to comm-central.

I guess we need to wait for the tests to cycle...
Comment 24 neil@parkwaycc.co.uk 2009-10-22 11:39:29 PDT
OK, browser_passwordmgrdlg tests seem to be passing now.
Comment 25 Serge Gautherie (:sgautherie) 2009-10-24 19:13:36 PDT
Created 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]
Comment 26 neil@parkwaycc.co.uk 2009-10-25 09:59:17 PDT
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
Comment 27 Robert Kaiser 2009-10-25 10:30:09 PDT
(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.
Comment 28 Robert Kaiser 2009-12-01 07:52:09 PST
*** Bug 523345 has been marked as a duplicate of this bug. ***
Comment 29 Serge Gautherie (:sgautherie) 2009-12-06 09:32:02 PST
Created attachment 416337 [details] [diff] [review]
(Av3) browser_sanitizer.js: Add needed executeSoon(), Re-enable disabled 'passwords' part
[Checkin: Comment 30 & 31]

Av2, browser_sanitizer.js part only, with comment 26 suggestion(s).
Comment 30 Serge Gautherie (:sgautherie) 2009-12-06 15:09:28 PST
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 31 Serge Gautherie (:sgautherie) 2009-12-06 16:00:14 PST
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
Comment 32 Serge Gautherie (:sgautherie) 2012-03-12 18:39:10 PDT
(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 33 Serge Gautherie (:sgautherie) 2012-03-12 20:32:17 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.