1159737 - Stop supporting binary XPCOM components except as part of the application

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Benjamin Smedberg]

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.

Comment 1

[Benjamin Smedberg]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=cef40097811f

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['@mozilla.org/js/xpc/test/native/ReturnCodeParent;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?

Comment 2

[Bobby Holley (:bholley)]

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.

Comment 3

[(not currently active) Ted Mielczarek]

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.

Comment 4

[Steve Fink [:sfink] [:s:]]

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

Comment 5

[(not currently active) Ted Mielczarek]

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.

Comment 6

[Benjamin Smedberg]

Attachment: Expose a separate xpcshell-only method to register testing components

Comment 7

[Benjamin Smedberg]

Attachment: Split up NS_APP_LOCATION and NS_EXTENSION_LOCATION

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8600348 [details] [diff] [review]
Split up NS_APP_LOCATION and NS_EXTENSION_LOCATION

sr=me

Comment 9

[Bobby Holley (:bholley)]

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.

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fd4b60f6f4ee

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/312707328997

Comment 12

[Liz Henry (:lizzard) (relman/hg->git project)]

Benjamin would a release note for 40 be useful here?

Comment 13

[Benjamin Smedberg]

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.

Comment 14

[Liz Henry (:lizzard) (relman/hg->git project)]

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)]:   https://developer.mozilla.org/en-US/Add-ons/SDK/Low-Level_APIs/system_child_process

OK, here's a draft. Please do improve the wording!

Comment 15

[Andrew McCreight [:mccr8]]

(In reply to Pulsebot from comment #11)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/312707328997

I filed bug 1161127 for this clobber need.

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/fd4b60f6f4ee
https://hg.mozilla.org/mozilla-central/rev/312707328997

Comment 17

[Sylvestre Ledru [:Sylvestre]]

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

Comment 18

[Sylvestre Ledru [:Sylvestre]]

We are going to do that in 41, updating the release notes.