299593 - Invalid read of size 4 in anyname_finalize

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Andrew Schultz]

At startup valgrind sees the folowing in suite trunk:

Invalid read of size 4
 anyname_finalize (jsxml.c:406)
 js_FinalizeObject (jsobj.c:2044)
 js_GC (jsgc.c:1831)
 js_ForceGC (jsgc.c:1502)
 js_DestroyContext (jscntxt.c:284)
 JS_DestroyContext (jsapi.c:945)
 XPCJSContextStack::SetSafeJSContext(JSContext*) (xpcthreadcontext.cpp:188)
 nsXPCThreadJSContextStackImpl::SetSafeJSContext(JSContext*)
(xpcthreadcontext.cpp:344)
 nsAppShellService::SetXPConnectSafeContext() (nsAppShellService.cpp:121)
 nsAppShellService::CreateHiddenWindow(nsIAppShell*) (nsAppShellService.cpp:195)
 nsAppStartup::CreateHiddenWindow() (nsAppStartup.cpp:152)
 main1(int, char**, nsISupports*) (nsAppRunner.cpp:1175)
Address 0x34705C70 is 8 bytes inside a block of size 24 free'd
 free (vg_replace_malloc.c:152)
 js_free_atom (jsatom.c:252)
 JS_HashTableRawRemove (jshash.c:280)
 JS_HashTableEnumerateEntries (jshash.c:382)
 js_SweepAtomState (jsatom.c:493)
 js_GC (jsgc.c:1804)
 js_ForceGC (jsgc.c:1502)
 js_DestroyContext (jscntxt.c:284)
 JS_DestroyContext (jsapi.c:945)
 XPCJSContextStack::SetSafeJSContext(JSContext*) (xpcthreadcontext.cpp:188)
 nsXPCThreadJSContextStackImpl::SetSafeJSContext(JSContext*)
(xpcthreadcontext.cpp:344)
 nsAppShellService::SetXPConnectSafeContext() (nsAppShellService.cpp:121)

the error is occuring in code from the last attachment of bug 294893.  valgrind
does not complain with builds from 6/30 or before.  The actual piece of memory
it's not happy about is 

ATOM_KEY(rt->atomState.lazy.anynameAtom)

Comment 1

[Brendan Eich [:brendan]]

Attachment: attempt at fix

Is the suite using some kind of turbo, or shutdown-all-JSContexts mode?  Or
maybe this is just finalization order differing between Firefox and the suite? 
Anyway, if this patch works, please comment and I'll check it in.

/be

Comment 2

[Andrew Schultz]

Attachment: debug patch

> Is the suite using some kind of turbo, or shutdown-all-JSContexts mode?  Or
> maybe this is just finalization order differing between Firefox and the
suite?

I tested firefox and it has the same problem.  Also, this is at startup rather
than shutdown.

> Anyway, if this patch works, please comment and I'll check it in.

That won't help because anynameAtom isn't NULL.  The problem is that
|anynameAtom->entry| was already freed.  Apply the attached patch.  At startup
it will say "oops".

Comment 3

[Brendan Eich [:brendan]]

Attachment: fix

Double-review so bz can join the club, and to share the love.

rt->atomState.lazy.* must be strong refs to lazily *pinned* atoms (there's no
such thing as an engine-supported weak ref to an atom, really).  But for
anyname and functionNamespace, we don't need or want atoms as middlemen -- we
just want weak, lazily-set refs from the runtime to these two objects.

/be

Comment 4

[Mike Shaver (:shaver emeritus)]

Comment on attachment 188386 [details] [diff] [review]
fix

> JSBool
> js_GetFunctionNamespace(JSContext *cx, jsval *vp)
> {
>     JSRuntime *rt;
>+    JSObject *obj;
>     JSAtom *atom;
>     JSString *prefix, *uri;
>-    JSObject *obj;
> 
>     /* An invalid URI, for internal use only, guaranteed not to collide. */
>     static const char anti_uri[] = "@mozilla.org/js/function";
> 
>     rt = cx->runtime;
>-    atom = rt->atomState.lazy.functionNamespaceAtom;
>-    if (!atom) {
>+    obj = rt->functionNamespaceObject;
>+    if (!obj) {
>         atom = js_Atomize(cx, js_function_str, 8, 0);
>         JS_ASSERT(atom);
>         prefix = ATOM_TO_STRING(atom);
> 
>         atom = js_Atomize(cx, anti_uri, sizeof anti_uri - 1, ATOM_PINNED);
>         if (!atom)
>             return JS_FALSE;
>         rt->atomState.lazy.functionNamespaceURIAtom = atom;
> 
>         uri = ATOM_TO_STRING(atom);
>         obj = js_NewXMLNamespaceObject(cx, prefix, uri, JS_FALSE);
>         if (!obj)
>             return JS_FALSE;
> 
>-        atom = js_AtomizeObject(cx, obj, 0);
>-        if (!atom)
>-            return JS_FALSE;
>-        rt->atomState.lazy.functionNamespaceAtom = atom;
>+        rt->functionNamespaceObject = obj;
>     }
>     *vp = ATOM_KEY(atom);
>     return JS_TRUE;
> }

On the second run through, when |obj| is non-null, it looks like we'll use an
uninitialized |atom| to fill |*vp|.

Comment 5

[Brendan Eich [:brendan]]

Comment on attachment 188386 [details] [diff] [review]
fix

Wrong patch, forgot to rediff after fixing.

/be

Comment 6

[Brendan Eich [:brendan]]

Attachment: the right fix

Pre-approving, ever the optimist ;-).  Actually, I'd already tested this with
the e4x suite and by hand (e4x suite doesn't use
<xml/>.function::toXMLString(), e.g.) and all was well.  Just didn't update the
uploaded diff.

/be

Comment 7

[Mike Shaver (:shaver emeritus)]

Comment on attachment 188390 [details] [diff] [review]
the right fix

r=shaver.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 188390 [details] [diff] [review]
the right fix

Ah, this is much happier!

Comment 9

[Brendan Eich [:brendan]]

Fixed.

/be