1181908 - Creating a CompileOptions from a ReadonlyCompileOptions doesn't properly copy isRunOnce and introducerFilename

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Boris Zbarsky [:bzbarsky]]

This was causing disfile to report different bytecode from what actually ran for me... quite annoying.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Looks like I introduced this in bug 679939.  Not sure this is worth backporting to 40, though...

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: The CompileOptions constructor should properly copy the introducerFilename and isRunOnce state

Comment 3

[Luke Wagner [:luke]]

Comment on attachment 8631384 [details] [diff] [review]
The CompileOptions constructor should properly copy the introducerFilename and isRunOnce state

Looks like introducerFilename is also exposed via Debuger API, so perhaps this could lead to a debugger regression (although I'm not sure under what circumstances the introducerFilename isn't copied).  Hard to judge whether it warrants a backport, though.  I'd be inclined to let it ride the trains.

Comment 4

[Boris Zbarsky [:bzbarsky]]

So this patch actually fails tests.  For example, it fails js/src/jit-test/tests/asm.js/testModuleFunctions.js when run with --args="--ion-eager --ion-offthread-compile=off"

The failure is a "flags && !(flags & ~ValidFlags()" assert in code reached via ICTypeUpdate_PrimitiveSet::Compiler::getStub.  The problem is that in ICUpdatedStub::addUpdateStubForValue we have:

(gdb) p val.ptr->data.debugView.tag
$14 = JSVAL_TAG_MAGIC
(gdb) p val.ptr->data.s.payload.why
$15 = JS_OPTIMIZED_ARGUMENTS

and this apparently tests true for isPrimitive() but the IC code then asserts that we don't have a JSVAL_TYPE_MAGIC.  But our flags are in fact TypeToFlag(JSVAL_TYPE_MAGIC).

I have verified that commenting out the isRunOnce assignment makes this test pass.  I guess I'll look into this more tomorrow...

Comment 5

[Boris Zbarsky [:bzbarsky]]

Oh, and if I apply this patch directly on top of the patches in bug 679939 I get the same failure.

In fact, I get it if I apply this patch on top of https://hg.mozilla.org/mozilla-central/rev/c538f12b63bd which is the first changeset that started passing in isRunOnce set to true.

Comment 6

[Boris Zbarsky [:bzbarsky]]

OK, if I just make IonBuilder::hasStaticScopeObject always return false things work correctly again, both on tip and back when bug 679939 landed.

More tomorrow.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Spun off that assert failure into bug 1182133.

Comment 8

[Boris Zbarsky [:bzbarsky]]

OK, so the failures are caused by the CompileOptions we create in BytecodeEmitter::emitFunction copying the isRunOnce flag, incorrectly; we always want isRunOnce to be false there.

Possible fixes are to set it explicitly to false in that code (and any other code that cares about that sort of thing; would need to check for that) or to not copy it and set it explicitly in the code that _does_ want to copy the option (e.g. the code that runs files in the shell).  So kinda need to look for all instances of the CompileOptions or OwningCompileOptions copy constructors anyway...

Luke, any preference between those approaches?

Comment 9

[Luke Wagner [:luke]]

If isRunOnce is always valid if 'false' (it just allows more aggressive optimization), then seems like we should not copy it and propagate explicitly.

Comment 10

[Boris Zbarsky [:bzbarsky]]

OK.  So if things like DXR and my compiler are not lying to me, we have the following places that call copyPODOptions right now:

1)  ParseTask::init, doing a copy() on OwningCompileOptions.  In this case we should copy the boolean, I believe.  Should OwningCompileOptions::copy just always copy it?

2)  BytecodeEmitter::emitFunction constructing a CompileOptions for the function script.  This wants the boolean false.

3)  Compile from filename in jsapi.cpp, creating a CompileOptions copy to set the filename.  This is what initially got me into this bug.

4)  CompileFunction in jsapi.cpp for no good reason I can see.  Looks like this used to setHasPollutedScope(true) in some cases but bug 1165486 removed that bit without removing the extra object.  I think this object should just go away.

5)  Evaluate in jsapi.cpp.  This then does explicit setIsRunOnce(true).

6)  Evaluate from filename in jsapi.cpp.  This should probably do the same as #5.

Agreed on those?

If so, I guess I can not copy by default, copy explicitly for #1 and #3, fix up #4 to not be silly, and fix #6 to set true.  We'll have a performance footgun any time someone instantiates a CompileOptions from another CompileOptions to frob something, though.  :(

Comment 11

[Luke Wagner [:luke]]

Now that you've enumerated the options (thanks!), it seems like the right default is for copy to, well, copy.  It seems like it's only (2) that is saying "make a copy, but really this is a logically new compilation, it's just that most of the options are the same, so I'm doing a copy then fixing up".  So now I'm thinking I was wrong in comment 9.

FWIW, I think it'd be more future proof to factor out a TransitiveCompileOptions out of CompileOptions with the meaning that "these are the compile options that are transitively propagated to nested functions".  Are there other fields of CompileOptions that are in the same boat as isRunOnce?

Comment 12

[Boris Zbarsky [:bzbarsky]]

emitFunction copies the compile options from parser->options(), then sets the muted errors flag and version from the parent script instead, and sets noScriptRval and forEval to false always.

Looking at the actual members of ReadonlyCompileOptions, the lineno and column are probably bunk in this case; not sure where those get overriden to the right thing.  The rest look ok to copy...

I'm not quite sure what class hierarchy you're thinking of with TransitiveCompileOptions.  Is the idea here that parser->options() would return a TransitiveCompileOptions reference, and ReadonlyCompileOptions would inherit from TransitiveCompileOptions?

Comment 13

[Luke Wagner [:luke]]

The image I had in my head was more like, emitFunction is written:
  const TransitiveCompileOptions& transitiveOptions = parser->options();
  CompileOptions funcOptions(transitiveOptions);
so the non-transitive options are explicitly sheared off.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Attachment: With TransitiveCompileOptions

Comment 15

[Luke Wagner [:luke]]

Comment on attachment 8641965 [details] [diff] [review]
With TransitiveCompileOptions

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

::: js/src/frontend/BytecodeEmitter.cpp
@@ +5774,1 @@
>                     .setVersion(parent->getVersion());

I would assume that |parent->mutatedErrors() == parser->options().mutedErrors()| and similarly for version.  Perhaps MOZ_ASSERT this, and if jit_tests pass, remove these two manual copies?  It'd be nice of TransitiveCompileOptions was the exact set of things that were propagated.

::: js/src/frontend/Parser.h
@@ +543,5 @@
>          MOZ_ASSERT(tokenStream.debugHasNoLookahead());
>          return pc->sc->setLocalStrictMode(strict);
>      }
>  
> +    const TransitiveCompileOptions& options() const {

In theory some code may want to access fields from ReadonlyCompileOptions and I don't see why they shouldn't, so it doesn't seem like we should upcast at this point but, rather, at the use site (which has the benefit of being more explicit about our intention).

::: js/src/jsapi.h
@@ +3490,5 @@
>  
>      // |introductionType| is a statically allocated C string:
>      // one of "eval", "Function", or "GeneratorFunction".
> +    // XXXbz should these, and introducerFilename_, be here or on
> +    // ReadOnlyCompileOptions?

I expect these are transitive as long as one interprets "introducer" as "who introduced the whole script being compiled".

@@ +3664,3 @@
>          filename_ = rhs.filename();
> +        introducerFilename_ = rhs.introducerFilename();
> +        sourceMapURL_ = rhs.sourceMapURL();

Could these three assignments be moved to copyPODTransitive()?

@@ +3680,1 @@
>          sourceMapURL_ = rhs.sourceMapURL();

ditto

Comment 16

[Boris Zbarsky [:bzbarsky]]

> Perhaps MOZ_ASSERT this, and if jit_tests pass, remove these two manual copies?

Seems to work at first glance.  OK.

> it doesn't seem like we should upcast at this point but, rather, at the use site

OK, will do.

> Could these three assignments be moved to copyPODTransitive()?

No, because these are not POD members.  In particular, CompileOptions and OwningCompileOptions manage these members differently.

Comment 17

[Boris Zbarsky [:bzbarsky]]

Attachment: The CompileOptions constructor should properly copy the introducerFilename and isRunOnce state

Comment 18

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2f16fb18314a

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2f16fb18314a

Comment 20

[Ryan VanderMeulen [:RyanVM]]

Backed out for suspicion of causing bug 1191492 and other topcrashes.

https://hg.mozilla.org/mozilla-central/rev/2fad09a0540d

Comment 21

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

That suspicion was apparently misplaced, as bc was still able to reproduce the issues post-backout.

Relanding to inbound in https://hg.mozilla.org/integration/mozilla-inbound/rev/fc5272ac152e

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/fc5272ac152e

Comment 23

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Backed this out again based on https://bugzilla.mozilla.org/show_bug.cgi?id=1191465#c25.

https://hg.mozilla.org/integration/mozilla-inbound/rev/fd69d51a4068

Comment 24

[Boris Zbarsky [:bzbarsky]]

Just for my reference, http://www.profootballhof.com/hof/teams.aspx seems to reliably reproduce the crash in question.

Comment 25

[Boris Zbarsky [:bzbarsky]]

OK, so some data: the problem is that with this patch we copy the isRunOnce flag in ParseTask::init (via the this->options.copy() call).  The ParseTask::init call is made from js::StartOffThreadParseScript.  This is being called from the scriptloader, so the isRunOnce flag is set, but we used to not copy it here...

Anyway, so we copy the flag.  Then I'm not sure what happens but the upshot is that we end up hitting a fatal assert in a debug build because ~LinkedList is called on a nonempty list.  This list is the "unboxedLayouts" member of a JSCompartment that's being destroyed (~JSCompartment, because it's being swept).

I checked, and the compartment being swept is the compartment of the global that was created in js::StartOffThreadParseScript.  So presumably the parsing compartment?

Anyway, is it expected that whether unboxedLayouts is empty depends on isRunOnce?

Comment 26

[Brian Hackett [Laid off!]]

Attachment: fix unboxedLayouts list

As discussed on IRC, the patch in this bug exposed functionality that we're supposed to be able to support off thread (emitting JSOP_OBJECT) but previously never did.  The unboxedLayouts list on the off thread compartment needs to be either transferred to the target compartment or cleared.  This patch does the latter, which is simpler (the unboxed layouts list is used in heuristics to select common layouts for unboxed objects that are subclasses of each other, and isn't required to be a complete list).

Comment 27

[Boris Zbarsky [:bzbarsky]]

Attachment: Minimalish testcase that crashes without bhackett's patch

I've confirmed that attachment 8653650 [details] [diff] [review] fixes both this testcase and the crashing site from comment 24.

Comment 28

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8cd6dd07c27a
https://hg.mozilla.org/integration/mozilla-inbound/rev/2d89bf9f70ee

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8cd6dd07c27a
https://hg.mozilla.org/mozilla-central/rev/2d89bf9f70ee