1182124 - Remove InternalHandle

Status: RESOLVED FIXED

(Core :: JavaScript: GC, defect)

Description

[Terrence Cole [:terrence]]

Attachment: remove_internalhandle-v0.diff

The last remaining use is in SIMD.cpp of all places and it doesn't appear to do anything at all, at least not with respect to the GC.

Comment 1

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8631643 [details] [diff] [review]
remove_internalhandle-v0.diff

Review of attachment 8631643 [details] [diff] [review]:
-----------------------------------------------------------------

Less code \o/

There might be an issue with the change though, as the GC can be active here, see comment below. The issue is trivial enough that no other review will be needed though.

::: js/src/builtin/SIMD.cpp
@@ +397,2 @@
>      for (unsigned i = 0; i < T::lanes; i++) {
> +        if (!T::toType(cx, args.get(i), &mem[i]))

ISTR jonco and i decided to use this because:
- T::toType can GC, as it calls e.g. ToNumber for T := Float32x4 (https://dxr.mozilla.org/mozilla-central/source/js/src/builtin/SIMD.h#276-282)
- as "result" can be moved because of a GC, the pointer to its typed memory can be dangling: for SIMD objects, typed memory is always attached as inlined storage of the typed object.

So I think the situation is not fixed here, as mem can still move. One way to test is to run the jit-tests and js-tests (you can restrict to SIMD tests, e.g. jit-tests.py ./dist/bin/js SIMD) with JS_GC_ZEAL=14 with your change. As this is how we've discovered the issue, if both test suites pass, we're fine. 

Instead, maybe we can just inline the definition of |mem| at the place where it's used? Less pretty, but clears the headache.

Elem tmp;
for (unsigned i = 0; i < T::lanes; i++) {
    if (!T::toType(cx, args.get(i), &tmp)
        return false;
    reinterpret_cast<Elem*>(result->typedMem())[i] = tmp;
}

Comment 2

[Terrence Cole [:terrence]]

Makes sense. Thanks for the context!

Comment 3

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/73d2c854d770

Comment 4

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/73d2c854d770

Comment 5

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d86fa46b62b8

Comment 6

[Benjamin Bouvier [:bbouvier] (inactive)]

NI terrence cause he backed out the patch, to make sure this bug doesn't get forgotten.

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/34debf553302

Comment 8

[Terrence Cole [:terrence]]

No change after backout. The regression appears to be noise.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/34debf553302