1108603 - evaluate() with saveBytecode results in "compartment cannot save singleton anymore" if --ion-eager is set

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jon Coppeard (:jonco)]

Executing the following testcase with --ion-eager --ion-offthread-compile=off:

  evaluate(cacheEntry(""), {saveBytecode: {value: true}});
  [[0]];

Gives the error: 

  Error: compartment cannot save singleton anymore.

If saveBytecode is specified then Evaluate() calls setCloneSingletons(true) on the compartment options to indicate that JSOP_OBJECT should deep clone object literals so that we can serialize the script after execution.

However if --ion-eager is set then we compile the script first before calling Evaluate().  When we do call it, it sees that object literals have been already used without cloning them and fails with this error.

I found this because the test code generated by fuzz bug 1108007 does this.

I'm not sure how serious this is.

Comment 1

[Nicolas B. Pierron [:nbp]]

(In reply to Jon Coppeard (:jonco) from comment #0)
> Executing the following testcase with --ion-eager
> --ion-offthread-compile=off:
> 
>   evaluate(cacheEntry(""), {saveBytecode: {value: true}});
>   [[0]];
> 
> Gives the error: 
> 
>   Error: compartment cannot save singleton anymore.
> 
> If saveBytecode is specified then Evaluate() calls setCloneSingletons(true)
> on the compartment options to indicate that JSOP_OBJECT should deep clone
> object literals so that we can serialize the script after execution.
> 
> However if --ion-eager is set then we compile the script first before
> calling Evaluate().  When we do call it, it sees that object literals have
> been already used without cloning them and fails with this error.
> 
> I found this because the test code generated by fuzz bug 1108007 does this.
> 
> I'm not sure how serious this is.

The only issue I see would be related to differential testing, but otherwise nothing to worry about.

The problem is that --ion-eager will encode JSOP_OBJECT based on the cloneSingleton value, and either bake in the constant, or it will produce a DeepClone call.  On the other hand, the evaluate happens before the JSOP_OBJECT of the array. (I wonder why it does not fail because of the argument)  And thus it is still safe to save the bytecode as the object referenced by the bytecode has not been mutated.

I guess the solution would be that as soon we started executing anything under a global, then we should refuse starting any execution which requires saving the bytecode, unless the cloneSingleton flag is set.

Comment 2

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

It seems like we need this before landing bug 1108007, so setting needinfo? in case this fell off the radar.

Comment 3

[Jon Coppeard (:jonco)]

No longer blocks bug 1108007.

Comment 4

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

This blocks fuzzing by randorderfuzz when it tries to use existing tests like http://hg.mozilla.org/mozilla-central/file/37d3dcbf23a9/js/src/jit-test/tests/basic/bug1061534.js for fuzzing, and it generates this false positive when doing differential testing.

Comment 5

[Jon Coppeard (:jonco)]

Attachment: evaluate-save-bytecode-gets-new-global

(In reply to Nicolas B. Pierron [:nbp] from comment #1)

How about we create a new global if the caller requests saveBytecode but doesn't supply a global?  Most existing calls come from the evalWithCache() library function which supplies its own global anyway.

Comment 6

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8583815 [details] [diff] [review]
evaluate-save-bytecode-gets-new-global

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

Thanks for making the patchj, unfortunately, this does not solve a similar issue which would do something like:

// |jit-test| --ion-eager; --no-threads
evaluate(cacheEntry(""), {saveBytecode: {value: true}, global: this});
[[0]];

as the global is explicitly given as argument to be the same global as the top-level one.

I think we should change the error case[1] to something which can be caught/ignored by fuzzers.

Gary, what can we do to make sure that fuzzers ignore such corner cases?

[1] https://dxr.mozilla.org/mozilla-central/source/js/src/shell/js.cpp#1283-1284

Comment 7

[Nicolas B. Pierron [:nbp]]

Is there another way to fail a builtin execution in a way which can be handled by fuzzers?

Comment 8

[Jesse Ruderman]

I'd prefer if you made the error happen consistently, in all modes (not just --ion-eager). But if you just want to quiet differential testing, you could do like ReportOutOfMemory:

#ifdef JS_MORE_DETERMINISTIC
    fprintf(stderr, "Compartment cannot save singleton anymore\n");
#endif

And then we could update the differential testing code to check for this string.

Comment 9

[Nicolas B. Pierron [:nbp]]

Attachment: Evaluate ensure that the global is configured such that we can always clone singletons.

This patch add a new configuration option to the newGlobal function which
configure the compartment options such that we clone singletons instead of
reusing them.

Then this patch change the Evaluate function to ensure that we check the
fact that we cannot clone singleton, to ensure that we get a consistent
error message independently of the execution method.

Comment 10

[Jon Coppeard (:jonco)]

Comment on attachment 8676267 [details] [diff] [review]
Evaluate ensure that the global is configured such that we can always clone singletons.

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

Looks good.

::: js/src/jit-test/tests/xdr/bug1108603.js
@@ +1,5 @@
> +var caught = false;
> +try {
> +  evaluate(cacheEntry(""), {saveBytecode: {value: true}, global: this});
> +  [[0]];
> +  

Nit: trailing whitespace.

Comment 11

[Pulsebot]

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

Comment 13

[Carsten Book [:Tomcat]]

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