Closed Bug 1050108 Opened 5 years ago Closed 5 years ago

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

Categories

(Toolkit :: Safe Browsing, defect, critical)

34 Branch
All
macOS
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla36
Tracking Status
firefox34 --- affected
firefox35 --- affected

People

(Reporter: danisielm, Assigned: gcp)

References

()

Details

(Keywords: crash, Whiteboard: [mozmill])

Crash Data

Attachments

(2 files)

This happened while running mozmill endurance testrun on this test:
https://hg.mozilla.org/qa/mozmill-tests/file/9c6ed92c41f5/firefox/tests/endurance/testTabbedBrowsing_PinAndUnpinAppTab/test1.js

where we simply pin/unpin tabs.

Happened on a OS X 10.8 Virtual Machine with the latest Nightly en-US.
https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/08/2014-08-06-03-02-01-mozilla-central/firefox-34.0a1.en-US.mac.dmg 

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
Duplicate of this bug: 1080472
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:
http://dxr.mozilla.org/mozilla-central/source/toolkit/components/url-classifier/nsUrlClassifierPrefixSet.cpp?from=nsUrlClassifierPrefixSet.cpp#258
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]:
-----------------------------------------------------------------

Thanks.

::: 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+
https://hg.mozilla.org/mozilla-central/rev/b17fa4f3eae4
https://hg.mozilla.org/mozilla-central/rev/1f089acdfd64
Assignee: nobody → gpascutto
Status: NEW → RESOLVED
Closed: 5 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.