1522608 - Port bug 1512949: replace use of eval - Total MozMill and Xpcshell debug failure on 2019-01-24 (all platforms): Assertion failure: false (do not use eval with system privileges), at caps/nsScriptSecurityManager.cpp:483

Status: RESOLVED FIXED

(Thunderbird :: General, task)

Description

[Jorg K (CEST = GMT+2)]

Assertion failure: false (do not use eval with system privileges), at /builds/worker/workspace/build/src/caps/nsScriptSecurityManager.cpp:483

PROCESS-CRASH | account | application crashed [@ nsScriptSecurityManager::ContentSecurityPolicyPermitsJSAction(JSContext*, JS::Handle<JS::Value>)]

M-C last good: 3e5f4a8daaee1b9ccad2fe88666a3cfadf
M-C first bad: 2c5eb9d8b8e2680e7ff3d8fe2714bfd78e
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=3e5f4a8daaee1b9ccad2fe88666a3cfadf&tochange=2c5eb9d8b8e2680e7ff3d8fe2714bfd78e

nsScriptSecurityManager.cpp was last changed more than a week ago
https://hg.mozilla.org/mozilla-central/log/tip/caps/nsScriptSecurityManager.cpp
so this is triggered some other way.

Comment 1

[Jorg K (CEST = GMT+2)]

I'd say a debug build won't even start, Aceman? I'm building one now.

Comment 2

[Jorg K (CEST = GMT+2)]

By running an Xpcshell test interactively I got a stack:

Assertion failure: false (do not use eval with system privileges), at c:/mozilla-source/comm-central/caps/nsScriptSecurityManager.cpp:483
#01: js::DirectEval (c:\mozilla-source\comm-central\js\src\builtin\Eval.cpp:438)
#02: Interpret (c:\mozilla-source\comm-central\js\src\vm\Interpreter.cpp:2970)
#03: js::RunScript (c:\mozilla-source\comm-central\js\src\vm\Interpreter.cpp:421)
#04: js::InternalCallOrConstruct (c:\mozilla-source\comm-central\js\src\vm\Interpreter.cpp:561)
#05: InternalCall (c:\mozilla-source\comm-central\js\src\vm\Interpreter.cpp:588)
#06: js::Call (c:\mozilla-source\comm-central\js\src\vm\Interpreter.cpp:604)
#07: JS_CallFunctionValue (c:\mozilla-source\comm-central\js\src\jsapi.cpp:2555)
#08: nsXPCWrappedJSClass::CallMethod (c:\mozilla-source\comm-central\js\xpconnect\src\XPCWrappedJSClass.cpp:1008)
#09: nsXPCWrappedJS::CallMethod (c:\mozilla-source\comm-central\js\xpconnect\src\XPCWrappedJS.cpp:610)
#10: PrepareAndDispatch (c:\mozilla-source\comm-central\xpcom\reflect\xptcall\md\win32\xptcstubs_x86_64.cpp:181)
#11: SharedStub (c:\mozilla-source\comm-central\xpcom\reflect\xptcall\md\win32\xptcstubs_asm_x86_64.asm:61)
#12: mozilla::net::`anonymous namespace'::ServerSocketListenerProxy::OnSocketAcceptedRunnable::Run (c:\mozilla-source\comm-central\netwerk\base\nsServerSocket.cpp:495)
#13: nsThread::ProcessNextEvent (c:\mozilla-source\comm-central\xpcom\threads\nsThread.cpp:1147)
#14: nsThreadManager::SpinEventLoopUntilInternal (c:\mozilla-source\comm-central\xpcom\threads\nsThreadManager.cpp:483)
#15: XPTC__InvokebyIndex (c:\mozilla-source\comm-central\xpcom\reflect\xptcall\md\win32\xptcinvoke_asm_x86_64.asm:99)

In this case the JS stack was:
0 createHandler(d = [object Object]) ["resource://testing-common/mailnews/IMAPpump.js":56:32]
this = [object Object]
1 onSocketAccepted(socket = [xpconnect wrapped nsIServerSocket @ 0x1dce57d2280 (native @ 0x1dcde3047a8)], trans = [xpconnect wrapped (nsISupports, nsISocketTransport, nsITransport, nsIDNSListener, nsIInterfaceRequestor) @ 0x1dce59a8940 (native @ 0x1dce5a56018)]) ["resource://testing-common/mailnews/maild.js":133:18]
this = [object Object]
2 _do_main() ["c:\mozilla-source\comm-central\testing\xpcshell\head.js":224:2]
this = [object BackstagePass @ 0x1dce50295e0 (native @ 0x1dcde31ebc0)]
3 _execute_test() ["c:\mozilla-source\comm-central\testing\xpcshell\head.js":528:4]
this = [object BackstagePass @ 0x1dce50295e0 (native @ 0x1dcde31ebc0)]
4 <TOP LEVEL> ["typein":1:0]

And IMAPpump.js line 56 is, ta, dah: mixinExtension(handler, eval("IMAP_" + part + "_extension"));

So, if eval doesn't work any more, we have a problem :-(

Looking for JS changes in the regression range, I see this:
7259ee92e345 Jan de Mooij — Bug 1522051 - Stop giving singleton types to call objects. r=tcampbell

Comment 3

[Jan de Mooij [:jandem]]

(In reply to Jorg K (GMT+1) from comment #2)

So, if eval doesn't work any more, we have a problem :-(

Looking for JS changes in the regression range, I see this:
7259ee92e345 Jan de Mooij — Bug 1522051 - Stop giving singleton types to call objects. r=tcampbell

Very unlikely to be related. I'd suggest bisecting this.

Comment 4

[Jan de Mooij [:jandem]]

In that range I'd say bug 1512949.

Comment 5

[Jorg K (CEST = GMT+2)]

Nope, backout of rev 7259ee92e345 didn't fix it. So next regression candidate, bug 1509441. I'll go to the changeset before, rev 24ad7d6e7775.

Comment 6

[Jorg K (CEST = GMT+2)]

Thanks, Jan, how did I miss "eval" in that commit message :-(

Comment 7

[Jorg K (CEST = GMT+2)]

Looking at bug 1512949, they are really replacing eval. Hmm, for the time being, I'll fix this with pref security.allow_eval_with_system_principal.

Aceman, can we get rid of the evals?

Comment 8

[:aceman]

debug build does start fine.
Should we stop using eval?

Comment 9

[Jorg K (CEST = GMT+2)]

Mine doesn't start, but I have an IMAP account in the profile. But most of MozMill fails.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/2774b8292eea
Allow eval with system principal after bug 1512949. rs=bustage-fix

Comment 11

[Jorg K (CEST = GMT+2)]

We don't have many evals in application code, but a fair few in testing code which would explain the test failures. I guess the one that killed me was:
comm/common/src/Overlays.jsm
488 this.window.eval(node.textContent);
since my testing profile has some overlay add-ons. I tried to attach the debugger, but it crashed before :-(

Here they all are:
https://dxr.mozilla.org/comm-central/search?q=eval(+path%3Acomm%2F+file%3A*.js&redirect=false

Comment 12

[Geoff Lankow (:darktrojan)]

I've made a patch for the IMAP stuff, since I already had it open for other reasons. It should clear up a lot of the XPCShell failures. It's currently on Try.

Don't take this as a sign I'm working on the other failures right now.

Comment 13

[:aceman]

Attachment: 1522608.patch

I tried to fix all occurrences, but skipped some code e.g. in mozmill that seems potentially unused. Let's see the try run:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=22d7e996451a13684e9f2134fcf91dc7e79ec908

Comment 14

[Jorg K (CEST = GMT+2)]

Geoff had further changes for the IMAP stuff:
https://hg.mozilla.org/try-comm-central/rev/4ce5843cc85d#l2.2

Surely he's the better reviewer for the lot.

Comment 15

[:aceman]

It's a different implementation for the same goal. Yes, I think it can be faster (less imports), but has more changes needed.

Comment 16

[:aceman]

The try is all green on opt so the replacements seem fine.
On debug mozmill is still broken, the last offender seems to be
https://searchfox.org/comm-central/source/common/src/Overlays.jsm#486
It loads script code (contents of <script> element) and executes it.
Any clever hack around that?
Save the string into a temp file and call Services.scriptloader.loadSubScript() as in the branch when the script is linked via 'src' ? :)

Comment 17

[Geoff Lankow (:darktrojan)]

I'm tempted to take it out and say scripts need to be in a file now.

Comment 18

[Geoff Lankow (:darktrojan)]

(In reply to :aceman from comment #16)

Save the string into a temp file and call Services.scriptloader.loadSubScript() as in the branch when the script is linked via 'src' ? :)

I've done almost that, but using a data URL instead: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=c179d3bed56b18522424dd82b9a0a1321cb0ac61

Comment 19

[:aceman]

Thanks, that does work.
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=0861540fb85919784acb9894d764397216f9cea6

Still crashes opening preferences, and I do not yet see why.

Comment 20

[:aceman]

Attachment: 1522608-darktrojan.patch [landed in comment 23]

Let's land what we have already working.

This is the part that Darktrojan did.

Comment 21

[:aceman]

Attachment: 1522608-aceman.patch

This makes the rest of mozmill work, except some tests that open preferences which still crash, but I do not yet see where 'eval' is called there.

Comment 22

[:aceman]

Comment on attachment 9039330 [details] [diff] [review]
1522608-aceman.patch

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

::: mail/test/resources/mozmill/mozmill/extension/content/modules/controller.js
@@ -688,5 @@
>  
>    frame.events.pass({'function':'controller.waitFor()'});
>  }
>  
> -MozMillController.prototype.waitForEval = function(expression, timeout, interval, subject) {

I remove this function as it seems unused.
But there is still assertJS() which is used in calendar tests so those should be updated first.

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/be6ed62ca1ec
remove uses of eval() in IMAP tests and Overlay.jsm. r=aceman

Comment 24

[Geoff Lankow (:darktrojan)]

Comment on attachment 9039330 [details] [diff] [review]
1522608-aceman.patch

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

::: mailnews/base/test/unit/test_nsIMsgFolderListenerLocal.js
@@ +175,3 @@
>    // Create a third folder for more testing.
> +  function addFolder2() { addFolder(gRootFolder, "folder3",
> +                                    function(folder) { gLocalFolder3 = folder; }); },

Can we put the function code on a new line please? This is not pretty. :(

::: mailnews/imap/test/unit/test_nsIMsgFolderListenerIMAP.js
@@ +155,5 @@
>    // Create another folder to move and copy messages around, and force initialization.
> +  function testAddFolder1() { addFolder(gRootFolder, "folder2",
> +                                        function(folder) { gIMAPFolder2 = folder; }) },
> +  function testAddFolder2() { addFolder(gRootFolder, "folder3",
> +                                        function(folder) { gIMAPFolder3 = folder; }) },

Same here.

Comment 25

[:aceman]

(In reply to Geoff Lankow (:darktrojan) from comment #24)

• function addFolder2() { addFolder(gRootFolder, "folder3",

•                                function(folder) { gLocalFolder3 = folder; }); },
  

Can we put the function code on a new line please? This is not pretty. :(

Sure, in what way?

Comment 26

[Geoff Lankow (:darktrojan)]

function addFolder2() {
  addFolder(gRootFolder, "folder3", function(folder) { gLocalFolder3 = folder; });
},

Comment 27

[:aceman]

Attachment: 1522608.patch [landed in comment 28]

Ok, thanks.

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/3728fc9ecdc3
remove most uses of eval() in C-C tests. r=darktrojan

Comment 29

[Jorg K (CEST = GMT+2)]

Thanks, how much is missing now?

Comment 30

[:aceman]

Still comment 19.

Comment 31

[:aceman]

It still exists and the function causing the assert (AssertEvalNotUsingSystemPrincipal)
only outputs the file chrome://messenger/content/preferences/preferences.xml . There does not seem to be any obvious eval, maybe there is one in the linked scripts. I couldn't dig deeper yet.

Comment 32

[Jan de Mooij [:jandem]]

(In reply to :aceman from comment #31)

There does not seem to be any obvious eval, maybe there is one in the linked scripts. I couldn't dig deeper yet.

FWIW, I see some "new Function" uses in that file (assuming I looked at the right file) and those are similar to eval.

Comment 33

[:aceman]

Yes, there is 'new Function' but that seems to still be used in m-c. Is it really a problem?

Comment 34

[:aceman]

Arai helped me to debug this in the JS engine and indeed the 'new Function()' calls are the problem.

We have a bunch of them:
https://dxr.mozilla.org/comm-central/search?q=%22new+Function%22+%2Bpath%3Acomm&redirect=false

Comment 35

[:aceman]

Attachment: 1522608-preftab.patch

This removes one use of 'new Function' used for execution of the onpaneload attribute. This converts it to .addEventListener() call containing the same code, but as a native function. This only handles onpaneload on <prefpane> inside <preftab>.
Those under <prefwindow> needs to be handled too, see bug 1522778 comment 38.

Try run:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=b3a2de20ba8419a4ea9a59e4b64852d07aac2c56

Now this same treatment needs to be done for onsyncfrompreference and onsynctopreference attributes. (Disabling those 2 makes opening the prefs no longer crash in the eval() check).

And possibly in calendar prefpane. Darktrojan, can you continue from here?

Comment 36

[Geoff Lankow (:darktrojan)]

Comment on attachment 9041305 [details] [diff] [review]
1522608-preftab.patch

Good. I never did like the way that worked.

Comment 37

[Tooru Fujisawa [:arai]]

Comment on attachment 9041305 [details] [diff] [review]
1522608-preftab.patch

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

::: mail/components/preferences/preferences.js
@@ -34,5 @@
>      radio.style.listStyleImage = pane.style.listStyleImage;
>      selector.appendChild(radio);
>  
>      pane.dispatchEvent(new CustomEvent("paneload"));
> -    new Function(pane.getAttribute("onpaneload")).call(pane);

isn't this necessary?
does dispatchEvent properly call the handler?

Comment 38

[:aceman]

(In reply to Tooru Fujisawa [:arai] from comment #37)

 pane.dispatchEvent(new CustomEvent("paneload"));

• new Function(pane.getAttribute("onpaneload")).call(pane);
  isn't this necessary?
  does dispatchEvent properly call the handler?

Yes, I tested the code in addEventListener (the various init functions) does run properly.

Comment 39

[Jorg K (CEST = GMT+2)]

I guess we'll be done here and I can back out the pref change from https://hg.mozilla.org/comm-central/rev/2774b8292eea.

Comment 40

[Jorg K (CEST = GMT+2)]

Oh, re-read comment #25, there's still more to it.

Comment 41

[:aceman]

Comment 35.

Comment 42

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/288ed882534b
remove uses of 'new Function()' in C-C Preferences onpaneload. r=darktrojan

Comment 43

[Geoff Lankow (:darktrojan)]

(In reply to :aceman from comment #34)

Arai helped me to debug this in the JS engine and indeed the 'new Function()' calls are the problem.

We have a bunch of them:
https://dxr.mozilla.org/comm-central/search?q=%22new+Function%22+%2Bpath%3Acomm&redirect=false

Mozilla-central also has a lot of them (example), how do they get away with it?

Comment 44

[:aceman]

I don't know. Maybe that custom element is not considered using 'system principal' ?
They also do not use 'var {object} = ChromeUtils.import()' that they forced on us at https://searchfox.org/mozilla-central/source/toolkit/content/preferencesBindings.js#12 .

But you can set security.allow_eval_with_system_principal to false in TB and you'll see the prefs crash.

Comment 45

[Geoff Lankow (:darktrojan)]

I'm going to change all the preferences stuff in bug 1527770, as it's too big to piggy-back on this bug.

Comment 46

[Tooru Fujisawa [:arai]]

I also don't know.

Comment 47

[Geoff Lankow (:darktrojan)]

Attachment: 1522608-calendar-eval-1.diff

This gets all the eval out of calendar. With this and bug 1527770 we're good to go here.

Comment 48

[Geoff Lankow (:darktrojan)]

Attachment: 1522608-calendar-eval-2.diff

In case somebody gets to this before I do and wants to complain that it doesn't apply.

Comment 49

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/e854574aae9f
Remove all uses of new Function and assertJS in calendar, and remove assertJS from mozmill. r=philipp DONTBUILD

Comment 50

[Geoff Lankow (:darktrojan)]

Attachment: 1522608-whitelist-1.diff

It looks like preferencesBindings.js is whitelisted and that's how it passes the test. Now I'm not quite sure how my previous Try run passed.

This patch adds some files to the whitelist and removes the temporary disabling of the check. Try passes with it (and the other stuff that needs to land).

Comment 51

[:aceman]

m-c cheats and has preferencesBindings.js whitelisted and thus can still use e.g. 'synctopreference' which calls 'new Function()' inside.

So the question is whether we want to migrate away from that sooner than them, or we wait.

Comment 52

[Geoff Lankow (:darktrojan)]

Either they'll fix it in a way that doesn't affect us, or we can copy what they do when they do it. I suspect they're probably not in a rush to do anything about it.

Comment 53

[:aceman]

Comment on attachment 9048658 [details] [diff] [review]
1522608-whitelist-1.diff

So this is more a technical direction decision now. The patch looks OK if it prefs tab does not crash with it. Does there need to be the trailing comma in the pref value?

Comment 54

[Geoff Lankow (:darktrojan)]

Oh no, that's a screw-up from when I reordered the values. I hope that hasn't affected whether this passes or not, and I suspect it could've.

Comment 55

[Geoff Lankow (:darktrojan)]

Attachment: 1522608-whitelist-2.diff

Okay, leaving the comma behind was a real mistake, no wonder everything was going so smoothly. Instead I've copied the pref from m-c and added the files from c-c that need to be in it. I've also put it in an #ifdef like on m-c, and left the other pref in the file.

Comment 56

[Magnus Melin [:mkmelin]]

Comment on attachment 9048718 [details] [diff] [review]
1522608-whitelist-2.diff

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

Better than what we had before. r=mkmelin

Comment 57

[Jorg K (CEST = GMT+2)]

I'm not sure whether to land this since comment #47 makes reference to bug 1527770.

Comment 58

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/f1ef44fd16c8
Whitelist some files that need to use eval with system privileges; r=mkmelin DONTBUILD

Comment 59

[Botond Ballo [:botond]]

Attachment: Bug 1522608 - Fix include-what-you-use violations in LayerAttributes.h. r=tnikkel

Depends on D48371

Comment 60

[Phabricator Automation]

Comment on attachment 9099264 [details]
Bug 1522608 - Fix include-what-you-use violations in LayerAttributes.h. r=tnikkel

Revision D48372 was moved to bug 1552608. Setting attachment 9099264 [details] to obsolete.

Comment 61

[Botond Ballo [:botond]]

(Sorry for the noise. I had a wrong bug number in a commit message.)

Comment 62

[Jorg K (CEST = GMT+2)]

I was starting to wonder ;-)