1806598 - Don't assume getters are effect-free in eager evaluation

Status: RESOLVED FIXED

(DevTools :: Console, task)

Description

[Nicolas Chevobbe [:nchevobbe][PTO - back on October 21]]

At the moment, we assume that getters are always effect-free in eager evaluation:

https://searchfox.org/mozilla-central/rev/17aeb39742eba71e0936ae44a51a54197100166d/devtools/server/actors/webconsole/eval-with-debugger.js#397,399,402

dbg.onNativeCall = (callee, reason) => {
...
    // Getters are never considered effectful, and setters are always effectful.
...
      reason == "get" ||

It is simply wrong, and we shouldn't do that.
As for native functions, let's try to generate the list of safe getters from WebIdls (using the Pure tag), and consume that list from eager evaluation.

Comment 1

[Tooru Fujisawa [:arai]]

Looking at Window interface, only a few attributes have Pure tag, and almost all doesn't.

for example, Window.name is not tagged Pure.
So, if we allow only attributes with Pure, window.name will be disallowed in eager evaluation.
is it acceptable?

https://searchfox.org/mozilla-central/rev/17aeb39742eba71e0936ae44a51a54197100166d/dom/webidl/Window.webidl#207,214

/*sealed*/ interface Window : EventTarget {
...
  [Throws] attribute DOMString name;

btw, according to the following comment, Pure sounds too strict.

https://searchfox.org/mozilla-central/rev/abcee8d2c97a5c8a1fbeaf84607ea427be72497a/dom/bindings/parser/WebIDL.py#4607-4608

- isPure is only True for idempotent functions, so it is not valid for
things like keys, values, etc. that return a new object every time.

we don't have to require the getter to be idempotent.
returning new object is fine in eager evaluation, as long as it doesn't affect existing objects.

Comment 2

[Nicolas Chevobbe [:nchevobbe][PTO - back on October 21]]

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

Looking at Window interface, only a few attributes have Pure tag, and almost all doesn't.

for example, Window.name is not tagged Pure.
So, if we allow only attributes with Pure, window.name will be disallowed in eager evaluation.
is it acceptable?

I don't think this is too much of a big deal if some things don't get eager evaluated anymore. I'm a bit more concerned about what it means for Bug 1806596 , where I was hoping to get rid of our special _findSafeGetters implementation to switch to eager evaluation (e.g. , at the moment, logging window in the console and expanding it, and its <default properties> node, name does show up evaluated).

btw, according to the following comment, Pure sounds too strict.

https://searchfox.org/mozilla-central/rev/abcee8d2c97a5c8a1fbeaf84607ea427be72497a/dom/bindings/parser/WebIDL.py#4607-4608

- isPure is only True for idempotent functions, so it is not valid for
things like keys, values, etc. that return a new object every time.

we don't have to require the getter to be idempotent.
returning new object is fine in eager evaluation, as long as it doesn't affect existing objects.

Would there be a suitable tag for this? Or if not, is this something we could detect when doing the eager evaluation somehow ?

Comment 3

[Tooru Fujisawa [:arai]]

I don't find any tag that directly means "has side effect".
All attributes without any special tag are all marked "affects == Everything", but I don't think that's really true for getters.

Perhaps all DOM-related attribute getters are considered side-effect free?
If that's enforced for all specs, we can just enumerate all getters from all DOM-related interfaces and add them to allowlist, so that almost all cases keeps working in the same way,
while we can prevent other random native functions (such like lazy module importer) getting called via getter in eager evaluation.

Comment 4

[Tooru Fujisawa [:arai]]

looks like we can assume all (or almost all) DOM-related getters are side-effect free, in term of eager evaluation.
getters can be lazy and the first invocation can perform some initialization and cache handling, but that's not observable.

so, I'll try adding all of them to allowlist.
we can revisit the decision whenever we found an unexpected side effect.

Comment 5

[Tooru Fujisawa [:arai]]

[Affects] extended attribute seems to be what we want.

Both Firefox and Chromium use it to annotate whether the attribute getter has JS observable side effect.

https://firefox-source-docs.mozilla.org/dom/webIdlBindings/index.html#affects

Used for a method or attribute getter to indicate what sorts of state can be affected when the function is called.

https://chromium.googlesource.com/chromium/src/+/master/third_party/blink/renderer/bindings/IDLExtendedAttributes.md#affects_m_a

[Affects=Nothing] indicates that a function must not produce JS-observable side effects, while [Affects=Everything] indicates that a function may produce JS-observable side effects.

Then there's difference about the default value between Firefox and Chromium.
Firefox uses "Affects=Everything" as default value for attribute getter, but Chromium uses "Affects=Nothing" as default value for attribute getter.

https://searchfox.org/mozilla-central/rev/cef96316b3643720769dec96542604c3209f1877/dom/bindings/parser/WebIDL.py#5134-5135,5169

class IDLAttribute(IDLInterfaceMember):
    def __init__(
...
        self.affects = "Everything"

https://chromium.googlesource.com/chromium/src/+/master/third_party/blink/renderer/bindings/IDLExtendedAttributes.md#affects_m_a

When neither [Affects=Nothing] nor [Affects=Everything] is specified, the default for operations is [Affects=Everything], while for attributes it's [Affects=Nothing].

I think we'd better looking into the default value here, given that affects JIT performance.
If the default value for attribute getter can be Affects=Nothing, we can use it here as well.

Comment 6

[Tooru Fujisawa [:arai]]

Another question about the inferfaces.

Currently only limited interfaces are scanned by GenerateDataFromWebIdls.py, which does not include Window.
So, all getters on Window will be treated as "has side effect".
should we expand the target interfaces list?

https://searchfox.org/mozilla-central/rev/cef96316b3643720769dec96542604c3209f1877/devtools/shared/webconsole/GenerateDataFromWebIdls.py#20-34

# This is an explicit list of interfaces to load [Pure] and [Constant]
# annotation for. There are a bunch of things that are pure in other interfaces
# that we don't care about in the context of the devtools.
PURE_INTERFACE_ALLOWLIST = set(
    [
        "Document",
        "Node",
        "DOMTokenList",
        "Element",
        "Performance",
        "URLSearchParams",
        "FormData",
        "Headers",
    ]
)

Comment 7

[Tooru Fujisawa [:arai]]

it turns out that if we don't have Window on the list, document.getElementsByTagName("a") also doesn't work on eager evaluation because of document getter on Window, which is considered effectful.

I'll try adding Window's attributes

Comment 8

[Peter Van der Beken [:peterv]]

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

https://chromium.googlesource.com/chromium/src/+/master/third_party/blink/renderer/bindings/IDLExtendedAttributes.md#affects_m_a

When neither [Affects=Nothing] nor [Affects=Everything] is specified, the default for operations is [Affects=Everything], while for attributes it's [Affects=Nothing].

I think we'd better looking into the default value here, given that affects JIT performance.
If the default value for attribute getter can be Affects=Nothing, we can use it here as well.

Given that Affects=Nothing can change the actual behaviour of code that's calling the getter, and Affects=Everything just has an effect on performance, I do not think we'd want to switch the default to Nothing. We want a safe default value, otherwise we surely will run into subtle and rare bugs where someone forgot to annotate to switch away from the default. It'd be nice to add more annotations, but that does require looking into the getters before adding it.

Comment 9

[Peter Van der Beken [:peterv]]

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

document getter on Window, which is considered effectful.

This is a good example of why the default value that we currently have makes sense. The document getter is quite complicated, you'd have to make sure it doesn't have side-effects.

Comment 10

[Tooru Fujisawa [:arai]]

Okay, let's keep the current Affects values as is.

Then, there's another issue with Window interface.
attributes on Window interface is defined as getter/setter on window object, not on prototype or anywhere (am I correct?)
Object.getOwnPropertyDescriptor(window, "document") returns the descriptor but Object.getOwnPropertyDescriptor(Window.prototype, "document") doesn't.

This mean we need window object to get the function reference, and to my understanding that's not available on sandbox we use below:

https://searchfox.org/mozilla-central/rev/cef96316b3643720769dec96542604c3209f1877/devtools/server/actors/webconsole/eager-function-allowlist.js#45-51

const sandbox = Cu.Sandbox(
  Components.Constructor("@mozilla.org/systemprincipal;1", "nsIPrincipal")(),
  {
    invisibleToDebugger: true,
    wantGlobalProperties: Object.keys(mergedFunctions),
  }
);

I'll look into a way to retrieve an unmodified window object from eval-with-debugger.js's context or somewhere.

Comment 11

[Peter Van der Beken [:peterv]]

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

attributes on Window interface is defined as getter/setter on window object, not on prototype or anywhere (am I correct?)

Yes, that's how [Global]s work in WebIDL, see https://webidl.spec.whatwg.org/#Global.

Comment 12

[Nicolas Chevobbe [:nchevobbe][PTO - back on October 21]]

(the discussion moved forward and I think that we do want, and need, Window on the list)

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

I'll look into a way to retrieve an unmodified window object from eval-with-debugger.js's context or somewhere.

yes, we have access to it there (e.g. https://searchfox.org/mozilla-central/rev/abf6758ed833c203f84703aa2e3e3d317571b1e9/devtools/server/actors/webconsole/eval-with-debugger.js#529-536)

Comment 13

[Tooru Fujisawa [:arai]]

(In reply to Nicolas Chevobbe [:nchevobbe] from comment #12)

yes, we have access to it there (e.g. https://searchfox.org/mozilla-central/rev/abf6758ed833c203f84703aa2e3e3d317571b1e9/devtools/server/actors/webconsole/eval-with-debugger.js#529-536)

Thanks.

so far, I can get the window reference, but it's the debuggee's window, and at the point of creating the debugger, the window's properties can already be modified.
(document property is not configurable, but for example performance property is configurable)
So, we cannot add the those getters to allowlist.

I'll see if I can get unmodified getter values via Xray or something from the window.

if that doesn't work, the other options I can think of are:

• use some other window, such as browser window itself
  • question: is there any case that devtools debugger gets initialized without any browser window?
• add special function to return window's all getters
• add special function that checks if given function is window getters
  • or maybe just check if given function is safe?

Comment 14

[Tooru Fujisawa [:arai]]

the current mechanism (compare the native function) doesn't work for DOM getters, because getters share common implementation GenericGetter.

https://searchfox.org/mozilla-central/rev/e1b4aec181f4e630fd9b08ac29c1d00a56ae5eed/dom/bindings/BindingUtils.cpp#3162-3163

template <typename ThisPolicy, typename ExceptionPolicy>
bool GenericGetter(JSContext* cx, unsigned argc, JS::Value* vp) {

So, for DOM getters, we cannot use the native function pointers as identity, but need to extract JSJitInfo and compare the referred functions.

https://searchfox.org/mozilla-central/rev/e1b4aec181f4e630fd9b08ac29c1d00a56ae5eed/js/src/debugger/Object.cpp#2544

bool DebuggerObject::isSameNative(JSContext* cx, Handle<DebuggerObject*> object,

Comment 15

[Nicolas Chevobbe [:nchevobbe][PTO - back on October 21]]

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

if that doesn't work, the other options I can think of are:

• use some other window, such as browser window itself
  • question: is there any case that devtools debugger gets initialized without any browser window?

Not sure if I understand this properly, but, looking around, it looks like we always call addAllGlobalsAsDebuggees (or addDebuggee(global)) after instantiating a Debugger object.

Comment 16

[Tooru Fujisawa [:arai]]

thanks :)

actually, it looks like I can get the expected functions via Xray.
I'm preparing a patch stack now.

Comment 17

[Tooru Fujisawa [:arai]]

I bumped issues that window.location isn't marked as "Pure", and also "Location" interface's attributes are not also marked as "Pure".
Some testcase expects window.location.href to be eager-evaluate-able.

I guess adding some extra special cases for them would be okay?

Comment 18

[Tooru Fujisawa [:arai]]

Attachment: Bug 1806598 - Part 1: Add DebuggerObject isSameNativeWithJitInfo. r?jandem!

Given DOM accessors share single native function, existing isSameNative cannot
distinguish 2 accessors.

Each accessor have its own JSJitInfo with native function pointer to the
actual getter implementation.
isSameNativeWithJitInfo checks if the JSJitInfo also matches.

Depends on D168334

Comment 19

[Tooru Fujisawa [:arai]]

Attachment: Bug 1806598 - Part 2: Use isSameNativeWithJitInfo in eager evaluation. r?nchevobbe!

In order to add getters to allowlist, debugger should use isSameNativeWithJitInfo
instead of isSameNative, to distinguish 2 getters with shared native function.

Depends on D168335

Comment 20

[Tooru Fujisawa [:arai]]

Attachment: Bug 1806598 - Part 3: Add ECMAScript built-in getters to eager-ecma-allowlist.js. r?nchevobbe!

Depends on D168336

Comment 21

[Tooru Fujisawa [:arai]]

Attachment: Bug 1806598 - Part 4: Collect safe DOM getters into webidl-pure-allowlist.js. r?nchevobbe!

Also refactor the allowlist structure to represent static vs prototype and
methods vs getters clearly.

And also moved the pre-defined pure methods list into GenerateDataFromWebIdls.py
for simplicity.

Depends on D168337

Comment 22

[Tooru Fujisawa [:arai]]

Attachment: Bug 1806598 - Part 5: Add Window accessors to allowlist. r?nchevobbe!

Add support for [Global] extended attribute and methods/getters on instances.

This retrieves the Xray getter function of the given eval global, so that even
if some configurable properties are modified by the web content, debugger
can see the original getters, and pass them to isSameNativeWithJitInfo.

Depends on D168338

Comment 23

[Tooru Fujisawa [:arai]]

Attachment: Bug 1806598 - Part 6: Add Location accessors, window.location, and document.location to allowlist. r?nchevobbe!

Location related accessors are not marked Pure, but some testcase expects
them to be eagerly evaluateable.

Depends on D168339

Comment 24

[Tooru Fujisawa [:arai]]

Attachment: Bug 1806598 - Part 7: Apply allowlist filter on getters in eager evaluation. r?nchevobbe!

Instead of treating all getters non-effectful, apply the filter also on
getter calls.

Depends on D168340

Comment 25

[Tooru Fujisawa [:arai]]

Attachment: Bug 1806598 - Part 8: Add testcases. r?nchevobbe!

Depends on D168341

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/297a91be123b
Part 1: Add DebuggerObject isSameNativeWithJitInfo. r=jandem
https://hg.mozilla.org/integration/autoland/rev/3fc6c5d9d620
Part 2: Use isSameNativeWithJitInfo in eager evaluation. r=nchevobbe
https://hg.mozilla.org/integration/autoland/rev/9be2509e8540
Part 3: Add ECMAScript built-in getters to eager-ecma-allowlist.js. r=nchevobbe
https://hg.mozilla.org/integration/autoland/rev/3808fc5bf528
Part 4: Collect safe DOM getters into webidl-pure-allowlist.js. r=nchevobbe
https://hg.mozilla.org/integration/autoland/rev/de9d04dc5014
Part 5: Add Window accessors to allowlist. r=nchevobbe
https://hg.mozilla.org/integration/autoland/rev/f97c01add0a5
Part 6: Add Location accessors, window.location, and document.location to allowlist. r=nchevobbe
https://hg.mozilla.org/integration/autoland/rev/ab04e3122f20
Part 7: Apply allowlist filter on getters in eager evaluation. r=nchevobbe
https://hg.mozilla.org/integration/autoland/rev/9e042da8472f
Part 8: Add testcases. r=nchevobbe

Comment 27

[Sandor Molnar[:smolnar]]

Backed out for causing dt failures in devtools/client/webconsole/test/browser/browser_console_evaluation_context_selector.js

Backout link: https://hg.mozilla.org/integration/autoland/rev/c4af9a62fcca732c7d39ea8bc8cdd36c37c4b046

Push with failures

Failure log: TEST-UNEXPECTED-FAIL | devtools/client/webconsole/test/browser/browser_console_evaluation_context_selector.js | Test timed out

Comment 28

[Tooru Fujisawa [:arai]]

Overlooked browser_console_evaluation_context_selector.js testcase, which tests eager-evaluation in worker.

https://searchfox.org/mozilla-central/rev/ddbeacddc15008936e8f619c8c3a05fac6eab8d8/devtools/client/webconsole/test/browser/browser_console_evaluation_context_selector.js#110

setInputValue(hud, "globalThis.location.href");

This requires adding WorkerGlobalScope and WorkerLocation into allowlist,

https://searchfox.org/mozilla-central/rev/ddbeacddc15008936e8f619c8c3a05fac6eab8d8/dom/webidl/WorkerGlobalScope.webidl#16

interface WorkerGlobalScope : EventTarget {

https://searchfox.org/mozilla-central/rev/ddbeacddc15008936e8f619c8c3a05fac6eab8d8/dom/webidl/WorkerLocation.webidl#16

interface WorkerLocation {

but this is problematic because Sandbox cannot be created on worker thread, and we cannot create allowlist by retrieving objects from Sandbox.
possible workaround is to use Xray functions also for prototype/static methods/getters in worker.

Comment 29

[Tooru Fujisawa [:arai]]

Xray functions cannot be retrieved here because we cannot get prototype object from the worker global via Xray.
I'll go with modifying the testcase not to rely on allowlist in worker, given the allowlist isn't the main focus of the testcase.

Comment 30

[Tooru Fujisawa [:arai]]

Attachment: Bug 1806598 - Part 6.5: Do not use WorkerGlobalScope.prototype.location and WorkerLocation.prototype.href in eager evaluation test. r?nchevobbe!

Comment 31

[Tooru Fujisawa [:arai]]

Then, if we want to support side-effect free getters in worker, the solution I can think of is to stop using sandbox, and create ChromeUtils method that takes function and checks if given function is safe or not.
So that the check doesn't require any preparation, and accessible on all threads.

Comment 32

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3f596908d106
Part 1: Add DebuggerObject isSameNativeWithJitInfo. r=jandem
https://hg.mozilla.org/integration/autoland/rev/d849405ab330
Part 2: Use isSameNativeWithJitInfo in eager evaluation. r=nchevobbe
https://hg.mozilla.org/integration/autoland/rev/0540b3a181dd
Part 3: Add ECMAScript built-in getters to eager-ecma-allowlist.js. r=nchevobbe
https://hg.mozilla.org/integration/autoland/rev/6b09818fea2d
Part 4: Collect safe DOM getters into webidl-pure-allowlist.js. r=nchevobbe
https://hg.mozilla.org/integration/autoland/rev/f5b8e273c0c7
Part 5: Add Window accessors to allowlist. r=nchevobbe
https://hg.mozilla.org/integration/autoland/rev/ffb56082a8b8
Part 6: Add Location accessors, window.location, and document.location to allowlist. r=nchevobbe
https://hg.mozilla.org/integration/autoland/rev/4f9bb52e32d2
Part 6.5: Do not use WorkerGlobalScope.prototype.location and WorkerLocation.prototype.href in eager evaluation test. r=nchevobbe
https://hg.mozilla.org/integration/autoland/rev/c34ed738ff6a
Part 7: Apply allowlist filter on getters in eager evaluation. r=nchevobbe
https://hg.mozilla.org/integration/autoland/rev/1c021f85cd52
Part 8: Add testcases. r=nchevobbe

Comment 33

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/3f596908d106
https://hg.mozilla.org/mozilla-central/rev/d849405ab330
https://hg.mozilla.org/mozilla-central/rev/0540b3a181dd
https://hg.mozilla.org/mozilla-central/rev/6b09818fea2d
https://hg.mozilla.org/mozilla-central/rev/f5b8e273c0c7
https://hg.mozilla.org/mozilla-central/rev/ffb56082a8b8
https://hg.mozilla.org/mozilla-central/rev/4f9bb52e32d2
https://hg.mozilla.org/mozilla-central/rev/c34ed738ff6a
https://hg.mozilla.org/mozilla-central/rev/1c021f85cd52