1418404 - Debugger shows wrong "this" value for global this in non-syntactic environment

Status: ASSIGNED

(Core :: JavaScript Engine, defect, P3)

Description

[Ted Campbell [:tcampbell]]

As a follow-up to Bug 1397049, there are also issues with computing JSOP_GLOBALTHIS, JSOP_FUNCTIONTHIS in debugger when non-syntactic environments are on the chain. These environments occur in browser code (JSMs, subscripts) but not content code.

Comment 1

[Ted Campbell [:tcampbell]]

Attachment: Bug 1418404 - Support non-syntactic |this| in debugger console

This supports DebugEnvironmentProxies in JSOP_FUNCTIONTHIS /
JSOP_GLOBALTHIS. This allows reporting correct 'this' value when working
with non-syntactic environments such as Gecko JSMs.

Review commit: https://reviewboard.mozilla.org/r/200796/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/200796/

Comment 2

[Ted Campbell [:tcampbell]]

Note to self, one test failed: https://treeherder.mozilla.org/logviewer.html#?job_id=145673246&repo=try&lineNumber=2665

This is due to Bug 1416070, which has since been fixed.

Comment 3

[Jason Orendorff [:jorendorff]]

Comment on attachment 8929520 [details]
Bug 1418404 - Support non-syntactic |this| in debugger console

https://reviewboard.mozilla.org/r/200796/#review207020

We discussed this a lot on IRC and got a ton of help from shu, which is of course above and beyond the call of duty.

Clearing review for now with the following requests:

* There are definitely values of `env` for which it would be way wrong to call `GetExtensibleLexicalEnvironment(env)`. Whatever the rules are, I think it can and should enforce them with assertions.

* `GetExtensibleLexicalEnvironment` needs a comment, because the fact that it's even a sane question to ask, given how specific a category that is and that the spec contains no such thing, is extraordinary and unexplained.

::: js/src/vm/Interpreter.cpp:154
(Diff revision 1)
>  js::GetNonSyntacticGlobalThis(JSContext* cx, HandleObject envChain, MutableHandleValue res)
>  {
> -    RootedObject env(cx, envChain);
> -    while (true) {
> -        if (IsExtensibleLexicalEnvironment(env)) {
> -            res.set(GetThisValueOfLexical(env));
> +    // Find innermost extensible LexicalEnvironmentObject. This will hold the
> +    // current global |this| value for current script execution. Every global
> +    // has an associated LexicalEnvironmentObject so this is always defined.
> +    JSObject * lexicalEnv = &GetExtensibleLexicalEnvironment(envChain);

Nit: delete the space in `JSObject *`.

Comment 4

[Ursula Sarracini (:ursula)]

Steps to reproduce as asked for by Ted:

Open up the file ActivityStreamMessageChannel.jsm in the debugger panel.
Put a breakpoint on the line ```for (let port of this.channel.messagePorts) {```
Open a new tab
Try to see what ```this.channel``` is when breakpoint is hit

Expected Results:
It exists

Actual Results:
Says it's undefined but it's not actually undefined since I can see it exists in the Scopes sidebar

Comment 5

[Ted Campbell [:tcampbell]]

STR repeats for me. I see that this is actually covered by Bug 1397049, but I forgot I hadn't landed it yet. My local build version does fix this ActivityStreamMessageChannel case.