The default bug view has changed. See this FAQ.

After bug 381269 landing, browser_sanitizer.js breaks browser_passwordmgrdlg.js

RESOLVED FIXED

Status

SeaMonkey
Passwords & Permissions
RESOLVED FIXED
8 years ago
5 years ago

People

(Reporter: sgautherie, Assigned: neil@parkwaycc.co.uk)

Tracking

({fixed-seamonkey2.0.1, fixed-seamonkey2.0.3, regression})

Trunk
fixed-seamonkey2.0.1, fixed-seamonkey2.0.3, regression
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 4 obsolete attachments)

3.35 KB, text/plain
Details
2.14 KB, patch
Ian Neal
: review+
Details | Diff | Splinter Review
1.94 KB, patch
neil@parkwaycc.co.uk
: review+
Details | Diff | Splinter Review
(Reporter)

Description

8 years ago
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)?

Updated

8 years ago
Summary: After bug 381269 landing, browser_sanitizer.js brakes browser_passwordmgrdlg.js → After bug 381269 landing, browser_sanitizer.js breaks browser_passwordmgrdlg.js
(Reporter)

Comment 1

8 years ago
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

8 years ago
(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.
(Reporter)

Updated

8 years ago
Depends on: 521305
(Reporter)

Comment 3

8 years ago
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.
Attachment #405330 - Flags: review?(kairo)

Comment 4

8 years ago
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?
Attachment #405330 - Flags: review?(kairo) → review?(neil)
(Assignee)

Comment 5

8 years ago
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.
(Reporter)

Comment 6

8 years ago
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...
Attachment #405330 - Attachment is obsolete: true
Attachment #405816 - Flags: review?(neil)
Attachment #405330 - Flags: review?(neil)
(Assignee)

Comment 7

8 years ago
(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?
(Reporter)

Comment 8

8 years ago
(In reply to comment #7)
> Oh? How is it possible to do just those two tests?

Manually backup/delete all the others ;->
(Assignee)

Updated

8 years ago
Attachment #405816 - Flags: review?(neil) → review-
(Assignee)

Comment 9

8 years ago
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!
(Reporter)

Comment 10

8 years ago
(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?
(Assignee)

Comment 11

8 years ago
(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.
(Assignee)

Comment 12

8 years ago
(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.
(Assignee)

Comment 13

8 years ago
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 :-(
(Reporter)

Updated

8 years ago
Depends on: 394686
(Reporter)

Comment 14

8 years ago
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).
Attachment #406036 - Flags: review?(neil)
(Reporter)

Comment 15

8 years ago
(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.
(Reporter)

Comment 17

8 years ago
Created attachment 406138 [details]
signon.debug cmd console log (1)

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

Updated

8 years ago
Depends on: 437904
(Reporter)

Comment 18

8 years ago
(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.
(Assignee)

Comment 19

8 years ago
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.
Assignee: nobody → neil
Attachment #406889 - Flags: review?(iann_bugzilla)

Comment 20

8 years ago
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?
Attachment #406889 - Flags: review?(iann_bugzilla) → review+
(Assignee)

Comment 21

8 years ago
(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.
(Assignee)

Comment 22

8 years ago
(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.
(Assignee)

Updated

8 years ago
Attachment #406889 - Flags: approval-seamonkey2.0.1?

Updated

8 years ago
Attachment #406889 - Flags: approval-seamonkey2.0.1? → approval-seamonkey2.0.1+

Updated

8 years ago
Blocks: 523345
(Assignee)

Comment 23

8 years ago
Pushed changeset ad433d192a3d to comm-central.

I guess we need to wait for the tests to cycle...
(Assignee)

Comment 24

8 years ago
OK, browser_passwordmgrdlg tests seem to be passing now.
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Keywords: fixed-seamonkey2.0.1
Resolution: --- → FIXED
(Reporter)

Updated

8 years ago
Attachment #405816 - Attachment is obsolete: true
(Reporter)

Updated

8 years ago
Attachment #406889 - Attachment description: Proposed patch → Proposed patch [Checkin: Comment 23]
(Reporter)

Comment 25

8 years ago
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]
Attachment #406036 - Attachment is obsolete: true
Attachment #408242 - Flags: review?(neil)
Attachment #406036 - Flags: review?(neil)
(Assignee)

Comment 26

8 years ago
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
Attachment #408242 - Flags: review?(neil) → review-

Comment 27

8 years ago
(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.

Updated

7 years ago
Duplicate of this bug: 523345
(Reporter)

Updated

7 years ago
No longer blocks: 523345
(Reporter)

Comment 29

7 years ago
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).
Attachment #416337 - Flags: review?(neil)
(Assignee)

Updated

7 years ago
Attachment #416337 - Flags: review?(neil) → review+
(Reporter)

Comment 30

7 years ago
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.
Attachment #416337 - Attachment description: (Av3) browser_sanitizer.js: Add needed executeSoon(), Re-enable disabled 'passwords' part. → (Av3) browser_sanitizer.js: Add needed executeSoon(), Re-enable disabled 'passwords' part [Checkin: Comment 30]
Attachment #416337 - Flags: approval-seamonkey2.0.2?

Updated

7 years ago
Attachment #416337 - Flags: approval-seamonkey2.0.2? → approval-seamonkey2.0.2+
(Reporter)

Comment 31

7 years ago
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
Attachment #416337 - Attachment description: (Av3) browser_sanitizer.js: Add needed executeSoon(), Re-enable disabled 'passwords' part [Checkin: Comment 30] → (Av3) browser_sanitizer.js: Add needed executeSoon(), Re-enable disabled 'passwords' part [Checkin: Comment 30 & 31]
(Reporter)

Updated

7 years ago
Flags: in-testsuite+
Keywords: fixed-seamonkey2.0.2
(Reporter)

Updated

5 years ago
No longer depends on: 521305
(Reporter)

Comment 32

5 years ago
(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.
(Reporter)

Comment 33

5 years ago
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.
Attachment #408242 - Attachment description: (Av2) browser_sanitizer.js: Add needed executeSoon(), Re-enable disabled 'passwords' part; Improve browser_feed_tab.js too → (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]
Attachment #408242 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.