Closed Bug 1138770 Opened 5 years ago Closed 5 years ago

Use-after-free of memory reporter during tests

Categories

(Core :: XPCOM, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla39
Tracking Status
firefox38 --- fixed
firefox39 --- fixed

People

(Reporter: njn, Assigned: njn)

References

Details

(Keywords: csectype-uaf)

Attachments

(1 file)

Margaret was seeing ASan failures for test_memoryReporters.xul in bug 1124011. 

The memory reporter manager ("Mgr") has a test-only facility whereby it can save the existing memory reporters, install some temporary ones, do some stuff, and then restore the original ones. The exact problem with this was as follows.

- Reporter R gets registered as a weak reporter. This means Mgr holds a weak reference to it.

- Mgr is instructed to stash all its reporters.

- Reporter R gets unregistered. The unregistration silently fails because stashed reporters don't get considered in this step.

- Reporter R is destroyed.

- Mgr is instructed to unstash all its stashed reporters.

- Mgr iterates over all its reporters, which includes the weak reference to the now-dead R. ASan rightly complains.

I couldn't reproduce this locally but I confirmed it via debugging printfs on the tryserver.

The fix is simple: the third step above should unregister reporters that are stashed. It's a test-only fix, which is good.
Comment on attachment 8571722 [details] [diff] [review]
Make memory reporters that have been stashed for testing eligible for unregistration

Review of attachment 8571722 [details] [diff] [review]:
-----------------------------------------------------------------

nice
Attachment #8571722 - Flags: review?(continuation) → review+
https://hg.mozilla.org/mozilla-central/rev/655ddeba3c94
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
We are trying to ship reading list/reader view in Firefox 38, so eventually we will want to turn on the toolbar button there as well. Could you uplift this patch to Aurora in anticipation of us enabling this feature on Aurora some time in the coming weeks?
Flags: needinfo?(n.nethercote)
Comment on attachment 8571722 [details] [diff] [review]
Make memory reporters that have been stashed for testing eligible for unregistration

Approval Request Comment

[Feature/regressing bug #]: This patches fixes a test bug that blocks Reader Mode (bug 1124011) from landing. It's intended to be uplifted to Aurora some time soon.

[User impact if declined]: Reader Mode cannot be uplifted.

[Describe test coverage new/current, TreeHerder]: the problem is with the test itself :)

[Risks and why]: Extremely low. It was a fix to some code that's only used to facilitate testing, and doesn't get used outside of tests.

[String/UUID change made/needed]: None.
Attachment #8571722 - Flags: approval-mozilla-aurora?
Flags: needinfo?(n.nethercote)
Attachment #8571722 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.