Remove enablePrivilege from password manager tests

RESOLVED FIXED in mozilla16

Status

()

defect
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: bholley, Assigned: jwilde)

Tracking

(Depends on 1 bug)

Trunk
mozilla16
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

We're running out of time on enablePrivilege. It needs to die in its current form in order for IonMonkey to land, which is coming up real soon now (last I heard was the 15th).

Removing enablePrivilege from our test suite by then isn't realistic, so I'm implementing a stopgap measure that causes enablePrivilege to be permanent on a global once called . This works for most tests, but for more involved tests it doesn't. The password manager tests are some of the most egregious offenders.

I've taken a stab at working on them, but I think I'm going to need some help from people who know them better (and, well, other people in general). Currently, the tests that fail with my interim hack are:

test_basic_form_autocomplete.html
test_prompt_async.html
test_xml_load.html
test_bug620145.html

But this might be a good time to just bite the bullet and rip out enablePrivilege from those tests entirely.

I think the big tests probably need to be converted to chrome tests with content iframes. The simpler cases can probably be fixed with SpecialPowers.wrap.

Dolske, can you help me find resources to get this done? It's pretty time-sensitive.
Posted patch Partial fix. v1 (obsolete) — Splinter Review
Here's a mostly-complete fix that I hacked up for test_basic_form_autocomplete.html, though I had to comment out case 14. The fix depends on another fix to the SpecialPowers wrapping infrastructure over in bug 762529. Hopefully this provides some insight into how tests can be converted (when chrome test conversion isn't necessary).
Should it be sufficient to replace all enablePrivilege in these tests (with SpecialPowers) and testing on trunk to make sure that the tests still pass? Or do we need to coordinate with your/ionmonkey changes to be sure?
(In reply to Justin Dolske [:Dolske] from comment #2)
> Should it be sufficient to replace all enablePrivilege in these tests (with
> SpecialPowers) and testing on trunk to make sure that the tests still pass?

Yep! The changes only affect code calling enablePrivilege, so once it's been excised there should be no problem.

Again though, I might suggest converting some of the nasty ones (like test_prompt_async.html) to chrome tests with content iframes if possible.
Assignee: nobody → jonathan
Status: NEW → ASSIGNED
Attachment #631011 - Attachment is obsolete: true
Attachment #631586 - Flags: review?(dolske)
Attachment #631586 - Flags: feedback?(fryn)
Comment on attachment 631586 [details] [diff] [review]
Complete patch for four tests in description

Review of attachment 631586 [details] [diff] [review]:
-----------------------------------------------------------------

Dolske, could you review this?

We're disabling test 14 in test_basic_form_autocomplete.html (see the added comment to that file), but it seems like something we can investigate in a followup.
I ran the updated tests locally, and they all pass. :)
Attachment #631586 - Flags: feedback?(fryn) → feedback+
Attachment #631586 - Attachment is obsolete: true
Attachment #631586 - Flags: review?(dolske)
Attachment #631592 - Flags: review?(dolske)
Attachment #631592 - Flags: feedback?(fryn)
Attachment #631592 - Attachment description: Complete patch for four tests in description (now with more clearer explanation of dialogMonitor changes) → Complete patch for four tests in description (now with clearer explanation of dialogMonitor changes)
Attachment #631592 - Flags: feedback?(fryn) → feedback+
Some weirdness that came up with SpecialPowers:

1) It doesn't seem as though comparing objects pulled from SpecialPowers-wrapped objects is reliable.

An earlier version of the dialogMonitor prototype tracked the windows currently open with an array of window objects.  Upon receiving `domwindowopened`, observe() pushed a QueryInterface'd window object into windowsOpen; `domwindowclosed` caused the observe() to search through windowsOpen for the corresponding QueryInterface'd window object and remove it.

Now that SpecialPowers is here, that sort of removal operation fails because of seemingly unreliable comparisons and removes nothing, despite receiving `domwindowclosed` successfully.

The current temporary fix is to make windowsOpen a number that we can increment when we receive `domwindowopened` and decrement when we receive `domwindowclosed`.

2) If you declare a variable in the global scope containing a SpecialPowers-wrapped object, errors regarding objects being deleted are sometimes thrown when you access that variable from inside a function.
(In reply to Jonathan Wilde from comment #7)
> Some weirdness that came up with SpecialPowers:
> 
> 1) It doesn't seem as though comparing objects pulled from
> SpecialPowers-wrapped objects is reliable.

Known bug: bug 718543.

> 2) If you declare a variable in the global scope containing a
> SpecialPowers-wrapped object, errors regarding objects being deleted are
> sometimes thrown when you access that variable from inside a function.

If you come up with a reduced testcase and file a bug blocking bug 762492, I'll take a look.
Awesome work! :-)

Looks like test_prompt is failing as well. Can you add that to the list? Actually, is there any way I can convince you to convert _all_ the password manager tests while you're at it? It's going to have to happen sometime, and there's no time like the present ;-)
> 2) If you declare a variable in the global scope containing a
> SpecialPowers-wrapped object, errors regarding objects being deleted are
> sometimes thrown when you access that variable from inside a function.

Turns out #2 was a failure in me and not SpecialPowers.  I was caching wrapped frames' contentDocument properties, then trying to access them after the src of the frames had changed.  Unsurprisingly, the contentDocument properties were garbage collected, causing SpecialPowers to throw errors about dead objects.
Attachment #631592 - Flags: review?(dolske)
This patch disables a check in test_xhr.html (which wasn't part of the original set to modify) because it was trying to compare objects pulled from a dependency of the other tests that I patched to use SpecialPowers.
Attachment #631592 - Attachment is obsolete: true
Attachment #632098 - Flags: review?(dolske)
Attachment #632098 - Flags: feedback?(fryn)
Comment on attachment 632098 [details] [diff] [review]
Existing patch, with extra fixes for test_prompt.html and text_xhr.html

Looks fine to me. It's unfortunate that a few things have to be disabled (esp. for the bug 718543 stuff), but no big deal. Frank's review will be sufficient here for landing.
Attachment #632098 - Flags: review?(fryn)
Attachment #632098 - Flags: review?(dolske)
Attachment #632098 - Flags: feedback?(fryn)
Attachment #632098 - Flags: feedback+
Comment on attachment 632098 [details] [diff] [review]
Existing patch, with extra fixes for test_prompt.html and text_xhr.html

Review of attachment 632098 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/passwordmgr/test/prompt_common.js
@@ +1,1 @@
>  var Ci = Components.interfaces;

Should Ci be wrapped by SpecialPowers too?

::: toolkit/components/passwordmgr/test/test_xhr.html
@@ +18,5 @@
>  <pre id="test">
>  <script class="testbody" type="text/javascript">
>  
>  /** Test for Login Manager: XHR prompts. **/
>  netscape.security.PrivilegeManager.enablePrivilege('UniversalXPConnect');

Is this intentionally kept in?
Depends on: 762529, 718543
(In reply to Frank Yan (:fryn) from comment #13)
> Comment on attachment 632098 [details] [diff] [review]
> Should Ci be wrapped by SpecialPowers too?

no need. Ci is (unfortunately) web-exposed.
Comment on attachment 632098 [details] [diff] [review]
Existing patch, with extra fixes for test_prompt.html and text_xhr.html

Review of attachment 632098 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/passwordmgr/test/test_xhr.html
@@ +18,5 @@
>  <pre id="test">
>  <script class="testbody" type="text/javascript">
>  
>  /** Test for Login Manager: XHR prompts. **/
>  netscape.security.PrivilegeManager.enablePrivilege('UniversalXPConnect');

Yes. With bholley's enablePrivilege workaround, this test was fine.  However, I had to make changes to this test because my modifications to prompt_common.js caused this test to break.
Attachment #632098 - Flags: review?(fryn) → review+
Landed on inbound! :)
https://hg.mozilla.org/integration/mozilla-inbound/rev/b14cfdd7e580

This should be sufficient to unblock Ionmonkey.

bholley told me that at some point a decision was made (of which I was unaware) that, when enablePrivilege is to be removed, it would be the responsibility of module owners to update code as needed. However, in the past, when platform changes are made, it has been the responsibility of the people making that change to fix any affected code, including tests. I think that this precedent is reasonable and continue to be applied here to any other tests that need to be updated in the event of enablePrivilege removal.
Target Milestone: --- → mozilla16
Version: unspecified → Trunk
Additionally, bholley estimated that updating all the toolkit tests might take "only 3 days' time". Might he have been volunteering to do it? ;)
(In reply to Frank Yan (:fryn) from comment #16)
> Landed on inbound! :)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/b14cfdd7e580
> 
> This should be sufficient to unblock Ionmonkey.

fryn++.
 
> bholley told me that at some point a decision was made (of which I was
> unaware) that, when enablePrivilege is to be removed, it would be the
> responsibility of module owners to update code as needed. However, in the
> past, when platform changes are made, it has been the responsibility of the
> people making that change to fix any affected code, including tests. I think
> that this precedent is reasonable and continue to be applied here to any
> other tests that need to be updated in the event of enablePrivilege removal.
..
> Additionally, bholley estimated that updating all the toolkit tests might
> take "only 3 days' time". Might he have been volunteering to do it? ;)

This is not a tenable way forward.

Dolske/Gavin, does this reflect the long-term position of the browser team on this issue? If so we probably need to have a conversation about it...
(to clarify, I'm not saying this work has to happen now)
https://hg.mozilla.org/mozilla-central/rev/b14cfdd7e580
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
"the browser team" (I'm not even sure what you mean by this) does not have a "long-term position" on this issue. I don't think it's useful to frame this as a policy problem. The project does not have and does not need a hard-line policy for whose responsibility it is to fix things; we've gotten by with using our judgement to determine who should fix what in the past, and I'm confident that will continue to be the case. If there are disagreements, we can resolve them with discussion.

Do we still need to discuss this, or are things now sufficiently unblocked?
You need to log in before you can comment on or make changes to this bug.