Closed Bug 1336086 Opened 5 years ago Closed 5 years ago
don't export mozilla::services::_external
More symbols that we don't need to export.
CC'ing TB people to let them know this is going to break things.
Thanks for the heads-up. Can you be more specific? What is going to break?
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).
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.
[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.
Attachment #8833063 - Flags: review?(rjesup) → review+
We should probably uplift this to 53.
Pushed by firstname.lastname@example.org: https://hg.mozilla.org/integration/mozilla-inbound/rev/b7e4323f4e73 remove mozilla::services::_external_Get*Service; r=bsmedberg,jesup
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+
You need to log in before you can comment on or make changes to this bug.