[dart compile js] getUint16 on UnmodifiableByteDataView inside class, results in "Uncaught TypeError: this.a.D is not a function""

[julemand101]

The following problem have been reproduced in:

Dart SDK version: 2.19.0-146.0.dev (dev) (Fri Aug 26 18:55:25 2022 -0700) on "windows_x64"

But NOT being able to be reproduced in:

Dart SDK version: 2.18.0 (stable) (Fri Aug 26 10:22:54 2022 +0000) on "windows_x64"

If running the following program inside a new Dart web project created with: dart create -t web and build with webdev build:

import 'dart:html';
import 'dart:typed_data';

void main() {
  final a = A(ByteData(10)..setUint16(0, 42));
  querySelector('#output')?.text = 'Number in ByteData: ${a.getUint16()}';
}

class A {
  final UnmodifiableByteDataView bytes;
  A(ByteData bytes) : bytes = UnmodifiableByteDataView(bytes);

  int getUint16() {
    return bytes.getUint16(0);
  }
}

It results in the following error:

Uncaught TypeError: this.a.D is not a function
    at X.t (main.dart.js:1228:20)
    at cs (main.dart.js:1000:65)
    at main.dart.js:1411:6
    at main.dart.js:1405:55
    at dartProgram (main.dart.js:1408:84)
    at main.dart.js:1411:15

It works if testing with webdev serve.

[rakudrama]

dart:html is not required. The following fails on stand-alone JavaScript:

import 'dart:typed_data';

void main() {
  final a = A(ByteData(10)..setUint16(0, 42));
  print('Number in ByteData: ${a.getUint16()}');
}

class A {
  final UnmodifiableByteDataView bytes;
  A(ByteData bytes) : bytes = UnmodifiableByteDataView(bytes);

  int getUint16() {
    return bytes.getUint16(0);
  }
}

z.js:2683: TypeError: this.bytes.getUint16$1 is not a function
      return this.bytes.getUint16$1(0, 0);
                        ^
TypeError: this.bytes.getUint16$1 is not a function
    at A.getUint16$0 (z.js:2683:25)
    at main (z.js:2242:54)
    at z.js:2937:7
    at z.js:2916:7
    at dartProgram (z.js:2931:5)
    at z.js:2939:3

The code in main is strange - it appears that dart2js believes that _UnmodifiableByteDataView.getUint16 is never called (it is missing (A), causing the reported error). A knock-on effect is that because it is believed to be unused, the field _UnmodifiableByteDataView._data is elided. This results in the native ByteData (DataView) not being passed to the _UnmodifiableByteDataView constructor (B).

    main() {
      B.NativeByteData_methods._setUint16$3(new DataView(new ArrayBuffer(10)), 0, 42, false);
      A.S(new A.A(new A._UnmodifiableByteDataView()).getUint16$0(0)); // (B)
    },
...
  A.A.prototype = {
    getUint16$0(_) {
      return this.bytes.getUint16$1(0, 0);
    }
  };
...
  A._UnmodifiableByteDataView.prototype = {$isByteData: 1}; // (A)

The culprit seems to be c1e67ac84f7 [vm] Recognize unmodifiable typed data views which refactored the views. I'm not sure what it is about this change that causes bad codegen.

• [ ] Create regression test

There are some other problems I see in the change, independent tickling a dart2js bug.

• The web version of UnmodifiableByteDataView allows views to be stacked (views of views). This is a problem since using a view will cause poor performance as the delegated calls with be fully dispatched. We must arrange that _UnmodifiableByteDataView._data has type NativeByteData to get acceptable performance (and similarly for typed lists).

• The use of mixins is slower on the web compilers than the VM since the VM clones the mixin code and can then see the specialized cloned types and compile each cloned method differently. In the case of _UnmodifiableListMixin, the code duplication results in code with a lot of simple types. The web compilers do not clone the mixin code (for size reasons) so do not get the same benefit of specialization that the VM gets. This makes the use of mixins in this pattern a worse choice for performance-critical in the web libraries.

/cc @rmacnak-google /cc @sigmundch - for triage

[sigmundch]

@rakudrama - I believe the unsoundness comes form a bug in the sources that were undetected by the CFE.

Taking a closer look at the patch, it appears that every redirecting factory in the patch file is forwarding to a class that doesn't implement the original class. This is normally checked by the CFE/analyzer when working on regular files, so I wonder if this is a specific issue with patch files.

/cc @johnniwinther - any thought on why this didn't fail to compile when creating the platform files?

So in the example above, adding implements UnmodifiableByteDataView to _UnmodifiableByteDataView seems to address the soundness issue.

[sigmundch]

Raising priority of this issue - I worry this unsoundness may surface in many ways. While I understand there are performance concerns with https://github.com/dart-lang/sdk/commit/c1e67ac84f7d862aee14bbb9ce09346cef21dfa0, I think we should treat that separately from the unsoundness issue, and land an update to the patch files to add the missing implements clauses on those ~15 types.

[sigmundch]

I'm creating https://dart-review.googlesource.com/c/sdk/+/264601 to address the runtime error introduced above. This doesn't address the performance regressions and cost highlighted by @rakudrama above. To improve our tracking, I will close this issue when that change lands, so I'd recommend we create a new issue to track the perf aspect separately.

[sigmundch]

The fix landed in 770c9d77c59fec5c538387901d9884f0fd45fda5

[johnniwinther]

@sigmundch We do have some missing checks for patched redirecting factories. Working on a fix.