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)

x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla40
Tracking Status
firefox40 --- fixed
b2g-master --- fixed

People

(Reporter: billm, Assigned: billm)

References

Details

(Keywords: sec-other, Whiteboard: [post-critsmash-triage][adv-main40-])

Attachments

(1 file)

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.
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).
(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.
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.)
Attached patch patchSplinter Review
The IPC code is cold enough that the cost won't matter, so let's just do this.
Assignee: nobody → wmccloskey
Status: NEW → ASSIGNED
Attachment #8588799 - Flags: review?(dvander)
Attachment #8588799 - Flags: review?(dvander) → review+
https://hg.mozilla.org/mozilla-central/rev/cecb29d62dbc
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Depends on: 1169457
Whiteboard: [post-critsmash-triage]
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main40-]
Group: core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.