1171177 - Try to remove VAROBJFIX and making arbitrary things qualified varobjs

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Shu-yu Guo [:shu]]

See if we still need VAROBJFIX and marking arbitrary scope chains as qualified varobjs in js::Execute.

Comment 1

[Shu-yu Guo [:shu]]

Attachment: Remove VAROBJFIX.

Comment 2

[Shu-yu Guo [:shu]]

Attachment: Remove {QUALIFIED,UNQUALIFIED}_VAROBJ Shape flags in favor of Class-checking.

Comment 3

[Shu-yu Guo [:shu]]

Waiting on try here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b07d0f1606e4

It's possible that XBL field scripts and subscript loader are still relying on their scopes being varobjs, in which case we still can't remove the QUALIFIED_VAROBJ shape flag. It would be very unfortunate if they do.

Comment 4

[Shu-yu Guo [:shu]]

Well then, that certainly failed spectacularly.

Comment 5

[Shu-yu Guo [:shu]]

The subscript loader requires its scopes to be varobjs or chrome code breaks, basically. Going to try to make non-syntactic DynamicWithObjects varobjs and see what else breaks.

Comment 6

[Shu-yu Guo [:shu]]

Attachment: Remove VAROBJFIX.

Comment 7

[Shu-yu Guo [:shu]]

Attachment: Remove UNQUALIFIED_VAROBJ Shape flags in favor of Class-checking.

Comment 8

[Luke Wagner [:luke]]

Comment on attachment 8621403 [details] [diff] [review]
Remove VAROBJFIX.

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

::: js/src/jsapi.cpp
@@ +3296,5 @@
>              return false;
> +
> +        // The XPConnect subscript loader, which may pass in its own dynamic
> +        // scopes to load scripts in, expects the dynamic scope chain to be a
> +        // qualified varobj. There is only sadness.

Instead of referring to "qualified varobj", how about explaining the expected behavior (w/o any reference to "qualified", since I didn't even know that's what "var a;" was).

Comment 9

[Luke Wagner [:luke]]

Comment on attachment 8621404 [details] [diff] [review]
Remove UNQUALIFIED_VAROBJ Shape flags in favor of Class-checking.

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

Great!

::: js/src/jsobj.h
@@ +219,5 @@
> +    // A "qualified" varobj is the object on which "qualified" variable
> +    // declarations (i.e., those defined with "var") are kept.
> +    //
> +    // Conceptually, when a var binding is defined, it is defined on the
> +    // lowest qualified varobj on the scope chain. This is subject to

How about "innermost" instead of "lowest"?

@@ +221,5 @@
> +    //
> +    // Conceptually, when a var binding is defined, it is defined on the
> +    // lowest qualified varobj on the scope chain. This is subject to
> +    // optimization. Unaliased locals inside functions, for instance, live
> +    // entirely on the frame.

How about explaining a bit more including:
 - every function scope (CallObject) is a qualified var obj and there can be no more-inner qualified varobj, so all references to local bindings can be statically bound to the function scope.
 - global scopes are qualified var objs and it is possible to statically know (for a given script) that there are no more-inner qualified varobjs so free variable references can be statically bound to the global.

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6f535aa71725
https://hg.mozilla.org/integration/mozilla-inbound/rev/18c286d070ad

Comment 11

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bb430c06683f

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out for Windows Spidermonkey bustage:

https://treeherder.mozilla.org/logviewer.html#?job_id=10805090&repo=mozilla-inbound

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c3a547a77df9
https://hg.mozilla.org/integration/mozilla-inbound/rev/09dc89be2cee

Comment 14

[Carsten Book [:Tomcat]]

sorry had to back this out since something in this push caused https://treeherder.mozilla.org/logviewer.html#?job_id=10818914&repo=mozilla-inbound to fail permanently

Comment 15

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7eb8196a0dd4
https://hg.mozilla.org/integration/mozilla-inbound/rev/f03508a65d6b

Comment 16

[Shu-yu Guo [:shu]]

Thanks for backout, will investigate.

Comment 17

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0a3b1cd87c26
https://hg.mozilla.org/integration/mozilla-inbound/rev/83954c7df8ab

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0a3b1cd87c26
https://hg.mozilla.org/mozilla-central/rev/83954c7df8ab

Comment 19

[Ryan VanderMeulen [:RyanVM]]

Backed out along with bug 1165486.
https://hg.mozilla.org/integration/mozilla-inbound/rev/178c1abf3fdb

Comment 20

[Ryan VanderMeulen [:RyanVM]]

Merge of backout:
https://hg.mozilla.org/mozilla-central/rev/178c1abf3fdb

Comment 21

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/04f4fec38c9d
https://hg.mozilla.org/integration/mozilla-inbound/rev/b41a69e1b81c

Comment 22

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/937562e73564

Comment 23

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/177cfe17e0d4
https://hg.mozilla.org/integration/mozilla-inbound/rev/cc5d4eaf1a5e

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/177cfe17e0d4
https://hg.mozilla.org/mozilla-central/rev/cc5d4eaf1a5e