servo / rust-mozjs

DEPRECATED - moved to servo/mozjs instead.
Mozilla Public License 2.0
293 stars 122 forks source link

conversions: Implement FromJSValConvertible for Vec<T> #226

Closed emilio closed 8 years ago

emilio commented 8 years ago

This will allow us to finally implement WebIDL sequence arguments (servo/servo#544).

Review on Reviewable

Ms2ger commented 8 years ago

This should implement the spec for "An ECMAScript value V is converted to an IDL sequence value" at https://heycam.github.io/webidl/#es-sequence. SpiderMonkey has the ForOfIterator struct to help with that. You'll need to initialize it to match

    explicit ForOfIterator(JSContext* cx) : cx_(cx), iterator(cx_), index(NOT_ARRAY) { }
    static const uint32_t NOT_ARRAY = UINT32_MAX;

See https://mxr.mozilla.org/mozilla-central/source/dom/bindings/Codegen.py?rev=151ce2b0e3f6#4514 for how Gecko calls it.

emilio commented 8 years ago

@Ms2ger

Added a minimal test case, that works if we remove the Rooted<T> destructor.

So... it seems that the hidden state Drop adds to Rooted<T> is what is corrupting our stack when interacting with SM, causing the trap an the latter assertion failure.

I don't think we can remove the Rooted destructor, so I don't think there's any way to circumvent this, at least for now.

Would it be ok to leave the first commit for now to land https://github.com/servo/servo/pull/9056 (so there's basic support for sequences and people can work on that kind of stuff), and leave this until we have a way to solve it using ForOfIterator?

If that sounds good, I'll make sure the corresponding issues are filled (both in here and in servo/servo).

I'd want to hear @jdm's opinion too.

emilio commented 8 years ago

Also, pinging @michaelwu in case he knows any way to circumvent this issue.

Ms2ger commented 8 years ago

Ah, crap. @tschneidereit, do you have thoughts on this?

Does the drop flag end up at the end or the start of the Rooted? If it's at the end, maybe a quick hack would be to move the fields of ForOfIterator around.

Ms2ger commented 8 years ago

Alternatively, can't we opt out of the drop flag?

Manishearth commented 8 years ago

#[unsafe_no_drop_flag], however it's then up to you to maintain the drop flag invariants yourself

emilio commented 8 years ago

I did a quick try moving the iterator to the end of ForOfIterator (both here and in mozjs) and no luck so far: it doesn't crash with SIGTRAP, but still crashes with our assertion in Drop.

Also tested with #[unsafe_no_drop_flag], but it seems even in "simple" cases like that test we don't maintain the drop invariants and we drop many times, causing a segfault.

nox commented 8 years ago

227 should fix that.

emilio commented 8 years ago

I just pushed a patch based on #227, but testing against the drop bitfield rust uses instead (see comment).

It works, but feels too hacky though. And we must touch the jsapi_* files manually too...

bors-servo commented 8 years ago

:umbrella: The latest upstream changes (presumably #229) made this pull request unmergeable. Please resolve the merge conflicts.

bors-servo commented 8 years ago

:umbrella: The latest upstream changes (presumably #230) made this pull request unmergeable. Please resolve the merge conflicts.

Ms2ger commented 8 years ago

229