Nika Layzell [:nika] (ni? for response) Assignee
	Description • 6 years ago

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).

	Andrew McCreight [:mccr8]
	Comment 3 • 6 years ago

How does the tracing work for JSVal* arguments? Are they rooted wherever calls into the method?

	Andrew McCreight [:mccr8]
	Comment 4 • 6 years ago

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. :)

	Nika Layzell [:nika] (ni? for response) Assignee
	Comment 5 • 6 years ago

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.

	Pulsebot
	Comment 6 • 6 years ago

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

	Andrew McCreight [:mccr8]
	Comment 7 • 6 years ago

(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.

	Nika Layzell [:nika] (ni? for response) Assignee
	Comment 8 • 6 years ago

I think this was backed out alongside bug 1461450, so I'll re-land it with the linting fixes soon :-).

	Nika Layzell [:nika] (ni? for response) Assignee
	Comment 9 • 6 years ago

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).

	Pulsebot
	Comment 10 • 6 years ago

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

	Cristian Brindusan [:cbrindusan]
	Comment 11 • 6 years ago

Backed out 15 changesets (bug 1475409, bug 1461450, bug 1474369, bug 1471726) for build bustages on xptcstubs_gcc_x86_unix.cpp:55:1.

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

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=79dbf5b9d8db577bba582a0853eb293d80eed0ba&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&selectedJob=190109107

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=190109107&repo=mozilla-inbound&lineNumber=12152

	Pulsebot
	Comment 12 • 6 years ago

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

	Cosmin Sabou [:CosminS]
	Comment 13 • 6 years ago

Backed out for causing rooting hazards and browser chrome failures

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&filter-searchStr=Linux%20x64%20pgo%20Mochitests%20with%20e10s%20test-linux64-pgo%2Fopt-mochitest-browser-chrome-e10s-1%20M-e10s(bc1)&fromchange=7ce27aa3ce6887c226baa88223c0bf95bc2c3c28&tochange=e4f654755cc5cb80fb0a5a91707e8a2eff425e3e&selectedJob=190915193

https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&fromchange=7ce27aa3ce6887c226baa88223c0bf95bc2c3c28&tochange=e4f654755cc5cb80fb0a5a91707e8a2eff425e3e&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=success&filter-classifiedState=unclassified&selectedJob=190903190&filter-searchStr=Linux%20x64%20debug%20hazard-linux64-haz%2Fdebug%20(H)

Failure log: 
browser chrome failures: https://treeherder.mozilla.org/logviewer.html#?job_id=190915193&repo=mozilla-inbound&lineNumber=3614
rooting hazards: https://treeherder.mozilla.org/logviewer.html#?job_id=190903190&repo=mozilla-inbound&lineNumber=47845

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

	Pulsebot
	Comment 14 • 6 years ago

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

	Dorel Luca [:dluca]
	Comment 15 • 6 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/7d6a28d53a8e
https://hg.mozilla.org/mozilla-central/rev/4e4d3dae15bf