Closed
Bug 1146471
Opened 9 years ago
Closed 9 years ago
IPC code should include release-mode assertions that it's used on the right thread
Categories
(Core :: IPC, defect)
Tracking
()
RESOLVED
FIXED
mozilla40
People
(Reporter: billm, Assigned: billm)
References
Details
(Keywords: sec-other, Whiteboard: [post-critsmash-triage][adv-main40-])
Attachments
(1 file)
1.52 KB,
patch
|
dvander
:
review+
|
Details | Diff | Splinter Review |
See bug 1146416.
Comment 1•9 years ago
|
||
Aren't normal assertions enough? Or do people not test their patches using debug builds? (that would be very worrisome)
DEBUG builds of B2G were painfully slow for a long time (maybe still are?) so release-mode assertions for threadsafety became a necessity long ago. But I think we limited that so that MOZ_RELEASE builds were unaffected.
Comment 3•9 years ago
|
||
I had mostly e10s in mind here. But sure, these IPC code paths shouldn't be that hot, at least not anything comparing to bug 907914, which caused significant performance regressions and was then disabled in normal cases (bug 919380).
Comment 4•9 years ago
|
||
(In reply to Ben Turner <unresponsive until 3/30> (use the needinfo flag!) from comment #2) > DEBUG builds of B2G were painfully slow for a long time (maybe still are?) Bug 845886 hasn't seen much in the way of informative updates since the not-entirely-nice comment I left on it in 2013. > so release-mode assertions for threadsafety became a necessity long ago. But > I think we limited that so that MOZ_RELEASE builds were unaffected. See also MOZ_DIAGNOSTIC_ASSERT from bug 1129247. For the specific case of bug 1146416, the initial patch may have been tested adequately on a debug build, but the automated tests don't cover the case where I introduced a thread-safety regression. As far as I know we're not doing automated coverage reporting to detect test deficiencies like that.
Comment 5•9 years ago
|
||
Bug 1151650 is another instance of off-main-thread main-thread-only IPC. (Bug 1148650 might catch these indirectly, assuming the overhead is acceptable, but I don't know offhand if it's guaranteed to do so.)
Assignee | ||
Comment 6•9 years ago
|
||
The IPC code is cold enough that the cost won't matter, so let's just do this.
Attachment #8588799 -
Flags: review?(dvander) → review+
Assignee | ||
Comment 7•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/cecb29d62dbc
Comment 8•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/cecb29d62dbc
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-b2g-master:
--- → fixed
status-firefox40:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Updated•9 years ago
|
Whiteboard: [post-critsmash-triage]
Updated•9 years ago
|
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main40-]
Updated•9 years ago
|
Group: core-security → core-security-release
Updated•8 years ago
|
Group: core-security-release
You need to log in
before you can comment on or make changes to this bug.
Description
•