Remove eval from tests and flip assertion flag

RESOLVED FIXED in Firefox 66

Status

()

enhancement
P3
normal
RESOLVED FIXED
4 months ago
3 months ago

People

(Reporter: vinoth, Assigned: vinoth)

Tracking

(Blocks 1 bug)

unspecified
mozilla66
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox66 fixed)

Details

(Whiteboard: [domsecurity-backlog1])

Attachments

(1 attachment)

(Assignee)

Description

4 months ago
Eval(), new Function() should never execute with system principal.It is being removed everywhere from our codebase as part of Bug 1473549.

In this bug, we rewrite the remaining test files with eval, add pref to xpcshell tests and turn on the assertion flag in all.js
Status: NEW → ASSIGNED
Whiteboard: [domsecurity-active]
(Assignee)

Comment 2

3 months ago

(In reply to Vinothkumar Nagasayanan [:vinoth] from comment #1)

Created attachment 9030207 [details]
Bug 1512949 - Remove eval from tests and flip assertion flag

toolkit/components/passwordmgr/test/mochitest/test_insecure_form_field_autocomplete.html test alone is failing after flipping the assertion flag. As per the discussion, plan is to land the patch to make sure no new eval() is added to the codebase. And to create a new bug to track test_insecure_form_field_autocomplete.html failure.

(Assignee)

Updated

3 months ago
Keywords: checkin-needed

Comment 3

3 months ago

Pushed by nbeleuzu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/92c56dcf69b8
Remove eval from tests and flip assertion flag r=ckerschb

Keywords: checkin-needed

Comment 5

3 months ago
Backout by ebalazs@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/800b3eb8a695
Backed out changeset 92c56dcf69b8 for mochitest failures setup | Login to delete is defined: login0 - false == true - got false, expected true (operator ==) CLOSED TREE
(Assignee)

Updated

3 months ago
Depends on: 1521057
(Assignee)

Updated

3 months ago
Assignee: cegvinoth → nobody
Status: ASSIGNED → NEW
Priority: P2 → P3
Whiteboard: [domsecurity-active] → [domsecurity-backlog1]

Hey Matthew,

within this bug we are trying to land an assertion so we do not use eval() in system privileged context anymore. The patch got backed out because we are facing a problem with test test_insecure_form_field_autocomplete.html. We tried to figure out the problem and it seems the problem is unrelated to our changes. In more detail, when I check out the latest central and run test_insecure_form_field_autocomplete.html on my linux machine I get the following errors (see underneath).

We mainly wanted to consult you to see if there might be any other issues with that test. Put differently, do you have any suggestions for us how to move forward with landing this bug?

[1] https://searchfox.org/mozilla-central/source/toolkit/components/passwordmgr/test/mochitest/test_insecure_form_field_autocomplete.html

1 INFO Passed: 130
2 INFO Failed: 14
3 INFO Todo: 0

Unexpected Results

toolkit/components/passwordmgr/test/mochitest/test_insecure_form_field_autocomplete.html
FAIL Checking form2 username is: singleuser5 - got "", expected "singleuser5"
FAIL Checking form2 password is: singlepass5 - got "", expected "singlepass5"
FAIL Checking form3 username is: singleuser5 - got "", expected "singleuser5"
FAIL Checking form3 password is: singlepass5 - got "", expected "singlepass5"
FAIL Checking form4 username is: singleuser5 - got "", expected "singleuser5"
FAIL Checking form4 password is: singlepass5 - got "", expected "singlepass5"
FAIL Checking form5 username is: singleuser5 - got "", expected "singleuser5"
FAIL Checking form5 password is: singlepass5 - got "", expected "singlepass5"
FAIL Checking form6 username is: singleuser5 - got "", expected "singleuser5"
FAIL Checking form6 password is: singlepass5 - got "", expected "singlepass5"
FAIL Checking form6 username is: singleuser5X - got "X", expected "singleuser5X"
FAIL Checking form6 password is: singlepass5 - got "", expected "singlepass5"
FAIL Checking form8 username is: form8user - got "", expected "form8user"
FAIL Checking form8 password is: form8pass - got "", expected "form8pass"

Flags: needinfo?(MattN+bmo)

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #7)

within this bug we are trying to land an assertion so we do not use eval() in system privileged context anymore. The patch got backed out because we are facing a problem with test test_insecure_form_field_autocomplete.html. We tried to figure out the problem and it seems the problem is unrelated to our changes.

I don't think that's the case…

Login to delete is defined: login0 - false == true - got false, expected true (operator ==)

Seems like exactly the type of error I would expect from the change to the test in this bug… I don't think the this[…] lookup works in that context. I confirmed using a debugger statement. You can instead defined the logins on an object and then change the code to lookup on that object instead of this:

let logins = {};
logins.login0 = new nsLoginInfo(…);

…
let login = logins[loginVariableName];

You should ignore any tier 2 test-verify failures and Linux ones (since it's disabled there) but don't ignore the other failures as they are real regressions AFAICT.

In more detail, when I check out the latest central and run test_insecure_form_field_autocomplete.html on my linux machine I get the following errors (see underneath).

This test is skipped on Linux so manually testing there is probably not useful for unblocking your landing: https://searchfox.org/mozilla-central/rev/6c784c93cfbd5119ed07773a170b59fbce1377ea/toolkit/components/passwordmgr/test/mochitest/mochitest.ini#40

Btw. I don't see where toolkit/components/passwordmgr/test/mochitest/test_basic_form_autocomplete.html is being handled.

Flags: needinfo?(MattN+bmo)
(Assignee)

Comment 9

3 months ago

(In reply to Matthew N. [:MattN] (PM me if requests are blocking you) from comment #8)

Seems like exactly the type of error I would expect from the change to the test in this bug… I don't think the this[…] lookup works in that context. I confirmed using a debugger statement. You can instead defined the logins on an object and then change the code to lookup on that object instead of this:

let logins = {};
logins.login0 = new nsLoginInfo(…);

…
let login = logins[loginVariableName];

Hey Matthew,

I tried as you suggested, but still I get the same failures,

TEST-UNEXPECTED-FAIL | toolkit/components/passwordmgr/test/mochitest/test_insecure_form_field_autocomplete.html | Checking form2 username is: singleuser5 - got "", expected "singleuser5"

1521077Intermittent toolkit/components/passwordmgr/test/mochitest/test_insecure_form_field_autocomplete.html | Checking form2 username is: singleuser5 - got "", expected "singleuser5"

1325778Intermittent toolkit/components/passwordmgr/test/mochitest/test_insecure_form_field_autocomplete.html | Test timed out.

TEST-UNEXPECTED-FAIL | toolkit/components/passwordmgr/test/mochitest/test_insecure_form_field_autocomplete.html | Checking form2 password is: singlepass5 - got "", expected "singlepass5"

1521077Intermittent toolkit/components/passwordmgr/test/mochitest/test_insecure_form_field_autocomplete.html | Checking form2 username is: singleuser5 - got "", expected "singleuser5"

1325778Intermittent toolkit/components/passwordmgr/test/mochitest/test_insecure_form_field_autocomplete.html | Test timed out.

Please take a look at the failure in OS X 10.10 debug in this TRY push,

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f7a61dee2b40aa5f701632723109df94b37d475f

Please kindly let us know if you have any other ideas about fixing this issue.

Flags: needinfo?(MattN+bmo)

(In reply to Vinothkumar Nagasayanan [:vinoth] from comment #9)

(In reply to Matthew N. [:MattN] (PM me if requests are blocking you) from comment #8)

Please take a look at the failure in OS X 10.10 debug in this TRY push,

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f7a61dee2b40aa5f701632723109df94b37d475f

Please kindly let us know if you have any other ideas about fixing this issue.

Those are test-verify jobs that I said you can ignore in comment 8:

You should ignore any tier 2 test-verify failures and Linux ones (since it's disabled there) but don't ignore the other failures as they are real regressions AFAICT.

Flags: needinfo?(MattN+bmo)
(Assignee)

Comment 11

3 months ago

(In reply to Matthew N. [:MattN] (PM me if requests are blocking you) from comment #10)

(In reply to Vinothkumar Nagasayanan [:vinoth] from comment #9)

(In reply to Matthew N. [:MattN] (PM me if requests are blocking you) from comment #8)

Please take a look at the failure in OS X 10.10 debug in this TRY push,

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f7a61dee2b40aa5f701632723109df94b37d475f

Please kindly let us know if you have any other ideas about fixing this issue.

Those are test-verify jobs that I said you can ignore in comment 8:

You should ignore any tier 2 test-verify failures and Linux ones (since it's disabled there) but don't ignore the other failures as they are real regressions AFAICT.

Hey Matthew,

Thanks for the clarification. So this solves the issue. since you told we can ignore this tier 2 test-verify failures, so this patch will be accepted for landing with the tier 2 test-verify failures or I need to disable those failing tier 2 test-verify jobs before trying to land?

Flags: needinfo?(MattN+bmo)

Tier 2 jobs do not result in a backout so you can ignore it, leave the tests as-is: https://wiki.mozilla.org/Sheriffing/Job_Visibility_Policy#Overview_of_the_Job_Visibility_Tiers

Flags: needinfo?(MattN+bmo)
(Assignee)

Updated

3 months ago
Assignee: nobody → cegvinoth
Keywords: checkin-needed

Comment 13

3 months ago

Pushed by apavel@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7a3cc3042d04
Remove eval from tests and flip assertion flag r=ckerschb

Keywords: checkin-needed

Comment 14

3 months ago
bugherder
Status: NEW → RESOLVED
Last Resolved: 3 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla66
You need to log in before you can comment on or make changes to this bug.