Reproduces on trunk. 20120605092342 http://hg.mozilla.org/mozilla-central/rev/c76497029f0d ================================================================= ==16916== ERROR: AddressSanitizer global-buffer-overflow on address 0x7fab8bdc5bb0 at pc 0x7fab8ab5e87d bp 0x7fab789bbdd0 sp 0x7fab789bbdc8 READ of size 4 at 0x7fab8bdc5bb0 thread T2 #0 0x7fab8ab5e87d in js::gc::MapAllocToTraceKind(js::gc::AllocKind) /usr/local/google/home/aarya/firefox/src/js/src/jsfriendapi.cpp:0 #1 0x7fab88c80099 in nsXMLHttpRequest::cycleCollection::Trace(void*, void (*)(void*, char const*, void*), void*) /usr/local/google/home/aarya/firefox/src/content/base/src/nsXMLHttpRequest.cpp:663 #2 0x7fab88c7c9ca in nsXHREventTarget::cycleCollection::Traverse(void*, nsCycleCollectionTraversalCallback&) /usr/local/google/home/aarya/firefox/src/content/base/src/nsXMLHttpRequest.cpp:287 #3 0x7fab8a2cf99e in nsCycleCollector::MarkRoots(GCGraphBuilder&) /usr/local/google/home/aarya/firefox/src/xpcom/base/nsCycleCollector.cpp:2054 #4 0x7fab8a2d1db7 in nsCycleCollector::BeginCollection(nsICycleCollectorListener*) /usr/local/google/home/aarya/firefox/src/xpcom/base/nsCycleCollector.cpp:2760 #5 0x7fab8a2d667e in nsCycleCollectorRunner::Run() /usr/local/google/home/aarya/firefox/src/xpcom/base/nsCycleCollector.cpp:3049 #6 0x7fab8a2b0824 in nsThread::ProcessNextEvent(bool, bool*) /usr/local/google/home/aarya/firefox/src/xpcom/threads/nsThread.cpp:624 #7 0x7fab8a21e16d in NS_ProcessNextEvent_P(nsIThread*, bool) /usr/local/google/home/aarya/firefox/src/objdir-ff-asan/xpcom/build/nsThreadUtils.cpp:213 #8 0x7fab8a2aebbd in nsThread::ThreadFunc(void*) /usr/local/google/home/aarya/firefox/src/xpcom/threads/nsThread.cpp:256 #9 0x7fab8edcf706 in _pt_root /usr/local/google/home/aarya/firefox/src/nsprpub/pr/src/pthreads/ptthread.c:158 0x7fab8bdc5bb0 is located 0 bytes to the right of global variable 'js::gc::MapAllocToTraceKind(js::gc::AllocKind)::map (/usr/local/google/home/aarya/firefox/src/js/src/jsfriendapi.cpp)' (0x7fab8bdc5b60) of size 80 'js::gc::MapAllocToTraceKind(js::gc::AllocKind)::map (/usr/local/google/home/aarya/firefox/src/js/src/jsfriendapi.cpp)' is ascii string '' ==16916== ABORTING Stats: 186M malloced (195M for red zones) by 404853 calls Stats: 46M realloced by 21407 calls Stats: 156M freed by 274895 calls Stats: 24M really freed by 46335 calls Stats: 412M (105532 full pages) mmaped in 103 calls mmaps by size class: 8:311277; 9:57337; 10:24570; 11:22517; 12:4096; 13:2560; 14:1792; 15:512; 16:640; 17:160; 18:208; 19:56; 20:20; mallocs by size class: 8:298456; 9:55207; 10:22104; 11:19974; 12:4008; 13:2088; 14:1570; 15:393; 16:638; 17:146; 18:196; 19:55; 20:18; frees by size class: 8:188758; 9:43497; 10:18577; 11:16765; 12:2858; 13:1836; 14:1370; 15:330; 16:533; 17:126; 18:181; 19:50; 20:14; rfrees by size class: 8:37269; 9:2590; 10:2242; 11:3443; 12:169; 13:216; 14:139; 15:48; 16:159; 17:29; 18:21; 19:9; 20:1; Stats: malloc large: 415 small slow: 2219 Shadow byte and word: 0x1ff5717b8b76: f9 0x1ff5717b8b70: 00 00 00 00 00 00 f9 f9 More shadow bytes: 0x1ff5717b8b50: 00 00 00 00 06 f9 f9 f9 0x1ff5717b8b58: f9 f9 f9 f9 05 f9 f9 f9 0x1ff5717b8b60: f9 f9 f9 f9 00 06 f9 f9 0x1ff5717b8b68: f9 f9 f9 f9 00 00 00 00 =>0x1ff5717b8b70: 00 00 00 00 00 00 f9 f9 0x1ff5717b8b78: f9 f9 f9 f9 00 00 00 00 0x1ff5717b8b80: 00 00 00 00 00 00 00 00 0x1ff5717b8b88: 00 00 00 00 00 00 00 00 0x1ff5717b8b90: 00 00 00 00 00 00 00 00

Here's what's happening here. nsXMLHttpRequest has a JSObject* field called mResultArrayBuffer. It's supposed to get traced via NS_IMPL_CYCLE_COLLECTION_TRACE_BEGIN. However, in this case we're not calling HoldJSObjects on the nsXMLHttpRequest, so the JS GC is freeing the JSObject while the XHR thinks it still owns it. We're supposed to be calling HoldJSObjects from RootResultArrayBuffer, which calls nsContentUtils::PreserveWrapper, which calls HoldJSObjects. However, that function returns early if the wrapper is already preserved; that's what's happening here. It's getting preserved from generated code, inside of mozilla::dom::XMLHttpRequestBinding::_addProperty, which looks like this: nsXMLHttpRequest* self = UnwrapDOMObject<nsXMLHttpRequest>(obj); JSCompartment* compartment = js::GetObjectCompartment(obj); xpc::CompartmentPrivate* priv = static_cast<xpc::CompartmentPrivate*>(JS_GetCompartmentPrivate(compartment)); if (!priv->RegisterDOMExpandoObject(obj)) { return false; } self->SetPreservingWrapper(true); return true; I'm not sure what the right solution is here. Should we stop relying on PreserveWrapper to hold the JS objects? Or should we fix the new DOM bindings so that they call HoldJSObjects? Or something else? Anyway, this seems like a pretty serious bug to me. I'm not sure when the new XHR bindings landed, but I'm guessing that's when this regressed.

Those landed for Firefox 14. See bug 740069. Sounds like _addProperty should perhaps make the relevant HoldJSObjects call...

Oh, one more thing. If you run this test case in a debug build, it asserts in some layout code. I had to comment out the assertion to see the more serious problem. I think the assertion is unrelated, but someone in layout should probably look at it.

(In reply to Boris Zbarsky (:bz) from comment #2) > Sounds like _addProperty should perhaps make the relevant HoldJSObjects > call... If so, we should probably add a comment to nsWrapperCache::SetPreservingWrapper saying something like "Never call this directly; use nsContentUtils instead." I hate having to do stuff like that, though, so it would be nice if there were a better way.

I also see a call to SetPreservingWrapper(true) in ListBase<LC>::ensureExpandoObject. It looks to me like that object is rooted by RegisterDOMExpandoObject.

Oh, right. I misunderstood the problem. To spell it out a bit more, the JSObject wrapper itself is held alive by the ensureExpandoObject, which serves the same role as NS_IMPL_CYCLE_COLLECTION_TRACE_PRESERVED_WRAPPER. nsXMLHttpRequest's Trace function visits two other JS fields, mResultArrayBuffer and mResultJSON, which it expects to be kept alive, but the XHR isn't added as a JS holder, so the GC don't call the trace function, so the GC doesn't know to keep those two fields alive. It is okay to set SetPreservingWrapper(true) on your wrapper if you aren't a JS holder, but only if you don't have a Trace method. That seems to be the case for ListBase, or at least a quick search couldn't find a Trace method for that function.

When you say "Trace" method, are you talking about a JSAPI class hook, or CC stuff, or something else?

Sorry, I meant the CC stuff. NS_IMPL_CYCLE_COLLECTION_TRACE_BEGIN and NS_IMPL_CYCLE_COLLECTION_TRACE_BEGIN_INHERITED. When a C++ thing is added as a JS holder, its CC trace function gets invoked to find what JS objects it keeps alive, is what I mean.

> Sorry, I meant the CC stuff. Ah. The ListBase itself doesn't have that, of course: it's a singleton proxy handler. The actual objects are various things like nsContentList, which have a wrapper cache and should definitely have Trace stuff attached to their CC.

We could go two ways with this, either start using HoldJSObjects to root/trace non-wrapper members or add a trace hook that calls the CC participant's Trace. I personally am more in favor of the second, but ideally we'd then need to have a way to only add the trace hook if we want to trace more than just the wrapper, and that seems a bit hard to know without hardcoding it in the config file.

+1 to everything in comment 10. I think sticking this in the config file is fine.

I don't understand most of the discussion here, but I'd like to point out that getting tracing correct is really hard as it is. Especially since testing reliably is hard. So relying on people to additionally remember to put something in the config file in order to avoid exploitable crashes scares me.

Has anybody had a change to look at this? Probably too late to fix for 14, but it looks bad. (XHR isn't properly rooting a JS object it holds onto.)

change/chance

Uh, it really sucks that we dropped the ball on this.

Let's fix this the slow but safe way for now. I'll file a followup bug to figure out a better approach with a real trace hook.

Comment on attachment 640985 [details] [diff] [review] v1 r=me if you add a check on our class having the nsISupports bool set. Or do we not even codegen this hook for non-nsISupports things? If so, we could just have a comment about that here, and one at the place where that check is done... Seems like we generate this for all concrete non-worker wrappercached things. Does that imply nsISupports?

Comment on attachment 640985 [details] [diff] [review] v1 [Approval Request Comment] Bug caused by (feature/regressing bug #): bug 740069 User impact if declined: crashes Testing completed (on m-c, etc.): landed on mozilla-inbound Risk to taking this patch (and alternatives if risky): low risk, this reuses existing wrapper preservation code which we already use for all other XPConnect bindings String or UUID changes made by this patch: none

Comment on attachment 640985 [details] [diff] [review] v1 approving for aurora now, we'll need more testing before approving for beta.

Comment on attachment 640985 [details] [diff] [review] v1 Can we land this on branches now? Let's get this into beta 2 for testing.

This was actually landed on Aurora on 7/18. https://hg.mozilla.org/releases/mozilla-aurora/rev/85275cda0f19

Peter's Aurora patch applied cleanly to Beta, so I went ahead and landed it. https://hg.mozilla.org/releases/mozilla-beta/rev/327b951ff6a8

I tried to verify this issue on Ubuntu 12.04 64 bit with the latest Firefox 17 debug build (ftp://ftp.mozilla.org/pub/firefox/nightly/2012-11-13-mozilla-beta-debug). Unfortunately, when I try to see the bug on Firefox 14 by uploading Arya's testcase, I get this crash: https://crash-stats.mozilla.com/report/index/bc7dc334-4b35-48ba-9566-6021b2121113 When I try to upload the same testcase in the Firefox 17 build mentioned above, I get the following crash: Fx 17: https://crash-stats.mozilla.com/report/index/8043d663-0e5b-4746-96b3-856da2121113 Are there any workarounds that could allow QA to verify this bug?

This can't be tested with a normal debug build, afaik. It requires an ASAN build. I'm reassigning this to Matt to see if he can help verify.

I'm getting the same crash as Ioana is, above, using latest ASAN builds for Aurora and Nightly, on Ubuntu 11.10 from 2012-11-14. AFAIC, it is specific to the testcase in this bug.

Matt, can you file a new bug for the crash you're hitting? Peter, what would a reduced testcase for this bug look like? I want to know if my fuzzer is missing something, and I'd like this bug to be regression-tested.

Filed new bug 821360 for the current crash.

Please see comment 3 about the crash in CSS code. It's unrelated to this problem.

rforbes-bugspam-for-setting-that-bounty-flag-20130719

mass remove verifyme requests greater than 4 months old