web icon indicating copy to clipboard operation
web copied to clipboard

Fix incorrect non-null assertion

Open mnordine opened this issue 1 year ago • 4 comments

I believe _target can be null here.

mnordine avatar Sep 22 '24 16:09 mnordine

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?

sigmundch avatar Sep 23 '24 15:09 sigmundch

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.

mnordine avatar Sep 23 '24 16:09 mnordine

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.

sigmundch avatar Sep 23 '24 16:09 sigmundch

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

mnordine avatar Sep 23 '24 18:09 mnordine

Going to close out this PR. Please reopen when you want to dig in again!

kevmoo avatar Dec 18 '24 02:12 kevmoo