Skip to content
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

Fix incorrect non-null assertion #306

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

mnordine
Copy link
Contributor

I believe _target can be null here.

I believe `_target` can be `null` here.
@sigmundch
Copy link
Member

mmm - I'm inclined to tighten this logic differently.

I believe the intended invariant was to have 2 states for the event stream subscriptions:

  • active: _onData and _target are not null
  • inactive: _onData is null. Typically subscriptions are inactive either because the onData handler hasn't been set or because the subscription was cancelled.

One way to tighten this differently would be to make the constructor parameter stricter and only accept a non-null _target value. Another option is to keep the type signature as it is, but immediately cancel the subscription when the provided _target is null (since there is no API to set the target after the subscription was created).

For reference - this non-null assertion has been in this logic not only here, but also in the equivalent logic we had in dart:html where this was ported from.

I'm curious - what scenario made you run into this issue, @mnordine?

@mnordine
Copy link
Contributor Author

mnordine commented Sep 23, 2024

One way to tighten this differently would be to make the constructor parameter stricter and only accept a non-null _target value.

I agree, just not sure why it wasn't done like that in the first place.

I'm curious - what scenario made you run into this issue, @mnordine?

I was looking into a stack trace generated from Sentry for our Dart web app that uses web:

NoSuchMethodError: Null check operator used on a null value
  at A._EventStreamSubscription.cancel.prototypeLc(package:web/src/helpers/events/streams.dart:258:9)
  at A.aS(package:web/src/helpers/events/streams.dart:160:3)
  at App.start(package:web/src/helpers/events/streams.dart:130:7)
  at A.bsd(org-dartlang-sdk:///lib/_internal/js_runtime/lib/async_patch.dart:303:19)
  at dartProgram(main.c20686344c6ca935.js:329:4)
  at dartProgram(main.c20686344c6ca935.js:279:67)
  at A._rootRunUnary(org-dartlang-sdk:///lib/async/zone.dart:1419:1)
  at installTearOffs(main.c20686344c6ca935.js:1405:3)
  at A.Gi.prototype_CustomZone.runUnary(org-dartlang-sdk:///lib/async/zone.dart:1315:12)
  at A._Future._propagateToListeners.handleValueCallback._Future._propagateToListeners.handleValueCallback$0(org-dartlang-sdk:///lib/async/future_impl.dart:862:13)

So I started looking into it.

I want to stress that the stacks generated from sentry-cli are very poor, and we have never been able to figure it out. It seems Sentry isn't terribly interested in fixing stack traces for the web.

@sigmundch
Copy link
Member

Do you by chance have access to the original js stack trace as well as as the source-map of the compiled code at the time this stack was produced? We have a script dart2js_tools/bin/deobfuscate that does similar work to what sentry does, but that includes logic for some extensions added by dart2js (including lookup of minified names and expansion of inlined stack frames). I have doubts that it will help in this example, but I'm curious how they compare.

@mnordine
Copy link
Contributor Author

Sorry, I don't have it. Will update here if I'm able to get it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants