Skip to content

Commit 023dcce

Browse files
committed
JS: Disable variable capture heuristic
Bailing out can be more expensive as the resulting jump steps themselves cause perf issues. The limit of 100 variables per scope has also been added in the interim, which handles the cases that this needed to cover.
1 parent 37676f4 commit 023dcce

File tree

1 file changed

+1
-25
lines changed

1 file changed

+1
-25
lines changed

javascript/ql/lib/semmle/javascript/dataflow/internal/VariableCapture.qll

Lines changed: 1 addition & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -23,35 +23,11 @@ module VariableCaptureConfig implements InputSig<js::DbLocation> {
2323
or
2424
isTopLevelLike(container.(js::ImmediatelyInvokedFunctionExpr).getEnclosingContainer())
2525
or
26-
// Functions declared in a top-level with no parameters and can't generate flow-through, except through 'this'
27-
// which we rule out with a few syntactic checks. In this case we treat its captured variables as singletons.
28-
// NOTE: This was done to prevent a blow-up in fiddlesalad where a function called 'Runtime' captures 7381 variables but is only called once.
29-
exists(js::Function fun |
30-
container = fun and
31-
fun.getNumParameter() = 0 and
32-
isTopLevelLike(fun.getEnclosingContainer()) and
33-
not mayHaveFlowThroughThisArgument(fun)
34-
)
35-
or
36-
// Container declaring >100 captured variables tend to be singletons and are too expensive anyway
26+
// Containers declaring >100 captured variables tend to be singletons and are too expensive anyway
3727
strictcount(js::LocalVariable v | v.isCaptured() and v.getDeclaringContainer() = container) >
3828
100
3929
}
4030

41-
private predicate hasLocalConstructorCall(js::Function fun) {
42-
fun = getLambdaFromVariable(any(js::NewExpr e).getCallee().(js::VarAccess).getVariable())
43-
}
44-
45-
private predicate mayHaveFlowThroughThisArgument(js::Function fun) {
46-
any(js::ThisExpr e).getBinder() = fun and
47-
not hasLocalConstructorCall(fun) and // 'this' argument is assumed to be a fresh object
48-
(
49-
exists(fun.getAReturnedExpr())
50-
or
51-
exists(js::YieldExpr e | e.getContainer() = fun)
52-
)
53-
}
54-
5531
class CapturedVariable extends LocalVariableOrThis {
5632
CapturedVariable() {
5733
DataFlowImplCommon::forceCachingInSameStage() and

0 commit comments

Comments
 (0)