Honza Bambas (:mayhemer) Reporter
	Description • 8 years ago

Introduced in https://hg.mozilla.org/mozilla-central/rev/08e7e6ddff87, bug 1208371.

Bug 956338 adds assertions to check whether weak referencing is correctly used only on a single thread.  For LoadManagerSingleton this fails.

https://treeherder.mozilla.org/logviewer.html#?job_id=26046916&repo=try#L13081
https://treeherder.mozilla.org/#/jobs?repo=try&revision=74cbb9192861e6cbc46a615940c28b0b24c5a3d8

tests: dom/media/tests/mochitest/test_peerConnection_*

 11:06:52     INFO -  Assertion failure: _empty || _owningThread == std::this_thread::get_id() (WeakPtr used on multiple threads), at /builds/slave/try-lx-d-000000000000000000000/build/src/obj-firefox/dist/include/mozilla/WeakPtr.h:146
 11:06:52     INFO -  #01: mozilla::detail::WeakReference<mozilla::LoadManagerSingleton>::get [mfbt/WeakPtr.h:146]
 11:06:52     INFO -  #02: mozilla::LoadManager::NormalUsage [dom/media/systemservices/LoadManager.h:111]
 11:06:52     INFO -  #03: webrtc::OveruseFrameDetector::Process [media/webrtc/trunk/webrtc/video_engine/overuse_frame_detector.cc:538]
 11:06:52     INFO -  #04: webrtc::ProcessThreadImpl::Process [media/webrtc/trunk/webrtc/modules/utility/source/process_thread_impl.cc:213]
 11:06:52     INFO -  #05: webrtc::ThreadPosix::Run [media/webrtc/trunk/webrtc/system_wrappers/source/thread_posix.cc:174]


This is likely a highly critical bug.

	Randell Jesup [:jesup] (needinfo me)
	Comment 1 • 8 years ago

Per mayhemer, marking as sec-critical (might be high, not sure of the failure modes for this threading issue without looking at the code for the WeakRef).

	Andreas Pehrson [:pehrsons] Assignee
	Comment 2 • 8 years ago

We check that the singleton is created and destroyed on main thread, and it's only destroyed by an xpcom-shutdown observer.

Question is how many LoadManager consumers are alive after xpcom-shutdown.

	Randell Jesup [:jesup] (needinfo me)
	Comment 3 • 8 years ago

(In reply to Andreas Pehrson [:pehrsons] (Telenor) from comment #2)
> We check that the singleton is created and destroyed on main thread, and
> it's only destroyed by an xpcom-shutdown observer.
> 
> Question is how many LoadManager consumers are alive after xpcom-shutdown.

None I hope, but this isn't at xpcom-shutdown; this is flagging that the WeakRef was used from two threads, which is invalid.

	Honza Bambas (:mayhemer) Reporter
	Comment 4 • 8 years ago

If you can strongly ensure that the weak referenced object over-lives all of its referrers, then you don't need support for weak referencing at all.  That would be the best solution here.

The problem the added assertions in bug 956338 try to catch is attempt to access weak ptr on one thread while the real object is just being destroyed on another thread.  Hence the simple restriction to query weak ptrs only on a single thread.

Dereference of a weak ptr to the same object on more then one thread (say A and B) is _LIKELY_ bad, at least it's a very bad programing architecture.  If you expect the real object go away on either thread A or B or even some thread X, then you cannot avoid very dangerous race conditions when referrers, still alive, keep the weak ref and try to query it.

	Andreas Pehrson [:pehrsons] Assignee
	Comment 5 • 8 years ago

Well it was added for a reason. IIRC there were intermittents at shutdown.

Isn't it less critical in this mode though? I.e., when we only reset the ptr at shutdown rather than doing it whenever?

	Andreas Pehrson [:pehrsons] Assignee
	Comment 7 • 8 years ago

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=bce98fc244f1

	Andreas Pehrson [:pehrsons] Assignee
	Comment 8 • 8 years ago

Comment on attachment 8784250 [details] [diff] [review]
Change LoadManagerSingleton WeakPtr to RefPtr

[Security approval request comment]
How easily could an exploit be constructed based on the patch? Not easily

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? No

Which older supported branches are affected by this flaw? 48 onwards

If not all supported branches, which bug introduced the flaw?

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? This should go clean on all the branches affected.

How likely is this patch to cause regressions; how much testing does it need? The only risk is a leak but initial try runs look good.

	Honza Bambas (:mayhemer) Reporter
	Comment 9 • 8 years ago

Thanks for fixing this so quickly!

	Al Billings [:abillings - ex-MoCo]
	Comment 10 • 8 years ago

sec-approval+ for trunk. Please nominate patches for Aurora and Beta as well. We need to get all of these in this week.

	Randell Jesup [:jesup] (needinfo me)
	Comment 11 • 8 years ago

Comment on attachment 8784250 [details] [diff] [review]
Change LoadManagerSingleton WeakPtr to RefPtr

Approval Request Comment
[Feature/regressing bug #]: Track cloning

[User impact if declined]: Security issue would be fixed on nightly, not beta/aurora.  May be very hard to exploit, even if you know where it is, since it's being released during shutdown.

[Describe test coverage new/current, TreeHerder]: Was found by Honza on a Try push I believe due to a new assertion.

[Risks and why]: Low risk - basically converting a weak ref to a strong one.  Biggest risk would be a leak.

[String/UUID change made/needed]: none

	Ryan VanderMeulen [:RyanVM]
	Comment 12 • 8 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/521e476f0c714ff4e92c7220e81b40e56972f58c

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 13 • 8 years ago

https://hg.mozilla.org/mozilla-central/rev/521e476f0c71

	Liz Henry (:lizzard) (relman/hg->git project)
	Comment 14 • 8 years ago

Comment on attachment 8784250 [details] [diff] [review]
Change LoadManagerSingleton WeakPtr to RefPtr

Fix for sec-critical issue, let's uplift this for beta 8.

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 15 • 8 years ago

https://hg.mozilla.org/releases/mozilla-aurora/rev/c20fd556d065
https://hg.mozilla.org/releases/mozilla-beta/rev/550118c88012