Remove support for binary-component manifest instruction

RESOLVED FIXED in Firefox 53

Status

()

defect
P2
normal
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: benjamin, Assigned: benjamin)

Tracking

unspecified
mozilla53
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox53 fixed)

Details

Attachments

(3 attachments)

Assignee

Description

3 years ago
We no longer want to allow binary components at all. Currently we disallow them in extensions but still allow them in app code. This will remove support completely.
Comment on attachment 8807161 [details]
Bug 1314955 part A - Move the binary xpconnect testing components into libxul so that we can remove support for external binary components.

https://reviewboard.mozilla.org/r/90424/#review90174
Attachment #8807161 - Flags: review?(mrbkap) → review+
Just to be clear, this will heavily impact Thunderbird, which still relies on (some) binary extensions, yes?  Is it feasible to delay this until 53, so Thunderbird can have an ESR cycle to deal with fallout, or is there immediate work that requires binary components to go away Right Now?

CC'ing some thunderbird people.

(Yes, yes, I know we're not *supposed* to care about Thunderbird, but this is a bigger deal than typical refactorings.)
Flags: needinfo?(benjamin)
Assignee

Comment 6

3 years ago
According to the dev.platform thread about this, "we [thunderbird devs] have agreed for some months now that Thunderbird 52 will no longer support binary extensions." https://groups.google.com/d/msg/mozilla.dev.platform/qB-9gJl9Jgs/nkb29idgBwAJ

They "would like" to wait until 53 but can live without, and in terms of hardening Firefox I really want to have this take effect for ESR52 in Firefox, so I'd like to land this now.
Flags: needinfo?(benjamin)
Comment on attachment 8807162 [details]
Bug 1314955 part B - Remove the tests for binary-component which is no longer supported.

https://reviewboard.mozilla.org/r/90426/#review90190
Attachment #8807162 - Flags: review?(nfroyd) → review+
Comment on attachment 8807163 [details]
Bug 1314955 part C - Make the `binary-component` manifest instruction obsolete: it will continue to be parsed and will report a deprecation notice to the browser console.

https://reviewboard.mozilla.org/r/90428/#review90194

You mentioned that you intend for `binary-component` bits in the manifest to print an error to the console; did you intend for that error message to come from `ManifestBinaryComponent`, or the generic parsing code that complains about unknown directives?  r-'ing because I'd like clarification on that point.

(I think it would be equally straightforward to hide the `binary-component` directive behind a flag, for which the Thunderbird developers could implement appropriate configury.  But meh, I suppose there'd be a fair amount of stuff to gate off that flag...)

::: xpcom/components/nsComponentManager.cpp:626
(Diff revision 1)
>  nsComponentManagerImpl::ManifestBinaryComponent(ManifestProcessingContext& aCx,
>                                                  int aLineNo,
>                                                  char* const* aArgv)

AFAICT, this function is dead code; the only reference to it was the `kParsingTable` entry in ManifestParser.cpp, which is also removed by this patch.
Attachment #8807163 - Flags: review?(nfroyd) → review-
(In reply to Benjamin Smedberg [:bsmedberg] from comment #6)
> According to the dev.platform thread about this, "we [thunderbird devs] have
> agreed for some months now that Thunderbird 52 will no longer support binary
> extensions."
> https://groups.google.com/d/msg/mozilla.dev.platform/qB-9gJl9Jgs/nkb29idgBwAJ
> 
> They "would like" to wait until 53 but can live without, and in terms of
> hardening Firefox I really want to have this take effect for ESR52 in
> Firefox, so I'd like to land this now.

Thank you for the pointer!  I had not fully internalized that mailing list thread.
Comment hidden (mozreview-request)
Assignee

Updated

3 years ago
Attachment #8807161 - Attachment is obsolete: true
Assignee

Updated

3 years ago
Attachment #8807162 - Attachment is obsolete: true
Comment on attachment 8807163 [details]
Bug 1314955 part C - Make the `binary-component` manifest instruction obsolete: it will continue to be parsed and will report a deprecation notice to the browser console.

https://reviewboard.mozilla.org/r/90428/#review91650

Thanks!
Attachment #8807163 - Flags: review?(nfroyd) → review+
Assignee

Updated

3 years ago
Attachment #8807161 - Attachment is obsolete: false
Assignee

Updated

3 years ago
Attachment #8807162 - Attachment is obsolete: false

Comment 12

3 years ago
You didn't land this on Gecko52 after all despite comment #6(?).
Assignee

Comment 13

3 years ago
No I ran out of time and there are tests still orange from my tree.

Comment 14

3 years ago
Pushed by bsmedberg@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c78f97c0b771
part A - Move the binary xpconnect testing components into libxul so that we can remove support for external binary components. Ideally we'd only compile these into the xul-gtest version, but currently we can't run xpcshell tests against that version, so I'm going to put them into the release xul, r=mrbkap
https://hg.mozilla.org/integration/mozilla-inbound/rev/a99c6ce96b6f
part B - Remove the tests for binary-component which is no longer supported. r=froydnj
https://hg.mozilla.org/integration/mozilla-inbound/rev/b99edf918b96
part C - Make the `binary-component` manifest instruction obsolete: it will continue to be parsed and will report a deprecation notice to the browser console. r=froydnj
Sorry had to back out these for Android XPCShell failures, i.e., https://treeherder.mozilla.org/logviewer.html#?job_id=39253398&repo=mozilla-inbound#L1603
Flags: needinfo?(benjamin)

Comment 16

3 years ago
Backout by ihsiao@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/024dd64e56c9
Backed out changeset b99edf918b96 for Android XPCShell failures
https://hg.mozilla.org/integration/mozilla-inbound/rev/ee36b7c4f4be
Backed out changeset a99c6ce96b6f 
https://hg.mozilla.org/integration/mozilla-inbound/rev/8e8aa918d2f5
Backed out changeset c78f97c0b771

Comment 17

3 years ago
Pushed by bsmedberg@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/82f8e8d7eb68
part A - Move the binary xpconnect testing components into libxul so that we can remove support for external binary components. Ideally we'd only compile these into the xul-gtest version, but currently we can't run xpcshell tests against that version, so I'm going to put them into the release xul, r=mrbkap
https://hg.mozilla.org/integration/mozilla-inbound/rev/c1554712210f
part B - Remove the tests for binary-component which is no longer supported. r=froydnj
https://hg.mozilla.org/integration/mozilla-inbound/rev/25f407284342
part C - Make the `binary-component` manifest instruction obsolete: it will continue to be parsed and will report a deprecation notice to the browser console. r=froydnj
Depends on: 1318195
See Also: → 1318735
Assignee

Updated

3 years ago
Flags: needinfo?(benjamin)
Blocks: 1330947
Blocks: 1335163
You need to log in before you can comment on or make changes to this bug.