Remove nonfunctional and crashy pacrunner caching #128
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Can you please rebase this? Let's try to get a new version ready (and then let's push for a 0.5 branch with dbus enabled - getting the js parser out of process) |
libproxy currently attempts to cache pacrunner objects in the pr member variable of its pacrunner_extension object. This is broken, though, because it relies on the pacrunner object also being stored in pacrunner_extension's last member variable, which is never written to. So that caching has never worked properly. In practice, it only does one thing: it causes a threadsafety bug, #68, because it causes the old pacrunner object to be deleted on the thread that is creating the new pacrunner, which is illegal for both the mozjs and WebKit-based pacrunner extensions that expect their objects to be deleted on the same thread they were created on. This patch was originally written by Dan Winship for Fedora 19. It got dropped in Fedora 24, then resurrected again for Fedora 28 after we noticed 30,000 crash reports. I've tweaked it a bit to completely remove the unused member variables. Finally, note that this code is not exception-safe: if an exception is thrown, the pacrunner could be leaked. But this seems to be a common problem throughout libproxy. It should be fixed by using std::unique_ptr instead of raw new and delete. https://bugzilla.redhat.com/show_bug.cgi?id=998232 Fixes #68
|
Rebased. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
libproxy currently attempts to cache pacrunner objects in the pr member
variable of its pacrunner_extension object. This is broken, though,
because it relies on the pacrunner object also being stored in
pacrunner_extension's last member variable, which is never written to.
So that caching has never worked properly. In practice, it only does one
thing: it causes a threadsafety bug, #68, because it causes the old
pacrunner object to be deleted on the thread that is creating the new
pacrunner, which is illegal for both the mozjs and WebKit-based
pacrunner extensions that expect their objects to be deleted on the same
thread they were created on.
This patch was originally written by Dan Winship for Fedora 19. It got
dropped in Fedora 24, then resurrected again for Fedora 28 after we
noticed 30,000 crash reports. I've tweaked it a bit to completely
remove the unused member variables.
https://bugzilla.redhat.com/show_bug.cgi?id=998232
Fixes #68