1280132 - Assertion failure: [unhandlable oom] preparing weak cache sweeping task list

Status: RESOLVED FIXED

(Core :: JavaScript: GC, defect)

Description

[Carsten Book [:Tomcat]]

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

Comment 1

[Carsten Book [:Tomcat]]

Attachment: bughunter crash report

Comment 2

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

:jorendorff mentions to needinfo? :terrence as a start.

Comment 3

[Carsten Book [:Tomcat]]

bc: can we extract a list of urls where this crash/oom happens

Comment 4

[Bob Clary [:bc] (inactive)]

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.

Comment 5

[Bob Clary [:bc] (inactive)]

Attachment: test-crash-on-load.js

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.

Comment 6

[Jon Coppeard (:jonco)]

Attachment: bug1280132-sweep-oom

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.

Comment 7

[Carsten Book [:Tomcat]]

[Tracking Requested - why for this release]:

Bughunter OOM crash

Comment 8

[Terrence Cole [:terrence]]

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();
}

Comment 9

[Steve Fink [:sfink] [:s:]]

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.

Comment 10

[Jon Coppeard (:jonco)]

Attachment: bug1280132-sweep-oom v2

> passing around a bool is a pretty awful code smell

Agreed.  Thanks for the suggestions.  Hopefully this is better.

Comment 11

[Terrence Cole [:terrence]]

Comment on attachment 8763829 [details] [diff] [review]
bug1280132-sweep-oom v2

Review of attachment 8763829 [details] [diff] [review]:
-----------------------------------------------------------------

Much nicer!

Comment 13

[Pulsebot]

Pushed by [e-mail]:
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

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b29c44f5d909

Comment 15

[Liz Henry (:lizzard) (relman/hg->git project)]

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?

Comment 16

[Jon Coppeard (:jonco)]

I think this is low risk, so let's uplift.

Comment 17

[Jon Coppeard (:jonco)]

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.

Comment 18

[Jon Coppeard (:jonco)]

Attachment: bug1280132-beta

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.

Comment 19

[Liz Henry (:lizzard) (relman/hg->git project)]

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.

Comment 20

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8766707 [details] [diff] [review]
bug1280132-beta

Fix an oom, taking it.

Comment 21

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/ddf1c2c6a9e3

Comment 22

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-beta/rev/c134bf637b15

Comment 23

[Gerry Chang [:gchang]]

Track this as this is oom issue ans it's in 48.