1512949 - Remove eval from tests and flip assertion flag

Status: RESOLVED FIXED

(Core :: DOM: Security, enhancement, P3)

Description

[Vinothkumar Nagasayanan [:vinoth]]

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

Comment 1

[Vinothkumar Nagasayanan [:vinoth]]

Attachment: Bug 1512949 - Remove eval from tests and flip assertion flag

Comment 2

[Vinothkumar Nagasayanan [:vinoth]]

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

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/92c56dcf69b8
Remove eval from tests and flip assertion flag r=ckerschb

Comment 4

[Eliza Balazs [:ebalazs_]]

Backed out changeset 92c56dcf69b8 (Bug 1512949) for mochitest failures setup | Login to delete is defined: login0 - false == true - got false, expected true (operator ==) CLOSED TREE

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception%2Cusercancel%2Crunnable&revision=92c56dcf69b8dadf31af02054dcbfda5802000d1&selectedJob=222683852

Failure logs:
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=222683852&repo=autoland&lineNumber=31852
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=222692520&repo=autoland&lineNumber=6296

Backout push: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception%2Cusercancel%2Crunnable&revision=cb526a4489cb07a2516dd27646b99446e1ed3aea

Comment 5

[Pulsebot]

Backout by [e-mail]:
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

Comment 6

[Vinothkumar Nagasayanan [:vinoth]]

(In reply to Eliza Balazs [:ebalazs_] from comment #4)

Backed out changeset 92c56dcf69b8 (Bug 1512949) for mochitest failures setup | Login to delete is defined: login0 - false == true - got false, expected true (operator ==) CLOSED TREE

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception%2Cusercancel%2Crunnable&revision=92c56dcf69b8dadf31af02054dcbfda5802000d1&selectedJob=222683852

Failure logs:
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=222683852&repo=autoland&lineNumber=31852
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=222692520&repo=autoland&lineNumber=6296

Backout push: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception%2Cusercancel%2Crunnable&revision=cb526a4489cb07a2516dd27646b99446e1ed3aea

Bug 1521057 created to fix the failure.

Comment 7

[Christoph Kerschbaumer [:ckerschb]]

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"

Comment 8

[Matthew N. [:MattN]]

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

Comment 9

[Vinothkumar Nagasayanan [:vinoth]]

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

Comment 10

[Matthew N. [:MattN]]

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

Comment 11

[Vinothkumar Nagasayanan [:vinoth]]

(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?

Comment 12

[Matthew N. [:MattN]]

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

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7a3cc3042d04
Remove eval from tests and flip assertion flag r=ckerschb

Comment 14

[Oana Pop-Rus]

https://hg.mozilla.org/mozilla-central/rev/7a3cc3042d04