Add descriptive error message for looper mismatch in BaseMediaSource

[NoNews]

Reusing a cached MediaSource across players with different playback threads throws a message-less IllegalArgumentException, silently swallowed into error code 1004.

Impact at Reddit Android App
We experienced ~2M errors/day. It only showed up in one specific edge case, so we had nothing to go on but telemetry with 1004 error code — and the empty exception made it much harder to pin down. (We spent a week debugging this).

We're currently experimenting with a single shared looper and seeing the problem addressed.

See #2263

Thank you!

[icbaker]

Did the stack trace you had not point to the line of the checkArgument call (and so the cause would be obvious by looking at the source)?

I think in general we write exception checks & messages on the assumption that the reader will cross-reference with the relevant source code for additional context. This is partly to keep APK bloat in check by avoiding long strings on every edge-case exception check.

[NoNews]

Hi @icbaker and thank you for the feedback!

Did the stack trace you had not point to the line of the checkArgument call (and so the cause would be obvious by looking at the source)?

Yes, once we were able to reproduce it (which was the hardest part as only tiny fraction of users were affected), we were able to see the trace.

I think in general we write exception checks & messages on the assumption that the reader will cross-reference with the relevant source code for additional context. This is partly to keep APK bloat in check by avoiding long strings on every edge-case exception check.

Got it! Do you think we can still have some distinction and use a shorter message, to reduce the impact? Like:

heckArgument(
      this.looper == null || this.looper == looper,
      "Wrong looper used");

The reason we ask: without a message, our error reporting only showed Unexpected error with code 1004 — nothing pointing to a looper or a MediaSource. We report the error code together with the exception message, so even a short string likeWrong looper used would have appeared in our telemetry right away and told us where to look, saving a week of investigation time

[icbaker]

Thanks for the extra context.

I chatted about this with others on the team, and we are a bit surprised that your error telemetry completely drops stack traces and only reports exception messages. We think it's unreasonable to expect every exception message in the library to be unique (in order to identify the root cause from just the message), and it would make more sense for error-reporting telemetry to include the stack trace. I assume this would be obfuscated, but could of course be unobfuscated with the appropriate mapping file.

Given this, we don't plan to add additional exception messages routinely in the codebase, and that includes this PR - so I'm going to close this.