369714 - Thunderbird crash on startup [@js_HashString()]

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Wladimir Palant]

I got a bug report about Thunderbird crashing on startup under certain conditions. These conditions are: Mac OS X 10.3.9, Thunderbird 1.5.0.9, Adblock Plus 0.7.2.4, FlashGot 0.5.97.03 and a specific patterns.ini that will be loaded by Adblock Plus on startup. The crash is stable:

TB29095135Z
TB29095158G
TB29095216X
TB29095954K
TB29096078K

Here is the stack from the talkback reports:

js_HashString()  [mozilla/js/src/jsstr.c, line 2825]
js_AtomizeString()  [mozilla/js/src/jsatom.c, line 651]
fun_xdrObject()  [mozilla/js/src/jsfun.c, line 1350]
js_XDRObject()  [mozilla/js/src/jsobj.c, line 4323]
JS_XDRValue()  [mozilla/js/src/jsxdrapi.c, line 544]
XDRAtomListElement()  [mozilla/js/src/jsscript.c, line 351]
XDRAtomMap()  [mozilla/js/src/jsscript.c, line 385]
js_XDRScript()  [mozilla/js/src/jsscript.c, line 519]
JS_XDRScript()  [mozilla/js/src/jsxdrapi.c, line 579]
nsXULPrototypeScript::Deserialize()  [mozilla/content/xul/content/src/nsXULElement.cpp, li]
nsXULPrototypeScript::DeserializeOutOfLine()  [mozilla/content/xul/content/src/nsXULElemen]
nsXULPrototypeElement::Deserialize()  [mozilla/content/xul/content/src/nsXULElement.cpp, l]
nsXULPrototypeDocument::Read()  [mozilla/content/xul/document/src/nsXULPrototypeDocument.]
nsXULPrototypeCache::GetPrototype()  [mozilla/content/xul/document/src/nsXULPrototypeCach]
nsChromeProtocolHandler::NewChannel()  [mozilla/chrome/src/nsChromeProtocolHandler.cpp, line 842]
nsIOService::NewChannelFromURI()
NS_NewChannel()  [mozilla/mailnews/mime/src/mimecth.cpp, line 172]
nsDocShell::DoURILoad()
nsDocShell::InternalLoad()
nsDocShell::LoadURI()
nsDocShell::LoadURI()
nsWebShellWindow::Initialize()
nsAppShellService::JustCreateTopWindow()
nsAppShellService::CreateHiddenWindow()
nsAppStartup::CreateHiddenWindow()
XRE_main()  [mozilla/toolkit/xre/nsAppRunner.cpp, line 2271]
_start()   start()

Not sure this crash can be reproduced but I will attach the patterns.ini file I was sent (should be placed into the adblockplus directory in the profile). Since this seems to depend on the objects that Adblock Plus creates and since js_HashString clearly gets a wrong memory location from somewhere I am marking this security sensitive just to be sure.

Comment 1

[Wladimir Palant]

Attachment: patterns.ini file required for the crash (gzipped)

Comment 2

[Wladimir Palant]

Tried to reproduce this crash, didn't work - neither on Mac OS X nor on Windows. Anybody able to make an educated guess from the stack trace?

Comment 3

[timeless]

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/js/src/jsfun.c&mark=1224,1228,1297,1349&rev=THUNDERBIRD_1_5_0_9_RELEASE#1224

personally, i'd be asking what protects atomstr from dying, especially given that, i think propname could evict it.

Comment 4

[Brendan Eich [:brendan]]

propname is initialized from JS_GetStringBytes, which deflates a string into the deflated string cache, but does not allocate a GC-thing. So atomstr still should be hanging by a thin reed. But it's a long way from where atomstr is created to where it's atomized, so yeah: JS_PUSH_TEMP_ROOT_STRING should be used.

Cc'ing folks who should fight to take this bug.

/be

Comment 5

[Brian Crowder]

I will take this one as soon as I am done with the array bug.

Comment 6

[Brian Crowder]

Attachment: add a root for atomstring

I don't have a working thunderbird build to test this with, so help would be appreciated.

Comment 7

[Brendan Eich [:brendan]]

Comment on attachment 256972 [details] [diff] [review]
add a root for atomstring

Don't add string to JSTempValueUnion. Do consolidate tvr per C function, there's no need for two.

Igor should be reviewer of choice on such patches, so I don't delay response and cuz he's tvr meister.

/be

Comment 8

[Brian Crowder]

Attachment: removing unnecessary tvr

Brendan:  I added JSString to the JSTempValueUnion structure because the macro rooting macro here which was ported from the trunk relies on it, and the macro/type-safe union seems cleaner than using a cast or void *.  Hopefully you or Igor will correct me if that is still not correct.  I have added the JSString * to the union in the same place as it is added on the trunk.

Comment 9

[Igor Bukanov]

Comment on attachment 259144 [details] [diff] [review]
removing unnecessary tvr


>     /* From here on, control flow must flow through label out. */
>     JS_PUSH_SINGLE_TEMP_ROOT(cx, OBJECT_TO_JSVAL(fun->object), &tvr);
>+    JS_PUSH_TEMP_ROOT_STRING(cx, atomstr, &tvr);
>     ok = JS_TRUE;

tvr stores the linked list next link so one can not use 2 pushes per one tvr. You need either 2 JSTempValueRooter instances here or better yet one tvr and a jsval[2] array together with code that populates it like in:

jsval roots[2];
...

roots[0] = OBJECT_TO_JSVAL(fun->object);
roots[1] = STRING_TO_JSVAL(atomstr);
JS_PUSH_TEMP_ROOT(cx, JS_ARRAY_LENGTH(roots), roots, &tvr);

Comment 10

[Brian Crowder]

Attachment: jsval array instead of multiple tvrs

Thanks for the explanation, Igor, I started with two tvrs in my original patch; is there a reason one tvr with a jsval array is superior, or is that primarily a style issue?

Comment 11

[Igor Bukanov]

(In reply to comment #10)
> Created an attachment (id=259156) [details]
> jsval array instead of multiple tvrs
> 
> Thanks for the explanation, Igor, I started with two tvrs in my original patch;
> is there a reason one tvr with a jsval array is superior, or is that primarily
> a style issue?

tvr with jsval[2] just brings less code as the extra linking/unlinking of tvr from the JSContext list is avoided.

Comment 12

[Igor Bukanov]

Comment on attachment 259156 [details] [diff] [review]
jsval array instead of multiple tvrs

>     /* From here on, control flow must flow through label out. */
>-    JS_PUSH_SINGLE_TEMP_ROOT(cx, OBJECT_TO_JSVAL(fun->object), &tvr);
>+    roots[0] = OBJECT_TO_JSVAL(fun->object);
>+    roots[1] = STRING_TO_JSVAL(atomstr);
>+    JS_PUSH_TEMP_ROOT(cx, sizeof(roots) / sizeof(roots[0]), roots, &tvr);
>     ok = JS_TRUE;
> 
>     if (!JS_XDRStringOrNull(xdr, &atomstr) ||

Sorry I did not see it the first time, but after decoding atomstr would contain a new string. It is unrooted as roots[1] contains unrelated value. So lets add a second tvr with JSString* member added and then use atomstrTvr.u.string, not atomstr in the rest of code.

Comment 13

[Brian Crowder]

Attachment: two tvrs again, and modified string usage

I like this patch less (accessing the union elements is ugly), I'd almost rather keep the jsval array and just set the string after the XDR decode.  Thoughts?  Also, I could of course remove atomstr entirely, but again it looks uglier.

Comment 14

[Igor Bukanov]

Comment on attachment 259226 [details] [diff] [review]
two tvrs again, and modified string usage

Yes, this is ugly, but at least it works. I also agree that atomizing the string right after JS_XDRStringOrNull and storing it as fun->atom ASAP would also work.

Comment 15

[Brian Crowder]

(In reply to comment #14)
> (From update of attachment 259226 [details] [diff] [review])
> I also agree that atomizing the string right after JS_XDRStringOrNull
> and storing it as fun->atom ASAP would also work. 

Do you mind explaining this part in more depth?

Comment 16

[Igor Bukanov]

(In reply to comment #15)
> (In reply to comment #14)
> > (From update of attachment 259226 [details] [diff] [review] [details])
> > I also agree that atomizing the string right after JS_XDRStringOrNull
> > and storing it as fun->atom ASAP would also work. 
> 
> Do you mind explaining this part in more depth?

The idea is to move 

    if (xdr->mode == JSXDR_DECODE) {
...
        if (atomstr) {
             /* XXX only if this was a top-level function! */
             fun->atom = js_AtomizeString(cx, atomstr, 0);
             if (!fun->atom)
                 goto bad;
         }
   }

right after JS_XDRStringOrNull(xdr, &atomstr). It this way there is no need to protect atomstr against GC.

Comment 17

[Brendan Eich [:brendan]]

Please avoid atomstrTvr type names -- too many consonants in a row. atomtvr would be nicer, but comment 16 has the better idea.

/be

Comment 18

[Brian Crowder]

Attachment: atomize the string earlier

I like this version best so far.  And, I'm pretty sure that moving the majority of this chunk up is safe.

Comment 19

[Brian Crowder]

The trunk code is way different, so this bug shouldn't be a 1.9 blocker.

Gonna try to test this out with tbird now.

Comment 20

[Brian Crowder]

Comment on attachment 259421 [details] [diff] [review]
atomize the string earlier

I believe this bug is either 1.8.0-only, or would present very differently elsewhere.

Comment 21

[Daniel Veditz [:dveditz]]

Comment on attachment 259421 [details] [diff] [review]
atomize the string earlier

approved for 1.8.0.12, a=dveditz for release-drivers

Comment 22

[Brian Crowder]

jsfun.c: 3.117.2.7.2.13

Comment 23

[Daniel Veditz [:dveditz]]

That's fixed1.8.0.12, right?

Comment 24

[Brian Crowder]

Yes, though in communicating with an external embedder, I'm thinking I may have just moved this or discovered another bug nearby.  I will post more when I have had time to work it out with them.

Comment 25

[Daniel Veditz [:dveditz]]

guessing sg:moderate, but could be sg:critical if there's a reliable web-content testcase that doesn't require particular extensions.