Closed
Bug 1280132
Opened 8 years ago
Closed 8 years ago
Assertion failure: [unhandlable oom] preparing weak cache sweeping task list
Categories
(Core :: JavaScript: GC, defect)
Core
JavaScript: GC
Tracking
()
RESOLVED
FIXED
mozilla50
People
(Reporter: cbook, Assigned: jonco)
References
()
Details
(Keywords: crash)
Attachments
(4 files, 1 obsolete file)
125.83 KB,
text/plain
|
Details | |
1.14 KB,
text/plain
|
Details | |
3.06 KB,
patch
|
terrence
:
review+
lizzard
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
3.23 KB,
patch
|
jonco
:
review+
Sylvestre
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
In bughunter we discovered opt crashes like : Assertion failure: [unhandlable oom] preparing weak cache sweeping task list with signatures like js::AutoEnterOOMUnsafeRegion::crash js::gc::GCRuntime::beginSweepingZoneGroup JSScript::traceChildren js::gcstats::AutoPhase::~AutoPhase js::gc::GCRuntime::endMarkingZoneGroup It seems that such crashes also happen in firefox beta opt. One url where this happend seems to be was http://wilmas.zoomin.se/blogg/allmant.html
Reporter | ||
Comment 1•8 years ago
|
||
Updated•8 years ago
|
Component: JavaScript Engine → JavaScript: GC
:jorendorff mentions to needinfo? :terrence as a start.
Flags: needinfo?(terrence)
Reporter | ||
Comment 3•8 years ago
|
||
bc: can we extract a list of urls where this crash/oom happens
Flags: needinfo?(bob)
Comment 4•8 years ago
|
||
You can get the list if you click on the fatal message, but I have also extracted the urls from the db directly. They are all very slow loading since they use a large amount of ram and I haven't finished reviewing them for nsfw content. If you want, I'll send them to you via email but talk to me before sharing. fwiw, I haven't been able to reproduce so for on win 7 32bit and nightly debug build.
Flags: needinfo?(bob)
Comment 5•8 years ago
|
||
This appears to require Spider and the userhook. It also occurs in an opt build which I don't understand. This does not occur in Firefox 47 but does in Firefox Beta 48 and Firefox Nightly 50. One of the urls reproduces on a Windows 7 32bit vm with 2G ram with the following: firefox -spider -start -quit -hook http://sisyphus.bughunter.ateam.phx1.mozilla.com/bughunter/media/userhooks/test-crash-on-load.js -url http://www.casadellibro.com/comunidad/lo-que-vendo/srcualquiera/7532498 You may need to repeat this several times to get it to reproduce. Get Spider from http://bclary.com/projects/spider/spider-0.1.0.3-sm+tb+fx+an+fn.xpi and the user hook is attached.
Assignee | ||
Comment 6•8 years ago
|
||
Patch to check for allocation failure while building the list of weak caches to sweep and sweep these on the main thread in that case. I changed the vector to use SystemAllocPolicy from the default of mozilla::MallocAllocPolicy because this supports simulated OOM testing. This is now exercised by our current tests.
Assignee: nobody → jcoppeard
Attachment #8763051 -
Flags: review?(terrence)
Assignee | ||
Updated•8 years ago
|
Flags: needinfo?(terrence)
Updated•8 years ago
|
Reporter | ||
Comment 7•8 years ago
|
||
[Tracking Requested - why for this release]: Bughunter OOM crash
Comment 8•8 years ago
|
||
Comment on attachment 8763051 [details] [diff] [review] bug1280132-sweep-oom Review of attachment 8763051 [details] [diff] [review]: ----------------------------------------------------------------- I'd like to see if the method I suggested works first. ::: js/src/jsgc.cpp @@ +4967,5 @@ > SweepCCWrappersTask sweepCCWrappersTask(rt); > SweepObjectGroupsTask sweepObjectGroupsTask(rt); > SweepRegExpsTask sweepRegExpsTask(rt); > SweepMiscTask sweepMiscTask(rt); > + bool haveSweepCacheTasks = true; Ugh, passing around a bool is a pretty awful code smell. How about we split this out into a separate method like: using WeakCacheTaskVector = mozilla::Vector<SweepWeakCacheTask, 0, SystemAllocPolicy>; WeakCacheTaskVector GCRuntime::prepareToSweepWeakCaches() { WeakCacheTaskVector out; for (GCZoneGroupIter zone(rt); !zone.done(); zone.next()) { for (JS::WeakCache<void*>* cache : zone->weakCaches_) { if (!out.append(SweepWeakCacheTask(rt, *cache))) { out.clear(); goto oom; } } } return out; oom: for (GCZoneGroupIter zone(rt); !zone.done(); zone.next()) { for (JS::WeakCache<void*>* cache : zone->weakCaches_) { SweepWeakCacheTask task(rt, *cache); task.runFromMainThread(rt); } } return WeakCacheTaskVector(); }
Attachment #8763051 -
Flags: review?(terrence)
Comment 9•8 years ago
|
||
You could avoid the goto with something like WeakCacheTaskVector GCRuntime::prepareToSweepWeakCaches() { WeakCacheTaskVector out; auto foregroundSweepOnOOM = [] { for (GCZoneGroupIter zone(rt); !zone.done(); zone.next()) { for (JS::WeakCache<void*>* cache : zone->weakCaches_) { SweepWeakCacheTask task(rt, *cache); task.runFromMainThread(rt); } } }; for (GCZoneGroupIter zone(rt); !zone.done(); zone.next()) { for (JS::WeakCache<void*>* cache : zone->weakCaches_) { if (!out.append(SweepWeakCacheTask(rt, *cache))) { out.clear(); foregroundSweepOnOOM(); return out; } } } return out; } but I admit it does put the error handling first, which is kind of weird.
Assignee | ||
Comment 10•8 years ago
|
||
> passing around a bool is a pretty awful code smell
Agreed. Thanks for the suggestions. Hopefully this is better.
Attachment #8763051 -
Attachment is obsolete: true
Attachment #8763829 -
Flags: review?(terrence)
Comment 11•8 years ago
|
||
Comment on attachment 8763829 [details] [diff] [review] bug1280132-sweep-oom v2 Review of attachment 8763829 [details] [diff] [review]: ----------------------------------------------------------------- Much nicer!
Attachment #8763829 -
Flags: review?(terrence) → review+
Comment 13•8 years ago
|
||
Pushed by jcoppeard@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/b29c44f5d909 Add fallback path in case we hit OOM while creating the list of weak cache sweeping tasks r=terrence
Reporter | ||
Comment 14•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/b29c44f5d909
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Comment 15•8 years ago
|
||
Is this a fix you would want to uplift, or would it be better to keep this fix on 50? We certainly would love to bring down the number of OOM crashes, but I can't judge the risk vs. possible impact from the comments here. Jon, terrence, what do you think?
Flags: needinfo?(terrence)
Flags: needinfo?(jcoppeard)
Assignee | ||
Comment 16•8 years ago
|
||
I think this is low risk, so let's uplift.
Assignee | ||
Comment 17•8 years ago
|
||
Comment on attachment 8763829 [details] [diff] [review] bug1280132-sweep-oom v2 Approval Request Comment [Feature/regressing bug #]: Bug 1263769. [User impact if declined]: Possible OOM crash. [Describe test coverage new/current, TreeHerder]: On m-c for the last week. [Risks and why]: Low. [String/UUID change made/needed]: None.
Attachment #8763829 -
Flags: approval-mozilla-aurora?
Assignee | ||
Comment 18•8 years ago
|
||
Approval Request Comment [Feature/regressing bug #]: Bug 1263769. [User impact if declined]: Possible OOM crash. [Describe test coverage new/current, TreeHerder]: On m-c for the last week. [Risks and why]: Low. [String/UUID change made/needed]: None.
Attachment #8766707 -
Flags: review+
Attachment #8766707 -
Flags: approval-mozilla-beta?
Comment 19•8 years ago
|
||
Comment on attachment 8763829 [details] [diff] [review] bug1280132-sweep-oom v2 Potential OOM crash fix, and we're still pretty early in the aurora cycle, let's uplift.
Attachment #8763829 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 20•8 years ago
|
||
Comment on attachment 8766707 [details] [diff] [review] bug1280132-beta Fix an oom, taking it.
Attachment #8766707 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment 21•8 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-aurora/rev/ddf1c2c6a9e3
Comment 22•8 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/c134bf637b15
You need to log in
before you can comment on or make changes to this bug.
Description
•