Disk consumption observers could get released on the cache thread

RESOLVED FIXED in mozilla38

Status

()

Core
Networking: Cache
--
critical
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: neil@parkwaycc.co.uk, Assigned: michal)

Tracking

33 Branch
mozilla38
x86_64
Windows 10
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Reporter)

Description

4 years ago
Created attachment 8543686 [details]
Stack backtrace

mozilla::net::CacheIndex::ChangeState clears its list of disk consumption observers. However these observers contain weak references which aren't safe to clear on the cache thread. I'm not sure how I triggered the problem however I'd restarted after turning off e10s in options and the crash happened shortly after I closed the options tab. Stack is from an opt with symbols build so some symbols may have been merged.
(Assignee)

Updated

4 years ago
Assignee: nobody → michal.novotny
(Reporter)

Comment 2

4 years ago
Comment on attachment 8544298 [details] [diff] [review]
fix

>+        nsIWeakReference *obs;
>+        mObserver.forget(&obs);
>+
>+        nsCOMPtr<nsIThread> mainThread = do_GetMainThread();
>+        if (mainThread) {
>+          NS_ProxyRelease(mainThread, obs);
>+        } else {
>+          NS_WARNING("Cannot get main thread, leaking weak reference to "
>+                     "CacheStorageConsumptionObserver.");
>+        }
It's a shame that NS_ProxyRelease crashes (by attempting to release the weak reference on the wrong thread) if you pass in a null thread otherwise you could just replace this with NS_ProxyRelease(mainThread, mObserver);
Attachment #8544298 - Flags: review?(jduell.mcbugs) → review+
https://hg.mozilla.org/mozilla-central/rev/8068a6826e68
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
You need to log in before you can comment on or make changes to this bug.