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

RESOLVED FIXED in mozilla36

Status

()

Toolkit
Safe Browsing
--
critical
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: Daniel Gherasim, Assigned: gcp)

Tracking

({crash})

34 Branch
mozilla36
All
Mac OS X
crash
Points:
---

Firefox Tracking Flags

(firefox34 affected, firefox35 affected)

Details

(Whiteboard: [mozmill], crash signature, URL)

Attachments

(2 attachments)

(Reporter)

Description

4 years ago
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

Updated

4 years ago
Duplicate of this bug: 1080472
Severity: normal → critical
status-firefox35: --- → affected
Keywords: crash
Hardware: x86 → All
Whiteboard: [mozmill]
(Assignee)

Comment 2

4 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

4 years ago
Created attachment 8504093 [details] [diff] [review]
Avoid race condition during memory report collection
Attachment #8504093 - Flags: review?(n.nethercote)
(Assignee)

Comment 4

4 years ago
Created attachment 8504094 [details] [diff] [review]
Remove superfluous member variables in PrefixSet
Attachment #8504094 - Flags: review?(mmc)
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
Last Resolved: 4 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.