Closed Bug 1050108 Opened 8 years ago Closed 8 years ago

Firefox crash [@ nsUrlClassifierPrefixSet::CollectReports(nsIMemoryReporterCallback*, nsISupports*, bool) ]


(Toolkit :: Safe Browsing, defect)

34 Branch
Not set



Tracking Status
firefox34 --- affected
firefox35 --- affected


(Reporter: danisielm, Assigned: gcp)




(Keywords: crash, Whiteboard: [mozmill])

Crash Data


(2 files)

This happened while running mozmill endurance testrun on this test:

where we simply pin/unpin tabs.

Happened on a OS X 10.8 Virtual Machine with the latest Nightly en-US. 

Last 10 items from thread:
0	XUL	nsUrlClassifierPrefixSet::CollectReports(nsIMemoryReporterCallback*, nsISupports*, bool)	obj-firefox/x86_64/dist/include/nsTArray-inl.h
1	XUL	nsMemoryReporterManager::GetReportsForThisProcessExtended(nsIMemoryReporterCallback*, nsISupports*, bool, __sFILE*)	xpcom/base/nsMemoryReporterManager.cpp
2	XUL	nsMemoryReporterManager::GetExplicit(long long*)	xpcom/base/nsMemoryReporterManager.cpp
3	XUL	NS_InvokeByIndex	xpcom/reflect/xptcall/md/unix/xptcinvoke_x86_64_unix.cpp
4	XUL	XPCWrappedNative::CallMethod(XPCCallContext&, XPCWrappedNative::CallMode)	js/xpconnect/src/XPCWrappedNative.cpp
5	XUL	XPC_WN_GetterSetter(JSContext*, unsigned int, JS::Value*)	js/xpconnect/src/xpcprivate.h
6	XUL	js::Invoke(JSContext*, JS::CallArgs, js::MaybeConstruct)	js/src/jscntxtinlines.h
7	XUL	js::Invoke(JSContext*, JS::Value const&, JS::Value const&, unsigned int, JS::Value const*, JS::MutableHandle<JS::Value>)	js/src/vm/Interpreter.cpp
8	XUL	js::InvokeGetterOrSetter(JSContext*, JSObject*, JS::Value, unsigned int, JS::Value*, JS::MutableHandle<JS::Value>)	js/src/vm/Interpreter.cpp
9	XUL	js::Shape::get(JSContext*, JS::Handle<JSObject*>, JSObject*, JSObject*, JS::MutableHandle<JS::Value>)	js/src/vm/Shape-inl.h
10	XUL	js::baseops::GetProperty(JSContext*, JS::Handle<JSObject*>, JS::Handle<JSObject*>, JS::Handle<jsid>, JS::MutableHandle<JS::Value>)	js/src/jsobj.cpp

Re-run the testrun to see if it reproduces.
Component: XPCOM → Phishing Protection
Product: Core → Toolkit
Severity: normal → critical
Keywords: crash
Hardware: x86 → All
Whiteboard: [mozmill]
Despite the timing of the crash starting, this can't be a dupe of bug 1074196 because the code:
checks the main mIndexDeltas array before indexing into it.

My guess: the memory reports collector is running on the main thread while there is a SafeBrowsing update happening on the UrlClassifier thread, and it's reading into that array which is concurrently being modified. The problem was exposed by bug 1074196 because it turns a single Length() call on the one-dimensional mIndexDeltas array (probably atomic in practice) into a loop over the elements.

I don't want to add locking just to get memory reporting, so I'll have nsUrlClassifierPrefixSet remember it's own size and atomically update it.
Attachment #8504094 - Flags: review?(mmc) → review+
Comment on attachment 8504093 [details] [diff] [review]
Avoid race condition during memory report collection

Review of attachment 8504093 [details] [diff] [review]:


::: toolkit/components/url-classifier/nsUrlClassifierPrefixSet.h
@@ +69,5 @@
>    nsTArray<nsTArray<uint16_t> > mIndexDeltas;
>    // how many prefixes we have.
>    uint32_t mTotalPrefixes;
> +  mozilla::Atomic<size_t> mMemoryInUse;

It would be good to have a comment that mentions this bug and explains the race condition and how pre-computing the size avoids it. This seems like a good place to put it.
Attachment #8504093 - Flags: review?(n.nethercote) → review+
Assignee: nobody → gpascutto
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Depends on: 1205358
You need to log in before you can comment on or make changes to this bug.