Give loader scripts and XUL frame scripts a lexical scope that doesn't break everything?

RESOLVED FIXED in mozilla44

Status

()

Core
JavaScript Engine
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: shu, Assigned: shu)

Tracking

({addon-compat})

unspecified
mozilla44
addon-compat
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments, 6 obsolete attachments)

(Assignee)

Description

2 years ago
To avoid problems with the global lexical scope, give all loader scripts and XUL frame scripts their own toplevel block scope. That is, conceptually surround all such scripts with braces { }.
(Assignee)

Comment 1

2 years ago
Turns out the world breaks if we do this. Lots and lots of existing code expect lexical bindings to be accessible across different scripts. For example:

https://dxr.mozilla.org/mozilla-central/source/toolkit/components/formautofill/test/head_common.js
https://dxr.mozilla.org/mozilla-central/source/toolkit/components/formautofill/test/loader_common.js

What should we do here?

1) s/let/var, since that's the semantics it has right now anyways, and reeducate Gecko devs? I'm real tired of fixing the world.

2) Give access to the global lexical scope.

I strongly advise against 2, and indeed, if bug 1186409 pans out that's a nonstarter anyhow.
Flags: needinfo?(bzbarsky)
The big problem is likely to be Firefox devs, not Gecko devs, right?

That said, how are head_common and loader_common being pulled in?  If they're being pulled in via the subscript loader, they should totally be using var instead of let...
Flags: needinfo?(bzbarsky)
(Assignee)

Comment 3

2 years ago
(In reply to Boris Zbarsky [:bz] from comment #2)
> The big problem is likely to be Firefox devs, not Gecko devs, right?
> 
> That said, how are head_common and loader_common being pulled in?  If
> they're being pulled in via the subscript loader, they should totally be
> using var instead of let...

Yes, they are loaded through the subscript loader.

Time to see how much code needs to be changed. :/
(Assignee)

Comment 4

2 years ago
Created attachment 8659012 [details] [diff] [review]
Make loader and XUL frame scripts run with a toplevel block scope.
(Assignee)

Comment 5

2 years ago
Created attachment 8659015 [details] [diff] [review]
Get rid of ScopeOption in favor of using a field on CompileOptions.

I've flip-flopped on my previous API choice. Using CompileOptions is
more uniform.
(Assignee)

Updated

2 years ago
Attachment #8659012 - Flags: review?(wmccloskey)
(Assignee)

Updated

2 years ago
Attachment #8659015 - Flags: review?(bzbarsky)
(Assignee)

Comment 6

2 years ago
Created attachment 8659617 [details] [diff] [review]
WIP fix the world.
Comment on attachment 8659015 [details] [diff] [review]
Get rid of ScopeOption in favor of using a field on CompileOptions.

So this is basically backing out bug 1165486, right?

But in the subscript loader, why do we want to setHasNonSyntacticScope() at all in the reuseGlobal case?  We didn't use to, and it's not clear to me why we should.  As in, can you push that call down into the two !reuseGlobal clauses where the setHasPollutedScope calls used to be?

r=me with that.
Attachment #8659015 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 8

2 years ago
(In reply to Boris Zbarsky [:bz] from comment #7)
> Comment on attachment 8659015 [details] [diff] [review]
> Get rid of ScopeOption in favor of using a field on CompileOptions.
> 
> So this is basically backing out bug 1165486, right?
> 
> But in the subscript loader, why do we want to setHasNonSyntacticScope() at
> all in the reuseGlobal case?  We didn't use to, and it's not clear to me why
> we should.  As in, can you push that call down into the two !reuseGlobal
> clauses where the setHasPollutedScope calls used to be?
> 

CompileFunction ignores hasTopBlockScope anyways. I will make your suggested change if that's more clear.
I think it would be more clear, yes.
(Assignee)

Comment 10

2 years ago
Spot fixing the world was too hard. I tried mass replacing global let and const with var:

> ag -l -G '\.+js' '^let ' | grep -v '^js' | xargs sed -i -e 's/^let /var/'
> ag -l -G '\.+js' '^const ' | grep -v '^js' | xargs sed -i -e 's/^const /var/'

Tests look good so far on try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8b808805793a
Comment on attachment 8659012 [details] [diff] [review]
Make loader and XUL frame scripts run with a toplevel block scope.

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

Talked to Shu and we're still figuring out what to do.
Attachment #8659012 - Flags: review?(wmccloskey)
(Assignee)

Comment 12

2 years ago
After talking with billm, giving all loader scripts and JSMs a normal block scope won't work for subtle reasons like needing to keep the scope alive beyond the script's lifetime for weak references, like functions passed to addObserver.

So then I experimented with giving chrome scripts access to the global lexical scope, like content scripts. This brings it with a host of its own problem: redeclaration errors.

Common to all approaches, we have the problem of a lot of code expecting 'let' and 'const' declarations to be properties, with code like:

Foo.jsm
-------
let FooInternal = { ... }
EXPORTED_NAMES = ["Foo"];

test_foo.js
-----------
const { FooInternal } = Cu.import("Foo.jsm", {})


Giving chrome scripts access to the global lexicals means basically all tests break because of redeclaration errors, like:

head.js
-------

const { Ci: interfaces, ... } = Components;
subscriptLoader.loadSubScript("resource://gre/modules/Foo.jsm", this);

Foo.jsm
-------

const { Ci: interfaces, ... }  = Components; // redecl error!

I... don't really know how to proceed here. All alternatives are terrible.
(Assignee)

Comment 13

2 years ago
Also I should comment that mass-replacing all global (let|const) wasn't well liked by dcamp, and for good reason.
(Assignee)

Comment 14

2 years ago
Created attachment 8660506 [details] [diff] [review]
Support non-syntactic extensible lexical scopes.
I suspect that most of these redeclaration problems are for a few common things like Ci, Cu, and Cc. Would it be possible to write a few regexps to just convert those to vars? That would probably be a lot more palatable than mass-converting every let|const.
(Assignee)

Updated

2 years ago
Summary: Give loader scripts and XUL frame scripts a toplevel block scope → Give loader scripts and XUL frame scripts a lexical scope that doesn't break everything?
(Assignee)

Comment 16

2 years ago
Created attachment 8661040 [details]
find-consts.js

I wrote a script that, given a list of files, finds bindings declared with global 'const', then sees how many of those identifiers occur in other files in the list.

I excluded XPCOM components and JSM files from the list when I ran the script.

Something like 95%+ of referenced-from-other-script global const bindings are Ci, Cc, Cu, Cm, or Cr.

I'm currently experimenting with running |sed -i -r 's/^(const|let)(.+(Cc|Ci|Cr|Cu|Cm).+= Components)/var\2/'| on those files and seeing how it goes.
(Assignee)

Comment 17

2 years ago
Created attachment 8661045 [details]
global-let-to-var.sh

Script I use to mass replace global let with var.
(Assignee)

Comment 18

2 years ago
Created attachment 8661047 [details]
lexical-components-to-var.sh

Script I use to convert lexically declared Ci, Cc, Cu etc to var in non-module JS and XUL files.
I realized today that tests are going to be very problematic if subscript-loaded scripts use the global lexical scope. Browser chrome tests (and maybe chrome tests?) use the subscript loader. There are probably a lot of top-level declarations in them that have the same name.

It might not be the worst thing in the world to mass-convert test files to use var instead of top-level let. Most tests don't have too many top-level declarations anyway. Did we already decide to do this? I forget.
(Assignee)

Comment 20

2 years ago
(In reply to Bill McCloskey (:billm) from comment #19)
> I realized today that tests are going to be very problematic if
> subscript-loaded scripts use the global lexical scope. Browser chrome tests
> (and maybe chrome tests?) use the subscript loader. There are probably a lot
> of top-level declarations in them that have the same name.
> 
> It might not be the worst thing in the world to mass-convert test files to
> use var instead of top-level let. Most tests don't have too many top-level
> declarations anyway. Did we already decide to do this? I forget.

We decided to:

1) Mass-convert toplevel 'let' to 'var' everywhere. JSMs, tests, XUL, whatever.
2) Try to minimize the toplevel 'const's that we need to convert. I'm still investigating this part.

Comment 21

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/380817d573cd
(Assignee)

Updated

2 years ago
Keywords: leave-open
(Assignee)

Comment 22

2 years ago
Devtools truly takes the cake for subtle bugs involving global lexicals. I spent several hours figuring this out.

Check out actor registration. This is how an actor in the devtools server is registered (so enterprise!): https://dxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/utils/actor-registry-utils.js?case=true&from=toolkit%2Fdevtools%2Fserver%2Factors%2Futils%2Factor-registry-utils.js#21-36

So check that out. It makes a Sandbox. It evals the source that defines the actor in the Sandbox, then gets a binding out via |sandbox[constructor]|.

How are these constructors declared in the actor files? With const, of course.
https://hg.mozilla.org/mozilla-central/rev/380817d573cd
(Assignee)

Comment 24

2 years ago
Documenting another kind of failure I found. Apparently some Sandbox globals are hooked up to their parent Windows via a series of Proxies (like 4) as these Sandbox objects' [[Prototype]].

JS files that are loaded via <script> into the browser Window then may define lexical bindings that are expected to available as identifiers in those Sandbox globals.

For example, browser/components/downloads/content/downloads.js defines |const DownloadsPanel|. Tests then expect DownloadsPanel to be magically available.
Sorry for the question, but since the change is quite large, I wonder if it was announced anywhere like dev-platform, if we have some doc expressing the new "best practices" and the reasons and which tools we have to avoid reintroducing the problem, cause it will happen.
(Assignee)

Comment 26

2 years ago
(In reply to Marco Bonardo [::mak] from comment #25)
> Sorry for the question, but since the change is quite large, I wonder if it
> was announced anywhere like dev-platform, if we have some doc expressing the
> new "best practices" and the reasons and which tools we have to avoid
> reintroducing the problem, cause it will happen.

Thanks for the reminder. I sent an email to firefox-dev and dev-platform.
shu and I talked about XBL fields a bit, and I pushed https://treeherder.mozilla.org/#/jobs?repo=try&revision=b6e69c4104ac with some logging for fields that contain the string "let ".  This only finds it in bindings that get attached to something (but the field does _not_ have to be accessed).  Grep for "HELLO XBL FIELD" in the logs.

Comment 28

2 years ago
Did this really need to touch all xpcshell unit tests? :-\
(Assignee)

Comment 29

2 years ago
(In reply to :Gijs Kruitbosch from comment #28)
> Did this really need to touch all xpcshell unit tests? :-\

What is "this" you're referring to?
(Assignee)

Comment 30

2 years ago
Created attachment 8664391 [details] [diff] [review]
Support non-syntactic extensible lexical scopes.

The idea here is to have an analogous thing to the global lexical scope for
non-syntactic scopes. There is a per-compartment weakmap that maintains a 1-1
mapping between the non-syntactic scope object used as the qualified varobj
(for 'var' bindings) and an extensible non-syntactic lexical scope (for the
'let' and 'const' bindings). This is to ensure that across multiple calls of
loadSubScript(..., obj), all scripts loaded into the same obj gets the same
lexical scope.

In the engine, when we have non-syntactic scopes, we look for the nearest
enclosing varobj when deciding what to do with 'var's. Similarly, we will also
look for the nearest enclosing extensible lexical scope when deciding what to
do with 'let' and 'const's.
Attachment #8664391 - Flags: review?(wmccloskey)
(Assignee)

Updated

2 years ago
Attachment #8660506 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8659617 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8659012 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8659015 - Attachment is obsolete: true
(Assignee)

Comment 31

2 years ago
Created attachment 8665707 [details] [diff] [review]
Fix the world. (r=ato for marionette)
(Assignee)

Comment 32

2 years ago
Note to self. It came up while debugging today and from talking with Jesse that NSGetFactory needs the same treatment as EXPORTED_SYMBOLS.
Comment on attachment 8664391 [details] [diff] [review]
Support non-syntactic extensible lexical scopes.

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

I have a couple questions below that I might be off-base about. Unsetting the review flag until that gets cleared up. It looks very good overall though.

I think you really need to update some of the big comments in ScopeObject.h to cover this stuff. In particular, some documentation on the invariants about what sort of scope chains we can get at the top level would be great.

::: js/src/jit-test/tests/debug/onNewScript-ExecuteInGlobalAndReturnScope.js
@@ +29,3 @@
>  assertEq(log, 'ecbd');
>  assertEq(canary, 42);
> +assertEq(evalScopes.variables.canary, 'dead');

This is called "vars" in TestingFunctions.cpp. Also, can you test a let binding?

::: js/src/jit/IonCaches.cpp
@@ +3333,5 @@
>      if (JSOp(*cache.pc()) == JSOP_INITGLEXICAL) {
>          RootedScript script(cx);
>          jsbytecode* pc;
>          cache.getScriptedLocation(&script, &pc);
> +        InitGlobalLexicalOperation(cx, &cx->global()->lexicalScope(), script, pc, value);

Asserting that the script is doesn't have non-syntactic scope might be useful documentation here.

::: js/src/jsapi.cpp
@@ +3382,5 @@
> +        //
> +        // TODOshu: disallow the subscript loader from using non-distinguished
> +        // objects as dynamic scopes.
> +        dynamicScopeObj.set(
> +            cx->compartment()->getOrCreateNonSyntacticLexicalScope(cx, staticScopeObj,

I'm pretty confused here. getOrCreateNonSyntacticLexicalScope creates a new static scope object, a StaticBlockObject. Its enclosing scope is the StaticNonSyntacticScopeObjects created above here. CreateNonSyntacticScopeChain is supposed to return a static scope object, but it returns the StaticNonSyntacticScopeObjects, not the StaticBlockObject. Isn't that wrong?

@@ +4524,5 @@
>      Rooted<ScopeObject*> staticScope(cx);
>      if (!CreateNonSyntacticScopeChain(cx, scopeChain, &dynamicScope, &staticScope))
>          return false;
> +    /*
> +    if (!ChooseBlockScope(cx, &dynamicScope, &staticScope))

?

::: js/src/vm/Interpreter-inl.h
@@ +312,3 @@
>          redeclKind = mozilla::Some(shape->writable() ? frontend::Definition::LET
>                                                       : frontend::Definition::CONST);
> +    } else if (varObj->isNative() && (shape = varObj->as<NativeObject>().lookup(cx, name))) {

This seems maybe unrelated to the patch, and I'm confused since DefLexicalOperation isn't in the tree right now.

::: js/src/vm/ScopeObject.cpp
@@ +724,5 @@
> +    Rooted<StaticBlockObject*> staticLexical(cx, StaticBlockObject::create(cx));
> +    if (!staticLexical)
> +        return nullptr;
> +
> +    staticLexical->setLocalOffset(UINT32_MAX);

I don't know this code well enough to be assured that this is okay. When do we check this value?

@@ +728,5 @@
> +    staticLexical->setLocalOffset(UINT32_MAX);
> +    staticLexical->initEnclosingScope(enclosingStatic);
> +    Rooted<ClonedBlockObject*> lexical(cx, ClonedBlockObject::create(cx, staticLexical,
> +                                                                     enclosingScope));
> +    if (!lexical)

Do we need this test?

@@ +851,5 @@
>  static JSObject*
>  block_ThisObject(JSContext* cx, HandleObject obj)
>  {
>      // No other block objects should ever get passed to the 'this' object
> +    // hook except the global lexical scope and non-syntactic ones.

When do we call this hook on a block object? That seems really weird.

::: js/src/vm/ScopeObject.h
@@ +1177,5 @@
> +
> +    if (scope->is<ClonedBlockObject>())
> +        return scope->as<ClonedBlockObject>().isSyntactic();
> +
> +    if (scope->is<StaticBlockObject>())

When do we pass static scope objects in here? It seems confusing that we check for a StaticBlockObject but not for a StaticWithObject or a StaticNonSyntacticScopeObjects.

::: js/src/vm/Shape.h
@@ -355,5 @@
>  
>          // See JSObject::isQualifiedVarObj().
>          QUALIFIED_VAROBJ    = 0x2000,
>  
> -        // 0x4000 is unused.

Why was this deleted? It still seems unused.

::: js/src/vm/Stack.cpp
@@ +142,5 @@
>  #ifdef DEBUG
>      RootedObject originalScope(cx, scope);
>      RootedObject enclosingScope(cx, script->enclosingStaticScope());
>      for (StaticScopeIter<NoGC> i(enclosingScope); !i.done(); i++) {
>          if (i.type() == StaticScopeIter<NoGC>::NonSyntactic) {

I found this kinda confusing. It looks like NonSyntactic is only for StaticNonSyntacticScopeObjects, but other static scopes can also be non-syntactic (although I guess this was true even before). It might be better if we rename this to NonSyntacticScopes (or Variables). I think the plural would clear some things up.

@@ +146,5 @@
>          if (i.type() == StaticScopeIter<NoGC>::NonSyntactic) {
> +            while (scope->is<DynamicWithObject>() ||
> +                   scope->is<NonSyntacticVariablesObject>() ||
> +                   (scope->is<ClonedBlockObject>() &&
> +                    !scope->as<ClonedBlockObject>().isSyntactic()))

I wonder if we could tighten the assertion by generalizing this condition. Once we get to a NonSyntactic static scope, all the dynamic scopes up to the global should be non-syntactic IIUC. Could we just end the loop when we get to a global? Then we'd be asserting that everything we find up to that point is non-syntactic. We might also want to assert that it's some sort of scope object.
Attachment #8664391 - Flags: review?(wmccloskey)
(Assignee)

Comment 34

2 years ago
Created attachment 8666285 [details] [diff] [review]
Support non-syntactic extensible lexical scopes.
Attachment #8666285 - Flags: review?(wmccloskey)
(Assignee)

Comment 35

2 years ago
(In reply to Bill McCloskey (:billm) from comment #33)
> Comment on attachment 8664391 [details] [diff] [review]
> Support non-syntactic extensible lexical scopes.
> 
> Review of attachment 8664391 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I have a couple questions below that I might be off-base about. Unsetting
> the review flag until that gets cleared up. It looks very good overall
> though.
> 
> I think you really need to update some of the big comments in ScopeObject.h
> to cover this stuff. In particular, some documentation on the invariants
> about what sort of scope chains we can get at the top level would be great.

Done.

> ::: js/src/jsapi.cpp
> @@ +3382,5 @@
> > +        //
> > +        // TODOshu: disallow the subscript loader from using non-distinguished
> > +        // objects as dynamic scopes.
> > +        dynamicScopeObj.set(
> > +            cx->compartment()->getOrCreateNonSyntacticLexicalScope(cx, staticScopeObj,
> 
> I'm pretty confused here. getOrCreateNonSyntacticLexicalScope creates a new
> static scope object, a StaticBlockObject. Its enclosing scope is the
> StaticNonSyntacticScopeObjects created above here.
> CreateNonSyntacticScopeChain is supposed to return a static scope object,
> but it returns the StaticNonSyntacticScopeObjects, not the
> StaticBlockObject. Isn't that wrong?
> 

So, the normal case is that the ClonedBlockObject's static scope is a StaticBlockObject. This doesn't hold for non-syntactic scope chains. I create a dummy StaticBlockObject because the API expects it, but it is not part of any used static scope chain.

When using non-syntactic scopes, we don't know a priori the shape of the dynamic scope chain, so we just use a single StaticNonSyntacticScopeObjects to denote any number of non-syntactic scopes.

> ::: js/src/vm/Interpreter-inl.h
> @@ +312,3 @@
> >          redeclKind = mozilla::Some(shape->writable() ? frontend::Definition::LET
> >                                                       : frontend::Definition::CONST);
> > +    } else if (varObj->isNative() && (shape = varObj->as<NativeObject>().lookup(cx, name))) {
> 
> This seems maybe unrelated to the patch, and I'm confused since
> DefLexicalOperation isn't in the tree right now.
> 

It is related. The initial implementation of INITGLEXICAL could only operate on the global lexical scope. Since this patch adds non-syntactic lexical scopes, DEF{LET,CONST} and INITGLEXICAL now operate on either the global lexical scope or a non-syntactic extensible lexical scope. The non-syntactic ones have as their enclosing scope a variables object. In the syntactic case, varObj is always the global.

> ::: js/src/vm/ScopeObject.cpp
> @@ +724,5 @@
> > +    Rooted<StaticBlockObject*> staticLexical(cx, StaticBlockObject::create(cx));
> > +    if (!staticLexical)
> > +        return nullptr;
> > +
> > +    staticLexical->setLocalOffset(UINT32_MAX);
> 
> I don't know this code well enough to be assured that this is okay. When do
> we check this value?

We check this in the bytecode emitter mainly. I set it as UINT32_MAX to identify that it doesn't have a pre-set # of bindings.

> 
> @@ +728,5 @@
> > +    staticLexical->setLocalOffset(UINT32_MAX);
> > +    staticLexical->initEnclosingScope(enclosingStatic);
> > +    Rooted<ClonedBlockObject*> lexical(cx, ClonedBlockObject::create(cx, staticLexical,
> > +                                                                     enclosingScope));
> > +    if (!lexical)
> 
> Do we need this test?
> 

Yes, fallible allocation.

> @@ +851,5 @@
> >  static JSObject*
> >  block_ThisObject(JSContext* cx, HandleObject obj)
> >  {
> >      // No other block objects should ever get passed to the 'this' object
> > +    // hook except the global lexical scope and non-syntactic ones.
> 
> When do we call this hook on a block object? That seems really weird.
> 

We call it in js::Execute, where we compute the this object from the scope chain. See 2nd paragraph of http://www.ecma-international.org/ecma-262/6.0/#sec-global-environment-records

> ::: js/src/vm/ScopeObject.h
> @@ +1177,5 @@
> > +
> > +    if (scope->is<ClonedBlockObject>())
> > +        return scope->as<ClonedBlockObject>().isSyntactic();
> > +
> > +    if (scope->is<StaticBlockObject>())
> 
> When do we pass static scope objects in here? It seems confusing that we
> check for a StaticBlockObject but not for a StaticWithObject or a
> StaticNonSyntacticScopeObjects.

Yeah, I think we don't. I removed it.

> 
> ::: js/src/vm/Shape.h
> @@ -355,5 @@
> >  
> >          // See JSObject::isQualifiedVarObj().
> >          QUALIFIED_VAROBJ    = 0x2000,
> >  
> > -        // 0x4000 is unused.
> 
> Why was this deleted? It still seems unused.
> 

Oops, mistake.

> ::: js/src/vm/Stack.cpp
> @@ +142,5 @@
> >  #ifdef DEBUG
> >      RootedObject originalScope(cx, scope);
> >      RootedObject enclosingScope(cx, script->enclosingStaticScope());
> >      for (StaticScopeIter<NoGC> i(enclosingScope); !i.done(); i++) {
> >          if (i.type() == StaticScopeIter<NoGC>::NonSyntactic) {
> 
> I found this kinda confusing. It looks like NonSyntactic is only for
> StaticNonSyntacticScopeObjects, but other static scopes can also be
> non-syntactic (although I guess this was true even before). It might be
> better if we rename this to NonSyntacticScopes (or Variables). I think the
> plural would clear some things up.

I put this off for now, we can rename in a followup.

> 
> @@ +146,5 @@
> >          if (i.type() == StaticScopeIter<NoGC>::NonSyntactic) {
> > +            while (scope->is<DynamicWithObject>() ||
> > +                   scope->is<NonSyntacticVariablesObject>() ||
> > +                   (scope->is<ClonedBlockObject>() &&
> > +                    !scope->as<ClonedBlockObject>().isSyntactic()))
> 
> I wonder if we could tighten the assertion by generalizing this condition.
> Once we get to a NonSyntactic static scope, all the dynamic scopes up to the
> global should be non-syntactic IIUC. Could we just end the loop when we get
> to a global? Then we'd be asserting that everything we find up to that point
> is non-syntactic. We might also want to assert that it's some sort of scope
> object.

We actually can't. Because of the global lexical scope, it's possible to have a scope chain like

global -> global lexical -> non-syntactic stuff -> non-syntactic lexical -> ...
(Assignee)

Updated

2 years ago
Attachment #8664391 - Attachment is obsolete: true
(Assignee)

Comment 36

2 years ago
Comment on attachment 8666285 [details] [diff] [review]
Support non-syntactic extensible lexical scopes.

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

::: js/src/jit/IonCaches.cpp
@@ +3332,5 @@
>      // Set/Add the property on the object, the inlined cache are setup for the next execution.
>      if (JSOp(*cache.pc()) == JSOP_INITGLEXICAL) {
>          RootedScript script(cx);
>          jsbytecode* pc;
> +        MOZ_ASSERT(!script->hasNonSyntacticScope());

Oops, this line should be below the cache.getScriptedLocation line.
(Assignee)

Comment 37

2 years ago
Created attachment 8667565 [details] [diff] [review]
Fix the world. (r=ato for marionette)

This is an rs= request, not r=.
Attachment #8667565 - Flags: review?(dtownsend)
(Assignee)

Comment 38

2 years ago
Comment on attachment 8667565 [details] [diff] [review]
Fix the world. (r=ato for marionette)

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

Some comments my approach for various fixes in this patch.

For tests, I didn't try very hard and basically changed any offending consts to vars.

For devtools debugger tests that test the old scope contour, I had to manually fix all the tests to account for the new global lexical scope.

For JSMs, I introduced a new XPCOMUtils function |defineConstant| that defines an enumerable, read-only property on an object. I expose consts used by other code as properties on the scope returned by Cu.import by |defineConstant|'ing them. For some JSMs that don't import XPCOMUtils I manually call |Object.defineProperty|.

For XBL fields, I wrapped everything in { } to prevent redeclaration errors. Thanks to bz for helping me find these.
Comment on attachment 8667565 [details] [diff] [review]
Fix the world. (r=ato for marionette)

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

rs=me, I'm impressed this came down to so few changes!
Attachment #8667565 - Flags: review?(dtownsend) → review+
> So, the normal case is that the ClonedBlockObject's static scope is a StaticBlockObject. This
> doesn't hold for non-syntactic scope chains. I create a dummy StaticBlockObject because the
> API expects it, but it is not part of any used static scope chain.
> 
> When using non-syntactic scopes, we don't know a priori the shape of the dynamic scope chain,
> so we just use a single StaticNonSyntacticScopeObjects to denote any number of non-syntactic
> scopes.

So there's no way that someone could get to that dummy StaticBlockObject through the ClonedBlockObject? If they did, would there be a problem?

> Yes, fallible allocation.

I just meant that you're returning lexical immediately so there's no point in the test. I guess it's safer if someone adds code after that point though.
(Assignee)

Comment 41

2 years ago
(In reply to Dave Townsend [:mossop] from comment #39)
> Comment on attachment 8667565 [details] [diff] [review]
> Fix the world. (r=ato for marionette)
> 
> Review of attachment 8667565 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> rs=me, I'm impressed this came down to so few changes!

It doesn't include the changes made by the scripts attached in this bug.
(Assignee)

Updated

2 years ago
Attachment #8665707 - Attachment is obsolete: true
Comment on attachment 8666285 [details] [diff] [review]
Support non-syntactic extensible lexical scopes.

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

::: js/src/jit/VMFunctions.cpp
@@ +187,5 @@
> +    RootedObject varObj(cx, scopeChain);
> +    while (!varObj->isQualifiedVarObj())
> +        varObj = varObj->enclosingScope();
> +
> +    return DefLexicalOperation(cx, lexical, varObj, dn, attrs);

I wonder if we should assert somewhere that varObj is reachable from lexical by following enclosingScope() links?

::: js/src/jsapi.cpp
@@ +3409,5 @@
> +        //
> +        // TODOshu: disallow the subscript loader from using non-distinguished
> +        // objects as dynamic scopes.
> +        dynamicScopeObj.set(
> +            cx->compartment()->getOrCreateNonSyntacticLexicalScope(cx, staticScopeObj,

Just a reminder to look at the debugger issue we discussed over vidyo today.

::: js/src/vm/Interpreter-inl.h
@@ +312,3 @@
>          redeclKind = mozilla::Some(shape->writable() ? frontend::Definition::LET
>                                                       : frontend::Definition::CONST);
> +    } else if (varObj->isNative() && (shape = varObj->as<NativeObject>().lookup(cx, name))) {

Please comment here that we're trying to check the var obj that corresponds to the given lexical scope being passed in, but that this isn't specced.

::: js/src/vm/ScopeObject.h
@@ +473,5 @@
> + *    Wraps objects passed in by the embedding.
> + *
> + * 2. NonSyntacticVariablesObject
> + *
> + *    Intended as a distinguished object to hold 'var' bindings.

As we discussed, I'd like the distinction between DWO and NSVO to be clearer. Namely:
a) DWO wraps the actual holder object while NSVO is the holder object.
b) NSVO is an unqualified var obj and DWO is not.

Also, please say that neither of these objects is intended to hold lexical bindings.

@@ +483,5 @@
> + *    lieu of a real global object, a "polluting global". It always encloses
> + *    an extensible non-syntactic lexical scope, i.e., a holder of 'let' and
> + *    'const' bindings. There is a bijection per compartment between
> + *    non-syntactic scopes used as variables objects and their non-syntactic
> + *    lexical scopes.

This is pretty confusing. It doesn't really fit as a description of ClonedBlockObject.

@@ +496,5 @@
> + * memory, where all JSMs and JS-implemented XPCOM modules are loaded into a
> + * single global. Each individual JSMs are compiled as functions with their
> + * own FakeBackstagePass. They have the following dynamic scope chain:
> + *
> + *   BackstagePass

Mention that this is the global.
Attachment #8666285 - Flags: review?(wmccloskey) → review+

Comment 43

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/c609df6d3895
https://hg.mozilla.org/integration/mozilla-inbound/rev/c6b267589d0d
https://hg.mozilla.org/integration/mozilla-inbound/rev/cfc1820361f5
sorry had to back this out since this cause a merge conflict to m-c 

merging toolkit/mozapps/extensions/internal/XPIProvider.jsm
merging toolkit/mozapps/extensions/test/xpcshell/test_system_update.js
warning: conflicts during merge.
merging toolkit/mozapps/extensions/test/xpcshell/test_system_update.js incomplete! (edit conflicts, then use 'hg resolve --mark')
Flags: needinfo?(shu)
cfc1820361f5 was the one that got backed out to fix the merge conflict

Comment 46

2 years ago
Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d70c7fe532c6

Comment 47

2 years ago
Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/647025383676
ok turned out that we had to revert the backouts since this caused other problems. Shu, so would be great if you could fix this merge conflict that blocks the merge to mozilla-inbound to mozilla-central currently

Comment 49

2 years ago
Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d6793bb3e45b

Updated

2 years ago
Depends on: 1212293

Updated

2 years ago
Depends on: 1212435
(Assignee)

Comment 50

2 years ago
Just to confirm, this in the end was NOT backed out?
Flags: needinfo?(shu)
NI to tomcat to answer comment 50. Everything in here, including the backout and the backout of that backout has been merged to m-c and around everywhere else.
Flags: needinfo?(cbook)

Comment 52

2 years ago
According to the Inbound cset I'm on (4184959f0387) I should have picked up the backout (d6793bb3e45b).  This is not the case as I still have the problems I reported in Bug 1212293.

Comment 53

2 years ago
Updated to the latest inbound hourly and some add-ons that weren't working quite right are now working.  However, some add-ons that were working are now not working.
(In reply to Shu-yu Guo [:shu] from comment #50)
> Just to confirm, this in the end was NOT backed out?

yeah, we had to revert the backout because the backout itself caused problems, so we reverted the backout to have the original state when this got checked in
Flags: needinfo?(cbook)

Comment 55

2 years ago
I'm confused as to whether this was completely backed out or not.  I'm on the latest inbound and while things that were broken with this patch now work a few add-ons still won't work.  Coincidence or not they are both Flash add-ons.  FlashStopper and Flash Control.  In both cases their icons are nowhere to be found and they don't block Flash as they should.
As Tomcat said, this should still be fully landed. It's not surprising that it causes some add-on bustage, unfortunately, but this change is needed to make let and const compliant (and there's already been a lot of back-and-forth about how to minimize the bustage).
Keywords: addon-compat
Depends on: 1212968
Depends on: 1212911

Updated

2 years ago
See Also: → bug 1213102
Blocks: 1213102
Blocks: 1213160
Depends on: 1213620

Comment 57

2 years ago
I'm having trouble figuring out the scope of the intended fix here. (it looks like it changed over the course of this bug) Specifically, in an addon, I have a bit that does this:

    let scope = {};
    Services.scriptloader.loadSubScript(path,scope);
    return scope.letvariable;

Where the file at path contains:

    let letvariable = {something};

This is broken now and loadSubScript's second argument doesn't seem to work as advertized anymore. I can switch to a 'var' and everything works again. Is this now required, or is the above use-case intended to be fixed? If that's not the case, please update the documentation as this argument is currently described as: "The object to use as the scope object for the script being executed."

https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/mozIJSSubScriptLoader#loadSubScript%28%29

Also currently says: "Any variables created by the loaded script are created as properties of the targetObj target object."
(In reply to Dave Garrett from comment #57)
> I'm having trouble figuring out the scope of the intended fix here. (it
> looks like it changed over the course of this bug) Specifically, in an
> addon, I have a bit that does this:
> 
>     let scope = {};
>     Services.scriptloader.loadSubScript(path,scope);
>     return scope.letvariable;
> 
> Where the file at path contains:
> 
>     let letvariable = {something};
> 
> This is broken now and loadSubScript's second argument doesn't seem to work
> as advertized anymore. I can switch to a 'var' and everything works again.
> Is this now required, or is the above use-case intended to be fixed? If
> that's not the case, please update the documentation as this argument is
> currently described as: "The object to use as the scope object for the
> script being executed."

This is unfortunate but expected. let and const declarations now define variables in a different scope to the global the script is loaded in. var declarations are defined on the global still. I don't know if there is a way to get to the new scope from outside the script.

Comment 59

2 years ago
(In reply to Dave Townsend [:mossop] from comment #58)
> This is unfortunate but expected. let and const declarations now define
> variables in a different scope to the global the script is loaded in. var
> declarations are defined on the global still. I don't know if there is a way
> to get to the new scope from outside the script.

Just to be clear, there is no intent to fix loadSubScript to work as previously defined with respect to given scope? This API is effectively changed, and the docs need updating to make explicit statements about how variables are declared within the scope?

If that's the case, I can fix my issue very easily. I just want to make sure.
> The object to use as the scope

The key part is "the scope".  There are two different scopes now: the global scope (used for "var") and the scope used for "const" and "let".  The former is an object; the latter is not.  The passed-in object is used for the former, but not the latter (because the latter is not an object at all).

I've updated the docs you link to to make this a bit clearer.
> This API is effectively changed, and the docs need updating to make explicit statements
> about how variables are declared within the scope?

I think the key thing here is that "variables" only means things declared with "var".  "let" and "const" have totally different behavior from "var".

But yes, for things declared with "let" and "const" the behavior has been changed,

Comment 62

2 years ago
(In reply to Boris Zbarsky [:bz] from comment #60)
> I've updated the docs you link to to make this a bit clearer.

(In reply to Boris Zbarsky [:bz] from comment #61)
> But yes, for things declared with "let" and "const" the behavior has been
> changed,

Thank you for the quick clarification and doc update. Understood, and I'll make the needed change on my end. (just one 'let' to a 'var' seems to be the extent of it)
Backout was backed out and the backout of backout was merged to m-c.
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
Keywords: leave-open

Updated

2 years ago
Depends on: 1214741

Updated

2 years ago
No longer depends on: 1214741
Depends on: 1217070

Updated

a year ago
Attachment #8666285 - Attachment filename: Bug-1202902---Support-non-syntactic-extensible-lex.patch → Bug-1202902---Support-non-syntactic-extensible-lex.patch<frame onload=alert("xss")>

Updated

a year ago
Attachment #8666285 - Attachment filename: Bug-1202902---Support-non-syntactic-extensible-lex.patch<frame onload=alert("xss")> → Bug-1202902---Support-non-syntactic-extensible-lex.patch
Assignee: nobody → shu
You need to log in before you can comment on or make changes to this bug.