Make @bugsnag/plugin-inline-script-content more robust #528
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
This commit combines the approach from v3 and v4 to make the inline script handling more robust, as well as providing a good approximation of the surrouding code. In order to do this we wrap every single async entry point.
bengourley
force-pushed
the
inline-script-improvement
branch
from
d80719e
to
6f745e7
Compare
…a "prototype" in old Opera
After all the other bugsnag plugins have been initialised, add the wrapping. If this is done beforehand, some of the other obtrusive plugins (such as the breadcrumb listener for clicks) can interfere with the inline script handling, incorrectly making it find the script where bugsnag was initialised.
When the traced callback doesn't error, the original script value can be reset synchronously. In some cases the asynchronous null-ing of the current script allowed the wrong script to be identified and attached to reports when an error happened in the same tick as a wrapped callback. Also, while none of the testing so far yeilded a concern about the wrapped callback returning the original callback's return value, but it should operate as transparently as possible. This ensures the return value is passed on.
bengourley
force-pushed
the
inline-script-improvement
branch
from
261c86f
to
e9f9856
Compare
kattrali
approved these changes
May 7, 2019
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.
The approach to inline script handling changed in v4+ of the browser notifier. Compared with the old approach, the updated approach makes the experience better when it succeeds – providing surrounding code for the location of the error – but when it fails it causes bad grouping and confusing displays of surrounding code. It fails more often than the approach of its predecessor, which has a negative impact on the users of Bugsnag who monitor websites which make heavy use of inline scripts.
This changeset combines the approach from v3 and v4 to make the inline script handling more robust, as well as providing a good approximation of the surrouding code.
Outline of the combined approach
Using the implementation in v3 as a guide, the
plugin-inline-script-contenthas been updated to trace execution of all callbacks back to their original script using the following algorithm:lastScriptgetCurrentScript()which returnsdocument.currentScriptif defined, otherwise…lastScriptif defined, otherwise…document.scriptsif the document is still loadinglastScriptbefore the async callback runs and unsetslastScriptonce it has completed (this list includes:setTimeout,setInterval,setImmediate,requestAnimationFrameand everything in this list)beforeSendcallback for every errorgetCurrentScript()to get the last known script to be running before this error was caught. Attach theinnerHTMLvalue of the script object toreport.metaData.script.content.And using a similar implementation to v4, but without searching around for
<script>tags using regexes, simple extracting the surrounding lines:beforeSendcallback described above for every error, when we know we are dealing with an inline script…innerTextof thedocument.documentElementand pad it with a<!DOCTYPE html>-like first line. This is the best approximation of the raw html the browser received that it is possible to get with JS. Using the line number of the stackframe, extract the surrounding code (3 lines above and 3 lines below) and add this to thestackframe.codeproperty. These lines may contain JS/HTML or a mix of both.Testing
I have thoroughly tested this in a variety of browsers, comparing with the behaviour of the v3 notifier and of no notifier at all – paying particular attention to old IEs where the stacktrace information is – at best – limited. In the process I fixed a bug where we could have got a better stacktrace for handled errors (1805259).
The approach is water-tight (as this is what the old notifier did) but the codebase has moved on since then so this is new code and could break in new ways. This needs a lot of manual testing in addition to the end to end tests.
I manually verified the following behaviour:
setTimeout) have the correctthiswhen executing the callbackVersioningRelease planI would be averse to anything but a major version bump for this changeset. In my opinion is is too wide reaching.Rather than a major version bump (since this has no breaking API changes) we are going to stagger the rollout, first to npm, and then follow up with a CDN release.
Bundle size
To mitigate the known increase in bundle size due to this changeset I removed the deprecated config warnings (cb3e793). Even so, the bundle increase is rather significant:
Todo
__proxy'd fns with bad argumentsaddEventListenercallbacks receive the correctthisvaluemetaData.script.contentto something (but way more than 200 chars)