1116760 - Add a shell builtin to test function relazification

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jan de Mooij [:jandem]]

Attachment: Patch

We've had a number of topcrashes caused by function relazification, the latest one is bug 1115776. These are usually hard to reproduce and track down because they depend on GC timing, JIT compilation of particular scripts etc.

Although it's possible to test relazification in the shell, it requires newGlobal() + eval (see the shell testcase in bug 1115776). The fuzzers do sometimes generate such tests, but I think it's harder or they would have found bug 1115776 a long time ago.

I'm attaching a patch that adds a new shell function, relazify(), to test relazification. This function:

* Must be called directly in the global scope (or it will just throw an exception and do nothing). If this is a problem for the fuzzers (langfuzz always uses eval?), we could maybe come up with something else.

* Accepts exactly the same arguments as gc().

Requesting feedback to see what the fuzzers come up with. It may find problems that won't happen without the patch but hopefully not too many.

Comment 1

[Jan de Mooij [:jandem]]

Attachment: Patch

Added a simple jit-test that uses this function:

jit-test/tests/basic/relazify.js

In case it helps the fuzzers.

Comment 2

[Christian Holler (:decoder)]

(In reply to Jan de Mooij [:jandem] from comment #0)
>
> * Must be called directly in the global scope (or it will just throw an
> exception and do nothing). If this is a problem for the fuzzers (langfuzz
> always uses eval?), we could maybe come up with something else.

LangFuzz typically uses load() or evaluate(). Would that also work?

Comment 3

[Jan de Mooij [:jandem]]

(In reply to Christian Holler (:decoder) from comment #2)
> LangFuzz typically uses load() or evaluate(). Would that also work?

Hm no, there should be only one script frame on the stack, the global script. If you use load or evaluate there will also be a frame for the caller of load/evaluate and it won't work.

Gary, is this also a problem for jsfunfuzz? Does it execute scripts directly or does it load/evaluate them?

Comment 4

[Jan de Mooij [:jandem]]

Attachment: Patch v2

This patch gets rid of the "call in global script" restriction. We now mark the scripts for frames that are on the stack as do-not-relazify, so it no longer matters how relazify() is called.

I temporarily added this behavior to gc() and all jit-tests/jstests passed, so hopefully we can do this...

The fuzzers may find some false positives. If there are too many we can add some restrictions maybe, if there are a few we should just fix them.

Comment 5

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

(In reply to Jan de Mooij [:jandem] from comment #3)
> Gary, is this also a problem for jsfunfuzz? Does it execute scripts directly
> or does it load/evaluate them?

Not directly for jsfunfuzz, but randorderfuzz uses load, so that fuzzer is affected.

Comment 6

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

Comment on attachment 8543271 [details] [diff] [review]
Patch v2

I gave this a cursory check and it seems to work ok (not blow up anything) on my side. At least, not yet, after half an hour.

(Reviews should go to a reviewer of the code? So setting feedback+)

Comment 7

[Jan de Mooij [:jandem]]

Comment on attachment 8543271 [details] [diff] [review]
Patch v2

(In reply to Gary Kwong [:gkw] [:nth10sd] from comment #6)
> I gave this a cursory check and it seems to work ok (not blow up anything)
> on my side. At least, not yet, after half an hour.

\o/

> (Reviews should go to a reviewer of the code? So setting feedback+)

Oops, when I posted the second patch I used review instead of feedback.

Comment 8

[Andrew McCreight [:mccr8]]

Marking sec-other because it looks like there is no security issue that has been found here.

Comment 9

[Jan de Mooij [:jandem]]

Comment on attachment 8543271 [details] [diff] [review]
Patch v2

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

According to Gary, jsfunfuzz didn't blow up with this patch applied, so we should probably get it in.

Comment 10

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

(In reply to Jan de Mooij [:jandem] from comment #9)
> According to Gary, jsfunfuzz didn't blow up with this patch applied, so we
> should probably get it in.

Hang on, this needs more testing.

Comment 11

[Jesse Ruderman]

Is combining relazification with gczeal likely to find more bugs than just testing at user code call points? If so, you should probably make this a setting, like a gcparam, rather than a separate function.

Please add regression tests for everything you can think of, such as:

* Trying to relazify a function that is on the stack
* Trying to relazify a generator that has yielded
* Whatever is going on in bug 1115776 (an interaction with ion?)

Comment 12

[Jan de Mooij [:jandem]]

(In reply to Jesse Ruderman from comment #11)
> Is combining relazification with gczeal likely to find more bugs than just
> testing at user code call points? If so, you should probably make this a
> setting, like a gcparam, rather than a separate function.

Separate function is intentional. We don't usually relazify if there are scripts on the stack from that compartment or if the compartment has been entered for another reason. GC's can be triggered in a *lot* more places than we can reenter JS, and we'd get many false positives. 

> Please add regression tests for everything you can think of, such as:
> 
> * Trying to relazify a function that is on the stack
> * Trying to relazify a generator that has yielded
> * Whatever is going on in bug 1115776 (an interaction with ion?)

I wrote a test for bug 1115776 that uses this new relazify() function but we can't add it yet until the fix is on all branches. Can add some others though.

Comment 13

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

Comment on attachment 8543271 [details] [diff] [review]
Patch v2

Ok, I'm fairly sure this doesn't expose any new bugs, after changing some of our testing code and running overnight.

Comment 14

[Till Schneidereit [:till]]

Comment on attachment 8543271 [details] [diff] [review]
Patch v2

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

Is there any reason for having this available in non-debug builds? I admit that it might reveal opt-only bugs, but enough so to make it worth having the extra checks and the flag on JSScript?

Anyway, r=me whichever decision you make regarding this.

::: js/src/builtin/TestingFunctions.cpp
@@ +2229,5 @@
>      JS_FN_HELP("gcparam", GCParameter, 2, 0,
>  "gcparam(name [, value])",
>  "  Wrapper for JS_[GS]etGCParameter. The name is one of " GC_PARAMETER_ARGS_LIST),
>  
> +    JS_FN_HELP("relazify", Relazify, 0, 0,

I think I would slightly prefer "relazifyFunctions". (We have other types of laziness in the engine, such as builtins that're installed lazily via resolve hooks.) I don't care enough to hold up the patch over it, though, so up to you.

Comment 15

[Jan de Mooij [:jandem]]

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

(In reply to Till Schneidereit [:till] from comment #14)
> Is there any reason for having this available in non-debug builds? I admit
> that it might reveal opt-only bugs, but enough so to make it worth having
> the extra checks and the flag on JSScript?

It's very little extra code so I don't think it matters.

> I think I would slightly prefer "relazifyFunctions". (We have other types of
> laziness in the engine, such as builtins that're installed lazily via
> resolve hooks.)

OK, I renamed it to relazifyFunctions :)

Comment 16

[Phil Ringnalda (:philor)]

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