-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
feat(replay): Use unwrapped setTimeout
to avoid e.g. angular change detection
#11924
Merged
Merged
Changes from all commits
Commits
Show all changes
11 commits
Select commit
Hold shift + click to select a range
c2e6da3
feat(replay): Ensure to use unwrapped `setTimeout` method
mydea dac6791
WIP
mydea 14667a0
jest 29
billyvg 182984d
remove only
billyvg 1015afc
Revert "jest 29"
billyvg 0f5f264
remove setCachedImplmentation, not used in tests anymore
billyvg 43614e5
fix import path
billyvg c51bff9
mock setTimeout and update tests to make sure use fake timers is impo…
billyvg 384d622
lint
billyvg d78efb9
update import
billyvg 8cd00ba
use native fn when possible
billyvg File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,131 @@ | ||
import { logger } from '@sentry/utils'; | ||
import { DEBUG_BUILD } from './debug-build'; | ||
import { WINDOW } from './types'; | ||
|
||
/** | ||
* We generally want to use window.fetch / window.setTimeout. | ||
* However, in some cases this may be wrapped (e.g. by Zone.js for Angular), | ||
* so we try to get an unpatched version of this from a sandboxed iframe. | ||
*/ | ||
|
||
interface CacheableImplementations { | ||
setTimeout: typeof WINDOW.setTimeout; | ||
fetch: typeof WINDOW.fetch; | ||
} | ||
|
||
const cachedImplementations: Partial<CacheableImplementations> = {}; | ||
|
||
/** | ||
* isNative checks if the given function is a native implementation | ||
*/ | ||
// eslint-disable-next-line @typescript-eslint/ban-types | ||
function isNative(func: Function): boolean { | ||
return func && /^function\s+\w+\(\)\s+\{\s+\[native code\]\s+\}$/.test(func.toString()); | ||
} | ||
|
||
/** | ||
* Get the native implementation of a browser function. | ||
* | ||
* This can be used to ensure we get an unwrapped version of a function, in cases where a wrapped function can lead to problems. | ||
* | ||
* The following methods can be retrieved: | ||
* - `setTimeout`: This can be wrapped by e.g. Angular, causing change detection to be triggered. | ||
* - `fetch`: This can be wrapped by e.g. ad-blockers, causing an infinite loop when a request is blocked. | ||
*/ | ||
export function getNativeImplementation<T extends keyof CacheableImplementations>( | ||
name: T, | ||
): CacheableImplementations[T] { | ||
const cached = cachedImplementations[name]; | ||
if (cached) { | ||
return cached; | ||
} | ||
|
||
let impl = WINDOW[name] as CacheableImplementations[T]; | ||
|
||
// Fast path to avoid DOM I/O | ||
if (isNative(impl)) { | ||
return (cachedImplementations[name] = impl.bind(WINDOW) as CacheableImplementations[T]); | ||
} | ||
|
||
const document = WINDOW.document; | ||
// eslint-disable-next-line deprecation/deprecation | ||
if (document && typeof document.createElement === 'function') { | ||
try { | ||
const sandbox = document.createElement('iframe'); | ||
sandbox.hidden = true; | ||
document.head.appendChild(sandbox); | ||
const contentWindow = sandbox.contentWindow; | ||
if (contentWindow && contentWindow[name]) { | ||
impl = contentWindow[name] as CacheableImplementations[T]; | ||
} | ||
document.head.removeChild(sandbox); | ||
} catch (e) { | ||
// Could not create sandbox iframe, just use window.xxx | ||
DEBUG_BUILD && logger.warn(`Could not create sandbox iframe for ${name} check, bailing to window.${name}: `, e); | ||
} | ||
} | ||
|
||
// Sanity check: This _should_ not happen, but if it does, we just skip caching... | ||
// This can happen e.g. in tests where fetch may not be available in the env, or similar. | ||
if (!impl) { | ||
return impl; | ||
} | ||
|
||
return (cachedImplementations[name] = impl.bind(WINDOW) as CacheableImplementations[T]); | ||
} | ||
|
||
/** Clear a cached implementation. */ | ||
export function clearCachedImplementation(name: keyof CacheableImplementations): void { | ||
cachedImplementations[name] = undefined; | ||
} | ||
|
||
/** | ||
* A special usecase for incorrectly wrapped Fetch APIs in conjunction with ad-blockers. | ||
* Whenever someone wraps the Fetch API and returns the wrong promise chain, | ||
* this chain becomes orphaned and there is no possible way to capture it's rejections | ||
* other than allowing it bubble up to this very handler. eg. | ||
* | ||
* const f = window.fetch; | ||
* window.fetch = function () { | ||
* const p = f.apply(this, arguments); | ||
* | ||
* p.then(function() { | ||
* console.log('hi.'); | ||
* }); | ||
* | ||
* return p; | ||
* } | ||
* | ||
* `p.then(function () { ... })` is producing a completely separate promise chain, | ||
* however, what's returned is `p` - the result of original `fetch` call. | ||
* | ||
* This mean, that whenever we use the Fetch API to send our own requests, _and_ | ||
* some ad-blocker blocks it, this orphaned chain will _always_ reject, | ||
* effectively causing another event to be captured. | ||
* This makes a whole process become an infinite loop, which we need to somehow | ||
* deal with, and break it in one way or another. | ||
* | ||
* To deal with this issue, we are making sure that we _always_ use the real | ||
* browser Fetch API, instead of relying on what `window.fetch` exposes. | ||
* The only downside to this would be missing our own requests as breadcrumbs, | ||
* but because we are already not doing this, it should be just fine. | ||
* | ||
* Possible failed fetch error messages per-browser: | ||
* | ||
* Chrome: Failed to fetch | ||
* Edge: Failed to Fetch | ||
* Firefox: NetworkError when attempting to fetch resource | ||
* Safari: resource blocked by content blocker | ||
*/ | ||
export function fetch(...rest: Parameters<typeof WINDOW.fetch>): ReturnType<typeof WINDOW.fetch> { | ||
return getNativeImplementation('fetch')(...rest); | ||
} | ||
|
||
/** | ||
* Get an unwrapped `setTimeout` method. | ||
* This ensures that even if e.g. Angular wraps `setTimeout`, we get the native implementation, | ||
* avoiding triggering change detection. | ||
*/ | ||
export function setTimeout(...rest: Parameters<typeof WINDOW.setTimeout>): ReturnType<typeof WINDOW.setTimeout> { | ||
return getNativeImplementation('setTimeout')(...rest); | ||
} |
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
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
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
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
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
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
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
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
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
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
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
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
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
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
File renamed without changes.
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
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
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
Oops, something went wrong.
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added this, which is a generic version of this: https://github.com/getsentry/sentry-javascript/blob/develop/packages/browser/src/transports/utils.ts#L56-L58
Without this
tracing/metrics/web-vitals-fid
was failing consistently. Is the sandboxed version just too slow for our browser tests? cc @mydeaThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm this looks reasonable. TBH the web-vitals tests are a bit of a black box to me, they appear/are brittle overall 😬 if that makes it pass, all good from my POV!