Last Comment Bug 369714 - Thunderbird crash on startup [@js_HashString()]
: Thunderbird crash on startup [@js_HashString()]
Status: RESOLVED FIXED
[sg:moderate?]
: fixed1.8.0.12
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: 1.8 Branch
: All All
: -- critical (vote)
: ---
Assigned To: Brian Crowder
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2007-02-08 01:31 PST by Wladimir Palant
Modified: 2007-05-30 15:35 PDT (History)
7 users (show)
dveditz: wanted1.8.1.x-
bob: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patterns.ini file required for the crash (gzipped) (77.79 KB, application/octet-stream)
2007-02-08 01:33 PST, Wladimir Palant
no flags Details
add a root for atomstring (2.86 KB, patch)
2007-03-01 16:24 PST, Brian Crowder
brendan: review-
Details | Diff | Review
removing unnecessary tvr (2.06 KB, patch)
2007-03-20 15:10 PDT, Brian Crowder
igor: review-
Details | Diff | Review
jsval array instead of multiple tvrs (1.57 KB, patch)
2007-03-20 16:53 PDT, Brian Crowder
igor: review-
Details | Diff | Review
two tvrs again, and modified string usage (3.46 KB, patch)
2007-03-21 10:11 PDT, Brian Crowder
igor: review+
Details | Diff | Review
atomize the string earlier (2.04 KB, patch)
2007-03-23 10:54 PDT, Brian Crowder
igor: review+
dveditz: approval1.8.0.12+
Details | Diff | Review

Description Wladimir Palant 2007-02-08 01:31:31 PST
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 2007-02-08 01:33:17 PST
Created attachment 254402 [details]
patterns.ini file required for the crash (gzipped)
Comment 2 Wladimir Palant 2007-02-08 02:34:53 PST
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 2007-03-01 15:14:43 PST
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] 2007-03-01 15:27:43 PST
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 2007-03-01 15:40:01 PST
I will take this one as soon as I am done with the array bug.
Comment 6 Brian Crowder 2007-03-01 16:24:17 PST
Created attachment 256972 [details] [diff] [review]
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] 2007-03-08 08:21:57 PST
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 2007-03-20 15:10:08 PDT
Created attachment 259144 [details] [diff] [review]
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 2007-03-20 16:33:22 PDT
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 2007-03-20 16:53:04 PDT
Created attachment 259156 [details] [diff] [review]
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 2007-03-20 18:18:50 PDT
(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 2007-03-20 18:24:15 PDT
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 2007-03-21 10:11:57 PDT
Created attachment 259226 [details] [diff] [review]
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 2007-03-21 13:03:37 PDT
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 2007-03-21 14:29:04 PDT
(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 2007-03-21 14:42:16 PDT
(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] 2007-03-21 23:10:18 PDT
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 2007-03-23 10:54:45 PDT
Created attachment 259421 [details] [diff] [review]
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 2007-03-23 12:33:42 PDT
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 2007-03-23 14:56:33 PDT
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] 2007-03-27 15:51:43 PDT
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 2007-04-04 09:03:42 PDT
jsfun.c: 3.117.2.7.2.13
Comment 23 Daniel Veditz [:dveditz] 2007-04-16 18:22:51 PDT
That's fixed1.8.0.12, right?
Comment 24 Brian Crowder 2007-04-16 18:27:34 PDT
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] 2007-05-01 15:10:59 PDT
guessing sg:moderate, but could be sg:critical if there's a reliable web-content testcase that doesn't require particular extensions.

Note You need to log in before you can comment on or make changes to this bug.