Closed
Bug 1471726
Opened 6 years ago
Closed 6 years ago
Incorrect codegen for [array] of jsval by xpidl
Categories
(Core :: XPConnect, enhancement, P2)
Core
XPConnect
Tracking
()
RESOLVED
FIXED
mozilla63
Tracking | Status | |
---|---|---|
firefox63 | --- | fixed |
People
(Reporter: nika, Assigned: nika)
References
Details
Attachments
(2 files)
(deleted),
patch
|
mccr8
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
mccr8
:
review+
|
Details | Diff | Splinter Review |
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).
Updated•6 years ago
|
Priority: -- → P2
Assignee | ||
Comment 1•6 years ago
|
||
Attachment #8990453 -
Flags: review?(continuation)
Assignee | ||
Comment 2•6 years ago
|
||
Attachment #8990454 -
Flags: review?(continuation)
Comment 3•6 years ago
|
||
How does the tracing work for JSVal* arguments? Are they rooted wherever calls into the method?
Flags: needinfo?(nika)
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. :)
Attachment #8990453 -
Flags: review?(continuation) → review+
Updated•6 years ago
|
Attachment #8990454 -
Flags: review?(continuation) → review+
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.
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
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.
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 :-).
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).
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
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
Flags: needinfo?(nika)
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
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
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
Comment 15•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/7d6a28d53a8e
https://hg.mozilla.org/mozilla-central/rev/4e4d3dae15bf
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox63:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Assignee | ||
Updated•6 years ago
|
Flags: needinfo?(nika)
You need to log in
before you can comment on or make changes to this bug.
Description
•