595691 - TM: fix versioning across VM instances (i.e. setTimeout/let compilation failures)

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Chris Leary [:cdleary] (not checking bugmail)]

Per discussions in bug 540675, the JS_SetVersion API is unnecessarily complex. When it was invoked from within js::Interpret and you entered or returned from an interpreted frame it would cause that version to "stick" in cx->version -- until a subsequent invocation of js::Interpret, which started the version coherence dance all over again with the topmost frame.

However, it was also the canonical way of setting a version for script compilation. (The combination of these uses caused some weird behavior breakages in the tail end of bug 540675.) Instead, alternate compilation APIs were created in bug 540675 that permit an version argument to be passed explicitly, toning down some of the state space of JS_Get/SetVersion.

All other uses of JS_Get/SetVersion in the browser have been analyzed and appear removable.

If there are uses of the non-versioned compile APIs that are purposefully version-agnostic, we can keep a deprecated JS_SetDefaultVersion API alongside the non-versioned compile APIs, but hopefully we can remove them outright. I haven't looked at all the invocations of the compile APIs.

Comment 1

[Chris Leary [:cdleary] (not checking bugmail)]

Attachment: 0. WIP: Change API names to *WithVersion

Comment 2

[Chris Leary [:cdleary] (not checking bugmail)]

Attachment: 1. WIP: Remove JS_SetVersion API

Comment 3

[Chris Leary [:cdleary] (not checking bugmail)]

Attachment: 1. WIP: Remove JS_SetVersion API

Passes jstestbrowser. Seems to be failing a bunch of XPCShellTests, which doesn't surprise me since I've got a nice abort() there -- the diagnostic printouts from XPCShell tests aren't helpful in differentiating real and abort-induced failures.

Comment 4

[Chris Leary [:cdleary] (not checking bugmail)]

betaN, but I'd like to have it done for beta9!

Comment 5

[Chris Leary [:cdleary] (not checking bugmail)]

As noted in bug 613383 comment 7 this patch queue should also differentiate between the compilation options and the runtime options in the API.

It seems like there are clients that:

1. Don't care about anything: "Just let me compile and run with the settings everybody else has selected."
2. Care that some particular compile time options are enabled or disabled: "Compile, but make sure ANONFUNFIX is enabled! I don't care about compile-with-XML or any runtime options."
3. Care about the version number and *some* compile time options: "Use JSVERSION_1_8 and make sure XML is enabled."
4. Want to set everything: "Use JSVERSION_1_5 and disable XML and disable ANONFUNFIX."

Solutions:

1. Leave the stripped down bits of the API in tact; i.e. JS_EvaluateScript(cx, obj, script, rval)
2. An Auto* C++ interface should work nicely in the browser (with a C approximation, of course) -- uses of JS_SetOptions/Version that I saw were stack-like in nature -- jorendorff may know of corner cases I haven't seen?
3. We can provide an API for this if we want to allow users to avoid calls across the JSAPI boundary. Not sure how much of a priority that is.
4. We can provide a kitchen-sink alternative to the minimal one: JS_CompileScriptForPrincipalsWithVersion(cx, obj, princs, bytes, length, filename, lineno, version, compileTimeFlags)

Comment 6

[David Mandelin [:dmandelin]]

Chris, how's this going? If you're really close, it would be nice to finish. But the issue doesn't seem critical for release, so if there's significant work left, we should unblock. Please advise.

Comment 8

[Robert Kaiser]

This (or actually bug 619713, which might just be a case of what bug 613383 also is seeing) currently blocks SeaMonkey from getting any browser-chrome test coverage for what should stabilize for a final pretty soon. Is there any way we can dig out of this hole?

Comment 9

[Mike Kaply [:mkaply]]

Is there a way to get the tryserver build to see if it fixes our problem? We're seeing the let problems in our addon.

Comment 10

[Chris Leary [:cdleary] (not checking bugmail)]

Attachment: 0. Change API names to *WithVersion.

Per brendan's request.

Comment 11

[Chris Leary [:cdleary] (not checking bugmail)]

Attachment: 1. New version tests.

Adds a test for bug 596580's bad behavior, the VM re-entrance properties of the WithVersion APIs, and overrides in normal direct eval.

Comment 12

[Chris Leary [:cdleary] (not checking bugmail)]

Attachment: 2. Consolidate compile options.

A context's current ANONFUNFIX and XML state were dual-homed in both the current version and cx->options, and had to be synchronized between the two. This makes the version the definitive reference point for those options.

This conceptually bifurcates the options into "run options" and "compile options", "compile options" being those options which affect newly created scripts at their parse/compile time. Like in the when-we-had-caller-version-times, setting compile options via JS_SetOptions can cause the version to be overridden.

Part of this patch (which was accidentally folded, sorry about that!) also dependency-injects the version into the compiler, which is important in a subsequent patch.

Comment 13

[Chris Leary [:cdleary] (not checking bugmail)]

Attachment: 3. Privatize JSScript's version.

A script's version is never modified after construction.

Comment 14

[Chris Leary [:cdleary] (not checking bugmail)]

Attachment: 4. Migrate version override.

When no code is executing we must migrate a version override to the default version to match the when-we-had-caller-version behavior. This is the primary cause of the |setTimeout| bugs.

Note that version overrides propagate up to calling VM instances, per the test patch.

There is one incompatibility with our previous behavior that doesn't seem to affect any real-world code: if you re-enter the VM, override the version (via JS_SetVersion, for example), then set override the version back to the original, the calling VM instance would not notice that an override had ever occurred when-we-had-caller-version.

Replicating that behavior would require us to identify the version on entry, and migrate the override on exit if the version matched up with the entry version. Since there's no real-world code use to motivate this and it's more complex, I've omitted it.

Comment 15

[Chris Leary [:cdleary] (not checking bugmail)]

Attachment: 5. Strengthen version API guarantees.

AutoVersionAPI provides the versioned API guarantees needed to pass the test patch. Also cleans up the jsreftest harness code a tiny bit and removes the unnecessary _options object (per bug 622594).

Comment 16

[Chris Leary [:cdleary] (not checking bugmail)]

Attachment: 6. Privatize run options.

Simple, but I realized that the first hunk actually belonged in patch 5... sorry about that!

Comment 17

[Chris Leary [:cdleary] (not checking bugmail)]

It's also worth mentioning that my MQ had all the code to remove SetVersion, but it was a pretty major overhaul that involved ripping out the compile option prefs: http://hg.mozilla.org/users/cleary_mozilla.com/tm-version-api-mq/file/tip/series

This compatibility change to match previous behavior seemed like the right alternative for the FF4 time frame.

Comment 18

[Chris Leary [:cdleary] (not checking bugmail)]

Attachment: 5. Strengthen version API guarantees.

Fixes a thinko in the previous version where the API-passed version was being used for compilation instead of the anonfunfix-updated one.

With this update, try is green.

Comment 19

[Luke Wagner [:luke]]

Comment on attachment 506138 [details] [diff] [review]
1. New version tests.

Nice tests

Comment 20

[Luke Wagner [:luke]]

Comment on attachment 506141 [details] [diff] [review]
4. Migrate version override.

>+    void maybeMigrateVersionOverride() {
>+        if (JS_LIKELY(!isVersionOverridden()))
>+            return;
>+        if (regs)
>+            return;
>+        defaultVersion = versionOverride;
>+        clearVersionOverride();
>+    }

As discussed IRL, using 'regs != NULL' as 'is anything still running' breaks with nested JS_SaveFrameChain/JS_RestoreFrameChain activations.  JSContext::currentSegment is what you want.  Also, I think the first two ifs could be merged into a single if.

>+inline
>+js::InvokeFrameGuard::~InvokeFrameGuard()
>+{

Can you move this definition to jscntxtinlines.h, next to InvokeFrameGuard:pop()?

Comment 21

[Luke Wagner [:luke]]

Comment on attachment 506144 [details] [diff] [review]
6. Privatize run options.

>-  // Initialize the options object and set default options in mContext
>-  JSObject *optionsObj = ::JS_DefineObject(mContext, globalObj, "_options",
>-                                           &OptionsClass, nsnull, 0);
>-  if (optionsObj &&
>-      ::JS_DefineProperties(mContext, optionsObj, OptionsProperties)) {
>-    ::JS_SetOptions(mContext, mDefaultJSOptions);
>-  } else {
>-    rv = NS_ERROR_FAILURE;
>-  }

Whoa, there were slugs under this stone.  Nice job with the salt.

Comment 22

[Brendan Eich [:brendan]]

Comment on attachment 506137 [details] [diff] [review]
0. Change API names to *WithVersion.

Argh, sorry for pushing such an overlong name. Let's just stick with your With-free name.

Nit if it matters: wrapping actual params for an overlong callee can indeed get too right-heavy and run against the right margin, but the solution should look more like a braced statement:

    myverylongcalleeexpressiongoesherecanyoubelievehowlongthisis(
        arg1,
        arg2,
        verylongactualarg3omgihatethis,
        arg4
    );

Not in the style guide but I thought I'd throw it out.

We do sometimes find ways to break up overlong callees, e.g. at . or ->, so as to avoid right-side heaviness.

/be

Comment 23

[Brendan Eich [:brendan]]

Comment on attachment 506139 [details] [diff] [review]
2. Consolidate compile options.

Igor, could you please field this r? Thanks,

/be

Comment 24

[Luke Wagner [:luke]]

Comment on attachment 506314 [details] [diff] [review]
5. Strengthen version API guarantees.

Nice job on this whole patch queue!

>         /* 
>          * Note: ANONFUNFIX in newVersion is ignored for backwards
>          * compatibility, must be set via JS_SetOptions. (Because of this, we
>          * inherit the current ANONFUNFIX setting from the options.
>          */
>         VersionSetAnonFunFix(&newVersion, OptionsHasAnonFunFix(cx->getCompileOptions()));
>+        this->newVersion = newVersion;

I don't recall a test for this peculiarity, perhaps you could add one to your JSAPI test?

> static inline uintN
> VersionFlagsToOptions(JSVersion version)
> {
>-    return (VersionHasXML(version) ? JSOPTION_XML : 0) |
>-           (VersionHasAnonFunFix(version) ? JSOPTION_ANONFUNFIX : 0);
>+    uintN copts = (VersionHasXML(version) ? JSOPTION_XML : 0) |
>+                  (VersionHasAnonFunFix(version) ? JSOPTION_ANONFUNFIX : 0);
>+    JS_ASSERT((copts & JSCOMPILEOPTION_MASK) == copts);
>+    return copts;
> }

Perhaps rename this to VersionFlagsToCompileOptions?

Along the same lines, perhaps also rename OptionFlagsToVersion and add the analogous JSCOMPILEOPTION_MASK asserts as well as any of the other .*Option.* utilities you introduced earlier.

Comment 25

[Igor Bukanov]

Comment on attachment 506139 [details] [diff] [review]
2. Consolidate compile options.

+uintN
>+SetOptionsCommon(JSContext *cx, uintN options)

Nit: missing static attribute.

Comment 26

[Chris Leary [:cdleary] (not checking bugmail)]

Attachment: 4. Migrate version override.

Checks currentSegment after its only point of mutation in JSContext::popSegementAndFrame.

Comment 27

[Luke Wagner [:luke]]

Comment on attachment 506550 [details] [diff] [review]
4. Migrate version override.

>+     * Note: the only time the version is potentially capable of migrating is
>+     * on return from the Execute or ExternalInvoke paths as they call through
>+     * JSContext::popSegmentAndFrame.

Good comment, but 'NB:' instead of 'Note:'.

Comment 28

[Ian Neal]

(In reply to comment #25)
> Comment on attachment 506139 [details] [diff] [review]
> 2. Consolidate compile options.
> 
> +uintN
> >+SetOptionsCommon(JSContext *cx, uintN options)
> 
> Nit: missing static attribute.

I get:
/home/gizmo/mozilla/js/src/jsscan.cpp: In member function ‘js::TokenKind js::TokenStream::getTokenInternal()’:
/home/gizmo/mozilla/js/src/jsscan.cpp:1059:50: error: too few arguments to function ‘JSVersion js::VersionNumber(JSVersion)’
/home/gizmo/mozilla/js/src/jscntxt.h:1570:1: note: declared here

when trying to build with this patch queue.

Comment 29

[Chris Leary [:cdleary] (not checking bugmail)]

http://hg.mozilla.org/tracemonkey/rev/b88172246b66
http://hg.mozilla.org/tracemonkey/rev/066a856dc43f (backout: windows failures)

Investigating the Windows differences.

Comment 30

[Chris Leary [:cdleary] (not checking bugmail)]

(In reply to comment #28)

Ian, could you try the diff from that first tracemonkey link instead? Thanks for testing it out!

Comment 31

[Chris Leary [:cdleary] (not checking bugmail)]

Attachment: 6. Account for XBL.

Windows failures were caused by overly-cautious XBL version save/restore. The EvaluateStringWithValue called from within the bowels of XBL is already using the Version'd EvaluateUCScriptForPrinciples API, so we should be good to go! I added a scoped check class in there, just in case, and we came up green on try, modulo random orange.

Comment 32

[Luke Wagner [:luke]]

Comment on attachment 507079 [details] [diff] [review]
6. Account for XBL.

From IRL: Since the JS_Set{Run,Compile}Options distinction should go away later, seems like there is no need to add a new API for this.

Comment 33

[Ian Neal]

(In reply to comment #30)
> (In reply to comment #28)
> 
> Ian, could you try the diff from that first tracemonkey link instead? Thanks
> for testing it out!

Yes, that compiled okay along with the "Account for XBL" patch too. I'll look at running some mochitest-browser-chrome tests tomorrow.

Comment 34

[Chris Leary [:cdleary] (not checking bugmail)]

http://hg.mozilla.org/tracemonkey/rev/dd71c1b1159e

Comment 35

[Chris Leary [:cdleary] (not checking bugmail)]

Feel free to test the tracemonkey builds before this lands on mozilla-central, though we should be merging it over later today: http://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/tracemonkey-win32/

Comment 36

[Ian Neal]

Tested with tracemonkey diff on various mochitest-browser-chrome tests on Linux with SeaMonkey and all seems good :D

Comment 37

[Chris Leary [:cdleary] (not checking bugmail)]

cdleary-bot mozilla-central merge info:
http://hg.mozilla.org/mozilla-central/rev/b88172246b66
http://hg.mozilla.org/mozilla-central/rev/066a856dc43f (backout)
http://hg.mozilla.org/mozilla-central/rev/dd71c1b1159e