don't export mozilla::services::_external_Get*Service*

RESOLVED FIXED in Firefox 53

Status

()

RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: froydnj, Assigned: froydnj)

Tracking

Trunk
mozilla54
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox52 unaffected, firefox53+ fixed, firefox54 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

2 years ago
More symbols that we don't need to export.
(Assignee)

Comment 1

2 years ago
CC'ing TB people to let them know this is going to break things.

Comment 2

2 years ago
Thanks for the heads-up. Can you be more specific? What is going to break?
(Assignee)

Comment 3

2 years ago
I don't know what's going to break because I don't know TB specifics.  Bug 714967 covers the addition of these symbols, and it sounds like there was some build config where TB code linked to libxul rather than being linked inside of libxul (?).  That sort of configuration would break with these changes (if it hasn't already broken because of other export removals).
(Assignee)

Comment 4

2 years ago
Created attachment 8833063 [details] [diff] [review]
remove mozilla::services::_external_Get*Service

Removing the exported symbols is straightforward enough.

The only wrinkle is that PeerConnectionCtx.cpp gets compiled with and
without MOZILLA_INTERNAL_API.  When compiling without
MOZILLA_INTERNAL_API, mozilla::services::Get*Service was redirected to
the _external_* symbol variants.  But as the _external_* symbols no
longer exist, PeerConnectionCtx.cpp's code no longer worked.

Fortunately, PeerConnectionCtx.cpp already contains a few #ifdef
MOZILLA_INTERNAL_API blocks to handle internal/external compilation;
fixing this newest issue was just a matter of extending existing blocks
and adding new ones.  The key observation is that we never added any
observers when compiling without MOZILLA_INTERNAL_API, so we can #ifdef
out the removals of observers, as those would have no effect, and simply
skip getting the observer service for the observer additions if we're
compiling without MOZILLA_INTERNAL_API.
Attachment #8833063 - Flags: review?(rjesup)
Attachment #8833063 - Flags: review?(benjamin)
(Assignee)

Updated

2 years ago
Assignee: nobody → nfroyd
(Assignee)

Comment 5

2 years ago
[Tracking Requested - why for this release]:

We removed some symbols from export in 53:

https://groups.google.com/forum/#!msg/mozilla.dev.platform/qB-9gJl9Jgs/O8LhMg1dBwAJ

this bug would remove more of them.  It seems better to have one release where we remove as many as possible all at once, rather than removing some in one release, some in another, and more in another.
status-firefox52: --- → unaffected
status-firefox53: --- → affected
tracking-firefox53: --- → ?

Updated

2 years ago
Attachment #8833063 - Flags: review?(rjesup) → review+

Updated

2 years ago
Attachment #8833063 - Flags: review?(benjamin) → review+

Comment 6

2 years ago
We should probably uplift this to 53.

Comment 7

2 years ago
Pushed by nfroyd@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b7e4323f4e73
remove mozilla::services::_external_Get*Service; r=bsmedberg,jesup

Comment 8

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/b7e4323f4e73
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox54: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
(Assignee)

Comment 9

2 years ago
Comment on attachment 8833063 [details] [diff] [review]
remove mozilla::services::_external_Get*Service

Approval Request Comment
[Feature/Bug causing the regression]: Not applicable.  (comment 5 has some context)
[User impact if declined]: More symbols exported from libxul, which might be used in all sorts of peculiar ways.
[Is this code covered by automated tests?]: No.
[Has the fix been verified in Nightly?]: Yes.  (Me, manually)
[Needs manual test from QE? If yes, steps to reproduce]: No.
[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: No.
[Why is the change risky/not risky?]: We are unexporting symbols which people shouldn't be using in the first place.
[String changes made/needed]: None.
Attachment #8833063 - Flags: approval-mozilla-aurora?
Comment on attachment 8833063 [details] [diff] [review]
remove mozilla::services::_external_Get*Service

Remove symbols from 53. Aurora53+.
Attachment #8833063 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Track 53+ as the symbols are removed from 53.
tracking-firefox53: ? → +

Comment 12

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/3d0d4b016285
status-firefox53: affected → fixed
You need to log in before you can comment on or make changes to this bug.