dart-lang / web

Lightweight browser API bindings built around JS static interop.
https://pub.dev/packages/web
BSD 3-Clause "New" or "Revised" License
128 stars 22 forks source link

Fix incorrect non-null assertion #306

Open mnordine opened 1 week ago

mnordine commented 1 week ago

I believe _target can be null here.

sigmundch commented 1 week ago

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

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

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 commented 1 week ago

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 commented 1 week ago

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 commented 1 week ago

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