Closed
Bug 1050108
Opened 10 years ago
Closed 10 years ago
Firefox crash [@ nsUrlClassifierPrefixSet::CollectReports(nsIMemoryReporterCallback*, nsISupports*, bool) ]
Categories
(Toolkit :: Safe Browsing, defect)
Tracking
()
RESOLVED
FIXED
mozilla36
People
(Reporter: danisielm, Assigned: gcp)
References
()
Details
(Keywords: crash, Whiteboard: [mozmill])
Crash Data
Attachments
(2 files)
7.43 KB,
patch
|
n.nethercote
:
review+
|
Details | Diff | Splinter Review |
4.86 KB,
patch
|
mmc
:
review+
|
Details | Diff | Splinter Review |
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.
Updated•10 years ago
|
Component: XPCOM → Phishing Protection
Product: Core → Toolkit
Updated•10 years ago
|
Severity: normal → critical
status-firefox35:
--- → affected
Keywords: crash
Hardware: x86 → All
Whiteboard: [mozmill]
Assignee | ||
Comment 2•10 years ago
|
||
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.
Assignee | ||
Comment 3•10 years ago
|
||
Attachment #8504093 -
Flags: review?(n.nethercote)
Assignee | ||
Comment 4•10 years ago
|
||
Attachment #8504094 -
Flags: review?(mmc)
Updated•10 years ago
|
Attachment #8504094 -
Flags: review?(mmc) → review+
Comment 5•10 years ago
|
||
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+
Assignee | ||
Comment 6•10 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=6dd2be81453b
Assignee | ||
Comment 7•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/b17fa4f3eae4 https://hg.mozilla.org/integration/mozilla-inbound/rev/1f089acdfd64
https://hg.mozilla.org/mozilla-central/rev/b17fa4f3eae4 https://hg.mozilla.org/mozilla-central/rev/1f089acdfd64
Assignee: nobody → gpascutto
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in
before you can comment on or make changes to this bug.
Description
•