Closed Bug 1331205 Opened 3 years ago Closed 3 years ago

move windows ipc locking code over to Vista+ synchronization primitives

Categories

(Core :: IPC, defect)

All
Windows
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox53 --- affected
firefox54 --- fixed

People

(Reporter: froydnj, Assigned: froydnj)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

"Upstream" IPC code has already done this; the backport is pretty straightforward, but to make it friendly, we need to wait until we can unconditionally reference Vista+ symbols in Gecko code.
This update is mostly because the upstream implementation is now free of
the gnarly, XP-required implementation of condition variables and
updating both the posix and windows implementations at the same time
seemed easier.
Attachment #8828874 - Flags: review?(jld)
Comment on attachment 8828874 [details] [diff] [review]
update ipc/'s copy of locks and condition variables from Chromium upstream

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

Looks good, but I'm a little confused by the remnants of upstream debug assertions we seem to have decided not to use.

::: ipc/chromium/src/base/lock.cc
@@ +3,5 @@
>  // found in the LICENSE file.
>  
> +// This file is used for debugging assertion support.  The Lock class
> +// is functionally a wrapper around the LockImpl class, so the only
> +// real intelligence in the class is in the debugging logic.

As for `lock.h`.  And do we even need this .cc file if we're not including the debugging bits?

::: ipc/chromium/src/base/lock.h
@@ +12,4 @@
>  
> +// A convenient wrapper for an OS specific critical section.  The only real
> +// intelligence in this class is in debug mode for the support for the
> +// AssertAcquired() method.

We don't seem to have the debug version of any of this?
Attachment #8828874 - Flags: review?(jld) → review+
(In reply to Jed Davis [:jld] {⏰UTC-7} from comment #2)
> ::: ipc/chromium/src/base/lock.cc
> @@ +3,5 @@
> >  // found in the LICENSE file.
> >  
> > +// This file is used for debugging assertion support.  The Lock class
> > +// is functionally a wrapper around the LockImpl class, so the only
> > +// real intelligence in the class is in the debugging logic.
> 
> As for `lock.h`.  And do we even need this .cc file if we're not including
> the debugging bits?

That's true.  I can remove this.

> ::: ipc/chromium/src/base/lock.h
> @@ +12,4 @@
> >  
> > +// A convenient wrapper for an OS specific critical section.  The only real
> > +// intelligence in this class is in debug mode for the support for the
> > +// AssertAcquired() method.
> 
> We don't seem to have the debug version of any of this?

The debugging code for locks here is rather gnarly:

https://cs.chromium.org/chromium/src/base/synchronization/lock.h?type=cs&sq=package:chromium

It seemed simpler to not try and import all that, especially when I would like to replace these with XPCOM's mutexes et al at some point.  Sound good?
Flags: needinfo?(jld)
Sounds good.
Flags: needinfo?(jld)
Blocks: xp-eol
No longer depends on: xp-eol
Summary: move windows ipc locking code over to Vista+ synchronization primitves → move windows ipc locking code over to Vista+ synchronization primitives
Pushed by nfroyd@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a8c76ba40a09
update ipc/'s copy of locks and condition variables from Chromium upstream; r=jld
https://hg.mozilla.org/mozilla-central/rev/a8c76ba40a09
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in before you can comment on or make changes to this bug.