Status

()

defect
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: bholley, Assigned: mccr8)

Tracking

(Blocks 1 bug)

unspecified
mozilla28
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments)

Reporter

Description

7 years ago
I split this off into its own bug.
Reporter

Updated

7 years ago
Depends on: 770840
Reporter

Updated

6 years ago
Blocks: 839535, 843923
Reporter

Updated

6 years ago
Blocks: 911303
Assignee

Comment 4

6 years ago
Not strictly related, but a simple cleanup related to making it single threaded.
Assignee

Comment 5

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

Updated

6 years ago
Attachment #8337420 - Flags: review?(bobbyholley+bmo)
Assignee

Updated

6 years ago
Attachment #8337421 - Flags: review?(bobbyholley+bmo)
Assignee

Comment 6

6 years ago
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.
Attachment #8337422 - Flags: review?(bobbyholley+bmo)
Reporter

Comment 7

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

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

Huzzah! Thanks for doing this.
Attachment #8337420 - Flags: review?(bobbyholley+bmo) → review+
Reporter

Updated

6 years ago
Attachment #8337421 - Flags: review?(bobbyholley+bmo) → review+
Reporter

Updated

6 years ago
Attachment #8337422 - Flags: review?(bobbyholley+bmo) → review+
Assignee

Comment 10

6 years ago
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.
Assignee

Comment 11

6 years ago
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.
Reporter

Comment 12

6 years ago
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.
Assignee

Comment 13

6 years ago
Oh, thanks for identifying that!  I just blindly deleted that entire section. ;)
(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().
You need to log in before you can comment on or make changes to this bug.