Skip to content

JS: Move getContainer to single rootdef (+fixes)#3403

Merged
asgerf merged 8 commits into
github:masterfrom
asger-semmle:js/getcontainer
May 6, 2020
Merged

JS: Move getContainer to single rootdef (+fixes)#3403
asgerf merged 8 commits into
github:masterfrom
asger-semmle:js/getcontainer

Conversation

@asgerf
Copy link
Copy Markdown
Contributor

@asgerf asgerf commented May 4, 2020

This moves the definition of getContainer into a single root-def, avoiding some spuriously materialized negation predicates otherwise needed by the dispatch predicate, and ensuring that the whole relation is cached.

I noticed this predicate getting a bit flaky and sometimes very expensive in response to unrelated changes. In particular, magic started pushing "the set of control-flow nodes that belong to a container" as the context, which is a pretty bad context to use anywhere.

I believe the complicated overriding pattern may have been at fault here:

  1. ControlFlowNode.getContainer explicitly referred to this.(Expr).getContainer() even though getContainer() is overridden in Expr, so you'd think the code was dead. Likewise for Stmt. (Ignore, this was just me not paying attention to the class hierarchy.)

  2. Due to multiple inheritance, there were several overrides that exist only to choose arbitrarily between two inherited definitions of getContainer, which were equivalent but not obviously so.

  3. There wasn't a single root-def for getContainer. For example ClassOrInterface.getContainer did not override ControlFlowNode.getContainer. Not all these root-defs were cached.

There are also a few follow-up performance fixes which are independently useful on their own, but needed to make this safe to land.

Evaluations:

@asgerf asgerf added the JS label May 4, 2020
@asgerf asgerf requested a review from a team as a code owner May 4, 2020 11:47
@asgerf
Copy link
Copy Markdown
Contributor Author

asgerf commented May 4, 2020

Oof. Did not expect these test failures. Maybe hold back on reviews until I've fixed.

erik-krogh
erik-krogh previously approved these changes May 5, 2020
Copy link
Copy Markdown
Contributor

@erik-krogh erik-krogh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are getting more and more pragma[nomagic] into our code.
But it improves performance, so LGTM 👍

Just a small optional comment about a parameter type.

Comment thread javascript/ql/src/semmle/javascript/internal/StmtContainers.qll Outdated
@asgerf asgerf added the depends on internal PR This PR should only be merged in sync with an internal Semmle PR label May 5, 2020
@asgerf
Copy link
Copy Markdown
Contributor Author

asgerf commented May 5, 2020

Hm, one evaluations based on this one started showing suspicious results on big-apps. I'll need to run a big-apps evaluation of this.

@asgerf
Copy link
Copy Markdown
Contributor Author

asgerf commented May 6, 2020

XSS on big-apps looks fine.

I've rebased and opened an internal PR with the dbscheme hash bump.

erik-krogh
erik-krogh previously approved these changes May 6, 2020
@asgerf
Copy link
Copy Markdown
Contributor Author

asgerf commented May 6, 2020

@erik-krogh the internal PR is green. Can I get approval to merge both?

@asgerf asgerf merged commit 5725814 into github:master May 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

depends on internal PR This PR should only be merged in sync with an internal Semmle PR JS

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants