Make mozIStorageConnection safe against pointer ownership misuses
Categories
(Toolkit :: Storage, task, P3)
Tracking
()
People
(Reporter: sg, Unassigned)
References
(Depends on 1 open bug)
Details
Attachments
(2 files)
In particular, replace raw pointer parameters that need to be acquired strong references from into smart pointers, such as in createFunction
.
Reporter | ||
Comment 1•4 years ago
|
||
Updated•4 years ago
|
Updated•4 years ago
|
Reporter | ||
Comment 2•4 years ago
|
||
Depends on D57089
Pushed by sgiesecke@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/a1e723439116 Part 1: Change mozIStorageAsyncConnection::CreateFunction to accept a nsCOMPtr rather than a raw pointer. r=asuth,mak https://hg.mozilla.org/integration/autoland/rev/05dbbf7c379f Part 2: Rename CreateFunction/RemoveFunction to RegisterFunction/UnregisterFunction. r=mak
Comment 4•4 years ago
|
||
Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&revision=05dbbf7c379f1d75d9cb80358e5add425588d352&selectedJob=291848144
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=291848144&repo=autoland
https://treeherder.mozilla.org/logviewer.html#?job_id=291846560&repo=autoland
https://treeherder.mozilla.org/logviewer.html#?job_id=291848143&repo=autoland
https://treeherder.mozilla.org/logviewer.html#?job_id=291848097&repo=autoland
Backout link: https://hg.mozilla.org/integration/autoland/rev/d6f1cc3bcb5124b27aa9ee4f937cace0c005aee8
There are multiple types of failures that has started with the above push please take a look over all of them. Thank you
Reporter | ||
Comment 5•4 years ago
|
||
When I was working on Part 1, i.e. https://phabricator.services.mozilla.com/D57089, I wasn't aware of the JS binding at all.
:nika, can you make a suggestion how to deal with this? Is it possible to remove the use of raw pointers in the signatures (and e.g. replace them by either already_AddRefed
or nsCOMPtr
) in a way that still makes this usable from JS? Is the JS binding of XPIDL functions still fully supported or is this deprecated (I thought that at least the latter was true, and actually thought that this isn't working anymore at all, but I may have misunderstood something). In case it's deprecated, another option might be to migrate the JS tests to C++ first.
Comment 6•4 years ago
|
||
Is this about register/unregisterFunction?
We should just make them non scriptable, see bug 1392238
They are broken for js consumers anyway, because you can't create a function that will properly run on the expected thread.
Comment 7•4 years ago
|
||
There is no way to remove the use of raw pointers from signatures of script-exposed XPCOM methods. This use of raw pointers is a core part of how XPCOM and XPConnect work. If you want to start using smart pointers within parameter lists, you'll need to stop using XPConnect to bind objects into JS.
If you end up using a non-scriptable interface, as :mak suggests, you'll probably want to stop using IDL entirely, and instead write your code using raw C++ header files. We want to discourage the use of XPIDL for non-scriptable interfaces.
Comment 8•4 years ago
|
||
(Drive-by comment: mozIStorageConnection
needs to remain scriptable, since we have lots of JS code that needs to get at the Places connection, but it's possible to mark RegisterFunction
and UnregisterFunction
as [noscript]
, and have them take a native
parameter like we do here).
Comment 9•4 years ago
|
||
There are some r+ patches which didn't land and no activity in this bug for 2 weeks.
:sg, could you have a look please?
For more information, please visit auto_nag documentation.
Reporter | ||
Comment 10•4 years ago
|
||
(In reply to Release mgmt bot [:sylvestre / :calixte / :marco for bugbug] from comment #9)
There are some r+ patches which didn't land and no activity in this bug for 2 weeks.
:sg, could you have a look please?
For more information, please visit auto_nag documentation.
It turned out that this requires more work before landing: The tests for RegisterFunction
/UnregisterFunction
need to be converted to C++ tests, and mozIStorageFunction
should be de-COM-taminated (see bug 1392238).
Reporter | ||
Updated•4 years ago
|
Reporter | ||
Updated•3 years ago
|
Updated•2 years ago
|
Description
•