958172 - Avoid cloning decoded functions by restoring the environment.

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Comment 1

[Nicolas B. Pierron [:nbp]]

Attachment: bug953256.patch

This patch copy the parent (target) of the JSFunction as being the environment in which the JSFunction would be run, or it takes the enclosing scope if it is defined.

This fix one issue detected with the patch attached to Bug 900789, which is that the encoded version of a decoded lazy script did not gave the same serialized content, because the lack of the environment on the JSFunction containing a LazyScript.

Comment 2

[Luke Wagner [:luke]]

Comment on attachment 8357902 [details] [diff] [review]
bug953256.patch

Is this the right patch?  Seems to be all Ion changes.

Comment 3

[Nicolas B. Pierron [:nbp]]

Attachment: 0006-Bug-958172-Set-the-environment-of-decoded-functions.patch

This should be the right patch.  Sorry for attaching the wrong file :/

Comment 4

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8361192 [details] [diff] [review]
0006-Bug-958172-Set-the-environment-of-decoded-functions.patch

I had more issues around this code, and at the end I do not think this patch resolve these issues.

At the end, I do not think I should restore the environment as the Parser is only defining the environment to either Null or the Global (in Parser<…>::newFunction).

As it seems that all uses of the functions are trying to reuse or clone the function and are at the same time computing the scope chain (environment) in which the script should be executed.  I think we can restore the same state as the Parser did, as long as we restore a state which is prior the execution of any JS.

As we currently have no good way to recover what made the Parser chose between Null and the Global, I testing the removal of the global case [1], which should not matter as all environments are now defined when the function is used by JSOP_LAMBDA & JSOP_DEFFUN.

[1] https://tbpl.mozilla.org/?tree=Try&rev=8f1b84cb6487

Comment 5

[Nicolas B. Pierron [:nbp]]

Attachment: Only set the environment while cloning a JSFunction.

As we are now cloning/reusing all JSFunction with JSOP_LAMBDA and JSOP_DEFUN, there should be no need of setting a scope chain during the parsing.

As the XDRDecode goal is to produce the same output as what would the parser produce before any execution, this simplify a lot the logic corresponding to the environment field, as we do not have to capture extra flags to restore the global scope chain.

The assertion in XDRInterpretedFunction check that all unused JSFunction that we are encoding have an null environment (not defined by the parser).

Comment 6

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8377207 [details] [diff] [review]
Only set the environment while cloning a JSFunction.

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

::: js/src/jsfun.cpp
@@ +414,5 @@
> +        // The environment of any function which is not reused will always be
> +        // null, it is later defined when a function is cloned or reused to
> +        // mirror the scope chain.
> +        JS_ASSERT_IF(!((lazy && lazy->hasBeenCloned()) || (script && script->hasBeenCloned())),
> +                     fun->environment() == nullptr);

I just weaken this assertion by adding "fun->hasSingletonType() &&", in order to mirror canReuseFunctionForClone.

The test suite was fine but testXDR from jsapi-test was failing, because it does not compile functions as singletons.  Thus every time the check for CanReuseFunctionForClone was used, it always fails (as it used to do before) which implies cloning the function.

Also note, that testXDR is doing something weird here, as it does the XDR of the cloned function and not the parsed one.

Comment 7

[Nicolas B. Pierron [:nbp]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1b97c5f78e19

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1b97c5f78e19