Status

()

enhancement
RESOLVED FIXED
9 months ago
8 months ago

People

(Reporter: froydnj, Assigned: froydnj)

Tracking

(Blocks 1 bug)

unspecified
mozilla64
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox64 fixed)

Details

Attachments

(10 attachments)

Assignee

Description

9 months ago
For the AArch64 Windows port, I wrote some dodgy code to use the x86 code from
MSVC for chromium atomics.  As you might imagine, x86(-64) and AArch64 have
completely dissimilar memory models, so while my changes might have compiled,
they were most likely semantically incorrect.  Rather than trying to puzzle out
what the exact AArch64 instruction sequences are, and inflicting the same on
some poor reviewer, let's just blow up the need for the chromium atomics in the
first place.
Assignee

Comment 1

9 months ago
Their uses are easily replaced with simpler classes.
Attachment #9013810 - Flags: review?(jld)
Assignee

Comment 2

9 months ago
C++11 provides guaranteed thread-safe static initialization, so we can
use that instead of ipc's baroque Singleton class.
Attachment #9013811 - Flags: review?(rjesup)
Assignee

Comment 3

9 months ago
C++11 provides guaranteed thread-safe static initialization, so we can
use that instead of ipc's baroque Singleton class.
Attachment #9013812 - Flags: review?(jld)
Assignee

Comment 4

9 months ago
C++11 provides guaranteed thread-safe static initialization, so we can
use that instead of ipc's baroque Singleton class.
Attachment #9013813 - Flags: review?(choller)
Assignee

Comment 5

9 months ago
C++11 provides guaranteed thread-safe static initialization, so we can
use that instead of ipc's baroque Singleton class.
Attachment #9013814 - Flags: review?(jld)
Assignee

Comment 6

9 months ago
When we remove singleton.h, some webrtc code will be unhappy, because of
cargo-culted definitions from singleton.h or included headers.  Let's
fix those first
Attachment #9013815 - Flags: review?(rjesup)
Assignee

Comment 7

9 months ago
C++11 provides guaranteed thread-safe static initialization, so we can
use that instead of ipc's baroque Singleton class.
Attachment #9013816 - Flags: review?(rjesup)
Assignee

Comment 8

9 months ago
These headers are no longer used.

Try is telling me there may be some small additional fixes required in webrtc code.
Attachment #9013817 - Flags: review?(jld)
Assignee

Comment 9

9 months ago
The only consumer of this code was Singleton, which we previously
removed, and everything that this code accomplished can be done more
simply and more foolproof-y by standard constructs these days.
Attachment #9013818 - Flags: review?(jld)
Assignee

Updated

9 months ago
Duplicate of this bug: 1250996
Comment on attachment 9013810 [details] [diff] [review]
remove ipc/'s copies of *AtomicSequenceNum classes

Review of attachment 9013810 [details] [diff] [review]:
-----------------------------------------------------------------

::: ipc/chromium/src/chrome/common/ipc_channel.cc
@@ +17,4 @@
>  namespace {
>  
>  // Global atomic used to guarantee channel IDs are unique.
> +mozilla::Atomic<int> g_last_id;

Should this be Atomic<unsigned>, given that it's printf'ed as %u?
Attachment #9013810 - Flags: review?(jld) → review+
Attachment #9013812 - Flags: review?(jld) → review+
Attachment #9013814 - Flags: review?(jld) → review+
Attachment #9013817 - Flags: review?(jld) → review+
Attachment #9013818 - Flags: review?(jld) → review+
Thanks for getting rid of this code.
Attachment #9013813 - Flags: review?(choller) → review+
Attachment #9013811 - Flags: review?(rjesup) → review+
Assignee

Comment 13

9 months ago
jesup indicated in an IRC conversation that touching imported WebRTC code isn't really desirable.  In light of that, I think what we want to do is:

1) Keep the C++11 statics changes for our own code, since IMHO it's clearer.
2) Redo singleton.h using C++11 atomics, which should be relatively straightforward.

It's *possible* that step 2 will, since it removes the need for Chromium atomics, still require touching WebRTC headers.  Would that be acceptable if it happened?

Jed, WDYT about the above plan?  Are you comfortable reviewing the patch for part 2?
Flags: needinfo?(rjesup)
Flags: needinfo?(jld)
I could review that patch, but that Singleton dependency isn't from upstream WebRTC; it's something we changed locally in bug 1219339.


WebRTC has its own renamed import of the Chromium base library, so generally they shouldn't need to borrow our Chromium bits like that unless it's our change.  Also, the upstream repo doesn't have static_instance.h at all anymore — they removed it a year ago in https://webrtc.googlesource.com/src/+/779d3657ef12b65eb55455c6f415b35522c07c97 — so we might not even need to merge it the next time it's updated.
Flags: needinfo?(jld)
Comment on attachment 9013816 [details] [diff] [review]
use C++11 statics in static_instance.h

Review of attachment 9013816 [details] [diff] [review]:
-----------------------------------------------------------------

r+; removed from upstream so we don't have to carry this
Attachment #9013816 - Flags: review?(rjesup) → review+
Comment on attachment 9013815 [details] [diff] [review]
fix webrtc header cargo culting

Review of attachment 9013815 [details] [diff] [review]:
-----------------------------------------------------------------

already fixed in upstream, r+
Attachment #9013815 - Flags: review?(rjesup) → review+
Assignee

Comment 17

9 months ago
Needed this to fix Windows builds.  upstream seems to have already done this:

https://webrtc.googlesource.com/src/+/master/modules/video_capture/windows/device_info_ds.h#14
Attachment #9014573 - Flags: review?(rjesup)
Attachment #9014573 - Flags: review?(rjesup) → review+

Comment 18

8 months ago
Pushed by nfroyd@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2296a2feb192
remove ipc/'s copies of *AtomicSequenceNum classes; r=jld
https://hg.mozilla.org/integration/mozilla-inbound/rev/cdd41eaece0f
use C++11 statics in CamerasChild; r=jesup
https://hg.mozilla.org/integration/mozilla-inbound/rev/6fc7db6d7326
use C++11 statics in ipc_channel_posix.cc; r=jld
https://hg.mozilla.org/integration/mozilla-inbound/rev/d95dd4c9cdca
use C++11 statics for Faulty instance; r=decoder
https://hg.mozilla.org/integration/mozilla-inbound/rev/06a1c7fa3ba9
use C++11 statics in time_win.cc; r=jld
https://hg.mozilla.org/integration/mozilla-inbound/rev/1fe17ae969d6
fix webrtc header cargo culting; r=jesup
https://hg.mozilla.org/integration/mozilla-inbound/rev/1815bfcf7984
use C++11 statics in static_instance.h; r=jesup
https://hg.mozilla.org/integration/mozilla-inbound/rev/a2a04672f049
fix windows includes of singleton.h; r=jesup
https://hg.mozilla.org/integration/mozilla-inbound/rev/323979f32e2d
remove singleton headers from ipc/; r=jld
https://hg.mozilla.org/integration/mozilla-inbound/rev/36356f99018a
remove chromium atomics code; r=jld
Assignee

Comment 20

8 months ago
We've landed this, I guess ni? on jesup is silly now.
Flags: needinfo?(rjesup)
You need to log in before you can comment on or make changes to this bug.