1339951 - Improvements to mscom weak references

Status: RESOLVED FIXED

(Core :: IPC, defect)

Description

[(No longer employed by Mozilla) Aaron Klotz]

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 1

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Bug 1339951: Refactor mscom weak reference support and establish lock hierarchy within;

Review commit: https://reviewboard.mozilla.org/r/112806/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/112806/

Comment 2

[(No longer employed by Mozilla) Aaron Klotz]

Comment on attachment 8837807 [details]
Bug 1339951: Refactor mscom weak reference support and establish lock hierarchy within;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/112806/diff/1-2/

Comment 3

[Jim Mathies [:jimm]]

Comment on attachment 8837807 [details]
Bug 1339951: Refactor mscom weak reference support and establish lock hierarchy within;

https://reviewboard.mozilla.org/r/112806/#review115040

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/06cd86f16c70
Refactor mscom weak reference support and establish lock hierarchy within; r=jimm

Comment 5

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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

Comment 6

[(No longer employed by Mozilla) Aaron Klotz]

Comment on attachment 8837807 [details]
Bug 1339951: Refactor mscom weak reference support and establish lock hierarchy within;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/112806/diff/2-3/

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b68489018777
Refactor mscom weak reference support and establish lock hierarchy within; r=jimm

Comment 8

[(No longer employed by Mozilla) Aaron Klotz]

OK. Some minor changes that I made in a later patch should have been in this one. I fixed that and re-pushed.

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b68489018777