Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Tab Crashing Fixes #203

Merged
merged 3 commits into from
Feb 23, 2024
Merged

Tab Crashing Fixes #203

merged 3 commits into from
Feb 23, 2024

Conversation

tmbrbr
Copy link
Contributor

@tmbrbr tmbrbr commented Feb 23, 2024

Fixing a few bugs introduced by recent changes:

  • Memory allocation issue in JSONParser - the CurrentJsonPath method was creating a lot of Strings in the nursery, which I think was overwhelming the GC at some point. Solution is to create the strings in the Tenured heap.
  • Memory leak caused by creation of raw StringTaint objects (which do not clean up when destroyed)

I also found the dump() method which was handy to debug the JSONParser issue. I've added a TAINT_DEBUG check to it so it can be enabled which needed a complete debug build.

The CurrentJsonPath method was causing tab crashes when
parsing large JSON files. These seem to occur when the JSON
parser was running at the same time as Nursery Strings were
moved to Tenured space. My feeling is that the JSONPath strings
were being created faster than they could be moved out of the
nursery, causing some nasty segfaults.

A quick for this appears to be to create the JSONPath strings
directly in the Tenured heap. This is probably not ideal, as
these strings are only used to create TaintOperations.

This is probably worth revisiting to see if there is a better
solution. There seems to be a lot of upcoming upstream changes
related to String GC, so probably worth waiting for the next
release (see e.g. 7e2b43b)
@tmbrbr tmbrbr self-assigned this Feb 23, 2024
@tmbrbr tmbrbr added bug Something isn't working javascript Pull requests that update Javascript code performance Performance Issues and Enhancements labels Feb 23, 2024
@tmbrbr tmbrbr requested review from leeN and vladidx February 23, 2024 09:21
@tmbrbr
Copy link
Contributor Author

tmbrbr commented Feb 23, 2024

@leeN: we should probably use this fix for any crawls to increase stability and stop horrendous memory leaks!

@vladidx: this should resolve the stability issues we discussed.

@tmbrbr tmbrbr changed the title Tabcrash fixes Tab Crashing Fixes Feb 23, 2024
Copy link
Collaborator

@leeN leeN left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@tmbrbr tmbrbr merged commit d85187f into SAP:main Feb 23, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working javascript Pull requests that update Javascript code performance Performance Issues and Enhancements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants