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

RESOLVED FIXED

Status

defect
RESOLVED FIXED
10 years ago
7 years ago

People

(Reporter: sgautherie, Assigned: neil)

Tracking

({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
iann_bugzilla
: review+
Details | Diff | Splinter Review
1.94 KB, patch
neil
: review+
Details | Diff | Splinter Review
Reporter

Description

10 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

10 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

10 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

10 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

10 years ago
Depends on: 521305
Reporter

Comment 3

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

10 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

10 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

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

10 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

10 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

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

Comment 9

10 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

10 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

10 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

10 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

10 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

10 years ago
Depends on: 394686
Reporter

Comment 15

10 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

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

Updated

10 years ago
Depends on: 437904
Reporter

Comment 18

10 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

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

10 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

10 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

10 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

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

Updated

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

Updated

10 years ago
Blocks: 523345
Assignee

Comment 23

10 years ago
Pushed changeset ad433d192a3d to comm-central.

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

Comment 24

10 years ago
OK, browser_passwordmgrdlg tests seem to be passing now.
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
Reporter

Updated

10 years ago
Attachment #405816 - Attachment is obsolete: true
Reporter

Updated

10 years ago
Attachment #406889 - Attachment description: Proposed patch → Proposed patch [Checkin: Comment 23]
Assignee

Comment 26

10 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

10 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

10 years ago
Duplicate of this bug: 523345
Reporter

Updated

10 years ago
No longer blocks: 523345
Assignee

Updated

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

Comment 30

10 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

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

Comment 31

10 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

10 years ago
Flags: in-testsuite+
Reporter

Updated

7 years ago
No longer depends on: 521305
Reporter

Comment 32

7 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

7 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.