770535 - Remove XPConnect locks

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Bobby Holley (:bholley)]

I split this off into its own bug.

Comment 1

[Bobby Holley (:bholley)]

Pushed to try:
https://tbpl.mozilla.org/?tree=Try&rev=424447906280

Comment 2

[Andrew McCreight [:mccr8] (PTO until 12-2)]

https://tbpl.mozilla.org/?tree=Try&rev=8aa8e67defd5

Comment 3

[Andrew McCreight [:mccr8] (PTO until 12-2)]

Attachment: part 1 - remove the locks

Comment 4

[Andrew McCreight [:mccr8] (PTO until 12-2)]

Attachment: Replace ThreadRunningGC with GCIsRunning

Not strictly related, but a simple cleanup related to making it single threaded.

Comment 5

[Andrew McCreight [:mccr8] (PTO until 12-2)]

Attachment: part 3: Fold mMainThreadWrappedNativeProtoMap into mWrappedNativeProtoMap

These two maps are identical with the removal of locks, so fold them together.

Comment 6

[Andrew McCreight [:mccr8] (PTO until 12-2)]

Comment on attachment 8337422 [details] [diff] [review]
part 3: Fold mMainThreadWrappedNativeProtoMap into mWrappedNativeProtoMap

This third patch enjoys a cute sort of bisimulation property.  I initially wrote it as two patches, one that deletes the non-mainthread-only map, then another that renames the mainthread-only to the same name as the non-mainthread-only, but folding them together results in this exact patch, modulo one bit of whitespace, thereby demonstrating the equivalence of the two maps.

Comment 7

[Bobby Holley (:bholley)]

Comment on attachment 8337420 [details] [diff] [review]
part 1 - remove the locks

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

Huzzah! Thanks for doing this.

Comment 8

[Andrew McCreight [:mccr8] (PTO until 12-2)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/592b49270490
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/0fbd934a80c8
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/6235d23be128

Comment 9

[Ryan VanderMeulen [:RyanVM]]

Backed out for Windows build bustage.
https://hg.mozilla.org/integration/mozilla-inbound/rev/0fe924afeb74

https://tbpl.mozilla.org/php/getParsedLog.php?id=31053286&tree=Mozilla-Inbound

Comment 10

[Andrew McCreight [:mccr8] (PTO until 12-2)]

The errors are this:

c:\builds\moz2_slave\m-in-w32-d-0000000000000000000\build\js\xpconnect\src\XPCJSRuntime.cpp(2934) : error C2220: warning treated as error - no 'object' file generated
c:\builds\moz2_slave\m-in-w32-d-0000000000000000000\build\js\xpconnect\src\XPCJSRuntime.cpp(2934) : warning C4355: 'this' : used in base member initializer list

c:\builds\moz2_slave\m-in-w32-d-0000000000000000000\build\js\xpconnect\src\XPCWrappedJS.cpp(347) : error C2220: warning treated as error - no 'object' file generated
c:\builds\moz2_slave\m-in-w32-d-0000000000000000000\build\js\xpconnect\src\XPCWrappedJS.cpp(347) : warning C4355: 'this' : used in base member initializer list

The two lines in question are
  mWatchdogManager(new WatchdogManager(this)),
and
  mRoot(root ? root : this),

which, okay, do use |this|, so it looks like a legit error message, but I didn't change anything in those lines.  My current theory is that something changed in the build configuration to enable warnings as errors for xpconnect, and nobody has touched these files since then.

Comment 11

[Andrew McCreight [:mccr8] (PTO until 12-2)]

Warnings as errors has been on for this directory since August, so that can't be it.  Maybe something about the way it is run or isn't run on Windows changed?  We'll see if the backout builds on Windows.

Comment 12

[Bobby Holley (:bholley)]

No, it's because you removed the MSVC pragma in part 1. That probably needs to go back, unless passing |this| is actually unsafe for some reason.

Comment 13

[Andrew McCreight [:mccr8] (PTO until 12-2)]

Oh, thanks for identifying that!  I just blindly deleted that entire section. ;)

Comment 14

[Andrew McCreight [:mccr8] (PTO until 12-2)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/c1f37de21c86
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/01908f98a35b
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/bc10e345db80

Comment 15

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

(In reply to Andrew McCreight [:mccr8] from comment #13)
> Oh, thanks for identifying that!  I just blindly deleted that entire
> section. ;)

Fwiw, you can also drop the pragma and use MOZ_THIS_IN_INITIALIZER_LIST().

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c1f37de21c86
https://hg.mozilla.org/mozilla-central/rev/01908f98a35b
https://hg.mozilla.org/mozilla-central/rev/bc10e345db80