Improvements to mscom weak references

RESOLVED FIXED in Firefox 54

Status

()

Core
IPC
RESOLVED FIXED
8 months ago
8 months ago

People

(Reporter: aklotz, Assigned: aklotz)

Tracking

Trunk
mozilla54
Unspecified
Windows
Points:
---

Firefox Tracking Flags

(firefox54 fixed)

Details

(Whiteboard: aes+)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

While testing the a11y smart proxy, I realized that the mscom weak reference stuff needs some improvements:

1) Deadlocks were possible due to lack of a lock hierarchy;
2) WeakReferenceSupport does not need an array of WeakRefs; since we are obtaining multiple weak references that always point to the same object, we only need one shared and thread-safe reference to the original object.

The following patch refactors the weakref stuff to get rid of that array and revise the locking scheme as follows:

1) There are two critical sections (using these due to reentrancy requirements): One for ThreadSafeQueryInterface (WeakReferenceSupport::mCSForQI) and one inside the shared reference (SharedRef::mCS).
2) If acquiring mCSForQI, it must always be taken before mCS.
3) SharedRef::mCS protects the raw, weak pointer, as well as the refcount in the WeakReferenceSupport class itself.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 3

8 months ago
mozreview-review
Comment on attachment 8837807 [details]
Bug 1339951: Refactor mscom weak reference support and establish lock hierarchy within;

https://reviewboard.mozilla.org/r/112806/#review115040
Attachment #8837807 - Flags: review?(jmathies) → review+

Comment 4

8 months ago
Pushed by aklotz@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/06cd86f16c70
Refactor mscom weak reference support and establish lock hierarchy within; r=jimm
Blocks: 1340733
Backed out in hope for fix for a11y related crashes e.g. in Mn-e10s on Windows 8 x64 debug test_accessibility.py TestAccessibility.test_click_raises_element_not_accessible:

https://hg.mozilla.org/integration/autoland/rev/45c6f5ea6b039b0a7aa64bd2f0c72a694fbf88c1

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=b97189d6853f5061afd00a94e4759a5c51597dc1&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log Mn-e10s: https://treeherder.mozilla.org/logviewer.html#?job_id=78476004&repo=autoland
Failure log M(a): https://treeherder.mozilla.org/logviewer.html#?job_id=78479410&repo=autoland
Failure log M-e10s(bc7): https://treeherder.mozilla.org/logviewer.html#?job_id=78476043&repo=autoland
Flags: needinfo?(aklotz)
Comment hidden (mozreview-request)

Comment 7

8 months ago
Pushed by aklotz@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b68489018777
Refactor mscom weak reference support and establish lock hierarchy within; r=jimm
OK. Some minor changes that I made in a later patch should have been in this one. I fixed that and re-pushed.
Flags: needinfo?(aklotz)
No longer blocks: 1340733

Comment 9

8 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/b68489018777
Status: ASSIGNED → RESOLVED
Last Resolved: 8 months ago
status-firefox54: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in before you can comment on or make changes to this bug.