[REPLAY] Avoid casting & add proper check instead of relying on try/catch

[ThibautGeriz]

Motivation

Avoid relying on try catch when the object is undefined

Changes

• remove casting
• add proper check

Testing

• Local
• Staging
• Unit
• End to end

---

I have gone over the contributing documentation.

[bcaudan]

Not sure to get the extra value of removing this case from the try catch since we don't do anything more with it 🤔
What motivated this change?

[ThibautGeriz]

Not sure to get the extra value of removing this case from the try catch since we don't do anything more with it 🤔 What motivated this change?

To me handle a case with a if is more graceful than using a try/catch but it should not change much. Maybe it's simply a personal preference

[bcaudan]

oh ok, not super fan of using try/catch either when we can avoid it.
I'd say that it could be nice if we could remove the try/catch completely and use only conditional logic or maybe reduce the scope of the try/catch.
Otherwise, we are adding an extra code path without any behaviour difference so it raised the question what is special about this case for it to be handled separately.

[ThibautGeriz]

I'd say that it could be nice if we could remove the try/catch completely and use only conditional logic or maybe reduce the scope of the try/catch.

I can remove the try catch as well and wrap the function into a monitor if something breaks we will know and the error would be contain. WDYT?

[bcaudan]

I don't have a lot of context on this specific code (@BenoitZugmeyer could know more), so I'd probably start by trying to understand a bit more what we were trying to achieve with this try/catch in the first place, if it is our implementation or if it comes from rrweb and depending on that see if we have a better strategy to than this try/catch.

Stepping a bit back, I would wonder if it is a fight worth picking and if it worth picking it now 🙂
Adding a task in the backlog to get to the bottom of this at some point could also be an option and we could decide to tackle it while working on a related topic.

[BenoitZugmeyer]

💬 suggestion: ‏I would reduce the try/catch scope to be minimal, and remove the monitor() above

function _getCssRulesString(cssStyleSheet: CSSStyleSheet | undefined | null): string | null {
  if (!cssStyleSheet) {
    return null
  }
  let rules
  try {
    rules = cssStyleSheet.rules || cssStyleSheet.cssRules
  } catch {
    // if css is protected by CORS we cannot access cssRules see: https://www.w3.org/TR/cssom-1/#the-cssstylesheet-interface
  }
  if (!rules) {
    return null
  }
  const styleSheetCssText = Array.from(rules, getCssRuleString).join('')
  return switchToAbsoluteUrl(styleSheetCssText, cssStyleSheet.href)
}

Or, if we want to keep a trace of unexpected exceptions:

function _getCssRulesString(cssStyleSheet: CSSStyleSheet | undefined | null): string | null {
  if (!cssStyleSheet) {
    return null
  }
  let rules
  try {
    rules = cssStyleSheet.rules || cssStyleSheet.cssRules
  } catch (error) {
    // if css is protected by CORS we cannot access cssRules see: https://www.w3.org/TR/cssom-1/#the-cssstylesheet-interface
    if (!(error instanceof DOMException) || error.name !== 'SecurityError') {
      addTelemetryError(error)
    }
  }
  if (!rules) {
    return null
  }
  const styleSheetCssText = Array.from(rules, getCssRuleString).join('')
  return switchToAbsoluteUrl(styleSheetCssText, cssStyleSheet.href)
}

[BenoitZugmeyer]

@bcaudan what do you think? Is it worth logging unexpected exceptions here to be aware of things that are failing?

[bcaudan]

Is it worth logging unexpected exceptions here to be aware of things that are failing?

ultimately, if accessing those rules or cssRules raises an exception, we would not be able to do much.
It could be interesting to see if there are other cases than the ones identified (for curiosity sake) but eventually we should probably just leave the catch clause empty.