IPC code should include release-mode assertions that it's used on the right thread

RESOLVED FIXED in Firefox 40, Firefox OS master

Status

()

Core
IPC
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: billm, Assigned: billm)

Tracking

({sec-other})

unspecified
mozilla40
x86_64
Linux
sec-other
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox40 fixed, b2g-master fixed)

Details

(Whiteboard: [post-critsmash-triage][adv-main40-])

Attachments

(1 attachment)

(Assignee)

Description

3 years ago
See bug 1146416.
(Assignee)

Updated

3 years ago
Keywords: sec-other
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.)
(Assignee)

Comment 6

3 years ago
Created attachment 8588799 [details] [diff] [review]
patch

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+
(Assignee)

Comment 7

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/cecb29d62dbc
https://hg.mozilla.org/mozilla-central/rev/cecb29d62dbc
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-b2g-master: --- → fixed
status-firefox40: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Blocks: 1155375
Depends on: 1169457
Whiteboard: [post-critsmash-triage]
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main40-]

Updated

2 years ago
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.