Sitelet https://github.com/nodejs/node/issues/3439
Skip to content

Extended TypedArray reverts to parent class. #3439

Closed
Closed
Extended TypedArray reverts to parent class.#3439
Labels
confirmed-bugIssues and PRs for confirmed bugs.v8 engineIssues and PRs related to the V8 dependency.

Description

@AaronAsAChimp
AaronAsAChimp
opened on Oct 19, 2015

After creating several instances (~135 on my machine) an extended TypedArray reverts to the TypedArray it was extended from.

Gist: https://gist.github.com/AaronAsAChimp/354374a7e1cdc68c4d9f
Output:

[snipped ...]
The method "double" is function
Is it still an ExtendedArray? true
Is it still an Float64Array? true
The method "double" is undefined
Is it still an ExtendedArray? false
Is it still an Float64Array? true
/folder/array-extend.js:15
        throw new Error('Method "double" should not be undefined');
        ^

Error: Method "double" should not be undefined
    at Object.<anonymous> (/folder/array-extend.js:15:9)
    at Module._compile (module.js:435:26)
    at Object.Module._extensions..js (module.js:442:10)
    at Module.load (module.js:356:32)
    at Function.Module._load (module.js:311:12)
    at Function.Module.runMain (module.js:467:10)
    at startup (node.js:134:18)
    at node.js:961:3

This was tested against Node v4.2.1

Activity

  1. Fishrock123
    added
    v8 engineIssues and PRs related to the V8 dependency.
    on Oct 19, 2015

  2. targos commented on Oct 19, 2015

    @targos
    targos
    on Oct 19, 2015
    Member

    I cannot reproduce this in d8 4.5.103.35

  3. skomski commented on Oct 19, 2015

    @skomski
    skomski
    on Oct 19, 2015
    Contributor

    The reason it is not showing in vanilla d8 is the v8 command flag --typed_array_max_size_in_heap=0 that node passes unconditionally. Reproduces with d8 from v8 4.8 master.

    Edit: But the overall issue is in NAMEConstructByLength in v8 (https://github.com/v8/v8/blob/8fafb2916c8ef046b8e841f2fa12b46a8ac8772a/src/js/typedarray.js#L95). It seems that %_TypedArrayInitialize is the culprit.

  4. trevnorris commented on Oct 19, 2015

    @trevnorris
    trevnorris
    on Oct 19, 2015
    Contributor

    Unfortunately said flag is necessary for Node to work. Otherwise Buffer data access from the native side may crash unexpectedly.

  5. skomski commented on Oct 19, 2015

    @skomski
    skomski
    on Oct 19, 2015
    Contributor

    @trevnorris The flag is not important. It only was the reason why it was not reproducible with the given example in vanilla d8. Needed change: new ExtendedArray(9);

    The underlying bug is an flawed interaction between new GlobalArrayBuffer() and a runtime function with multiple args because %_TypedArrayInitialize can be completely empty and it still reproduces.

    function NAMEConstructByLength(obj, length) {
  %_TypedArrayInitialize(obj, ARRAY_ID, new GlobalArrayBuffer(72), 0, 0, true);
}
RUNTIME_FUNCTION(Runtime_TypedArrayInitialize) {
  return isolate->heap()->undefined_value();
}

    With --always-opt it crashes immediately which makes it clear crankshaft is doing some bad optimization.

  6. bnoordhuis commented on Oct 20, 2015

    @bnoordhuis
    bnoordhuis
    on Oct 20, 2015
    Member

    Did someone file a V8 bug yet? If yes, can you post the link?

  7. skomski commented on Oct 20, 2015

    @skomski
    skomski
    on Oct 20, 2015
    Contributor

    @bnoordhuis https://code.google.com/p/v8/issues/detail?id=4503

  8. jasnell commented on Mar 9, 2016

    @jasnell
    jasnell
    on Mar 9, 2016
    Member

    @trevnorris @bnoordhuis ... do we need to keep this one open?

  9. bnoordhuis commented on Mar 9, 2016

    @bnoordhuis
    bnoordhuis
    on Mar 9, 2016
    Member

    It seems to be fixed in V8 4.9 but not in 4.5 and 4.6; node v4 and v5 are still affected. If there is a single bug fix commit, I wasn't able to find it - the delta between 4.6 and 4.9 is pretty big - but it may be one of the patches @littledan landed late last year.

  10. littledan commented on Mar 9, 2016

    @littledan
    littledan
    on Mar 9, 2016

    Actually I'm not sure what fixed this. I've definitely made a bunch of changes to that code. But I didn't change what happens in Crankshaft or C++, and I don't see any changes with respect to TypedArrayInitialize between 4.6 and 4.9. So if the fix was on the JS side, it might be not too hard to backport. /cc @ofrobots

  11. ofrobots
    added
    confirmed-bugIssues and PRs for confirmed bugs.
    on Mar 11, 2016

  12. ofrobots commented on Mar 11, 2016

    @ofrobots
    ofrobots
    on Mar 11, 2016
    Contributor

    This was fixed in V8 4.8.196 by v8/v8@4490ce8. See associated bug: https://bugs.chromium.org/p/v8/issues/detail?id=4419.

    I fear that the change is a bit too complex to be back-ported to V8 4.5 and 4.6.

  13. jasnell commented on Apr 2, 2016

    @jasnell
    jasnell
    on Apr 2, 2016
    Member

    this appears to be fixed in master and may be too complex to backport, I'm not sure if there's anything else than can be done with this one relative to v4 and v5. Recommend closing.

  14. jasnell
    closed this as completedon Apr 22, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    confirmed-bugIssues and PRs for confirmed bugs.v8 engineIssues and PRs related to the V8 dependency.

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions