Closed Bug 1336086 Opened 5 years ago Closed 5 years ago

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

Categories

(Core :: XPCOM, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox52 --- unaffected
firefox53 + fixed
firefox54 --- fixed

People

(Reporter: froydnj, Assigned: froydnj)

References

Details

Attachments

(1 file)

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.
Attachment #8833063 - Flags: review?(rjesup)
Attachment #8833063 - Flags: review?(benjamin)
Assignee: nobody → nfroyd
[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+
Attachment #8833063 - Flags: review?(benjamin) → review+
We should probably uplift this to 53.
Pushed by nfroyd@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b7e4323f4e73
remove mozilla::services::_external_Get*Service; r=bsmedberg,jesup
https://hg.mozilla.org/mozilla-central/rev/b7e4323f4e73
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
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.
You need to log in before you can comment on or make changes to this bug.