fix(utils): Try-catch monkeypatching to handle frozen objects/functions #9031
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
lforst
reviewed
Sep 15, 2023
| const proto = original.prototype || {}; | ||
| wrapped.prototype = original.prototype = proto; | ||
| addNonEnumerableProperty(wrapped, '__sentry_original__', original); | ||
| } catch (o_O) {} // eslint-disable-line no-empty |
There was a problem hiding this comment.
h: I would add a log here (maybe even non-debug!). I can already see myself debugging an issue that some instrumentation isn't working. This is a very opaque way to fail.
There was a problem hiding this comment.
I can add a debug log, but wouldn't do a log. Because it is really not a user problem, the linked issue:
function callback() {
console.log("Hello");
}
setTimeout(callback, 50); // OK
Object.freeze(callback);
setTimeout(callback, 50); // BOOM!Is generally totally OK to write but would result in this. the user shouldn't see a log because of this IMHO.
There was a problem hiding this comment.
I added a debug log & integration tests to actually make sure the issue raised is fixed. Interestingly, if the function if frozen the actual monkey patching still works, only the setting of the wrapped property fails. So the error is still captured even if the callback is frozen, which is good I guess.
size-limit report 📦
|
mydea
force-pushed
the
fn/fix-wrap-freezed
branch
from
89115ab
to
ca34264
Compare
Lms24
approved these changes
Sep 15, 2023
mydea
force-pushed
the
fn/fix-wrap-freezed
branch
from
231207f
to
b37c594
Compare
billyvg
pushed a commit
that referenced
this pull request
Sep 15, 2023
…ns (#9031) This try-catches any monkeypatching we do on objects, to avoid us throwing e.g. if trying to patch a frozen object. Obv. the monkey patching will _not_ actually work then, but I guess it's better to not wrap stuff than to error out in userland. I also added some tests for this! Fixes #9030
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.
This try-catches any monkeypatching we do on objects, to avoid us throwing e.g. if trying to patch a frozen object.
Obv. the monkey patching will not actually work then, but I guess it's better to not wrap stuff than to error out in userland. I also added some tests for this!
Fixes #9030