Stop supporting binary XPCOM components except as part of the application

RESOLVED FIXED in Firefox 40

Status

()

RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: benjamin, Assigned: benjamin)

Tracking

(Depends on: 1 bug, {addon-compat, dev-doc-needed})

unspecified
mozilla40
addon-compat, dev-doc-needed
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox40 fixed, relnote-firefox 41+)

Details

Attachments

(2 attachments)

(Assignee)

Description

3 years ago
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.
(Assignee)

Comment 1

3 years ago
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?
Flags: needinfo?(bobbyholley)
(Assignee)

Updated

3 years ago
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.
(Assignee)

Comment 6

3 years ago
Created attachment 8600344 [details] [diff] [review]
Expose a separate xpcshell-only method to register testing components
Attachment #8600344 - Flags: review?(bobbyholley)
(Assignee)

Comment 7

3 years ago
Created attachment 8600348 [details] [diff] [review]
Split up NS_APP_LOCATION and NS_EXTENSION_LOCATION
Attachment #8600348 - Flags: superreview?
Attachment #8600348 - Flags: review?(nfroyd)
(Assignee)

Updated

3 years ago
Attachment #8600348 - Flags: superreview? → superreview?(bzbarsky)
Comment on attachment 8600348 [details] [diff] [review]
Split up NS_APP_LOCATION and NS_EXTENSION_LOCATION

sr=me
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)
(Assignee)

Comment 13

3 years ago
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)]:   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!
relnote-firefox: --- → ?
(In reply to Pulsebot from comment #11)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/312707328997

I filed bug 1161127 for this clobber need.
Blocks: 1161212
https://hg.mozilla.org/mozilla-central/rev/fd4b60f6f4ee
https://hg.mozilla.org/mozilla-central/rev/312707328997
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox40: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Depends on: 1161616

Updated

3 years ago
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).
relnote-firefox: ? → 40+
Keywords: dev-doc-needed

Updated

3 years ago
Depends on: 1179902
We are going to do that in 41, updating the release notes.
relnote-firefox: 40+ → 41+
You need to log in before you can comment on or make changes to this bug.