Closed Bug 1471726 Opened 3 years ago Closed 3 years ago

Incorrect codegen for [array] of jsval by xpidl

Categories

(Core :: XPConnect, enhancement, P2)

enhancement

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
firefox63 --- fixed

People

(Reporter: nika, Assigned: nika)

References

Details

Attachments

(2 files)

It used to be that doing this would cause a runtime error. Now what happens is that xpidl generates the wrong type for the binding (HandleValue*, uint32_t).

We should instead make it use the correct type: (JS::Value*, uint32_t).
Priority: -- → P2
How does the tracing work for JSVal* arguments? Are they rooted wherever calls into the method?
Flags: needinfo?(nika)
Comment on attachment 8990453 [details] [diff] [review]
Part 1: Correct codegen for XPIDL arrays of JSVals

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

r=me assuming there's something actually rooting these arrays that I've forgotten about. :)
Attachment #8990453 - Flags: review?(continuation) → review+
Attachment #8990454 - Flags: review?(continuation) → review+
The rooting logic is here: https://searchfox.org/mozilla-central/rev/fd5c37f1dd9a0d1e327a6c6b4d81ea92f52c4330/js/xpconnect/src/XPCWrappedNative.cpp#1706-1742

I made this change somewhat recently (bug 1457972 part 5). I was originally doing this on the way to supporting nsTArray (which I never got around to doing 'cause I'm pretty busy), so I was adding support for rooting arrays.

In effect, rooting arrays is much easier with this new architecture rather than the old one, as we can determine which array elements to root dynamically when a collection happens, rather than pinning specific memory addresses.

The case where we don't automatically root the array for you is in the C++ -> JS case. If you're calling from C++ into JS you'll have to root the arrays you pass in yourself. I considered doing an extra rooting on your behalf, and I could do that as well, but that isn't how it's currently implemented.
Flags: needinfo?(nika)
Pushed by nika@thelayzells.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c4df8eab6d1c
Part 1: Correct codegen for XPIDL arrays of JSVals, r=mccr8
https://hg.mozilla.org/integration/mozilla-inbound/rev/1907567a85c5
Part 2: Add basic tests for jsval array codegen, r=mccr8
(In reply to :Nika Layzell from comment #5)
> I made this change somewhat recently (bug 1457972 part 5). I was originally
> doing this on the way to supporting nsTArray (which I never got around to
> doing 'cause I'm pretty busy), so I was adding support for rooting arrays.
Ah, thanks. It has been too long since I looked at that patch.

> I considered doing an extra rooting on your
> behalf, and I could do that as well, but that isn't how it's currently
> implemented.

Nah, that's fine. It is standard to require that the caller takes care of rooting things it passes in.
I think this was backed out alongside bug 1461450, so I'll re-land it with the linting fixes soon :-).
I might also just wait until https://bugzilla.mozilla.org/show_bug.cgi?id=1474369 has been reviewed, and encourage people to use Sequence<jsval> over [array] jsval (perhaps even explicitly preventing [array] jsval).
Pushed by nika@thelayzells.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/da79e75b9cb3
Part 1: Correct codegen for XPIDL arrays of JSVals, r=mccr8
https://hg.mozilla.org/integration/mozilla-inbound/rev/40885fbf99c6
Part 2: Add basic tests for jsval array codegen, r=mccr8
Pushed by nika@thelayzells.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/408961783c95
Part 1: Correct codegen for XPIDL arrays of JSVals, r=mccr8
https://hg.mozilla.org/integration/mozilla-inbound/rev/cf2448b2635f
Part 2: Add basic tests for jsval array codegen, r=mccr8
Pushed by nika@thelayzells.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7d6a28d53a8e
Part 1: Correct codegen for XPIDL arrays of JSVals, r=mccr8
https://hg.mozilla.org/integration/mozilla-inbound/rev/4e4d3dae15bf
Part 2: Add basic tests for jsval array codegen, r=mccr8
https://hg.mozilla.org/mozilla-central/rev/7d6a28d53a8e
https://hg.mozilla.org/mozilla-central/rev/4e4d3dae15bf
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Flags: needinfo?(nika)
You need to log in before you can comment on or make changes to this bug.