1620087 - XPIDL methods with side-effects are executed with instant evaluation

Status: VERIFIED FIXED

(DevTools :: Console, defect, P2)

Description

[Matthew N. [:MattN]]

Thank you for helping make Firefox better. If you are reporting a defect, please complete the following:

What were you doing?

Please tell us what site you were on, and what steps led to the error you are reporting

1. Enable devtools.chrome.enabled and extensions.formautofill.reauth.enabled
2. Execute let {OSKeyStore} = ChromeUtils.import("resource://formautofill/OSKeyStore.jsm"); in the Browser Console.
3. Paste OSKeyStore.ensureLoggedIn(""); in the Browser Console input (with instant evaluation on).

What happened?

The browser crashes since it executed code that has side effects

What should have happened?

The code shouldn't have executed since it's an XPIDL method with side-effects.

Anything else we should know?

You can change the "" argument to a non-empty string to avoid the crash but still get unwanted execution causing a dialog to appear.

We should probably disable this feature by default in chrome contexts (Browser Console, Browser Toolbox).

Edit: added extensions.formautofill.reauth.enabled pref

Comment 1

[Matthew N. [:MattN]]

Attachment: Screen Recording using console history

Comment 2

[Matthew N. [:MattN]]

In the Browser Toolbox I instead get a devtools error from instant eval.:

InvisibleToDebugger: Error
ThreadSafeDevToolsUtils.js:90:13
    reportException resource://devtools/shared/ThreadSafeDevToolsUtils.js:90
    evaluateJSAsync resource://devtools/server/actors/webconsole.js:1089
    makeInfallible resource://devtools/shared/ThreadSafeDevToolsUtils.js:111

Comment 3

[Logan Smyth [:loganfsmyth]]

Looks like this is a side-effects of us having commented out https://bugzilla.mozilla.org/show_bug.cgi?id=1610532 to attempt to address the memory leak. I haven't looked closely, but presumably the browser console doesn't consider the jsm-scope global to be a debuggee by default, meaning that things in that realm can run with side-effects.

I'm hoping that we can just uncomment that line now that https://bugzilla.mozilla.org/show_bug.cgi?id=1617666 has landed, but since we never actually tracked down the cause of the leak, it's hard to be sure, we'll have to see whether we are still seeing leaks.

If the leaks persist and we can't uncomment that line, I agree that we'd have to consider disabling this feature for the browser console.

Comment 4

[Logan Smyth [:loganfsmyth]]

Here's a Try run with the line uncommented: https://treeherder.mozilla.org/#/jobs?repo=try&revision=173eaacd348ea5fe3db9f4e8c798126aeaea0629

@jlast @nchevobbe Where was the leak in https://bugzilla.mozilla.org/show_bug.cgi?id=1610532 showing up? That issue unfortunately doesn't provide much info for me to go off of.

Comment 5

[Logan Smyth [:loganfsmyth]]

Attachment: Bug 1620087 - Part 3: Treat all realms as debuggees to avoid unexpected sideeffects. r=jlast!

Comment 6

[Nicolas Chevobbe [:nchevobbe]]

The leak was in devtools/client/debugger/test/mochitest/browser_dbg-log-points.js (see https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=284967214&repo=autoland&lineNumber=24979)

Comment 7

[Logan Smyth [:loganfsmyth]]

Attachment: Bug 1620087 - Part 1: Split the ECMAScript eager whitelist into its own file. r=jlast!

Splitting this list into its own file reduces the chances of accidentally
referencing incorrect globals.

The implementation's original placement in eval-with-debugger for instance
was accidentally pulling Reflect from resource://gre/modules/reflect.jsm
because that file also makes use of Reflect.parse. Rather than try to
keep it in that file and rename things, placing the list in its own
lazy-loaded file seems like the easiest and most future-proof approach.

Comment 8

[Logan Smyth [:loganfsmyth]]

Attachment: Bug 1620087 - Part 2: Exceptions during eager whitelist validation should terminate execution. r=jlast!

I've run into this a couple times now, where as I'm tweaking things, I
accidentally introduce something that throws. The code as it was defaulted
to allowing calls to any native function that throws, which seems like a
bad idea, so this now defaults to termination when an error is encountered.

Depends on D65466

Comment 9

[Logan Smyth [:loganfsmyth]]

Here's a new test run for the new patchs, since there was a failure in the old test run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f19324023175409a0b390e561c95bbeab649dd0e

Technically I think the change of placement for ensureSideEffectFreeNatives in Part 3 is enough to fix the test failure, but clean things up a lot too.

Comment 10

[Logan Smyth [:loganfsmyth]]

I also don't see any leaks in devtools/client/debugger/test/mochitest/browser_dbg-log-points.js

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/62d4008a089a
Part 1: Split the ECMAScript eager whitelist into its own file. r=nchevobbe
https://hg.mozilla.org/integration/autoland/rev/a6efc9fb5e12
Part 2: Exceptions during eager whitelist validation should terminate execution. r=nchevobbe
https://hg.mozilla.org/integration/autoland/rev/9e09d30270d6
Part 3: Treat all realms as debuggees to avoid unexpected sideeffects. r=nchevobbe

Comment 12

[Andrei Ciure[:aciure]]

Backed out 3 changesets (bug 1620087) for causing browser/browser_console_eager_eval.js failures
https://hg.mozilla.org/integration/autoland/rev/325992e1fd5f6ad590b9a7e8ba0b1c5b614f84e8

push that caused the backout: https://treeherder.mozilla.org/#/jobs?repo=autoland&group_state=expanded&collapsedPushes=656020&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel%2Crunnable&revision=9e09d30270d6cc660fabb406e132e2e09db870a5&searchStr=tv

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2bd4780e44f2
Part 1: Split the ECMAScript eager whitelist into its own file. r=nchevobbe
https://hg.mozilla.org/integration/autoland/rev/8bbd93eba9e3
Part 2: Exceptions during eager whitelist validation should terminate execution. r=nchevobbe
https://hg.mozilla.org/integration/autoland/rev/36c3c857a5a7
Part 3: Treat all realms as debuggees to avoid unexpected sideeffects. r=nchevobbe

Comment 14

[Cosmin Sabou [:CosminS]]

Was expecting for this to fail also on tier 1 jobs, that's why be backed out, but looks like it didn't, so I relanded: https://treeherder.mozilla.org/#/jobs?repo=autoland&group_state=expanded&collapsedPushes=656020&searchStr=linux%2C18.04%2Cx64%2Cdebug%2Cmochitests%2Ctest-linux1804-64%2Fdebug-mochitest-devtools-chrome-e10s-6%2Cm%28dt6%29&revision=9e09d30270d6cc660fabb406e132e2e09db870a5

Logan, are these TVs just false-positives? https://treeherder.mozilla.org/#/jobs?repo=autoland&group_state=expanded&collapsedPushes=656020&resultStatus=testfailed%2Cbusted%2Cexception&revision=9e09d30270d6cc660fabb406e132e2e09db870a5&searchStr=TV&selectedJob=291797191

Comment 15

[Logan Smyth [:loganfsmyth]]

Hmm, I'm not familiar with TV tests, is there a way for me to run this locally to understand what is going wrong? I'm expecting that test to pass and they do pass locally for me, so I'll have to look more into why they might fail in other conditions.

Comment 16

[Nicolas Chevobbe [:nchevobbe]]

from the screenshot, we get a redeclaration of let loader(which make sense) Don't you see this when running the test with--repeat 2(or--verify`)?

I guess we can switch to use var which shouldn't trigger this error

Comment 17

[Cosmin Sabou [:CosminS]]

Filed Bug 1620222 to track those failures.

Comment 18

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/2bd4780e44f2
https://hg.mozilla.org/mozilla-central/rev/8bbd93eba9e3
https://hg.mozilla.org/mozilla-central/rev/36c3c857a5a7

Comment 19

[Cristian Fogel [:cfogel]]

Confirmed issue with 75.0a1 Build ID 20200304161940 on Windows 10.
Verified with 75.0b5 on Windows 10, macOS 10.15, Ubuntu 19.04.