Closed Bug 1159737 Opened 8 years ago Closed 8 years ago

Stop supporting binary XPCOM components except as part of the application


(Core :: XPCOM, defect)

Not set



Tracking Status
firefox40 --- fixed
relnote-firefox --- 41+


(Reporter: benjamin, Assigned: benjamin)



(Keywords: addon-compat, dev-doc-needed)


(2 files)

It is time to stop supporting binary XPCOM components in extensions. These represent significant compatibility and stability risk to Firefox users. Ideally we'd stop supporting them altogether, but they are still used by the application in a few important cases.

My plan is to stop supporting these components for addon chrome.manifest loads as well as nsIComponentRegistrar.autoRegister.

TEST-UNEXPECTED-FAIL | js/xpconnect/tests/unit/test_attributes.js | xpcshell return code: 0
TypeError: Cc[contractid] is undefined at /builds/slave/test/build/tests/xpcshell/tests/js/xpconnect/tests/unit/test_attributes.js:24
TEST-UNEXPECTED-FAIL | js/xpconnect/tests/unit/test_params.js | xpcshell return code: 0
TypeError: Cc[contractid] is undefined at /builds/slave/test/build/tests/xpcshell/tests/js/xpconnect/tests/unit/test_params.js:22
TEST-UNEXPECTED-FAIL | js/xpconnect/tests/unit/test_returncode.js | xpcshell return code: 0
TypeError: Cc[';1'] is undefined at /builds/slave/test/build/tests/xpcshell/tests/js/xpconnect/tests/unit/test_returncode.js:28
TEST-UNEXPECTED-FAIL | xpcom/tests/unit/test_bug656331.js | xpcshell return code: 0
TEST-UNEXPECTED-FAIL | xpcom/tests/unit/test_bug656331.js | run_test/< - [run_test/< : 36] false == true
TEST-UNEXPECTED-FAIL | xpcom/tests/unit/test_compmgr_warnings.js | xpcshell return code: 0
TEST-UNEXPECTED-FAIL | xpcom/tests/unit/test_compmgr_warnings.js | run_test/< - [run_test/< : 69] false == true 

I think I can fix the xpcom/tests ones myself, by removing or modifying the tests.

I'm less clear on what to do about the xpconnect ones. If I understand them correctly, these are tests to make sure that xpconnect/binary bridging works properly, and so just removing the binary parts of that test would essentially cripple them. bholley, do you have suggestions?
Flags: needinfo?(bobbyholley)
Assignee: nobody → benjamin
Per IRC discussion, this code is testing cross-language argument conversion. The code doesn't need to be in a binary component per se. I'll leave it to you to figure out the best thing to do here.
Flags: needinfo?(bobbyholley)
Linking them into gtest seems like the sensible solution, although I don't know that we have any existing gtests that run JS as part of the test, so testing that part might be a PITA. I guess given that XRE_XPCShellMain lives inside libxul nowadays you could probably hack up a test function that essentially invoked xpcshell on some JS files/strings inside a gtest.
gtest code that touches JSAPI tends to trigger hazard analysis false positives, because comparison functions and other things use some function pointers. (Hopefully I'm talking about the right thing; fitzgen ran into this a bunch and I'm still working on a decent solution for it.)
I wasn't suggesting writing JSAPI directly, I was thinking we could just call into XRE_XPCShellMain to make the test act as if it was calling xpcshell.
Attachment #8600348 - Flags: superreview?
Attachment #8600348 - Flags: review?(nfroyd)
Attachment #8600348 - Flags: superreview? → superreview?(bzbarsky)
Comment on attachment 8600348 [details] [diff] [review]

Attachment #8600348 - Flags: superreview?(bzbarsky) → superreview+
Comment on attachment 8600344 [details] [diff] [review]
Expose a separate xpcshell-only method to register testing components

Review of attachment 8600344 [details] [diff] [review]:

Looks good, thanks.

::: js/xpconnect/src/XPCShellImpl.cpp
@@ +649,5 @@
> +    Rooted<JSObject*> arg1(cx, &args[0].toObject());
> +    nsCOMPtr<nsIFile> file;
> +    if (!XPCConvert::JSObject2NativeInterface(
> +             getter_AddRefs(file), arg1, &NS_GET_IID(nsIFile), nullptr,
> +             nullptr)) {

This is borderline internal-external here, but I'd say we should minimize the direct usage of XPCConvert - please use nsXPConnect::Get()->WrapJS() here.
Attachment #8600344 - Flags: review?(bobbyholley) → review+
Attachment #8600348 - Flags: review?(nfroyd) → review+
Benjamin would a release note for 40 be useful here?
Flags: needinfo?(benjamin)
Yes, I think a relnote would be valuable for the prerelease channels; the audience would be addon developers, not end-user, so I'm not sure whether it makes sense to mention this on the final release-channel notes or not.
Flags: needinfo?(benjamin)
Release Note Request (optional, but appreciated)
[Why is this notable]: Useful for addon developers to know. Note is for Firefox 40. 
[Suggested wording]: Removed support for binary XPCOM components in extensions. The addon SDK "system/child_process" pipe mechanism for native binaries should be used instead. 
[Links (documentation, blog post, etc)]:

OK, here's a draft. Please do improve the wording!
relnote-firefox: --- → ?
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Depends on: 1161616
Depends on: 1162300
Added to the release notes with Liz's wording.
However, I think we need a documentation explaining that this feature has been deprecated (and pointing to the new doc).
Depends on: 1179902
We are going to do that in 41, updating the release notes.
You need to log in before you can comment on or make changes to this bug.