Closed
Bug 1103375
Opened 10 years ago
Closed 9 years ago
Firefox crashes when measuring the memory in about:memory ( [@ StatsCellCallback<int> ] ) with Yandex Elements extension installed
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
People
(Reporter: zeidspex, Assigned: n.nethercote)
Details
(Keywords: crash, crashreportid, topcrash)
Crash Data
Attachments
(1 file, 1 obsolete file)
2.80 KB,
patch
|
n.nethercote
:
review+
Sylvestre
:
approval-mozilla-aurora+
Sylvestre
:
approval-mozilla-beta+
Sylvestre
:
approval-mozilla-release+
Sylvestre
:
approval-mozilla-esr38+
|
Details | Diff | Splinter Review |
User Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:33.0) Gecko/20100101 Firefox/33.0 Build ID: 20141106120505 Steps to reproduce: After some heavy use of Firefox, I entered about:memory to check out the different memory usages. Actual results: Firefox crashed Firefox 33.1 Crash Report [@ StatsCellCallback<int> ] https://crash-stats.mozilla.com/report/index/7946894c-1f66-45d6-9e77-5b80f2141122 Expected results: Showing the results of memory measurement.
To clarify, I clicked the "Measure" button under "Show memory reports", and then the crash happened.
Comment 2•9 years ago
|
||
Can you reproduce this, in other words, does this happen every time you use that button in about:memory?
Crash Signature: [@ StatsCellCallback<int> ]
Component: Untriaged → JavaScript Engine
Flags: needinfo?(zeidspex)
Keywords: crash,
crashreportid
Product: Firefox → Core
Summary: Firefox crashes when measuring the memory in about:memory → Firefox crashes when measuring the memory in about:memory ( [@ StatsCellCallback<int> ] )
Assignee | ||
Comment 3•9 years ago
|
||
The JS memory reporting code iterates over every cell in the GC heap. If there's any kind of GC heap corruption there's a good chance it'll trip over it.
Comment 4•9 years ago
|
||
I can confirm this issue on the latest Aurora 38.0a2(buildID: 20150224004223) on Windows 7 64bit: after some heavy use of Firefox (high memory usage: ~1,4 GB RAM), I entered about:memory and click on Measure button -> crash occurs. Here is the crash report: https://crash-stats.mozilla.com/report/index/8f8eac43-c11f-43d5-a56b-d6a3e2150225
[Tracking Requested - why for this release]: This signature is in the top-10 on 37.0.1. There is high correlation with the Yandex toolbars: 100% (340/341) vs. 4% (2335/59423) yasearch@yandex.ru (Yandex.Bar, https://addons.mozilla.org/addon/3495) 91% (309/341) vs. 4% (2477/59423) vb@yandex.ru I'd normally send this to njn but he's away. Till are you the right person to fill in?
tracking-firefox38:
--- → ?
Flags: needinfo?(till)
Comment 6•9 years ago
|
||
> Till are you the right person to fill in?
I'm not, but maybe Terrence is. Looks like an invalid JSObject pointer.
Flags: needinfo?(till) → needinfo?(terrence)
Comment 7•9 years ago
|
||
Tracking this since it's a top crash.
Comment 8•9 years ago
|
||
Jorge, do we have contacts at yandex for their toolbar? There might be a bug on our side, but it's also a bit strange that the yandex toolbar shows up so prominently, so it might be a problem with the addon.
Flags: needinfo?(jorge)
Comment 9•9 years ago
|
||
I think Mike Connor has good contacts there.
Flags: needinfo?(jorge) → needinfo?(mconnor)
Comment 10•9 years ago
|
||
This is consistently reproducible for me with 38 beta: - Install Yandex Elements (successor to Yandex.Bar) from https://elements.yandex.com.tr - Go to about:memory - Click on Measure - Crash
Comment 11•9 years ago
|
||
David, is this happening because of a c++ binary the toolbar is injecting, or is this a bug in our JS engine that is exposed by the addon somehow?
Flags: needinfo?(dmajor)
Comment 12•9 years ago
|
||
I haven't seen any signs of binary injection, so I have to assume the latter.
Flags: needinfo?(dmajor)
Comment 13•9 years ago
|
||
This is too late for 38 but we can take a patch for 39.
Comment 14•9 years ago
|
||
in fact, it crashes not only when measuring in about:memory, but also in random moments of time.
Comment 15•9 years ago
|
||
It is unfortunate that this bug has been sitting for so long. Although it doesn't look so prominent on our overall crash reports, it's a severe crash for a subset of users: among locale==ru this is easily the #1 crash on release, more than twice the rate of OOM|small.
Comment 16•9 years ago
|
||
(In reply to David Major [:dmajor] from comment #15) > It is unfortunate that this bug has been sitting for so long. Although it > doesn't look so prominent on our overall crash reports, it's a severe crash > for a subset of users: among locale==ru this is easily the #1 crash on > release, more than twice the rate of OOM|small. This statement seems to validate comment 14, that this crash is not limited to the use of about:memory. If that's the case I think we need to prioritize this bug more highly. Is it expected that we need a fix from Yandex, in which case I'll follow up with mconnor, or is it that we think that we need a fix in Firefox? (Clearly a Firefox fix to prevent an add-on from causing this behaviour is a good idea either way.) Naveed - Can you please help prioritize this bug so that we can try to address the issue in the 39 cycle?
Flags: needinfo?(nihsanullah)
Assignee | ||
Comment 17•9 years ago
|
||
> This statement seems to validate comment 14, that this crash is not limited > to the use of about:memory. That's surprising. AFAIK calls to this function (StatsCellCallback()) should not be triggered outside of about:memory. Except maybe from https://dxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/utils/memory-bridge.js#344, but I would have thought that was obscure.
Comment 18•9 years ago
|
||
Reproduced on a fresh install of Windows 7 with FF38 as per comment 10.
Comment 19•9 years ago
|
||
@Lawrence I pinged GC team last night on this bug. Hopefully we will see quick progress since Jon can reproduce it now.
Flags: needinfo?(nihsanullah)
Comment 20•9 years ago
|
||
It seems that our memory reporters are causing JS to run while we're iterating the heap. This happens via calling methods on an nsIURI object in GetCompartmentName() and causes an assertion failure on debug builds:
> xul.dll!js::AssertHeapIsIdle(JSRuntime * rt) Line 195 C++
xul.dll!JS::AutoSaveExceptionState::AutoSaveExceptionState(JSContext * cx) Line 5295 C++
xul.dll!mozilla::Maybe<JS::AutoSaveExceptionState>::emplace<JSContext * &>(JSContext * & <aArgs_0>) Line 387 C++
xul.dll!AutoScriptEvaluate::StartEvaluating(JS::Handle<JSObject *> scope) Line 55 C++
xul.dll!nsXPCWrappedJSClass::CallMethod(nsXPCWrappedJS * wrapper, unsigned short methodIndex, const XPTMethodDescriptor * info_, nsXPTCMiniVariant * nativeParams) Line 954 C++
xul.dll!nsXPCWrappedJS::CallMethod(unsigned short methodIndex, const XPTMethodDescriptor * info, nsXPTCMiniVariant * params) Line 533 C++
xul.dll!PrepareAndDispatch(nsXPTCStubBase * self, unsigned int methodIndex, unsigned int * args, unsigned int * stackBytesToPop) Line 85 C++
xul.dll!SharedStub() Line 113 C++
xul.dll!xpc::CompartmentPrivate::GetLocation() Line 3701 C++
xul.dll!GetCompartmentName(JSCompartment * c, nsCString & name, int * anonymizeID, bool replaceSlashes) Line 1705 C++
xul.dll!xpc::XPCJSRuntimeStats::initExtraCompartmentStats(JSCompartment * c, JS::CompartmentStats * cstats) Line 2791 C++
xul.dll!StatsCompartmentCallback(JSRuntime * rt, void * data, JSCompartment * compartment) Line 345 C++
xul.dll!IterateCompartmentsArenasCells(JSRuntime * rt, JS::Zone * zone, void * data, void (JSRuntime *, void *, JSCompartment *) * compartmentCallback, void (JSRuntime *, void *, js::gc::Arena *, JSGCTraceKind, unsigned int) * arenaCallback, void (JSRuntime *, void *, void *, JSGCTraceKind, unsigned int) * cellCallback) Line 41 C++
xul.dll!js::IterateZonesCompartmentsArenasCells(JSRuntime * rt, void * data, void (JSRuntime *, void *, JS::Zone *) * zoneCallback, void (JSRuntime *, void *, JSCompartment *) * compartmentCallback, void (JSRuntime *, void *, js::gc::Arena *, JSGCTraceKind, unsigned int) * arenaCallback, void (JSRuntime *, void *, void *, JSGCTraceKind, unsigned int) * cellCallback) Line 73 C++
xul.dll!JS::CollectRuntimeStats(JSRuntime * rt, JS::RuntimeStats * rtStats, JS::ObjectPrivateVisitor * opv, bool anonymize) Line 728 C++
xul.dll!xpc::JSReporter::CollectReports(nsDataHashtable<nsUint64HashKey,nsCString> * windowPaths, nsDataHashtable<nsUint64HashKey,nsCString> * topWindowPaths, nsIMemoryReporterCallback * cb, nsISupports * closure, bool anonymize) Line 2884 C++
xul.dll!nsWindowMemoryReporter::CollectReports(nsIMemoryReporterCallback * aCb, nsISupports * aClosure, bool aAnonymize) Line 547 C++
I'm guessing that the toolbar changes something that causes a method to be implemented in JS.
Nick, any idea what the best way forward is? Maybe we do the compartment name lookup after we finish iterating the heap.
Assignee | ||
Comment 21•9 years ago
|
||
I can reproduce. Comment 20 is definitely bad. I'll look into a fix tomorrow.
Flags: needinfo?(zeidspex)
Flags: needinfo?(terrence)
Flags: needinfo?(mconnor)
Assignee | ||
Comment 22•9 years ago
|
||
So there's a general problem which is that add-ons can implement certain XPCOM interfaces via JS code, and then those interfaces get called while getting compartment names, which causes havoc. There is already some code in the JS reporter that is working around this fact, but the Yandex Elements add-on exposes two more cases. I don't have a good solution to avoid this problem in general, so for now I suggest we paper over the two new cases. That's enough to fix the obvious crashes obtained when clicking on "Measure" in about:memory when the add-on is installed. I can't promise that this will fix all the crashes -- see comment 17 -- but it will hopefully get rid of a lot.
Assignee | ||
Updated•9 years ago
|
Summary: Firefox crashes when measuring the memory in about:memory ( [@ StatsCellCallback<int> ] ) → Firefox crashes when measuring the memory in about:memory ( [@ StatsCellCallback<int> ] ) with Yandex Elements extension installed
Assignee | ||
Comment 23•9 years ago
|
||
We can't call JS code while iterating over the JS heap in the JS memory reporter. The Yandex Elements add-on is causing this in two cases. - The add-on implements some nsIURI objects. This one's easy to work around, because GetLocation() can just skip any JS-implemented nsIURI objects. - The add-on implements some nsIProtocolHandler objects in order to implement a custom "xb://" scheme. This one is harder to workaround because the call to the JS object's method occurs deep within NS_NewURI(), well beyond the JS reporter code. So we just skip "xb://" URLs.
Attachment #8604418 -
Flags: review?(mrbkap)
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → n.nethercote
Status: NEW → ASSIGNED
Assignee | ||
Comment 24•9 years ago
|
||
Nils, you should take note of this -- one of the two cases was triggered from GetLocationURI(), which I think you added.
Comment 25•9 years ago
|
||
(In reply to Nicholas Nethercote [:njn] from comment #23) > - The add-on implements some nsIProtocolHandler objects in order to > implement a custom "xb://" scheme. This one is harder to workaround because > the call to the JS object's method occurs deep within NS_NewURI(), well > beyond the JS reporter code. So we just skip "xb://" URLs. Won't this affect any JS-implemented protocolhandlers, not just the Yandex ones? Would it make sense to whitelist the URI schemes we're interested in (chrome, resource, file, jar) instead? Soon we won't have any binary XPCOM components in add-ons, so presumably any add-on provided protocol handler will be implemented in JS, so it seems like we might as well be restrictive here...
Assignee | ||
Comment 26•9 years ago
|
||
> Would it make sense to whitelist the URI schemes we're interested in
> (chrome, resource, file, jar) instead?
If you're confident that those are the only ones that need whitelisting and that any others that occur will be from add-ons, then it sounds like a good idea. (Is it likely that we'll add any new builtin URI schemes in the future?)
Comment 27•9 years ago
|
||
(In reply to Nicholas Nethercote [:njn] from comment #26) > > Would it make sense to whitelist the URI schemes we're interested in > > (chrome, resource, file, jar) instead? > > If you're confident that those are the only ones that need whitelisting and > that any others that occur will be from add-ons, then it sounds like a good > idea. (Is it likely that we'll add any new builtin URI schemes in the > future?) I mean, there *are* other builtin URI schemes, like moz-page-thumb and the favicon service's URLs, then there's "data:" and remote ones like http, https, ftp. I don't know much about memory reporting to know what the context is here. It just struck me that it should be easier. As best I can tell from code inspection of XPCJSRuntime.cpp, the only case where this function is used is for the source location of privileged JS. That should never be http/https/ftp or other remote things, so I am 90% sure data, file, jar, chrome and resource should cover everything. It might be wise to ask someone like bz/bholley to doublecheck that I'm not forgetting something, or add some kind of telemetry info about other protocols that we hit?
Comment 28•9 years ago
|
||
[Tracking Requested - why for this release]: This is the #9 crash on 38 release with 1.6% of all crashes, would be awesome if we'd get a fix in before we ship 38.0.5.
status-firefox38.0.5:
--- → affected
tracking-firefox38.0.5:
--- → ?
Comment 29•9 years ago
|
||
> This is the #9 crash on 38 release with 1.6% of all crashes
And 12.6% of release crashes for Russian-locale users.
Comment 30•9 years ago
|
||
Comment on attachment 8604418 [details] [diff] [review] Fix some crashes triggered from about:memory Review of attachment 8604418 [details] [diff] [review]: ----------------------------------------------------------------- This seems fine to me, though a whitelist (as suggested by Gijs would certainly be safer). One other idea would be to set a bit somewhere in XPConnect when we're iterating the JS heap and to refuse (in nsXPCWrappedJSClass) to call back into JS from C++ (we'd probably want to assert in debug builds as well as throwing in optimized builds) when the bit is set. That way, other bugs like this wouldn't crash us. ::: js/xpconnect/src/XPCJSRuntime.cpp @@ +287,5 @@ > + // calling into the extension's own JS-implemented nsIProtocolHandler > + // object, which we can't allow while we're iterating over the JS heap. > + // So just skip any such URL. > + // -- GROSS HACK ALERT -- > + if (StringBeginsWith(uristr, NS_LITERAL_CSTRING("xb"))) { Nit: No {} for single-line if statements in XPConnect. ::: js/xpconnect/src/xpcprivate.h @@ +3731,5 @@ > + // We cannot call into JS-implemented nsIURI objects, because > + // we are iterating over the JS heap at this point. > + location = > + NS_LITERAL_CSTRING("<JS-implemented nsIURI location>"); > + Nit: Nuke the extra newline here. Non-nit (above and below): location.AssignLiteral(...)
Attachment #8604418 -
Flags: review?(mrbkap) → review+
Assignee | ||
Comment 32•9 years ago
|
||
I landed the patch as-is because it's good enough to fix the crashes: https://hg.mozilla.org/integration/mozilla-inbound/rev/9193e0e55d64 I filed bug 1166209 about a more general fix.
Comment 33•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/9193e0e55d64
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox41:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Assignee | ||
Comment 35•9 years ago
|
||
(In reply to David Major [:dmajor] from comment #34) > Can we uplift this to 38.0.5? Yes. Do I need to do an approval-mozilla-release request?
Flags: needinfo?(n.nethercote)
Comment 36•9 years ago
|
||
Yes please. Request all the things!
Assignee | ||
Comment 37•9 years ago
|
||
This is the version that landed. It was slightly tweaked from the earlier version. Approval Request Comment [Feature/regressing bug #]: Yandex Elements addon reliably causes crashes when the "Measure" button is pressed in about:memory. [User impact if declined]: More crashes. [Describe test coverage new/current, TreeHerder]: Landed on m-i yesterday. [Risks and why]: Low. It's a small change that makes us take simpler code paths in two places. [String/UUID change made/needed]: None.
Attachment #8604418 -
Attachment is obsolete: true
Attachment #8608411 -
Flags: review+
Attachment #8608411 -
Flags: approval-mozilla-release?
Attachment #8608411 -
Flags: approval-mozilla-aurora?
Assignee | ||
Updated•9 years ago
|
Attachment #8608411 -
Flags: approval-mozilla-beta?
Updated•9 years ago
|
Comment 38•9 years ago
|
||
Comment on attachment 8608411 [details] [diff] [review] Fix some crashes triggered from about:memory Top crash #10 in the release. Taking it (in ESR38 too)
Attachment #8608411 -
Flags: approval-mozilla-release?
Attachment #8608411 -
Flags: approval-mozilla-release+
Attachment #8608411 -
Flags: approval-mozilla-esr38+
Attachment #8608411 -
Flags: approval-mozilla-beta?
Attachment #8608411 -
Flags: approval-mozilla-beta+
Attachment #8608411 -
Flags: approval-mozilla-aurora?
Attachment #8608411 -
Flags: approval-mozilla-aurora+
Comment 42•9 years ago
|
||
https://hg.mozilla.org/releases/mozilla-esr38/rev/fc53ba6df772
status-firefox-esr38:
--- → fixed
Updated•9 years ago
|
Flags: qe-verify+
Comment 43•9 years ago
|
||
Reproduced on 38.0b1 with steps from comment 10: * bp-69569266-771c-4db4-a12a-5f9d42150616 * bp-ffca3a20-49cf-4b2c-a599-c47622150616 No crash encountered with 39.0b6 (Build ID: 20150615125213) under Windows 7 64-bit and Windows 8.1 64-bit. After this fix, there is 1 crash for 39.0b3: https://crash-stats.mozilla.com/signature/?signature=StatsCellCallback%3Cint%3E&_columns=date&_columns=product&_columns=version&_columns=build_id&_columns=platform&_columns=reason&_columns=address&page=1 Since there is only 1 crash in Socorro and none with latest Beta build, marking as verified.
Comment 44•9 years ago
|
||
Also verified fixed with ESR 38.1.0 (Build ID: 20150624141534), latest 40.0a2 and 41.0a1 (both from 2015-06-25), under Windows 7 64-bit and Windows 8.1 64-bit.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Updated•9 years ago
|
tracking-firefox-esr38:
--- → 39+
Updated•9 years ago
|
relnote-firefox:
--- → 39+
You need to log in
before you can comment on or make changes to this bug.
Description
•