342180 - crash in js_MarkScriptFilenames on startup with WAY_TOO_MUCH_GC with null rt->scriptFilenamePrefixes

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Each time I update and try to run my WAY_TOO_MUCH_GC build, I crash earlier.  Now I'm crashing on startup, within the first (presumably) call to js_NewContext, before js_InitRuntimeScriptState has been called:

(gdb) bt
#0  js_MarkScriptFilenames (rt=0x62da30, gcflags=0)
    at /home/dbaron/builds/trunk/mozilla/js/src/jsscript.c:1219
#1  0x00002aaaaaea74d5 in js_GC (cx=0x569e90, gcflags=2)
    at /home/dbaron/builds/trunk/mozilla/js/src/jsgc.c:2413
#2  0x00002aaaaaea7fee in js_ForceGC (cx=0x569e90, gcflags=2)
    at /home/dbaron/builds/trunk/mozilla/js/src/jsgc.c:2128
#3  0x00002aaaaae8707d in js_DestroyContext (cx=0x569e90, gcmode=JS_NO_GC)
    at /home/dbaron/builds/trunk/mozilla/js/src/jscntxt.c:340
#4  0x00002aaaaae8745c in js_NewContext (rt=0x62da30, stackChunkSize=256)
    at /home/dbaron/builds/trunk/mozilla/js/src/jscntxt.c:253
#5  0x00002aaab41e1050 in mozJSComponentLoader::ReallyInit (this=0x559bd0)
    at /home/dbaron/builds/trunk/mozilla/js/src/xpconnect/loader/mozJSComponentLoader.cpp:425
#6  0x00002aaab41e145a in mozJSComponentLoader::LoadModule (this=0x559bd0,
    aComponentFile=0x558210, aResult=0x7fffff8d51e0)
    at /home/dbaron/builds/trunk/mozilla/js/src/xpconnect/loader/mozJSComponentLoader.cpp:486
#7  0x00002aaaab2498db in nsComponentManagerImpl::AutoRegisterComponent (
    this=0x534b80, aComponentFile=0x558210, aDeferred=@0x7fffff8d5350,
    minLoader=0)
    at /home/dbaron/builds/trunk/mozilla/xpcom/components/nsComponentManager.cpp:3030
#8  0x00002aaaab249bdb in nsComponentManagerImpl::LoadLeftoverComponents (
    this=0x534b80, aLeftovers=@0x7fffff8d5360, aDeferred=@0x7fffff8d5350,
    minLoader=1)
    at /home/dbaron/builds/trunk/mozilla/xpcom/components/nsComponentManager.cpp:3085
#9  0x00002aaaab24b8e2 in nsComponentManagerImpl::AutoRegister (this=0x534b80,
    aSpec=0x0)
    at /home/dbaron/builds/trunk/mozilla/xpcom/components/nsComponentManager.cpp:3333
#10 0x00002aaaab1faf45 in NS_InitXPCOM3_P (result=0x7fffff8d58e0, binDirectory=Variable "binDirectory" is not available.

) at /home/dbaron/builds/trunk/mozilla/xpcom/build/nsXPComInit.cpp:637
#11 0x00002aaaaad12f7a in ScopedXPCOMStartup::Initialize (this=0x7fffff8d58e0)
    at /home/dbaron/builds/trunk/mozilla/toolkit/xre/nsAppRunner.cpp:595
#12 0x00002aaaaad15b5c in XRE_main (argc=Variable "argc" is not available.
)
    at /home/dbaron/builds/trunk/mozilla/toolkit/xre/nsAppRunner.cpp:2189
#13 0x00002aaaac9d8e54 in __libc_start_main (main=0x4006d0 <main>, argc=4,
    ubp_av=0x7fffff8d5ad8, init=Variable "init" is not available.
) at libc-start.c:231


(gdb) f 0
#0  js_MarkScriptFilenames (rt=0x62da30, gcflags=0)
    at /home/dbaron/builds/trunk/mozilla/js/src/jsscript.c:1219
1219            js_MarkScriptFilename(sfp->name);
(gdb) p rt->scriptFilenamePrefixes
$1 = {next = 0x0, prev = 0x0}


This code doesn't seem to be tolerant of rt->scriptFilenamePrefixes being null (since PR_INIT_CLIST hasn't been called yet).

I have no idea why I didn't hit this before.

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(I'm guessing I didn't hit it before because something is causing |ok| to be false, thus causing the js_DestroyContext call.)

Comment 2

[Brendan Eich [:brendan]]

Attachment: fix

Just on modularity principles, jsscript.c code has to defend here.  Note how js_FinishRuntimeScriptState already does.

/be

Comment 3

[Brendan Eich [:brendan]]

Fixed.

/be

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

The underlying problem is hitting the null return in js_NewGCThing:

 992     if (doGC && !rt->gcClosePhase) {
 993         /*
 994          * Keep rt->gcLock across the call into js_GC so we don't starve
 995          * and lose to racing threads who deplete the heap just after
 996          * js_GC has replenished it (or has synchronized with a racing
 997          * GC that collected a bunch of garbage).  This unfair scheduling
 998          * can happen on certain operating systems. For the gory details,
 999          * see bug 162779 at https://bugzilla.mozilla.org/.
1000          */
1001         if (!js_GC(cx, GC_KEEP_ATOMS | GC_LAST_DITCH)) {
1002             /* js_GC ensures that GC is unlocked after GC was canceled. */
1003             return NULL;
1004         }
1005         METER(rt->gcStats.retry++);
1006     }


#0  js_NewGCThing (cx=0x569d90, flags=1, nbytes=Variable "nbytes" is not available.
)
    at /home/dbaron/builds/trunk/mozilla/js/src/jsgc.c:1004
#1  0x00002aaaaaef96f2 in js_NewString (cx=0x569d90, chars=0x558fd0, length=9,
    gcflag=Variable "gcflag" is not available.
) at /home/dbaron/builds/trunk/mozilla/js/src/jsstr.c:2437
#2  0x00002aaaaaef9888 in js_NewStringCopyN (cx=0x569d90, s=0x7fffffb54a00,
    n=9, gcflag=0) at /home/dbaron/builds/trunk/mozilla/js/src/jsstr.c:2552
#3  0x00002aaaaae83feb in js_AtomizeString (cx=0x569d90, str=Variable "str" is not available.
)
    at /home/dbaron/builds/trunk/mozilla/js/src/jsatom.c:654
#4  0x00002aaaaae84153 in js_Atomize (cx=0x569d90, bytes=Variable "bytes" is not available.
)
    at /home/dbaron/builds/trunk/mozilla/js/src/jsatom.c:734
#5  0x00002aaaaae84520 in js_InitPinnedAtoms (cx=0x569d90, state=0x62dab8)
    at /home/dbaron/builds/trunk/mozilla/js/src/jsatom.c:297
#6  0x00002aaaaae84e08 in js_InitAtomState (cx=0x569d90, state=0x62dab8)
    at /home/dbaron/builds/trunk/mozilla/js/src/jsatom.c:278
#7  0x00002aaaaae8743f in js_NewContext (rt=0x62d930, stackChunkSize=256)
    at /home/dbaron/builds/trunk/mozilla/js/src/jscntxt.c:240


and that problem is probably still a problem...

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

And that's because of:

Breakpoint 2, js_GC (cx=0x569d90, gcflags=Variable "gcflags" is not available.
)
    at /home/dbaron/builds/trunk/mozilla/js/src/jsgc.c:2241
2241            if (gcflags & GC_LAST_DITCH)

here:

#0  js_GC (cx=0x569d90, gcflags=Variable "gcflags" is not available.
)
    at /home/dbaron/builds/trunk/mozilla/js/src/jsgc.c:2241
#1  0x00002aaaaaea8c8a in js_NewGCThing (cx=0x569d90, flags=1, nbytes=Variable "nbytes" is not available.
)
    at /home/dbaron/builds/trunk/mozilla/js/src/jsgc.c:1001
#2  0x00002aaaaaef9632 in js_NewString (cx=0x569d90, chars=0x558fd0, length=9,
    gcflag=Variable "gcflag" is not available.
) at /home/dbaron/builds/trunk/mozilla/js/src/jsstr.c:2437
#3  0x00002aaaaaef97c8 in js_NewStringCopyN (cx=0x569d90, s=0x7fffffa1d220,
    n=9, gcflag=0) at /home/dbaron/builds/trunk/mozilla/js/src/jsstr.c:2552
#4  0x00002aaaaae83f8e in js_AtomizeString (cx=0x569d90, str=0x7fffffa1d270,
    flags=129) at /home/dbaron/builds/trunk/mozilla/js/src/jsatom.c:653
#5  0x00002aaaaae840c3 in js_Atomize (cx=0x569d90, bytes=Variable "bytes" is not available.
)
    at /home/dbaron/builds/trunk/mozilla/js/src/jsatom.c:728
#6  0x00002aaaaae84490 in js_InitPinnedAtoms (cx=0x569d90, state=0x62dab8)
    at /home/dbaron/builds/trunk/mozilla/js/src/jsatom.c:297
#7  0x00002aaaaae84d78 in js_InitAtomState (cx=0x569d90, state=0x62dab8)
    at /home/dbaron/builds/trunk/mozilla/js/src/jsatom.c:278
#8  0x00002aaaaae873af in js_NewContext (rt=0x62d930, stackChunkSize=256)
    at /home/dbaron/builds/trunk/mozilla/js/src/jscntxt.c:240

causing js_GC to return false, which makes js_NewGCThing fail, which propagates back to js_NewContext.

Note that with the previous patch the eventual cause of the crash is the GC_LAST_CONTEXT GC, crashing inside js_SweepScriptFilenames because rt->scriptFilenameTable is null.

Comment 6

[Brendan Eich [:brendan]]

I didn't take this bug when I patched it because I'm supposedly on vacation.  So I hope you'll understand that I'm finding it hard to justify making more time for it (much as I would like to :-/).  Igor, can you knock this down?  It may just be a matter of null-defense in js_SweepScriptFilenames.

/be

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

The null-defense would be nice, but something also needs to fix the fact that the first js_NewContext fails with WAY_TOO_MUCH_GC.

Comment 8

[Igor Bukanov]

Attachment: Fix for sweep

A trivial NULL protection is enough to fix this.

Comment 9

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 226796 [details] [diff] [review]
Fix for sweep

So this is a worthy patch, but at the risk of repeating dbaron, this isn't enough to fix this bug.

The bug is that if we run a WAY_TOO_MUCH_GC GC on the first allocation, then we aren't able to create the first context. These crashes are all during the GC that occurs during js_DestroyContext, which we shouldn't be in.

Comment 10

[Igor Bukanov]

(In reply to comment #9)
> The bug is that if we run a WAY_TOO_MUCH_GC GC on the first allocation, then we
> aren't able to create the first context. These crashes are all during the GC
> that occurs during js_DestroyContext, which we shouldn't be in.

Accessing NULL pointer in js_SweepScriptFilenames AFAICS is the last hurdle that prevents GC running when no GC-related things was allocated. Surely not running GC in such situation would also solve the bug but that would mean one more case to consider.

Perhaps a better approach would be to make sure that js_InitRuntimeScriptState is called before js_GC but checking for NULL is also good enough IMO.

Comment 11

[Igor Bukanov]

I committed the patch from comment 8.

Comment 12

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

OK, I'll file another bug about this bug.

Comment 13

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Filed bug 342625.